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 ROUND() returning incorrect results for edge cases #18451

Merged
merged 1 commit into from Oct 5, 2022

Conversation

sviscaino
Copy link
Contributor

@sviscaino sviscaino commented Oct 5, 2022

ROUND(double, integer) and ROUND(real, integer) are returning wrong results for edge cases:

  • if the first argument does not fit in a long: currently ROUND(double'1e100', 0) returns 9223372036854800000
  • if the second argument is too big (around ~20), which makes Math.round(num*factor) overflow: currently ROUND(double'1', 20) returns 0.092233720368548

Instead, we use this approach by scaling down a BigDecimal.

Test plan -
mvn -Dtest="TestMathFunctions" test

== RELEASE NOTES ==

General Changes
* Fix :func:`round` returning incorrect results for edge cases

@sviscaino sviscaino requested a review from a team as a code owner October 5, 2022 15:27
@kaikalur kaikalur requested a review from highker October 5, 2022 15:48
@highker highker self-assigned this Oct 5, 2022
@highker highker merged commit 8677292 into prestodb:master Oct 5, 2022
@sviscaino sviscaino deleted the fix/round-invalid-results branch October 6, 2022 08:56
@arunthirupathi
Copy link

This caused a huge performance regression on the 0.278 rollout for some queries.
Same query profiled on the 0.277
Screen Shot 2022-12-05 at 11 02 45 PM

Regressed query profiled on the 0.278
Screen Shot 2022-12-05 at 11 01 46 PM

The query had a large map<int, float> and it was casting it to map<bigint, bigint> and it regressed a lot.

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.

None yet

4 participants