presto icon indicating copy to clipboard operation
presto copied to clipboard

Published 20 hours ago • verified publisher icon prestodb

(v2) Fix ROUND() returning incorrect results for edge cases

Open sviscaino opened this issue 2 years ago • 4 comments

This achieves the same as #18451, which was reverted (#18768) because of a performance regression (see comment). Instead of systematically using BigDecimal in round(), we call it only when we detect that we are on an edge case scenario of num*factor overflowing a long.

Test plan - Unit test: mvn -Dtest="TestMathFunctions" test Performance: SqlRoundBenchmark.java (did not commit it):

  • result for the original round() (that is wrong for edge cases)
  • result for the version that systematically uses BigDecimal (performance regression)
  • result for this PR's version (no performance regression)
== NO RELEASE NOTE ==

sviscaino avatar Dec 07 '22 12:12 sviscaino

Can you add the benchmark that you said did not commit so we can be sure? Thanks!

kaikalur avatar Dec 07 '22 21:12 kaikalur

Can you add the benchmark that you said did not commit so we can be sure? Thanks!

I put the link to the gist in the PR description. Do you want me to commit it?

sviscaino avatar Dec 07 '22 21:12 sviscaino

BTW, many thanks for working on this bug fix, appreciate it!

arunthirupathi avatar Dec 07 '22 21:12 arunthirupathi

Can you please add the Benchmark as well ?

Ok, I committed the benchmark as well.

sviscaino avatar Jan 23 '23 22:01 sviscaino