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

Fix f64 dec limit edge case #678

Merged
merged 5 commits into from
Sep 18, 2024

Conversation

finnbear
Copy link
Contributor

@finnbear finnbear commented Sep 18, 2024

Right now the f64

79228162514264337593543950336

Converts to the decimal

79228162514264337593543950330

Under Decimal::from_f64 or Decimal::from_f64_retain.

Which is wrong since:

  1. the float overflowed and that's not the max decimal
  2. conversion to decimal should have returned None anyway

The root cause is losing precision when dividing by 5, leading to missing an overflow when later multiplying by 10.

The included test (specifically the middle assertion) would fail without the patch.

@paupino
Copy link
Owner

paupino commented Sep 18, 2024

Thank you for this!

I was just doing some playing around here and I believe there are some other edge cases too.
As an example Decimal::MAX (as f64) and Decimal::MAX - 1 will both also fail since the current max for the f64 parsing algorithm is 79_228_162_514_264_337_593_543_950_330.

That said, I think this is taking things in the right direction so will merge this in.

@paupino paupino merged commit ca982d6 into paupino:master Sep 18, 2024
15 checks passed
@finnbear
Copy link
Contributor Author

finnbear commented Sep 18, 2024

@paupino AFAICT those problems don't exist since f64 can't represent those values. My test covers three adjacent f64 values, found via the unstable f64::next_down.

In the test you considered adding, the value loses precision before converting to decimal.

@finnbear finnbear deleted the fix_f64_dec_limit_edge_case branch September 18, 2024 22:21
@paupino
Copy link
Owner

paupino commented Sep 18, 2024

@finnbear Oh cool - I wasn't aware of that function. A handy one to have in the toolkit - particularly for testing floats!

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.

2 participants