Skip to content

Commit 0cd2141

Browse files
committed
ESQL: Fix ROUND() with unsigned longs throwing in some edge cases (#119536)
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
1 parent 8f663ef commit 0cd2141

File tree

11 files changed

+477
-43
lines changed

11 files changed

+477
-43
lines changed

docs/changelog/119536.yaml

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
pr: 119536
2+
summary: Fix ROUND() with unsigned longs throwing in some edge cases
3+
area: ES|QL
4+
type: bug
5+
issues: []

docs/reference/esql/functions/kibana/definition/round.json

Lines changed: 90 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

docs/reference/esql/functions/types/round.asciidoc

Lines changed: 5 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

x-pack/plugin/esql/qa/testFixtures/src/main/resources/math.csv-spec

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -791,6 +791,28 @@ ul:ul
791791
18446744073709551615
792792
;
793793

794+
roundMaxULWithBigNegativeDecimals
795+
required_capability: fn_round_ul_fixes
796+
row
797+
ul1 = round(18446744073709551615, -6144415263046370459::long),
798+
ul2 = round(18446744073709551615, -20::long),
799+
ul3 = round(12446744073709551615, -19::long);
800+
801+
ul1:ul | ul2:ul | ul3:ul
802+
0 | 0 | 10000000000000000000
803+
;
804+
805+
roundBigULWithRoundULOverflow
806+
required_capability: fn_round_ul_fixes
807+
row ul = round(18446744073709551615, -19::long);
808+
809+
warning:Line 1:10: evaluation of [round(18446744073709551615, -19::long)] failed, treating result as null. Only first 20 failures recorded.
810+
warning:Line 1:10: java.lang.ArithmeticException: unsigned_long overflow
811+
812+
ul:ul
813+
null
814+
;
815+
794816
mvAvg
795817
from employees | where emp_no > 10008 | eval salary_change = mv_avg(salary_change) | sort emp_no | keep emp_no, salary_change.int, salary_change | limit 7;
796818

x-pack/plugin/esql/src/main/generated/org/elasticsearch/xpack/esql/expression/function/scalar/math/RoundUnsignedLongEvaluator.java

Lines changed: 16 additions & 5 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -84,6 +84,11 @@ public enum Cap {
8484
*/
8585
FN_SUBSTRING_EMPTY_NULL,
8686

87+
/**
88+
* Fixes on function {@code ROUND} that avoid it throwing exceptions on runtime for unsigned long cases.
89+
*/
90+
FN_ROUND_UL_FIXES,
91+
8792
/**
8893
* All functions that take TEXT should never emit TEXT, only KEYWORD. #114334
8994
*/

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/Round.java

Lines changed: 18 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@
3535
import static org.elasticsearch.xpack.esql.core.expression.TypeResolutions.ParamOrdinal.FIRST;
3636
import static org.elasticsearch.xpack.esql.core.expression.TypeResolutions.ParamOrdinal.SECOND;
3737
import static org.elasticsearch.xpack.esql.core.expression.TypeResolutions.isNumeric;
38-
import static org.elasticsearch.xpack.esql.core.expression.TypeResolutions.isWholeNumber;
38+
import static org.elasticsearch.xpack.esql.core.expression.TypeResolutions.isType;
3939
import static org.elasticsearch.xpack.esql.core.util.NumericUtils.unsignedLongAsNumber;
4040
import static org.elasticsearch.xpack.esql.type.EsqlDataTypeConverter.bigIntegerToUnsignedLong;
4141
import static org.elasticsearch.xpack.esql.type.EsqlDataTypeConverter.longToUnsignedLong;
@@ -63,7 +63,7 @@ public Round(
6363
@Param(
6464
optional = true,
6565
name = "decimals",
66-
type = { "integer" }, // TODO long is supported here too
66+
type = { "integer", "long" },
6767
description = "The number of decimal places to round to. Defaults to 0. If `null`, the function returns `null`."
6868
) Expression decimals
6969
) {
@@ -103,7 +103,15 @@ protected TypeResolution resolveType() {
103103
return resolution;
104104
}
105105

106-
return decimals == null ? TypeResolution.TYPE_RESOLVED : isWholeNumber(decimals, sourceText(), SECOND);
106+
return decimals == null
107+
? TypeResolution.TYPE_RESOLVED
108+
: isType(
109+
decimals,
110+
dt -> dt.isWholeNumber() && dt != DataType.UNSIGNED_LONG,
111+
sourceText(),
112+
SECOND,
113+
"whole number except unsigned_long or counter types"
114+
);
107115
}
108116

109117
@Override
@@ -123,11 +131,16 @@ static int process(int val, long decimals) {
123131

124132
@Evaluator(extraName = "Long")
125133
static long process(long val, long decimals) {
126-
return Maths.round(val, decimals).longValue();
134+
return Maths.round(val, decimals);
127135
}
128136

129-
@Evaluator(extraName = "UnsignedLong")
137+
@Evaluator(extraName = "UnsignedLong", warnExceptions = ArithmeticException.class)
130138
static long processUnsignedLong(long val, long decimals) {
139+
if (decimals <= -20) {
140+
// Unsigned long max value is 2^64 - 1, which has 20 digits
141+
return longToUnsignedLong(0, false);
142+
}
143+
131144
Number ul = unsignedLongAsNumber(val);
132145
if (ul instanceof BigInteger bi) {
133146
BigInteger rounded = Maths.round(bi, decimals);

x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/VerifierTests.java

Lines changed: 6 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -284,27 +284,26 @@ public void testRoundFunctionInvalidInputs() {
284284
error("row a = 1, b = \"c\" | eval x = round(b)")
285285
);
286286
assertEquals(
287-
"1:31: second argument of [round(a, b)] must be [integer], found value [b] type [keyword]",
287+
"1:31: second argument of [round(a, b)] must be [whole number except unsigned_long or counter types], "
288+
+ "found value [b] type [keyword]",
288289
error("row a = 1, b = \"c\" | eval x = round(a, b)")
289290
);
290291
assertEquals(
291-
"1:31: second argument of [round(a, 3.5)] must be [integer], found value [3.5] type [double]",
292+
"1:31: second argument of [round(a, 3.5)] must be [whole number except unsigned_long or counter types], "
293+
+ "found value [3.5] type [double]",
292294
error("row a = 1, b = \"c\" | eval x = round(a, 3.5)")
293295
);
294296
}
295297

296298
public void testImplicitCastingErrorMessages() {
297-
assertEquals(
298-
"1:23: Cannot convert string [c] to [INTEGER], error [Cannot parse number [c]]",
299-
error("row a = round(123.45, \"c\")")
300-
);
299+
assertEquals("1:23: Cannot convert string [c] to [LONG], error [Cannot parse number [c]]", error("row a = round(123.45, \"c\")"));
301300
assertEquals(
302301
"1:27: Cannot convert string [c] to [DOUBLE], error [Cannot parse number [c]]",
303302
error("row a = 1 | eval x = acos(\"c\")")
304303
);
305304
assertEquals(
306305
"1:33: Cannot convert string [c] to [DOUBLE], error [Cannot parse number [c]]\n"
307-
+ "line 1:38: Cannot convert string [a] to [INTEGER], error [Cannot parse number [a]]",
306+
+ "line 1:38: Cannot convert string [a] to [LONG], error [Cannot parse number [a]]",
308307
error("row a = 1 | eval x = round(acos(\"c\"),\"a\")")
309308
);
310309
assertEquals(

x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/AbstractScalarFunctionTestCase.java

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -95,6 +95,24 @@ protected static Iterable<Object[]> parameterSuppliersFromTypedDataWithDefaultCh
9595
return parameterSuppliersFromTypedData(anyNullIsNull(entirelyNullPreservesType, randomizeBytesRefsOffset(suppliers)));
9696
}
9797

98+
/**
99+
* Converts a list of test cases into a list of parameter suppliers.
100+
* Also, adds a default set of extra test cases.
101+
* <p>
102+
* Use if possible, as this method may get updated with new checks in the future.
103+
* </p>
104+
*
105+
* @param nullsExpectedType See {@link #anyNullIsNull(List, ExpectedType, ExpectedEvaluatorToString)}
106+
* @param evaluatorToString See {@link #anyNullIsNull(List, ExpectedType, ExpectedEvaluatorToString)}
107+
*/
108+
protected static Iterable<Object[]> parameterSuppliersFromTypedDataWithDefaultChecksNoErrors(
109+
ExpectedType nullsExpectedType,
110+
ExpectedEvaluatorToString evaluatorToString,
111+
List<TestCaseSupplier> suppliers
112+
) {
113+
return parameterSuppliersFromTypedData(anyNullIsNull(randomizeBytesRefsOffset(suppliers), nullsExpectedType, evaluatorToString));
114+
}
115+
98116
/**
99117
* Converts a list of test cases into a list of parameter suppliers.
100118
* Also, adds a default set of extra test cases.
Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,44 @@
1+
/*
2+
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
3+
* or more contributor license agreements. Licensed under the Elastic License
4+
* 2.0; you may not use this file except in compliance with the Elastic License
5+
* 2.0.
6+
*/
7+
8+
package org.elasticsearch.xpack.esql.expression.function.scalar.math;
9+
10+
import org.elasticsearch.xpack.esql.core.expression.Expression;
11+
import org.elasticsearch.xpack.esql.core.tree.Source;
12+
import org.elasticsearch.xpack.esql.core.type.DataType;
13+
import org.elasticsearch.xpack.esql.expression.function.ErrorsForCasesWithoutExamplesTestCase;
14+
import org.elasticsearch.xpack.esql.expression.function.TestCaseSupplier;
15+
import org.hamcrest.Matcher;
16+
17+
import java.util.List;
18+
import java.util.Set;
19+
20+
import static org.hamcrest.Matchers.equalTo;
21+
22+
public class RoundErrorTests extends ErrorsForCasesWithoutExamplesTestCase {
23+
@Override
24+
protected List<TestCaseSupplier> cases() {
25+
return paramsToSuppliers(RoundTests.parameters());
26+
}
27+
28+
@Override
29+
protected Expression build(Source source, List<Expression> args) {
30+
return new Round(source, args.get(0), args.size() == 1 ? null : args.get(1));
31+
}
32+
33+
@Override
34+
protected Matcher<String> expectedTypeErrorMatcher(List<Set<DataType>> validPerPosition, List<DataType> signature) {
35+
return equalTo(
36+
typeErrorMessage(
37+
true,
38+
validPerPosition,
39+
signature,
40+
(v, p) -> p == 0 ? "numeric" : "whole number except unsigned_long or counter types"
41+
)
42+
);
43+
}
44+
}

0 commit comments

Comments
 (0)