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

ESQL: Fix ROUND() with unsigned longs throwing in some edge cases #119536

Merged
merged 18 commits into from
Jan 17, 2025

Conversation

ivancea
Copy link
Contributor

@ivancea ivancea commented Jan 3, 2025

There were different error cases with ROUND(number, decimals):

  • Decimals accepted unsigned longs, but threw a 500 with a can't process [unsigned_long -> long] in the cast evaluator
    • Fixed by improving the resolveType()
  • If the number was a BigInteger unsigned long, there were 2 cases throwing an exception:
    1. Negative decimals outside the range of integer: Error
    2. Negative decimals insie the range of integer, but "big enough" for BigInteger.TEN.pow(...) to throw a BigInteger would overflow supported range
    3. -19 decimals with big unsigned longs like 18446744073709551615 was throwing an unsigned_long overflow

Also, when the number is a BigInteger and the decimals is a big negative (but not big enough to throw), it may be very slow. Taking many seconds for a single computation (It tries to calculate a 10^(big number). I didn't do anything here, but I wonder if we should limit it.

To solve most of the cases, a warnExceptions was added for the overflow case, and a guard clause to return 0 for <-19 decimals on unsigned longs.

Another issue is that rounding to a number like 7 to -1 returns 0 instead of 10, which may be considered an error. But it's consistent, so I'm leaving it to another PR

@ivancea ivancea added >bug Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) auto-backport Automatically create backport pull requests when merged :Analytics/ES|QL AKA ESQL v9.0.0 v8.18.0 labels Jan 3, 2025
Copy link
Contributor

github-actions bot commented Jan 3, 2025

Documentation preview:

@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-analytical-engine (Team:Analytics)

@elasticsearchmachine
Copy link
Collaborator

Hi @ivancea, I've created a changelog YAML for you.

@nik9000
Copy link
Member

nik9000 commented Jan 6, 2025

Also, when the number is a BigInteger and the decimals is a big negative (but not big enough to throw), it may be very slow. Taking many seconds for a single computation (It tries to calculate a 10^(big number). I didn't do anything here, but I wonder if we should limit it.

In a follow up change I think it'd be pretty reasonable to limit the inputs to decimals. unsigned_long really doesn't have that many digits. Look like 20 decimal digits max. If you round to higher than that I think the consistent thing is for us to give you 0? Or throw an error? Something like that.

@ivancea
Copy link
Contributor Author

ivancea commented Jan 13, 2025

In a follow up change I think it'd be pretty reasonable to limit the inputs to decimals. unsigned_long really doesn't have that many digits. Look like 20 decimal digits max. If you round to higher than that I think the consistent thing is for us to give you 0? Or throw an error? Something like that.

I should have started there... I'm removing the warnExceptions and adding the condition instead, returning 0

@ivancea
Copy link
Contributor Author

ivancea commented Jan 13, 2025

Updated the PR by:

  • Keeping only 1 warnExceptions for the overflow case
  • Adding a guard clause for big negative decimals on UL
  • Extra edge case tests
  • Added a RoundErrorsTest class

Copy link
Contributor

@luigidellaquila luigidellaquila left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Comment on lines +139 to +142
if (decimals <= -20) {
// Unsigned long max value is 2^64 - 1, which has 20 digits
return longToUnsignedLong(0, false);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@ivancea ivancea enabled auto-merge (squash) January 17, 2025 12:34
@ivancea ivancea merged commit acb46af into elastic:main Jan 17, 2025
16 checks passed
@ivancea ivancea deleted the esql-round-function-ul-fix branch January 17, 2025 13:38
@elasticsearchmachine
Copy link
Collaborator

💚 Backport successful

Status Branch Result
8.x

elasticsearchmachine pushed a commit that referenced this pull request Jan 17, 2025
…19536) (#120381)

There were different error cases with `ROUND(number, decimals)`:
- Decimals accepted unsigned longs, but threw a 500 with a `can't process [unsigned_long -> long]` in the cast evaluator
  - Fixed by improving the `resolveType()`
- If the number was a BigInteger unsigned long, there were 2 cases throwing an exception:
  1. Negative decimals outside the range of integer: Error
  2. Negative decimals insie the range of integer, but "big enough" for `BigInteger.TEN.pow(...)` to throw a `BigInteger would overflow supported range`
  3. -19 decimals with big unsigned longs like `18446744073709551615` was throwing an `unsigned_long overflow`

Also, when the number is a BigInteger and the decimals is a big negative (but not big enough to throw), it may be **very** slow. Taking _many_ seconds for a single computation (It tries to calculate a `10^(big number)`. I didn't do anything here, but I wonder if we should limit it.

To solve most of the cases, a warnExceptions was added for the overflow case, and a guard clause to return 0 for <-19 decimals on unsigned longs.

Another issue is that rounding to a number like 7 to -1 returns 0 instead of 10, which may be considered an error. But it's consistent, so I'm leaving it to another PR
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Analytics/ES|QL AKA ESQL auto-backport Automatically create backport pull requests when merged >bug Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) v8.18.0 v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants