ESQL: Fix to_degrees() returning infinity (elastic#103209)

This checks on `to_degrees()` return value and converts infinities to `null`.
not-napoleon · Dec 12, 2023 · 5b42f5c · 5b42f5c
1 parent 8ee4721
commit 5b42f5c
Show file tree

Hide file tree

Showing 8 changed files with 76 additions and 31 deletions.
diff --git a/docs/changelog/103209.yaml 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
@@ -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/...d/org/elasticsearch/xpack/esql/expression/function/scalar/convert/ToDegreesEvaluator.java b/...d/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/.../main/java/org/elasticsearch/xpack/esql/expression/function/scalar/convert/ToDegrees.java b/.../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/.../esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/Pow.java b/.../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/.../java/org/elasticsearch/xpack/esql/expression/function/scalar/convert/ToDegreesTests.java b/.../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.TestCase> testCaseSupplier) {
         this.testCase = testCaseSupplier.get();
@@ -62,14 +60,33 @@ public static Iterable<Object[]> 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<String> 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/.../src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/PowTests.java b/.../src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/PowTests.java
@@ -74,7 +74,7 @@ public static Iterable<Object[]> 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
@@ -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;
+    }
 }