Skip to content

Commit

Permalink
ESQL: Fix ROUND() with unsigned longs throwing in some edge cases (el…
Browse files Browse the repository at this point in the history
…astic#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
  • Loading branch information
ivancea authored Jan 17, 2025
1 parent b8cb080 commit acb46af
Show file tree
Hide file tree
Showing 11 changed files with 477 additions and 43 deletions.
5 changes: 5 additions & 0 deletions docs/changelog/119536.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
pr: 119536
summary: Fix ROUND() with unsigned longs throwing in some edge cases
area: ES|QL
type: bug
issues: []
90 changes: 90 additions & 0 deletions docs/reference/esql/functions/kibana/definition/round.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions docs/reference/esql/functions/types/round.asciidoc

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -771,6 +771,28 @@ ul:ul
18446744073709551615
;

roundMaxULWithBigNegativeDecimals
required_capability: fn_round_ul_fixes
row
ul1 = round(18446744073709551615, -6144415263046370459::long),
ul2 = round(18446744073709551615, -20::long),
ul3 = round(12446744073709551615, -19::long);

ul1:ul | ul2:ul | ul3:ul
0 | 0 | 10000000000000000000
;

roundBigULWithRoundULOverflow
required_capability: fn_round_ul_fixes
row ul = round(18446744073709551615, -19::long);

warning:Line 1:10: evaluation of [round(18446744073709551615, -19::long)] failed, treating result as null. Only first 20 failures recorded.
warning:Line 1:10: java.lang.ArithmeticException: unsigned_long overflow

ul:ul
null
;

mvAvg
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;

Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,11 @@ public enum Cap {
*/
FN_SUBSTRING_EMPTY_NULL,

/**
* Fixes on function {@code ROUND} that avoid it throwing exceptions on runtime for unsigned long cases.
*/
FN_ROUND_UL_FIXES,

/**
* All functions that take TEXT should never emit TEXT, only KEYWORD. #114334
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@
import static org.elasticsearch.xpack.esql.core.expression.TypeResolutions.ParamOrdinal.FIRST;
import static org.elasticsearch.xpack.esql.core.expression.TypeResolutions.ParamOrdinal.SECOND;
import static org.elasticsearch.xpack.esql.core.expression.TypeResolutions.isNumeric;
import static org.elasticsearch.xpack.esql.core.expression.TypeResolutions.isWholeNumber;
import static org.elasticsearch.xpack.esql.core.expression.TypeResolutions.isType;
import static org.elasticsearch.xpack.esql.core.util.NumericUtils.unsignedLongAsNumber;
import static org.elasticsearch.xpack.esql.type.EsqlDataTypeConverter.bigIntegerToUnsignedLong;
import static org.elasticsearch.xpack.esql.type.EsqlDataTypeConverter.longToUnsignedLong;
Expand Down Expand Up @@ -63,7 +63,7 @@ public Round(
@Param(
optional = true,
name = "decimals",
type = { "integer" }, // TODO long is supported here too
type = { "integer", "long" },
description = "The number of decimal places to round to. Defaults to 0. If `null`, the function returns `null`."
) Expression decimals
) {
Expand Down Expand Up @@ -103,7 +103,15 @@ protected TypeResolution resolveType() {
return resolution;
}

return decimals == null ? TypeResolution.TYPE_RESOLVED : isWholeNumber(decimals, sourceText(), SECOND);
return decimals == null
? TypeResolution.TYPE_RESOLVED
: isType(
decimals,
dt -> dt.isWholeNumber() && dt != DataType.UNSIGNED_LONG,
sourceText(),
SECOND,
"whole number except unsigned_long or counter types"
);
}

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

@Evaluator(extraName = "Long")
static long process(long val, long decimals) {
return Maths.round(val, decimals).longValue();
return Maths.round(val, decimals);
}

@Evaluator(extraName = "UnsignedLong")
@Evaluator(extraName = "UnsignedLong", warnExceptions = ArithmeticException.class)
static long processUnsignedLong(long val, long decimals) {
if (decimals <= -20) {
// Unsigned long max value is 2^64 - 1, which has 20 digits
return longToUnsignedLong(0, false);
}

Number ul = unsignedLongAsNumber(val);
if (ul instanceof BigInteger bi) {
BigInteger rounded = Maths.round(bi, decimals);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -284,27 +284,26 @@ public void testRoundFunctionInvalidInputs() {
error("row a = 1, b = \"c\" | eval x = round(b)")
);
assertEquals(
"1:31: second argument of [round(a, b)] must be [integer], found value [b] type [keyword]",
"1:31: second argument of [round(a, b)] must be [whole number except unsigned_long or counter types], "
+ "found value [b] type [keyword]",
error("row a = 1, b = \"c\" | eval x = round(a, b)")
);
assertEquals(
"1:31: second argument of [round(a, 3.5)] must be [integer], found value [3.5] type [double]",
"1:31: second argument of [round(a, 3.5)] must be [whole number except unsigned_long or counter types], "
+ "found value [3.5] type [double]",
error("row a = 1, b = \"c\" | eval x = round(a, 3.5)")
);
}

public void testImplicitCastingErrorMessages() {
assertEquals(
"1:23: Cannot convert string [c] to [INTEGER], error [Cannot parse number [c]]",
error("row a = round(123.45, \"c\")")
);
assertEquals("1:23: Cannot convert string [c] to [LONG], error [Cannot parse number [c]]", error("row a = round(123.45, \"c\")"));
assertEquals(
"1:27: Cannot convert string [c] to [DOUBLE], error [Cannot parse number [c]]",
error("row a = 1 | eval x = acos(\"c\")")
);
assertEquals(
"1:33: Cannot convert string [c] to [DOUBLE], error [Cannot parse number [c]]\n"
+ "line 1:38: Cannot convert string [a] to [INTEGER], error [Cannot parse number [a]]",
+ "line 1:38: Cannot convert string [a] to [LONG], error [Cannot parse number [a]]",
error("row a = 1 | eval x = round(acos(\"c\"),\"a\")")
);
assertEquals(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,24 @@ protected static Iterable<Object[]> parameterSuppliersFromTypedDataWithDefaultCh
return parameterSuppliersFromTypedData(anyNullIsNull(entirelyNullPreservesType, randomizeBytesRefsOffset(suppliers)));
}

/**
* Converts a list of test cases into a list of parameter suppliers.
* Also, adds a default set of extra test cases.
* <p>
* Use if possible, as this method may get updated with new checks in the future.
* </p>
*
* @param nullsExpectedType See {@link #anyNullIsNull(List, ExpectedType, ExpectedEvaluatorToString)}
* @param evaluatorToString See {@link #anyNullIsNull(List, ExpectedType, ExpectedEvaluatorToString)}
*/
protected static Iterable<Object[]> parameterSuppliersFromTypedDataWithDefaultChecksNoErrors(
ExpectedType nullsExpectedType,
ExpectedEvaluatorToString evaluatorToString,
List<TestCaseSupplier> suppliers
) {
return parameterSuppliersFromTypedData(anyNullIsNull(randomizeBytesRefsOffset(suppliers), nullsExpectedType, evaluatorToString));
}

/**
* Converts a list of test cases into a list of parameter suppliers.
* Also, adds a default set of extra test cases.
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
*/

package org.elasticsearch.xpack.esql.expression.function.scalar.math;

import org.elasticsearch.xpack.esql.core.expression.Expression;
import org.elasticsearch.xpack.esql.core.tree.Source;
import org.elasticsearch.xpack.esql.core.type.DataType;
import org.elasticsearch.xpack.esql.expression.function.ErrorsForCasesWithoutExamplesTestCase;
import org.elasticsearch.xpack.esql.expression.function.TestCaseSupplier;
import org.hamcrest.Matcher;

import java.util.List;
import java.util.Set;

import static org.hamcrest.Matchers.equalTo;

public class RoundErrorTests extends ErrorsForCasesWithoutExamplesTestCase {
@Override
protected List<TestCaseSupplier> cases() {
return paramsToSuppliers(RoundTests.parameters());
}

@Override
protected Expression build(Source source, List<Expression> args) {
return new Round(source, args.get(0), args.size() == 1 ? null : args.get(1));
}

@Override
protected Matcher<String> expectedTypeErrorMatcher(List<Set<DataType>> validPerPosition, List<DataType> signature) {
return equalTo(
typeErrorMessage(
true,
validPerPosition,
signature,
(v, p) -> p == 0 ? "numeric" : "whole number except unsigned_long or counter types"
)
);
}
}
Loading

0 comments on commit acb46af

Please sign in to comment.