From 91d9df34665d21ef7b0d4d2c2645bf643654bd2d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Iv=C3=A1n=20Cea=20Fontenla?= Date: Fri, 3 Jan 2025 16:53:38 +0100 Subject: [PATCH 01/15] Catch Round UL error on big negative decimals --- .../src/main/resources/math.csv-spec | 12 +++++++++++ .../math/RoundUnsignedLongEvaluator.java | 21 ++++++++++++++----- .../xpack/esql/action/EsqlCapabilities.java | 5 +++++ .../function/scalar/math/Round.java | 3 ++- 4 files changed, 35 insertions(+), 6 deletions(-) diff --git a/x-pack/plugin/esql/qa/testFixtures/src/main/resources/math.csv-spec b/x-pack/plugin/esql/qa/testFixtures/src/main/resources/math.csv-spec index 2fe2feb3bc219..9c9b606464a80 100644 --- a/x-pack/plugin/esql/qa/testFixtures/src/main/resources/math.csv-spec +++ b/x-pack/plugin/esql/qa/testFixtures/src/main/resources/math.csv-spec @@ -771,6 +771,18 @@ ul:ul 18446744073709551615 ; +roundBigULWithBigNegativeDecimals +required_capability: fn_round_ul_fixes +ROW ul = round(16144415263046370459::unsigned_long, -6144415263046370459::long) +; + +warning:Line 1:10: evaluation of [round(16144415263046370459::unsigned_long, -6144415263046370459::long)] failed, treating result as null. Only first 20 failures recorded. +warning:Line 1:10: org.elasticsearch.xpack.esql.core.InvalidArgumentException: [6144415263046370459] out of [integer] range + +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; diff --git a/x-pack/plugin/esql/src/main/generated/org/elasticsearch/xpack/esql/expression/function/scalar/math/RoundUnsignedLongEvaluator.java b/x-pack/plugin/esql/src/main/generated/org/elasticsearch/xpack/esql/expression/function/scalar/math/RoundUnsignedLongEvaluator.java index c9c6e3806cb04..2470114d7e65f 100644 --- a/x-pack/plugin/esql/src/main/generated/org/elasticsearch/xpack/esql/expression/function/scalar/math/RoundUnsignedLongEvaluator.java +++ b/x-pack/plugin/esql/src/main/generated/org/elasticsearch/xpack/esql/expression/function/scalar/math/RoundUnsignedLongEvaluator.java @@ -15,6 +15,7 @@ import org.elasticsearch.compute.operator.EvalOperator; import org.elasticsearch.compute.operator.Warnings; import org.elasticsearch.core.Releasables; +import org.elasticsearch.xpack.esql.core.InvalidArgumentException; import org.elasticsearch.xpack.esql.core.tree.Source; /** @@ -52,7 +53,7 @@ public Block eval(Page page) { if (decimalsVector == null) { return eval(page.getPositionCount(), valBlock, decimalsBlock); } - return eval(page.getPositionCount(), valVector, decimalsVector).asBlock(); + return eval(page.getPositionCount(), valVector, decimalsVector); } } } @@ -82,16 +83,26 @@ public LongBlock eval(int positionCount, LongBlock valBlock, LongBlock decimalsB result.appendNull(); continue position; } - result.appendLong(Round.processUnsignedLong(valBlock.getLong(valBlock.getFirstValueIndex(p)), decimalsBlock.getLong(decimalsBlock.getFirstValueIndex(p)))); + try { + result.appendLong(Round.processUnsignedLong(valBlock.getLong(valBlock.getFirstValueIndex(p)), decimalsBlock.getLong(decimalsBlock.getFirstValueIndex(p)))); + } catch (InvalidArgumentException e) { + warnings().registerException(e); + result.appendNull(); + } } return result.build(); } } - public LongVector eval(int positionCount, LongVector valVector, LongVector decimalsVector) { - try(LongVector.FixedBuilder result = driverContext.blockFactory().newLongVectorFixedBuilder(positionCount)) { + public LongBlock eval(int positionCount, LongVector valVector, LongVector decimalsVector) { + try(LongBlock.Builder result = driverContext.blockFactory().newLongBlockBuilder(positionCount)) { position: for (int p = 0; p < positionCount; p++) { - result.appendLong(p, Round.processUnsignedLong(valVector.getLong(p), decimalsVector.getLong(p))); + try { + result.appendLong(Round.processUnsignedLong(valVector.getLong(p), decimalsVector.getLong(p))); + } catch (InvalidArgumentException e) { + warnings().registerException(e); + result.appendNull(); + } } return result.build(); } diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java index 5c259caa9c940..294d180a64215 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java @@ -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 */ diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/Round.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/Round.java index b1baa6c55ce47..6fdceba613f41 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/Round.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/Round.java @@ -126,10 +126,11 @@ static long process(long val, long decimals) { return Maths.round(val, decimals).longValue(); } - @Evaluator(extraName = "UnsignedLong") + @Evaluator(extraName = "UnsignedLong", warnExceptions = InvalidArgumentException.class) static long processUnsignedLong(long val, long decimals) { Number ul = unsignedLongAsNumber(val); if (ul instanceof BigInteger bi) { + // May throw InvalidArgumentException if "decimals" is negative and too large BigInteger rounded = Maths.round(bi, decimals); return bigIntegerToUnsignedLong(rounded); } else { From 520123198fbb3ab4c7ab1f14b880a9c4690a35e7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Iv=C3=A1n=20Cea=20Fontenla?= Date: Fri, 3 Jan 2025 18:06:13 +0100 Subject: [PATCH 02/15] Cath round exceptions and add tests for edge cases --- .../functions/kibana/definition/round.json | 90 +++++++++++ .../esql/functions/types/round.asciidoc | 5 + .../math/RoundUnsignedLongEvaluator.java | 5 +- .../function/scalar/math/Round.java | 7 +- .../function/scalar/math/RoundTests.java | 150 +++++++++++++++--- 5 files changed, 228 insertions(+), 29 deletions(-) diff --git a/docs/reference/esql/functions/kibana/definition/round.json b/docs/reference/esql/functions/kibana/definition/round.json index 4f4ddd36daa05..4ef20aa162b42 100644 --- a/docs/reference/esql/functions/kibana/definition/round.json +++ b/docs/reference/esql/functions/kibana/definition/round.json @@ -34,6 +34,24 @@ "variadic" : false, "returnType" : "double" }, + { + "params" : [ + { + "name" : "number", + "type" : "double", + "optional" : false, + "description" : "The numeric value to round. If `null`, the function returns `null`." + }, + { + "name" : "decimals", + "type" : "long", + "optional" : true, + "description" : "The number of decimal places to round to. Defaults to 0. If `null`, the function returns `null`." + } + ], + "variadic" : false, + "returnType" : "double" + }, { "params" : [ { @@ -64,6 +82,24 @@ "variadic" : false, "returnType" : "integer" }, + { + "params" : [ + { + "name" : "number", + "type" : "integer", + "optional" : false, + "description" : "The numeric value to round. If `null`, the function returns `null`." + }, + { + "name" : "decimals", + "type" : "long", + "optional" : true, + "description" : "The number of decimal places to round to. Defaults to 0. If `null`, the function returns `null`." + } + ], + "variadic" : false, + "returnType" : "integer" + }, { "params" : [ { @@ -94,6 +130,24 @@ "variadic" : false, "returnType" : "long" }, + { + "params" : [ + { + "name" : "number", + "type" : "long", + "optional" : false, + "description" : "The numeric value to round. If `null`, the function returns `null`." + }, + { + "name" : "decimals", + "type" : "long", + "optional" : true, + "description" : "The number of decimal places to round to. Defaults to 0. If `null`, the function returns `null`." + } + ], + "variadic" : false, + "returnType" : "long" + }, { "params" : [ { @@ -105,6 +159,42 @@ ], "variadic" : false, "returnType" : "unsigned_long" + }, + { + "params" : [ + { + "name" : "number", + "type" : "unsigned_long", + "optional" : false, + "description" : "The numeric value to round. If `null`, the function returns `null`." + }, + { + "name" : "decimals", + "type" : "integer", + "optional" : true, + "description" : "The number of decimal places to round to. Defaults to 0. If `null`, the function returns `null`." + } + ], + "variadic" : false, + "returnType" : "unsigned_long" + }, + { + "params" : [ + { + "name" : "number", + "type" : "unsigned_long", + "optional" : false, + "description" : "The numeric value to round. If `null`, the function returns `null`." + }, + { + "name" : "decimals", + "type" : "long", + "optional" : true, + "description" : "The number of decimal places to round to. Defaults to 0. If `null`, the function returns `null`." + } + ], + "variadic" : false, + "returnType" : "unsigned_long" } ], "examples" : [ diff --git a/docs/reference/esql/functions/types/round.asciidoc b/docs/reference/esql/functions/types/round.asciidoc index 2c0fe768741f6..9b102edf6ed94 100644 --- a/docs/reference/esql/functions/types/round.asciidoc +++ b/docs/reference/esql/functions/types/round.asciidoc @@ -6,10 +6,15 @@ |=== number | decimals | result double | integer | double +double | long | double double | | double integer | integer | integer +integer | long | integer integer | | integer long | integer | long +long | long | long long | | long +unsigned_long | integer | unsigned_long +unsigned_long | long | unsigned_long unsigned_long | | unsigned_long |=== diff --git a/x-pack/plugin/esql/src/main/generated/org/elasticsearch/xpack/esql/expression/function/scalar/math/RoundUnsignedLongEvaluator.java b/x-pack/plugin/esql/src/main/generated/org/elasticsearch/xpack/esql/expression/function/scalar/math/RoundUnsignedLongEvaluator.java index 2470114d7e65f..8442a2f805e30 100644 --- a/x-pack/plugin/esql/src/main/generated/org/elasticsearch/xpack/esql/expression/function/scalar/math/RoundUnsignedLongEvaluator.java +++ b/x-pack/plugin/esql/src/main/generated/org/elasticsearch/xpack/esql/expression/function/scalar/math/RoundUnsignedLongEvaluator.java @@ -4,6 +4,7 @@ // 2.0. package org.elasticsearch.xpack.esql.expression.function.scalar.math; +import java.lang.ArithmeticException; import java.lang.IllegalArgumentException; import java.lang.Override; import java.lang.String; @@ -85,7 +86,7 @@ public LongBlock eval(int positionCount, LongBlock valBlock, LongBlock decimalsB } try { result.appendLong(Round.processUnsignedLong(valBlock.getLong(valBlock.getFirstValueIndex(p)), decimalsBlock.getLong(decimalsBlock.getFirstValueIndex(p)))); - } catch (InvalidArgumentException e) { + } catch (InvalidArgumentException | ArithmeticException e) { warnings().registerException(e); result.appendNull(); } @@ -99,7 +100,7 @@ public LongBlock eval(int positionCount, LongVector valVector, LongVector decima position: for (int p = 0; p < positionCount; p++) { try { result.appendLong(Round.processUnsignedLong(valVector.getLong(p), decimalsVector.getLong(p))); - } catch (InvalidArgumentException e) { + } catch (InvalidArgumentException | ArithmeticException e) { warnings().registerException(e); result.appendNull(); } diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/Round.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/Round.java index 6fdceba613f41..0fa4a9efb4ff4 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/Round.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/Round.java @@ -14,6 +14,7 @@ import org.elasticsearch.compute.ann.Evaluator; import org.elasticsearch.compute.operator.EvalOperator.ExpressionEvaluator; import org.elasticsearch.xpack.esql.EsqlIllegalArgumentException; +import org.elasticsearch.xpack.esql.core.InvalidArgumentException; import org.elasticsearch.xpack.esql.core.expression.Expression; import org.elasticsearch.xpack.esql.core.expression.predicate.operator.math.Maths; import org.elasticsearch.xpack.esql.core.tree.NodeInfo; @@ -35,7 +36,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; @@ -63,7 +64,7 @@ public Round( @Param( optional = true, name = "decimals", - type = { "integer" }, // TODO long is supported here too + type = { "integer", "long" }, // TODO long is supported here too description = "The number of decimal places to round to. Defaults to 0. If `null`, the function returns `null`." ) Expression decimals ) { @@ -126,7 +127,7 @@ static long process(long val, long decimals) { return Maths.round(val, decimals).longValue(); } - @Evaluator(extraName = "UnsignedLong", warnExceptions = InvalidArgumentException.class) + @Evaluator(extraName = "UnsignedLong", warnExceptions = { InvalidArgumentException.class, ArithmeticException.class }) static long processUnsignedLong(long val, long decimals) { Number ul = unsignedLongAsNumber(val); if (ul instanceof BigInteger bi) { diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/RoundTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/RoundTests.java index c05388a9708da..83e81e0dcc9da 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/RoundTests.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/RoundTests.java @@ -11,6 +11,7 @@ import com.carrotsearch.randomizedtesting.annotations.ParametersFactory; import org.elasticsearch.test.ESTestCase; +import org.elasticsearch.xpack.esql.core.InvalidArgumentException; import org.elasticsearch.xpack.esql.core.expression.Expression; import org.elasticsearch.xpack.esql.core.expression.predicate.operator.math.Maths; import org.elasticsearch.xpack.esql.core.tree.Source; @@ -19,6 +20,7 @@ import org.elasticsearch.xpack.esql.expression.function.AbstractScalarFunctionTestCase; import org.elasticsearch.xpack.esql.expression.function.TestCaseSupplier; +import java.math.BigInteger; import java.util.ArrayList; import java.util.List; import java.util.function.BiFunction; @@ -37,11 +39,13 @@ public RoundTests(@Name("TestCase") Supplier testCase @ParametersFactory public static Iterable parameters() { List suppliers = new ArrayList<>(); + + // Double field suppliers.add( supplier( "", DataType.DOUBLE, - () -> 1 / randomDouble(), + () -> randomDoubleBetween(-Double.MAX_VALUE, Double.MAX_VALUE, true), "RoundDoubleNoDecimalsEvaluator[val=Attribute[channel=0]]", d -> Maths.round(d, 0) ) @@ -50,36 +54,128 @@ public static Iterable parameters() { supplier( ", ", DataType.DOUBLE, - () -> 1 / randomDouble(), + () -> randomDoubleBetween(-Double.MAX_VALUE, Double.MAX_VALUE, true), DataType.INTEGER, () -> between(-30, 30), "RoundDoubleEvaluator[val=Attribute[channel=0], decimals=CastIntToLongEvaluator[v=Attribute[channel=1]]]", Maths::round ) ); - // TODO randomized cases for more types - // TODO errorsForCasesWithoutExamples - suppliers = anyNullIsNull( - suppliers, - (nullPosition, nullValueDataType, original) -> nullPosition == 0 ? nullValueDataType : original.expectedType(), - (nullPosition, nullData, original) -> original + suppliers.add( + supplier( + ", ", + DataType.DOUBLE, + () -> randomDoubleBetween(-Double.MAX_VALUE, Double.MAX_VALUE, true), + DataType.LONG, + () -> randomLongBetween(-30, 30), + "RoundDoubleEvaluator[val=Attribute[channel=0], decimals=Attribute[channel=1]]", + Maths::round + ) ); - suppliers.add(new TestCaseSupplier("two doubles", List.of(DataType.DOUBLE, DataType.INTEGER), () -> { - double number1 = 1 / randomDouble(); - double number2 = 1 / randomDouble(); - int precision = between(-30, 30); - return new TestCaseSupplier.TestCase( - List.of( - new TestCaseSupplier.TypedData(List.of(number1, number2), DataType.DOUBLE, "number"), - new TestCaseSupplier.TypedData(precision, DataType.INTEGER, "decimals") - ), - "RoundDoubleEvaluator[val=Attribute[channel=0], decimals=CastIntToLongEvaluator[v=Attribute[channel=1]]]", - DataType.DOUBLE, - is(nullValue()) - ).withWarning("Line -1:-1: evaluation of [] failed, treating result as null. Only first 20 failures recorded.") - .withWarning("Line -1:-1: java.lang.IllegalArgumentException: single-value function encountered multi-value"); - })); + // Long decimals + suppliers.add( + supplier( + ", ", + DataType.INTEGER, + ESTestCase::randomInt, + DataType.LONG, + ESTestCase::randomLong, + "RoundIntEvaluator[val=Attribute[channel=0], decimals=Attribute[channel=1]]", + (n, d) -> Maths.round((Number) n, d) + ) + ); + suppliers.add( + supplier( + ", ", + DataType.LONG, + ESTestCase::randomLong, + DataType.LONG, + ESTestCase::randomLong, + "RoundLongEvaluator[val=Attribute[channel=0], decimals=Attribute[channel=1]]", + (n, d) -> Maths.round((Number) n, d) + ) + ); + suppliers.add( + supplier( + ", ", + DataType.UNSIGNED_LONG, + ESTestCase::randomLong, + DataType.LONG, + // Safe negative integer to not trigger an exception and not slow down the test + () -> randomLongBetween(-10_000, Long.MAX_VALUE), + "RoundUnsignedLongEvaluator[val=Attribute[channel=0], decimals=Attribute[channel=1]]", + (n, d) -> Maths.round(NumericUtils.unsignedLongAsBigInteger(n), d) + ) + ); + + // Integer decimals + suppliers.add( + supplier( + ", ", + DataType.INTEGER, + ESTestCase::randomInt, + DataType.INTEGER, + ESTestCase::randomInt, + "RoundIntEvaluator[val=Attribute[channel=0], decimals=CastIntToLongEvaluator[v=Attribute[channel=1]]]", + (n, d) -> Maths.round((Number) n, d) + ) + ); + suppliers.add( + supplier( + ", ", + DataType.LONG, + ESTestCase::randomLong, + DataType.INTEGER, + ESTestCase::randomInt, + "RoundLongEvaluator[val=Attribute[channel=0], decimals=CastIntToLongEvaluator[v=Attribute[channel=1]]]", + (n, d) -> Maths.round((Number) n, d) + ) + ); + suppliers.add( + supplier( + ", ", + DataType.UNSIGNED_LONG, + ESTestCase::randomLong, + DataType.INTEGER, + // Safe negative integer to not trigger an exception and not slow down the test + () -> randomIntBetween(-10_000, Integer.MAX_VALUE), + "RoundUnsignedLongEvaluator[val=Attribute[channel=0], decimals=CastIntToLongEvaluator[v=Attribute[channel=1]]]", + (n, d) -> Maths.round(NumericUtils.unsignedLongAsBigInteger(n), d) + ) + ); + + // Unsigned long errors + suppliers.add( + new TestCaseSupplier(", ", + List.of(DataType.UNSIGNED_LONG, DataType.LONG), () -> + new TestCaseSupplier.TestCase( + List.of( + new TestCaseSupplier.TypedData(new BigInteger("16144415263046370459"), DataType.UNSIGNED_LONG, "number"), + new TestCaseSupplier.TypedData(-9223372036854775808L, DataType.LONG, "decimals") + ), + "RoundUnsignedLongEvaluator[val=Attribute[channel=0], decimals=Attribute[channel=1]]", + DataType.UNSIGNED_LONG, + equalTo(null) + ).withWarning("Line -1:-1: evaluation of [] failed, treating result as null. Only first 20 failures recorded.") + .withWarning("Line -1:-1: org.elasticsearch.xpack.esql.core.InvalidArgumentException: [-9223372036854775808] out of [integer] range") + ) + ); + suppliers.add( + new TestCaseSupplier(", ", + List.of(DataType.UNSIGNED_LONG, DataType.LONG), () -> + new TestCaseSupplier.TestCase( + List.of( + new TestCaseSupplier.TypedData(new BigInteger("16144415263046370459"), DataType.UNSIGNED_LONG, "number"), + new TestCaseSupplier.TypedData(-2147483647L, DataType.LONG, "decimals") + ), + "RoundUnsignedLongEvaluator[val=Attribute[channel=0], decimals=Attribute[channel=1]]", + DataType.UNSIGNED_LONG, + equalTo(null) + ).withWarning("Line -1:-1: evaluation of [] failed, treating result as null. Only first 20 failures recorded.") + .withWarning("Line -1:-1: java.lang.ArithmeticException: BigInteger would overflow supported range") + ) + ); // Integer or Long without a decimals parameter is a noop suppliers.add(supplier("", DataType.INTEGER, ESTestCase::randomInt, "Attribute[channel=0]", Function.identity())); @@ -128,7 +224,13 @@ public static Iterable parameters() { suppliers.add(supplier(0, 0, 0)); suppliers.add(supplier(123, 2, 123)); suppliers.add(supplier(123, -1, 120)); - return parameterSuppliersFromTypedData(suppliers); + + return parameterSuppliersFromTypedDataWithDefaultChecks( + (nullPosition, nullValueDataType, original) -> nullPosition == 0 ? nullValueDataType : original.expectedType(), + (nullPosition, nullData, original) -> original, + suppliers, + (v, p) -> p == 0 ? "numeric" : "whole number except unsigned_long or counter types" + ); } private static TestCaseSupplier supplier(double v, double expected) { From 97de0a15109e46c21bc61a04a77d697af5d21d43 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Iv=C3=A1n=20Cea=20Fontenla?= Date: Fri, 3 Jan 2025 18:06:44 +0100 Subject: [PATCH 03/15] Don't allow UL as decimals value. Currently raises a 500 --- .../esql/expression/function/scalar/math/Round.java | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/Round.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/Round.java index 0fa4a9efb4ff4..3b72f88369522 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/Round.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/Round.java @@ -104,7 +104,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 From e8aa3ca9c5d23820bf3fb333aaecea0e250c0ed9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Iv=C3=A1n=20Cea=20Fontenla?= Date: Fri, 3 Jan 2025 18:13:23 +0100 Subject: [PATCH 04/15] Format --- .../function/scalar/math/RoundTests.java | 21 ++++++++++--------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/RoundTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/RoundTests.java index 83e81e0dcc9da..6a763eacf8bb6 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/RoundTests.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/RoundTests.java @@ -11,7 +11,6 @@ import com.carrotsearch.randomizedtesting.annotations.ParametersFactory; import org.elasticsearch.test.ESTestCase; -import org.elasticsearch.xpack.esql.core.InvalidArgumentException; import org.elasticsearch.xpack.esql.core.expression.Expression; import org.elasticsearch.xpack.esql.core.expression.predicate.operator.math.Maths; import org.elasticsearch.xpack.esql.core.tree.Source; @@ -28,8 +27,6 @@ import java.util.function.Supplier; import static org.hamcrest.Matchers.equalTo; -import static org.hamcrest.Matchers.is; -import static org.hamcrest.Matchers.nullValue; public class RoundTests extends AbstractScalarFunctionTestCase { public RoundTests(@Name("TestCase") Supplier testCaseSupplier) { @@ -147,9 +144,10 @@ public static Iterable parameters() { // Unsigned long errors suppliers.add( - new TestCaseSupplier(", ", - List.of(DataType.UNSIGNED_LONG, DataType.LONG), () -> - new TestCaseSupplier.TestCase( + new TestCaseSupplier( + ", ", + List.of(DataType.UNSIGNED_LONG, DataType.LONG), + () -> new TestCaseSupplier.TestCase( List.of( new TestCaseSupplier.TypedData(new BigInteger("16144415263046370459"), DataType.UNSIGNED_LONG, "number"), new TestCaseSupplier.TypedData(-9223372036854775808L, DataType.LONG, "decimals") @@ -158,13 +156,16 @@ public static Iterable parameters() { DataType.UNSIGNED_LONG, equalTo(null) ).withWarning("Line -1:-1: evaluation of [] failed, treating result as null. Only first 20 failures recorded.") - .withWarning("Line -1:-1: org.elasticsearch.xpack.esql.core.InvalidArgumentException: [-9223372036854775808] out of [integer] range") + .withWarning( + "Line -1:-1: org.elasticsearch.xpack.esql.core.InvalidArgumentException: [-9223372036854775808] out of [integer] range" + ) ) ); suppliers.add( - new TestCaseSupplier(", ", - List.of(DataType.UNSIGNED_LONG, DataType.LONG), () -> - new TestCaseSupplier.TestCase( + new TestCaseSupplier( + ", ", + List.of(DataType.UNSIGNED_LONG, DataType.LONG), + () -> new TestCaseSupplier.TestCase( List.of( new TestCaseSupplier.TypedData(new BigInteger("16144415263046370459"), DataType.UNSIGNED_LONG, "number"), new TestCaseSupplier.TypedData(-2147483647L, DataType.LONG, "decimals") From acaa288cf374a22df9fd357a0118c36c9db942ca Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Iv=C3=A1n=20Cea=20Fontenla?= Date: Fri, 3 Jan 2025 18:15:52 +0100 Subject: [PATCH 05/15] Update docs/changelog/119536.yaml --- docs/changelog/119536.yaml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 docs/changelog/119536.yaml diff --git a/docs/changelog/119536.yaml b/docs/changelog/119536.yaml new file mode 100644 index 0000000000000..e4b0fca2bd8db --- /dev/null +++ b/docs/changelog/119536.yaml @@ -0,0 +1,5 @@ +pr: 119536 +summary: Fix ROUND() with unsigned longs throwing in some edge cases +area: ES|QL +type: bug +issues: [] From f3d8e6de114e19f6267b016d014af94a9759131d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Iv=C3=A1n=20Cea=20Fontenla?= Date: Fri, 3 Jan 2025 18:29:33 +0100 Subject: [PATCH 06/15] Format --- .../xpack/esql/expression/function/scalar/math/RoundTests.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/RoundTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/RoundTests.java index 6a763eacf8bb6..9a9cf6489b934 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/RoundTests.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/RoundTests.java @@ -157,7 +157,8 @@ public static Iterable parameters() { equalTo(null) ).withWarning("Line -1:-1: evaluation of [] failed, treating result as null. Only first 20 failures recorded.") .withWarning( - "Line -1:-1: org.elasticsearch.xpack.esql.core.InvalidArgumentException: [-9223372036854775808] out of [integer] range" + "Line -1:-1: org.elasticsearch.xpack.esql.core.InvalidArgumentException: " + + "[-9223372036854775808] out of [integer] range" ) ) ); From 0391d682a746d16cb7167a94df5d955d25a1b60e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Iv=C3=A1n=20Cea=20Fontenla?= Date: Mon, 13 Jan 2025 13:40:08 +0100 Subject: [PATCH 07/15] Add error cases test class --- .../function/scalar/math/Round.java | 2 +- .../AbstractScalarFunctionTestCase.java | 20 ++++++++++ .../function/scalar/math/RoundErrorTests.java | 38 +++++++++++++++++++ .../function/scalar/math/RoundTests.java | 7 ++-- 4 files changed, 62 insertions(+), 5 deletions(-) create mode 100644 x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/RoundErrorTests.java diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/Round.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/Round.java index 3b72f88369522..9ed37b920fe2a 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/Round.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/Round.java @@ -64,7 +64,7 @@ public Round( @Param( optional = true, name = "decimals", - type = { "integer", "long" }, // 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 ) { diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/AbstractScalarFunctionTestCase.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/AbstractScalarFunctionTestCase.java index 65b9c447170f4..0d15b2c267da6 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/AbstractScalarFunctionTestCase.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/AbstractScalarFunctionTestCase.java @@ -93,6 +93,26 @@ protected static Iterable 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. + *

+ * Use if possible, as this method may get updated with new checks in the future. + *

+ * + * @param nullsExpectedType See {@link #anyNullIsNull(List, ExpectedType, ExpectedEvaluatorToString)} + * @param evaluatorToString See {@link #anyNullIsNull(List, ExpectedType, ExpectedEvaluatorToString)} + */ + protected static Iterable parameterSuppliersFromTypedDataWithDefaultChecksNoErrors( + ExpectedType nullsExpectedType, + ExpectedEvaluatorToString evaluatorToString, + List 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. diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/RoundErrorTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/RoundErrorTests.java new file mode 100644 index 0000000000000..cbb1a58cfafee --- /dev/null +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/RoundErrorTests.java @@ -0,0 +1,38 @@ +/* + * 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 cases() { + return paramsToSuppliers(RoundTests.parameters()); + } + + @Override + protected Expression build(Source source, List args) { + return new Round(source, args.get(0), args.size() == 1 ? null : args.get(1)); + } + + @Override + protected Matcher expectedTypeErrorMatcher(List> validPerPosition, List signature) { + return equalTo(typeErrorMessage(true, validPerPosition, signature, + (v, p) -> p == 0 ? "numeric" : "whole number except unsigned_long or counter types")); + } +} diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/RoundTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/RoundTests.java index 9a9cf6489b934..f5ac9cbf8da98 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/RoundTests.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/RoundTests.java @@ -159,7 +159,7 @@ public static Iterable parameters() { .withWarning( "Line -1:-1: org.elasticsearch.xpack.esql.core.InvalidArgumentException: " + "[-9223372036854775808] out of [integer] range" - ) + ) ) ); suppliers.add( @@ -227,11 +227,10 @@ public static Iterable parameters() { suppliers.add(supplier(123, 2, 123)); suppliers.add(supplier(123, -1, 120)); - return parameterSuppliersFromTypedDataWithDefaultChecks( + return parameterSuppliersFromTypedDataWithDefaultChecksNoErrors( (nullPosition, nullValueDataType, original) -> nullPosition == 0 ? nullValueDataType : original.expectedType(), (nullPosition, nullData, original) -> original, - suppliers, - (v, p) -> p == 0 ? "numeric" : "whole number except unsigned_long or counter types" + suppliers ); } From 15f6717e9cb4184cdc5cf9ac65d06acee523733b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Iv=C3=A1n=20Cea=20Fontenla?= Date: Mon, 13 Jan 2025 13:55:33 +0100 Subject: [PATCH 08/15] Updated RoundTests to check UL edge cases --- .../math/RoundUnsignedLongEvaluator.java | 5 +- .../function/scalar/math/Round.java | 10 +++- .../function/scalar/math/RoundTests.java | 59 ++++++++++++++++--- 3 files changed, 59 insertions(+), 15 deletions(-) diff --git a/x-pack/plugin/esql/src/main/generated/org/elasticsearch/xpack/esql/expression/function/scalar/math/RoundUnsignedLongEvaluator.java b/x-pack/plugin/esql/src/main/generated/org/elasticsearch/xpack/esql/expression/function/scalar/math/RoundUnsignedLongEvaluator.java index 8442a2f805e30..9cc233b8aff0c 100644 --- a/x-pack/plugin/esql/src/main/generated/org/elasticsearch/xpack/esql/expression/function/scalar/math/RoundUnsignedLongEvaluator.java +++ b/x-pack/plugin/esql/src/main/generated/org/elasticsearch/xpack/esql/expression/function/scalar/math/RoundUnsignedLongEvaluator.java @@ -16,7 +16,6 @@ import org.elasticsearch.compute.operator.EvalOperator; import org.elasticsearch.compute.operator.Warnings; import org.elasticsearch.core.Releasables; -import org.elasticsearch.xpack.esql.core.InvalidArgumentException; import org.elasticsearch.xpack.esql.core.tree.Source; /** @@ -86,7 +85,7 @@ public LongBlock eval(int positionCount, LongBlock valBlock, LongBlock decimalsB } try { result.appendLong(Round.processUnsignedLong(valBlock.getLong(valBlock.getFirstValueIndex(p)), decimalsBlock.getLong(decimalsBlock.getFirstValueIndex(p)))); - } catch (InvalidArgumentException | ArithmeticException e) { + } catch (ArithmeticException e) { warnings().registerException(e); result.appendNull(); } @@ -100,7 +99,7 @@ public LongBlock eval(int positionCount, LongVector valVector, LongVector decima position: for (int p = 0; p < positionCount; p++) { try { result.appendLong(Round.processUnsignedLong(valVector.getLong(p), decimalsVector.getLong(p))); - } catch (InvalidArgumentException | ArithmeticException e) { + } catch (ArithmeticException e) { warnings().registerException(e); result.appendNull(); } diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/Round.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/Round.java index 9ed37b920fe2a..45c94079fbec9 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/Round.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/Round.java @@ -132,14 +132,18 @@ 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", warnExceptions = { InvalidArgumentException.class, ArithmeticException.class }) + @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) { - // May throw InvalidArgumentException if "decimals" is negative and too large BigInteger rounded = Maths.round(bi, decimals); return bigIntegerToUnsignedLong(rounded); } else { diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/RoundTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/RoundTests.java index f5ac9cbf8da98..c27159fc9c433 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/RoundTests.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/RoundTests.java @@ -149,33 +149,74 @@ public static Iterable parameters() { List.of(DataType.UNSIGNED_LONG, DataType.LONG), () -> new TestCaseSupplier.TestCase( List.of( - new TestCaseSupplier.TypedData(new BigInteger("16144415263046370459"), DataType.UNSIGNED_LONG, "number"), + new TestCaseSupplier.TypedData(new BigInteger("18446744073709551615"), DataType.UNSIGNED_LONG, "number"), new TestCaseSupplier.TypedData(-9223372036854775808L, DataType.LONG, "decimals") ), "RoundUnsignedLongEvaluator[val=Attribute[channel=0], decimals=Attribute[channel=1]]", DataType.UNSIGNED_LONG, - equalTo(null) - ).withWarning("Line -1:-1: evaluation of [] failed, treating result as null. Only first 20 failures recorded.") - .withWarning( - "Line -1:-1: org.elasticsearch.xpack.esql.core.InvalidArgumentException: " - + "[-9223372036854775808] out of [integer] range" + equalTo(BigInteger.ZERO) ) ) ); suppliers.add( new TestCaseSupplier( - ", ", + ", ", List.of(DataType.UNSIGNED_LONG, DataType.LONG), () -> new TestCaseSupplier.TestCase( List.of( - new TestCaseSupplier.TypedData(new BigInteger("16144415263046370459"), DataType.UNSIGNED_LONG, "number"), + new TestCaseSupplier.TypedData(new BigInteger("18446744073709551615"), DataType.UNSIGNED_LONG, "number"), new TestCaseSupplier.TypedData(-2147483647L, DataType.LONG, "decimals") ), "RoundUnsignedLongEvaluator[val=Attribute[channel=0], decimals=Attribute[channel=1]]", DataType.UNSIGNED_LONG, + equalTo(BigInteger.ZERO) + ) + ) + ); + suppliers.add( + new TestCaseSupplier( + ", <-20>", + List.of(DataType.UNSIGNED_LONG, DataType.LONG), + () -> new TestCaseSupplier.TestCase( + List.of( + new TestCaseSupplier.TypedData(new BigInteger("18446744073709551615"), DataType.UNSIGNED_LONG, "number"), + new TestCaseSupplier.TypedData(-20L, DataType.LONG, "decimals") + ), + "RoundUnsignedLongEvaluator[val=Attribute[channel=0], decimals=Attribute[channel=1]]", + DataType.UNSIGNED_LONG, + equalTo(BigInteger.ZERO) + ) + ) + ); + suppliers.add( + new TestCaseSupplier( + ", <-19>", + List.of(DataType.UNSIGNED_LONG, DataType.LONG), + () -> new TestCaseSupplier.TestCase( + List.of( + new TestCaseSupplier.TypedData(new BigInteger("18446744073709551615"), DataType.UNSIGNED_LONG, "number"), + new TestCaseSupplier.TypedData(-19L, DataType.LONG, "decimals") + ), + "RoundUnsignedLongEvaluator[val=Attribute[channel=0], decimals=Attribute[channel=1]]", + DataType.UNSIGNED_LONG, equalTo(null) ).withWarning("Line -1:-1: evaluation of [] failed, treating result as null. Only first 20 failures recorded.") - .withWarning("Line -1:-1: java.lang.ArithmeticException: BigInteger would overflow supported range") + .withWarning("Line -1:-1: java.lang.ArithmeticException: unsigned_long overflow") + ) + ); + suppliers.add( + new TestCaseSupplier( + ", <-19>", + List.of(DataType.UNSIGNED_LONG, DataType.LONG), + () -> new TestCaseSupplier.TestCase( + List.of( + new TestCaseSupplier.TypedData(new BigInteger("14446744073709551615"), DataType.UNSIGNED_LONG, "number"), + new TestCaseSupplier.TypedData(-19L, DataType.LONG, "decimals") + ), + "RoundUnsignedLongEvaluator[val=Attribute[channel=0], decimals=Attribute[channel=1]]", + DataType.UNSIGNED_LONG, + equalTo(new BigInteger("10000000000000000000")) + ) ) ); From 9ce3e3a0d3c51693e5a5e3fc435eb9f862740969 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Iv=C3=A1n=20Cea=20Fontenla?= Date: Mon, 13 Jan 2025 13:55:44 +0100 Subject: [PATCH 09/15] CSV tests with Ul edge cases --- .../src/main/resources/math.csv-spec | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/x-pack/plugin/esql/qa/testFixtures/src/main/resources/math.csv-spec b/x-pack/plugin/esql/qa/testFixtures/src/main/resources/math.csv-spec index 9c9b606464a80..b2b4f15860484 100644 --- a/x-pack/plugin/esql/qa/testFixtures/src/main/resources/math.csv-spec +++ b/x-pack/plugin/esql/qa/testFixtures/src/main/resources/math.csv-spec @@ -771,13 +771,23 @@ ul:ul 18446744073709551615 ; -roundBigULWithBigNegativeDecimals +roundMaxULWithBigNegativeDecimals required_capability: fn_round_ul_fixes -ROW ul = round(16144415263046370459::unsigned_long, -6144415263046370459::long) +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 ; -warning:Line 1:10: evaluation of [round(16144415263046370459::unsigned_long, -6144415263046370459::long)] failed, treating result as null. Only first 20 failures recorded. -warning:Line 1:10: org.elasticsearch.xpack.esql.core.InvalidArgumentException: [6144415263046370459] out of [integer] range +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 From ea1cc59e503d868c277f27fdfd1002ab71090b1f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Iv=C3=A1n=20Cea=20Fontenla?= Date: Mon, 13 Jan 2025 14:34:13 +0100 Subject: [PATCH 10/15] Added max value tests for longs and integers --- .../function/scalar/math/RoundTests.java | 78 +++++++++++++++++++ 1 file changed, 78 insertions(+) diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/RoundTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/RoundTests.java index c27159fc9c433..e7a8d2d7ef9d4 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/RoundTests.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/RoundTests.java @@ -220,6 +220,84 @@ public static Iterable parameters() { ) ); + // Max longs and overflows + suppliers.add( + new TestCaseSupplier( + ", <-20>", + List.of(DataType.LONG, DataType.LONG), + () -> new TestCaseSupplier.TestCase( + List.of( + new TestCaseSupplier.TypedData(Long.MAX_VALUE, DataType.LONG, "number"), + new TestCaseSupplier.TypedData(-20L, DataType.LONG, "decimals") + ), + "RoundLongEvaluator[val=Attribute[channel=0], decimals=Attribute[channel=1]]", + DataType.LONG, + equalTo(0L) + ) + ) + ); + suppliers.add( + new TestCaseSupplier( + ", <-19>", + List.of(DataType.LONG, DataType.LONG), + () -> new TestCaseSupplier.TestCase( + List.of( + new TestCaseSupplier.TypedData(Long.MAX_VALUE, DataType.LONG, "number"), + new TestCaseSupplier.TypedData(-19L, DataType.LONG, "decimals") + ), + "RoundLongEvaluator[val=Attribute[channel=0], decimals=Attribute[channel=1]]", + DataType.LONG, + equalTo(0L) + ) + ) + ); + suppliers.add( + new TestCaseSupplier( + ", <-18>", + List.of(DataType.LONG, DataType.LONG), + () -> new TestCaseSupplier.TestCase( + List.of( + new TestCaseSupplier.TypedData(Long.MAX_VALUE, DataType.LONG, "number"), + new TestCaseSupplier.TypedData(-18L, DataType.LONG, "decimals") + ), + "RoundLongEvaluator[val=Attribute[channel=0], decimals=Attribute[channel=1]]", + DataType.LONG, + equalTo(9000000000000000000L) + ) + ) + ); + // Max integers and overflows + suppliers.add( + new TestCaseSupplier( + ", <-10>", + List.of(DataType.INTEGER, DataType.LONG), + () -> new TestCaseSupplier.TestCase( + List.of( + new TestCaseSupplier.TypedData(Integer.MAX_VALUE, DataType.INTEGER, "number"), + new TestCaseSupplier.TypedData(-10L, DataType.LONG, "decimals") + ), + "RoundIntEvaluator[val=Attribute[channel=0], decimals=Attribute[channel=1]]", + DataType.INTEGER, + equalTo(0) + ) + ) + ); + suppliers.add( + new TestCaseSupplier( + ", <-9>", + List.of(DataType.INTEGER, DataType.LONG), + () -> new TestCaseSupplier.TestCase( + List.of( + new TestCaseSupplier.TypedData(Integer.MAX_VALUE, DataType.INTEGER, "number"), + new TestCaseSupplier.TypedData(-9L, DataType.LONG, "decimals") + ), + "RoundIntEvaluator[val=Attribute[channel=0], decimals=Attribute[channel=1]]", + DataType.INTEGER, + equalTo(2000000000) + ) + ) + ); + // Integer or Long without a decimals parameter is a noop suppliers.add(supplier("", DataType.INTEGER, ESTestCase::randomInt, "Attribute[channel=0]", Function.identity())); suppliers.add(supplier("", DataType.LONG, ESTestCase::randomLong, "Attribute[channel=0]", Function.identity())); From d81883a6e78f4b34f77e82c92a825b628614412d Mon Sep 17 00:00:00 2001 From: elasticsearchmachine Date: Mon, 13 Jan 2025 13:41:07 +0000 Subject: [PATCH 11/15] [CI] Auto commit changes from spotless --- .../esql/expression/function/scalar/math/Round.java | 1 - .../function/AbstractScalarFunctionTestCase.java | 4 +--- .../function/scalar/math/RoundErrorTests.java | 10 ++++++++-- 3 files changed, 9 insertions(+), 6 deletions(-) diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/Round.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/Round.java index 45c94079fbec9..7c977fd1ce5a6 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/Round.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/Round.java @@ -14,7 +14,6 @@ import org.elasticsearch.compute.ann.Evaluator; import org.elasticsearch.compute.operator.EvalOperator.ExpressionEvaluator; import org.elasticsearch.xpack.esql.EsqlIllegalArgumentException; -import org.elasticsearch.xpack.esql.core.InvalidArgumentException; import org.elasticsearch.xpack.esql.core.expression.Expression; import org.elasticsearch.xpack.esql.core.expression.predicate.operator.math.Maths; import org.elasticsearch.xpack.esql.core.tree.NodeInfo; diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/AbstractScalarFunctionTestCase.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/AbstractScalarFunctionTestCase.java index 0d15b2c267da6..7328f356f07df 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/AbstractScalarFunctionTestCase.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/AbstractScalarFunctionTestCase.java @@ -108,9 +108,7 @@ protected static Iterable parameterSuppliersFromTypedDataWithDefaultCh ExpectedEvaluatorToString evaluatorToString, List suppliers ) { - return parameterSuppliersFromTypedData( - anyNullIsNull(randomizeBytesRefsOffset(suppliers), nullsExpectedType, evaluatorToString) - ); + return parameterSuppliersFromTypedData(anyNullIsNull(randomizeBytesRefsOffset(suppliers), nullsExpectedType, evaluatorToString)); } /** diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/RoundErrorTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/RoundErrorTests.java index cbb1a58cfafee..54020317bbcfd 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/RoundErrorTests.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/RoundErrorTests.java @@ -32,7 +32,13 @@ protected Expression build(Source source, List args) { @Override protected Matcher expectedTypeErrorMatcher(List> validPerPosition, List signature) { - return equalTo(typeErrorMessage(true, validPerPosition, signature, - (v, p) -> p == 0 ? "numeric" : "whole number except unsigned_long or counter types")); + return equalTo( + typeErrorMessage( + true, + validPerPosition, + signature, + (v, p) -> p == 0 ? "numeric" : "whole number except unsigned_long or counter types" + ) + ); } } From 8ad05dc0e202231771b78d39119703434534a9ce Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Iv=C3=A1n=20Cea=20Fontenla?= Date: Mon, 13 Jan 2025 16:45:53 +0100 Subject: [PATCH 12/15] Updated VerifierTests --- .../org/elasticsearch/xpack/esql/analysis/VerifierTests.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/VerifierTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/VerifierTests.java index d78c4bfa21ced..99e63fcfd3c32 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/VerifierTests.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/VerifierTests.java @@ -284,11 +284,11 @@ 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)") ); } From 5321b0058786574432061d7fb245e41081341433 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Iv=C3=A1n=20Cea=20Fontenla?= Date: Mon, 13 Jan 2025 16:49:24 +0100 Subject: [PATCH 13/15] Fix another VeritiferTests test --- .../elasticsearch/xpack/esql/analysis/VerifierTests.java | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/VerifierTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/VerifierTests.java index 99e63fcfd3c32..ded986ad6a9a9 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/VerifierTests.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/VerifierTests.java @@ -294,17 +294,14 @@ public void testRoundFunctionInvalidInputs() { } 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( From 1416a25bdf37761f4776f0a78d1dc117f550e9f4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Iv=C3=A1n=20Cea=20Fontenla?= Date: Mon, 13 Jan 2025 18:20:19 +0100 Subject: [PATCH 14/15] Format fix --- .../elasticsearch/xpack/esql/analysis/VerifierTests.java | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/VerifierTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/VerifierTests.java index ded986ad6a9a9..d708f62151e35 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/VerifierTests.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/VerifierTests.java @@ -284,11 +284,13 @@ public void testRoundFunctionInvalidInputs() { error("row a = 1, b = \"c\" | eval x = round(b)") ); assertEquals( - "1:31: second argument of [round(a, b)] must be [whole number except unsigned_long or counter types], 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 [whole number except unsigned_long or counter types], 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)") ); } From aa291bd912268ebd852debb54fb35a96248040dd Mon Sep 17 00:00:00 2001 From: elasticsearchmachine Date: Mon, 13 Jan 2025 17:27:50 +0000 Subject: [PATCH 15/15] [CI] Auto commit changes from spotless --- .../elasticsearch/xpack/esql/analysis/VerifierTests.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/VerifierTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/VerifierTests.java index d708f62151e35..8e2ef9d53216f 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/VerifierTests.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/VerifierTests.java @@ -284,13 +284,13 @@ public void testRoundFunctionInvalidInputs() { error("row a = 1, b = \"c\" | eval x = round(b)") ); assertEquals( - "1:31: second argument of [round(a, b)] must be [whole number except unsigned_long or counter types], " + - "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 [whole number except unsigned_long or counter types], " + - "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)") ); }