From 5b42f5c75ee616622e249560cdd9dc8009fab127 Mon Sep 17 00:00:00 2001 From: Bogdan Pintea Date: Tue, 12 Dec 2023 13:45:42 +0100 Subject: [PATCH] ESQL: Fix `to_degrees()` returning infinity (#103209) This checks on `to_degrees()` return value and converts infinities to `null`. --- docs/changelog/103209.yaml | 6 ++++ .../src/main/resources/math.csv-spec | 8 ++--- .../scalar/convert/ToDegreesEvaluator.java | 31 ++++++++++++++----- .../function/scalar/convert/ToDegrees.java | 5 +-- .../expression/function/scalar/math/Pow.java | 11 ++----- .../scalar/convert/ToDegreesTests.java | 31 ++++++++++++++----- .../function/scalar/math/PowTests.java | 2 +- .../xpack/ql/util/NumericUtils.java | 13 ++++++++ 8 files changed, 76 insertions(+), 31 deletions(-) create mode 100644 docs/changelog/103209.yaml diff --git a/docs/changelog/103209.yaml b/docs/changelog/103209.yaml new file mode 100644 index 0000000000000..05ae8c13bcb5c --- /dev/null +++ b/docs/changelog/103209.yaml @@ -0,0 +1,6 @@ +pr: 103209 +summary: "ESQL: Fix `to_degrees()` returning infinity" +area: ES|QL +type: bug +issues: + - 102987 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 a6e24e9d45289..daf153051bb89 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 @@ -328,14 +328,14 @@ base:integer | exponent:double | s:double // end::powID-sqrt-result[] ; -powSqrtNeg#[skip:-8.11.99,reason:return type changed in 8.12] +powSqrtNeg#[skip:-8.12.99,reason:warning message changed in 8.13] // tag::powNeg-sqrt[] ROW base = -4, exponent = 0.5 | EVAL s = POW(base, exponent) // end::powNeg-sqrt[] ; warning:Line 2:12: evaluation of [POW(base, exponent)] failed, treating result as null. Only first 20 failures recorded. -warning:Line 2:12: java.lang.ArithmeticException: invalid result when computing pow +warning:Line 2:12: java.lang.ArithmeticException: not a finite double number: NaN // tag::powNeg-sqrt-result[] base:integer | exponent:double | s:double @@ -407,10 +407,10 @@ x:double 1.0 ; -powIntULOverrun#[skip:-8.11.99,reason:return type changed in 8.12] +powIntULOverrun#[skip:-8.12.99,reason:warning message changed in 8.13] row x = pow(2, 9223372036854775808); warning:Line 1:9: evaluation of [pow(2, 9223372036854775808)] failed, treating result as null. Only first 20 failures recorded. -warning:Line 1:9: java.lang.ArithmeticException: invalid result when computing pow +warning:Line 1:9: java.lang.ArithmeticException: not a finite double number: Infinity x:double null diff --git a/x-pack/plugin/esql/src/main/java/generated/org/elasticsearch/xpack/esql/expression/function/scalar/convert/ToDegreesEvaluator.java b/x-pack/plugin/esql/src/main/java/generated/org/elasticsearch/xpack/esql/expression/function/scalar/convert/ToDegreesEvaluator.java index bdf1fd8616559..8b581cbac5980 100644 --- a/x-pack/plugin/esql/src/main/java/generated/org/elasticsearch/xpack/esql/expression/function/scalar/convert/ToDegreesEvaluator.java +++ b/x-pack/plugin/esql/src/main/java/generated/org/elasticsearch/xpack/esql/expression/function/scalar/convert/ToDegreesEvaluator.java @@ -4,6 +4,7 @@ // 2.0. package org.elasticsearch.xpack.esql.expression.function.scalar.convert; +import java.lang.ArithmeticException; import java.lang.Override; import java.lang.String; import org.elasticsearch.compute.data.Block; @@ -34,11 +35,21 @@ public Block evalVector(Vector v) { DoubleVector vector = (DoubleVector) v; int positionCount = v.getPositionCount(); if (vector.isConstant()) { - return driverContext.blockFactory().newConstantDoubleBlockWith(evalValue(vector, 0), positionCount); + try { + return driverContext.blockFactory().newConstantDoubleBlockWith(evalValue(vector, 0), positionCount); + } catch (ArithmeticException e) { + registerException(e); + return driverContext.blockFactory().newConstantNullBlock(positionCount); + } } try (DoubleBlock.Builder builder = driverContext.blockFactory().newDoubleBlockBuilder(positionCount)) { for (int p = 0; p < positionCount; p++) { - builder.appendDouble(evalValue(vector, p)); + try { + builder.appendDouble(evalValue(vector, p)); + } catch (ArithmeticException e) { + registerException(e); + builder.appendNull(); + } } return builder.build(); } @@ -61,13 +72,17 @@ public Block evalBlock(Block b) { boolean positionOpened = false; boolean valuesAppended = false; for (int i = start; i < end; i++) { - double value = evalValue(block, i); - if (positionOpened == false && valueCount > 1) { - builder.beginPositionEntry(); - positionOpened = true; + try { + double value = evalValue(block, i); + if (positionOpened == false && valueCount > 1) { + builder.beginPositionEntry(); + positionOpened = true; + } + builder.appendDouble(value); + valuesAppended = true; + } catch (ArithmeticException e) { + registerException(e); } - builder.appendDouble(value); - valuesAppended = true; } if (valuesAppended == false) { builder.appendNull(); diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/convert/ToDegrees.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/convert/ToDegrees.java index 44f8507d880d8..c858bdbdb3993 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/convert/ToDegrees.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/convert/ToDegrees.java @@ -15,6 +15,7 @@ import org.elasticsearch.xpack.ql.tree.NodeInfo; import org.elasticsearch.xpack.ql.tree.Source; import org.elasticsearch.xpack.ql.type.DataType; +import org.elasticsearch.xpack.ql.util.NumericUtils; import java.util.List; import java.util.Map; @@ -64,8 +65,8 @@ public DataType dataType() { return DOUBLE; } - @ConvertEvaluator + @ConvertEvaluator(warnExceptions = { ArithmeticException.class }) static double process(double deg) { - return Math.toDegrees(deg); + return NumericUtils.asFiniteNumber(Math.toDegrees(deg)); } } diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/Pow.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/Pow.java index 0658dcccbbb48..57f32cf2212d3 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/Pow.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/Pow.java @@ -19,6 +19,7 @@ import org.elasticsearch.xpack.ql.tree.Source; import org.elasticsearch.xpack.ql.type.DataType; import org.elasticsearch.xpack.ql.type.DataTypes; +import org.elasticsearch.xpack.ql.util.NumericUtils; import java.util.Arrays; import java.util.List; @@ -71,15 +72,7 @@ public Object fold() { @Evaluator(warnExceptions = { ArithmeticException.class }) static double process(double base, double exponent) { - return validateAsDouble(base, exponent); - } - - private static double validateAsDouble(double base, double exponent) { - double result = Math.pow(base, exponent); - if (Double.isNaN(result) || Double.isInfinite(result)) { - throw new ArithmeticException("invalid result when computing pow"); - } - return result; + return NumericUtils.asFiniteNumber(Math.pow(base, exponent)); } @Override diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/convert/ToDegreesTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/convert/ToDegreesTests.java index b3e0c65f0c8f8..776782b3828f5 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/convert/ToDegreesTests.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/convert/ToDegreesTests.java @@ -10,7 +10,6 @@ import com.carrotsearch.randomizedtesting.annotations.Name; import com.carrotsearch.randomizedtesting.annotations.ParametersFactory; -import org.apache.lucene.tests.util.LuceneTestCase; import org.elasticsearch.xpack.esql.expression.function.AbstractFunctionTestCase; import org.elasticsearch.xpack.esql.expression.function.TestCaseSupplier; import org.elasticsearch.xpack.ql.expression.Expression; @@ -23,7 +22,6 @@ import java.util.function.Function; import java.util.function.Supplier; -@LuceneTestCase.AwaitsFix(bugUrl = "https://github.com/elastic/elasticsearch/issues/102987") public class ToDegreesTests extends AbstractFunctionTestCase { public ToDegreesTests(@Name("TestCase") Supplier testCaseSupplier) { this.testCase = testCaseSupplier.get(); @@ -62,14 +60,33 @@ public static Iterable parameters() { UNSIGNED_LONG_MAX, List.of() ); - TestCaseSupplier.forUnaryDouble( + TestCaseSupplier.forUnaryDouble(suppliers, "ToDegreesEvaluator[field=Attribute[channel=0]]", DataTypes.DOUBLE, d -> { + double deg = Math.toDegrees(d); + return Double.isNaN(deg) || Double.isInfinite(deg) ? null : deg; + }, Double.NEGATIVE_INFINITY, Double.POSITIVE_INFINITY, d -> { + double deg = Math.toDegrees(d); + ArrayList warnings = new ArrayList<>(2); + if (Double.isNaN(deg) || Double.isInfinite(deg)) { + warnings.add("Line -1:-1: evaluation of [] failed, treating result as null. Only first 20 failures recorded."); + warnings.add("Line -1:-1: java.lang.ArithmeticException: not a finite double number: " + deg); + } + return warnings; + }); + TestCaseSupplier.unary( suppliers, "ToDegreesEvaluator[field=Attribute[channel=0]]", + List.of( + new TestCaseSupplier.TypedDataSupplier("Double.MAX_VALUE", () -> Double.MAX_VALUE, DataTypes.DOUBLE), + new TestCaseSupplier.TypedDataSupplier("-Double.MAX_VALUE", () -> -Double.MAX_VALUE, DataTypes.DOUBLE), + new TestCaseSupplier.TypedDataSupplier("Double.POSITIVE_INFINITY", () -> Double.POSITIVE_INFINITY, DataTypes.DOUBLE), + new TestCaseSupplier.TypedDataSupplier("Double.NEGATIVE_INFINITY", () -> Double.NEGATIVE_INFINITY, DataTypes.DOUBLE) + ), DataTypes.DOUBLE, - Math::toDegrees, - Double.NEGATIVE_INFINITY, - Double.POSITIVE_INFINITY, - List.of() + d -> null, + d -> List.of( + "Line -1:-1: evaluation of [] failed, treating result as null. Only first 20 failures recorded.", + "Line -1:-1: java.lang.ArithmeticException: not a finite double number: " + ((double) d > 0 ? "Infinity" : "-Infinity") + ) ); return parameterSuppliersFromTypedData(errorsForCasesWithoutExamples(anyNullIsNull(true, suppliers))); diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/PowTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/PowTests.java index c8b316d8e6bfb..f4cf955c46bb8 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/PowTests.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/PowTests.java @@ -74,7 +74,7 @@ public static Iterable parameters() { Double.POSITIVE_INFINITY, List.of( "Line -1:-1: evaluation of [] failed, treating result as null. Only first 20 failures recorded.", - "Line -1:-1: java.lang.ArithmeticException: invalid result when computing pow" + "Line -1:-1: java.lang.ArithmeticException: not a finite double number: Infinity" ) ) ); diff --git a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/util/NumericUtils.java b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/util/NumericUtils.java index 041e3f360cecd..b46f94f958433 100644 --- a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/util/NumericUtils.java +++ b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/util/NumericUtils.java @@ -132,4 +132,17 @@ public static long unsignedLongMultiplyHigh(long x, long y) { private static long twosComplement(long l) { return l ^ TWOS_COMPLEMENT_BITMASK; } + + /** + * Check if the provided double is both finite and a number (i.e. not Double.NaN). + * @param dbl The double to verify. + * @return The input value. + * @throws ArithmeticException if the provided double is either infinite or not a number. + */ + public static double asFiniteNumber(double dbl) { + if (Double.isNaN(dbl) || Double.isInfinite(dbl)) { + throw new ArithmeticException("not a finite double number: " + dbl); + } + return dbl; + } }