Skip to content

Commit

Permalink
[ESQL] Fix parsing of large magnitude negative numbers (#110665)
Browse files Browse the repository at this point in the history
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`.
  • Loading branch information
not-napoleon authored Jul 11, 2024
1 parent 0f6c01a commit 745a3cc
Show file tree
Hide file tree
Showing 5 changed files with 34 additions and 1 deletion.
6 changes: 6 additions & 0 deletions docs/changelog/110665.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
pr: 110665
summary: "[ESQL] Fix parsing of large magnitude negative numbers"
area: ES|QL
type: bug
issues:
- 104323
Original file line number Diff line number Diff line change
Expand Up @@ -354,6 +354,9 @@ public static Number parseIntegral(String string) throws InvalidArgumentExceptio
}
return bi;
}
if (bi.compareTo(BigInteger.valueOf(Long.MIN_VALUE)) < 0) {
throw new InvalidArgumentException("Magnitude of negative number [{}] is too large", string);
}
// try to downsize to int if possible (since that's the most common type)
if (bi.intValue() == bi.longValue()) { // ternary operator would always promote to Long
return bi.intValueExact();
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,11 @@
// Floating point types-specific tests
parseLargeMagnitudeValues
required_capability: fix_parsing_large_negative_numbers
row a = 92233720368547758090, b = -9223372036854775809;

a:double | b:double
9.223372036854776E+19 | -9.223372036854776E+18
;

inDouble
from employees | keep emp_no, height, height.float, height.half_float, height.scaled_float | where height in (2.03, 2.0299999713897705, 2.029296875, 2.0300000000000002) | sort emp_no;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,13 @@ public enum Cap {
/**
* Fix for union-types when aggregating over an inline conversion with conversion function. Done in #110652.
*/
UNION_TYPES_INLINE_FIX;
UNION_TYPES_INLINE_FIX,

/**
* Fix a parsing issue where numbers below Long.MIN_VALUE threw an exception instead of parsing as doubles.
* see <a href="https://github.com/elastic/elasticsearch/issues/104323"> Parsing large numbers is inconsistent #104323 </a>
*/
FIX_PARSING_LARGE_NEGATIVE_NUMBERS;

private final boolean snapshotOnly;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,17 @@ public void testRowCommandHugeInt() {
);
}

public void testRowCommandHugeNegativeInt() {
assertEquals(
new Row(EMPTY, List.of(new Alias(EMPTY, "c", literalDouble(-92233720368547758080d)))),
statement("row c = -92233720368547758080")
);
assertEquals(
new Row(EMPTY, List.of(new Alias(EMPTY, "c", literalDouble(-18446744073709551616d)))),
statement("row c = -18446744073709551616")
);
}

public void testRowCommandDouble() {
assertEquals(new Row(EMPTY, List.of(new Alias(EMPTY, "c", literalDouble(1.0)))), statement("row c = 1.0"));
}
Expand Down

0 comments on commit 745a3cc

Please sign in to comment.