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
Merged
Show file tree
Hide file tree
Changes from 17 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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 @@ -85,6 +85,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);
}
Comment on lines +139 to +142
Copy link
Contributor

Choose a reason for hiding this comment

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

👍


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