-
Notifications
You must be signed in to change notification settings - Fork 78
Fix encoding for decimals in scientific notation #165
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
Fix encoding for decimals in scientific notation #165
Conversation
case sign do | ||
0 -> Decimal.new(-1, value, -scale) | ||
_ -> Decimal.new(1, value, -scale) | ||
1 -> Decimal.new(1, value, -scale) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then raise"Sign value out of range" for _
. It is bad practice to leave uncovered cover cases even tho outer code ATM guarantees it is always 1 or 0.... but one day, someone will make mistake and it will be very hard to understand what is wrong
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, I added the raise
test/types/types_test.exs
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you really want to test this correctly this is what it should cover
Expected Result | Precision | Scale | Value |
---|---|---|---|
Should decode to 123.45 | 5 | 2 | 123.45 |
Should decode to -123.45 | 5 | 2 | -123.45 |
Should round to 123.46 | 5 | 2 | 123.456 |
Should round to 123.45 | 5 | 2 | 123.454 |
Should decode to 99999.99999 | 10 | 5 | 99999.99999 |
Should decode to exact value | 38 | 18 | 999999999999999999999.999999999999999999 |
Should raise an error or truncate | 38 | 18 | 999999999999999999999.9999999999999999999 |
Should decode to 9.9 | 2 | 1 | 9.9 |
Should raise an error or truncate | 2 | 1 | 9.99 |
Should decode to exact value | 38 | 0 | 99999999999999999999999999999999999999 |
Should decode to NULL | 5 | 2 | NULL |
Should round to 12300 | 5 | -2 | 12345 |
Should round to -0.00 | 5 | 2 | -0.00001 |
This is same as it would behave if you try to cast string to decimal with given precision and scale in SQL Server query
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey Milan, thanks for the quick review and suggestions. I added all the test cases for inserting into the db, except for the -2 scale one. Let me know if I missed anything. Not sure how to handle the negative scale as the database does not support it from what I can see (SQL error: "Specified scale -2 is invalid."), unless the encoder does?
@kevinkoltz thank you for contribution ❤️ , could you please check my comments and correct 🙏 |
Currently
Decimal.new("1E+3")
(1000.0) gets encoded as1
in the database. This fixes the encoder to convert the decimal to a string using:normal
(without an exponent) instead of using thevalue.coef
which is 1.This was discovered when using
Decimal.from_float(1000.0)
which results in the exponential representationDecimal.new("1E+3")
, which is valid in elixir.