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 incorrect result for decimal division #963

Merged
merged 3 commits into from Jun 12, 2019

Conversation

3 participants
@martint
Copy link
Member

commented Jun 11, 2019

When the first divisor doesn't fit in the first "32-bit" digit of the
dividend and the whole 64-bit dividend is too big to fit in an unsigned
long, the value becomes negative and regular division breaks down.

Fixes #958

@cla-bot cla-bot bot added the cla-signed label Jun 11, 2019

@dain

dain approved these changes Jun 11, 2019

martint added some commits Jun 11, 2019

Fix incorrect result for decimal division
When the first divisor doesn't fit in the first "32-bit" digit of the
dividend and the whole 64-bit dividend is too big to fit in an unsigned
long, the value becomes negative and regular division breaks down.
Improve Decimal128Arithmetic.divideUnsignedLong
Simplify the implementation based on Hacker's Delight 9-3

@martint martint force-pushed the martint:decimal-division branch from 3c5e599 to 7b1f56a Jun 11, 2019

@martint

This comment has been minimized.

Copy link
Member Author

commented Jun 11, 2019

I updated the code to use the existing divideUnsigned, which is more efficient than Java's Long.divideUnsigned when one of the numbers is large (i.e., negative).

@dain

dain approved these changes Jun 11, 2019

@martint martint merged commit 3726e22 into prestosql:master Jun 12, 2019

2 checks passed

Travis CI - Pull Request Build Passed
Details
verification/cla-signed
Details

@electrum electrum added this to the 315 milestone Jun 13, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.