Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add bigdecimal #132

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

greenbigfrog
Copy link
Contributor

@greenbigfrog greenbigfrog commented Jan 21, 2018

Most likely not the optimal solution, but it'll do the job. Require #131 to work.
(This will most likely only work on master crystal)


This change is Reviewable

@greenbigfrog
Copy link
Contributor Author

Attempt of #126 ( @pynixwang )

@pynixwang
Copy link

directly replace PG:: Numeric with BigDecimal will be better?

Copy link

@marksiemers marksiemers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See inline comments.

src/pg_ext/big_decimal.cr Show resolved Hide resolved
src/pg_ext/big_decimal.cr Show resolved Hide resolved
@greenbigfrog
Copy link
Contributor Author

@will have you looked at this yet?

@will
Copy link
Owner

will commented Feb 6, 2018

@greenbigfrog thanks. I'm curious though why you said it requires #131?

@greenbigfrog
Copy link
Contributor Author

It requires a few patches that haven't been released yet (and back when I opened this PR, I had no idea rly how crystal's compiling worked). It only requires following PRs:

https://github.com/crystal-lang/crystal/pull/5582
https://github.com/crystal-lang/crystal/pull/5589
(and maybe https://github.com/crystal-lang/crystal/pull/5525 , not sure if it's required for the functionality of this PR. (I did need it for my project though))

struct Numeric
# Returns a BigDecimal representation of the numeric. This retains all precision.
def to_big_d
return BigDecimal.new("0") if nan? || ndigits == 0
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just noticed that this should be BigDecimal.new(0) to avoid unnecessary allocation...

@will
Copy link
Owner

will commented Feb 11, 2018

The new patch has a travis failure @greenbigfrog

 1) PG::Numeric to_big_d
       Invalid BigDecimal: -9.0e-5 (Unexpected 'e' character)
       /usr/share/crystal/src/big/big_decimal.cr:0:9 in 'initialize'
       /usr/share/crystal/src/big/big_decimal.cr:78:5 in 'initialize'
       /usr/share/crystal/src/big/big_decimal.cr:77:3 in 'new'
       /numeric_spec.cr:14:3 in 'bd'
       /usr/share/crystal/src/spec/expectations.cr:0:7 in '~procProc(Nil)'
       /usr/share/crystal/src/spec/methods.cr:255:3 in 'it'
       /usr/share/crystal/src/spec/methods.cr:0:5 in '~procProc(Nil)'
       /usr/share/crystal/src/spec/context.cr:255:3 in 'describe'
       /usr/share/crystal/src/spec/methods.cr:16:5 in 'describe'
       spec/pg/encoder_spec.cr:1:1 in '__crystal_main'
       /usr/share/crystal/src/crystal/main.cr:11:3 in '_crystal_main'
       /usr/share/crystal/src/crystal/main.cr:112:5 in 'main_user_code'
       /usr/share/crystal/src/crystal/main.cr:101:7 in 'main'
       /usr/share/crystal/src/crystal/main.cr:135:3 in 'main'
       __libc_start_main
       ???
       ???

@pynixwang
Copy link

scientific notation is not support ?

@greenbigfrog
Copy link
Contributor Author

As said, you need the patches I linked in #132 (comment)
Mainly actually only crystal-lang/crystal#5582 now that I think of it.

@greenbigfrog
Copy link
Contributor Author

Realized today that this doesn't allow to do math in the query itself (eg -1 * 1.1) since it only passes it in as string (-1 * '1.1')

# Returns a BigDecimal representation of the numeric. This retains all precision.
def to_big_d
return BigDecimal.new(0) if nan? || ndigits == 0
BigDecimal.new(to_s)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is pg/pg_ext/big_rational so this method can become:

def to_big_d : BigDecimal
  to_big_r.to_big_d
end

In fact, maybe both files could be merged into a single pg/pg_ext/big file?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants