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

Parsing large numbers is inconsistent #104323

Closed
alex-spies opened this issue Jan 12, 2024 · 3 comments · Fixed by #110665
Closed

Parsing large numbers is inconsistent #104323

alex-spies opened this issue Jan 12, 2024 · 3 comments · Fixed by #110665
Assignees
Labels
:Analytics/ES|QL AKA ESQL >bug Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo)

Comments

@alex-spies
Copy link
Contributor

Elasticsearch Version

main

Installed Plugins

No response

Java Version

17

OS Version

Ubuntu 23.04

Problem Description

  • Evaluating the query row a=0 | EVAL b=1 > -9223372036854775809 returns

    {"error":{"root_cause":[{"type":"arithmetic_exception","reason":"BigInteger out of long 
    range"}],"type":"arithmetic_exception","reason":"BigInteger out of long range"},"status":500}
    

    This should at least return a 400.

  • row a=0 | EVAL b=1 > 9223372036854775809 returns

    {"error":{"root_cause":[{"type":"verification_exception","reason":"Found 1 problem\nline 1:18: first argument of [1 > 9223372036854775809] is [integer] and second is [unsigned_long]. [unsigned_long] can only be operated on together with another [unsigned_long]"}],
    "type":"verification_exception","reason":"Found 1 problem\nline 1:18: first argument of [1 > 9223372036854775809] is [integer] and second is [unsigned_long]. [unsigned_long] can only be operated on together with another [unsigned_long]"},"status":400}
    

    This is generally correct, but it's unexpected given that the following query runs fine:
    row a=0 | EVAL b=1 > 92233720368547758090 (just add a 0 at the end).

    Maybe we should not cast literals to unsigned long, unless the user explicitly writes to_ul(...)?

Steps to Reproduce

See above.

Logs (if relevant)

No response

@alex-spies alex-spies added >bug needs:triage Requires assignment of a team area label :Analytics/ES|QL AKA ESQL labels Jan 12, 2024
@elasticsearchmachine elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Jan 12, 2024
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-analytics-geo (Team:Analytics)

@elasticsearchmachine elasticsearchmachine removed the needs:triage Requires assignment of a team area label label Jan 12, 2024
@alex-spies alex-spies removed the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Jan 12, 2024
@elasticsearchmachine elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Jan 12, 2024
@alex-spies alex-spies self-assigned this Apr 23, 2024
@elasticsearchmachine
Copy link
Collaborator

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

@alex-spies alex-spies removed their assignment Jul 4, 2024
@not-napoleon not-napoleon self-assigned this Jul 8, 2024
@not-napoleon
Copy link
Member

I've opened #110663 to address the second bullet. I think the cast is okay for now, but we really should get around to supporting mixed operations on unsigned longs soon. That particular one is pretty easy. I'm working on a PR to fix the first bullet, should be up shortly.

not-napoleon added a commit to not-napoleon/elasticsearch that referenced this issue Jul 11, 2024
Resolves elastic#104323

This fixes and adds tests for the first of the two bullets in the linked
issue.  `ExpressionBuilder#visitIntegerValue` will attempt to parse a
string as an integral value, and return a Literal of the appropriate
type.  The actual parsing happens in `StringUtils#parseIntegral`.  That
function has special handling for values that are larger than
`Long.MAX_VALUE` where it attempts to turn them into unsigned longs, and
if the number is still out of range, throw `InvalidArgumentException`.
`ExpressionBuilder` catches that `InvalidArgumentException` and tries to
parse a `double` instead.  If, on the other hand, the value is smaller
than `Long.MIN_VALUE`, `StringUtils` never enters the unsigned long path
and just calls `intValueExact`, which throws `ArithmeticException`.
This PR solves the issue by catching that `ArithmeticException` and
rethrowing it as an `InvalidArgumentException`.
elasticsearchmachine pushed a commit that referenced this issue Jul 11, 2024
…0758)

Resolves #104323

This fixes and adds tests for the first of the two bullets in the linked
issue.  `ExpressionBuilder#visitIntegerValue` will attempt to parse a
string as an integral value, and return a Literal of the appropriate
type.  The actual parsing happens in `StringUtils#parseIntegral`.  That
function has special handling for values that are larger than
`Long.MAX_VALUE` where it attempts to turn them into unsigned longs, and
if the number is still out of range, throw `InvalidArgumentException`.
`ExpressionBuilder` catches that `InvalidArgumentException` and tries to
parse a `double` instead.  If, on the other hand, the value is smaller
than `Long.MIN_VALUE`, `StringUtils` never enters the unsigned long path
and just calls `intValueExact`, which throws `ArithmeticException`.
This PR solves the issue by catching that `ArithmeticException` and
rethrowing it as an `InvalidArgumentException`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Analytics/ES|QL AKA ESQL >bug Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants