[ESQL] Divide by zero should return null (#107076)

This PR wires up our full suite of testing for div and mod. Doing so revealed that there were still cases where we returned NaNs and infinite values (mostly when dividing by zero), so I also fixed that to return null with a warning. The warning is not correct in all cases - it is possible to get an infinite without dividing by zero (e.g. a very large double divided by a number very close to zero, the result of which would exceed the maximum finite double value), but the error message will still report "/ by zero". I think this is fine for now, and we can always add finer grained warnings if we feel the need.
elastic · Apr 12, 2024 · 05a8498 · 05a8498
1 parent 0660f7f
commit 05a8498
Show file tree

Hide file tree

Showing 15 changed files with 383 additions and 332 deletions.
diff --git a/...n/esql/qa/server/src/main/java/org/elasticsearch/xpack/esql/qa/rest/RestEsqlTestCase.java b/...n/esql/qa/server/src/main/java/org/elasticsearch/xpack/esql/qa/rest/RestEsqlTestCase.java
@@ -94,8 +94,6 @@ public abstract class RestEsqlTestCase extends ESRestTestCase {
 
     // larger than any (unsigned) long
     private static final String HUMONGOUS_DOUBLE = "1E300";
-    private static final String INFINITY = "1.0/0.0";
-    private static final String NAN = "0.0/0.0";
 
     public static boolean shouldLog() {
         return false;
@@ -431,22 +429,19 @@ public void testOutOfRangeComparisons() throws IOException {
         String equalPlusMinus = randomFrom(" == ", " == -");
         // TODO: once we do not support infinity and NaN anymore, remove INFINITY/NAN cases.
         // https://github.com/elastic/elasticsearch/issues/98698#issuecomment-1847423390
-        String humongousPositiveLiteral = randomFrom(HUMONGOUS_DOUBLE, INFINITY);
-        String nanOrNull = randomFrom(NAN, "to_double(null)");
 
         List<String> trueForSingleValuesPredicates = List.of(
-            lessOrLessEqual + humongousPositiveLiteral,
-            largerOrLargerEqual + " -" + humongousPositiveLiteral,
-            inEqualPlusMinus + humongousPositiveLiteral,
-            inEqualPlusMinus + NAN
+            lessOrLessEqual + HUMONGOUS_DOUBLE,
+            largerOrLargerEqual + " -" + HUMONGOUS_DOUBLE,
+            inEqualPlusMinus + HUMONGOUS_DOUBLE
         );
         List<String> alwaysFalsePredicates = List.of(
-            lessOrLessEqual + " -" + humongousPositiveLiteral,
-            largerOrLargerEqual + humongousPositiveLiteral,
-            equalPlusMinus + humongousPositiveLiteral,
-            lessOrLessEqual + nanOrNull,
-            largerOrLargerEqual + nanOrNull,
-            equalPlusMinus + nanOrNull,
+            lessOrLessEqual + " -" + HUMONGOUS_DOUBLE,
+            largerOrLargerEqual + HUMONGOUS_DOUBLE,
+            equalPlusMinus + HUMONGOUS_DOUBLE,
+            lessOrLessEqual + "to_double(null)",
+            largerOrLargerEqual + "to_double(null)",
+            equalPlusMinus + "to_double(null)",
             inEqualPlusMinus + "to_double(null)"
         );
 

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
@@ -1213,21 +1213,6 @@ a:double
 // end::floor-result[]
 ;
 
-ceilFloorOfInfinite
-row i = 1.0/0.0 | eval c = ceil(i), f = floor(i);
-
-i:double | c:double | f:double
-Infinity | Infinity | Infinity
-;
-
-ceilFloorOfNegativeInfinite
-row i = -1.0/0.0 | eval c = ceil(i), f = floor(i);
-
-i:double  | c:double  | f:double
--Infinity | -Infinity | -Infinity
-;
-
-
 ceilFloorOfInteger
 row i = 1 | eval c = ceil(i), f = floor(i);
 

diff --git a/...lasticsearch/xpack/esql/expression/predicate/operator/arithmetic/DivDoublesEvaluator.java b/...lasticsearch/xpack/esql/expression/predicate/operator/arithmetic/DivDoublesEvaluator.java
@@ -4,6 +4,7 @@
 // 2.0.
 package org.elasticsearch.xpack.esql.expression.predicate.operator.arithmetic;
 
+import java.lang.ArithmeticException;
 import java.lang.IllegalArgumentException;
 import java.lang.Override;
 import java.lang.String;
@@ -50,7 +51,7 @@ public Block eval(Page page) {
         if (rhsVector == null) {
           return eval(page.getPositionCount(), lhsBlock, rhsBlock);
         }
-        return eval(page.getPositionCount(), lhsVector, rhsVector).asBlock();
+        return eval(page.getPositionCount(), lhsVector, rhsVector);
       }
     }
   }
@@ -80,16 +81,26 @@ public DoubleBlock eval(int positionCount, DoubleBlock lhsBlock, DoubleBlock rhs
           result.appendNull();
           continue position;
         }
-        result.appendDouble(Div.processDoubles(lhsBlock.getDouble(lhsBlock.getFirstValueIndex(p)), rhsBlock.getDouble(rhsBlock.getFirstValueIndex(p))));
+        try {
+          result.appendDouble(Div.processDoubles(lhsBlock.getDouble(lhsBlock.getFirstValueIndex(p)), rhsBlock.getDouble(rhsBlock.getFirstValueIndex(p))));
+        } catch (ArithmeticException e) {
+          warnings.registerException(e);
+          result.appendNull();
+        }
       }
       return result.build();
     }
   }
 
-  public DoubleVector eval(int positionCount, DoubleVector lhsVector, DoubleVector rhsVector) {
-    try(DoubleVector.Builder result = driverContext.blockFactory().newDoubleVectorBuilder(positionCount)) {
+  public DoubleBlock eval(int positionCount, DoubleVector lhsVector, DoubleVector rhsVector) {
+    try(DoubleBlock.Builder result = driverContext.blockFactory().newDoubleBlockBuilder(positionCount)) {
       position: for (int p = 0; p < positionCount; p++) {
-        result.appendDouble(Div.processDoubles(lhsVector.getDouble(p), rhsVector.getDouble(p)));
+        try {
+          result.appendDouble(Div.processDoubles(lhsVector.getDouble(p), rhsVector.getDouble(p)));
+        } catch (ArithmeticException e) {
+          warnings.registerException(e);
+          result.appendNull();
+        }
       }
       return result.build();
     }

diff --git a/...lasticsearch/xpack/esql/expression/predicate/operator/arithmetic/ModDoublesEvaluator.java b/...lasticsearch/xpack/esql/expression/predicate/operator/arithmetic/ModDoublesEvaluator.java
@@ -4,6 +4,7 @@
 // 2.0.
 package org.elasticsearch.xpack.esql.expression.predicate.operator.arithmetic;
 
+import java.lang.ArithmeticException;
 import java.lang.IllegalArgumentException;
 import java.lang.Override;
 import java.lang.String;
@@ -50,7 +51,7 @@ public Block eval(Page page) {
         if (rhsVector == null) {
           return eval(page.getPositionCount(), lhsBlock, rhsBlock);
         }
-        return eval(page.getPositionCount(), lhsVector, rhsVector).asBlock();
+        return eval(page.getPositionCount(), lhsVector, rhsVector);
       }
     }
   }
@@ -80,16 +81,26 @@ public DoubleBlock eval(int positionCount, DoubleBlock lhsBlock, DoubleBlock rhs
           result.appendNull();
           continue position;
         }
-        result.appendDouble(Mod.processDoubles(lhsBlock.getDouble(lhsBlock.getFirstValueIndex(p)), rhsBlock.getDouble(rhsBlock.getFirstValueIndex(p))));
+        try {
+          result.appendDouble(Mod.processDoubles(lhsBlock.getDouble(lhsBlock.getFirstValueIndex(p)), rhsBlock.getDouble(rhsBlock.getFirstValueIndex(p))));
+        } catch (ArithmeticException e) {
+          warnings.registerException(e);
+          result.appendNull();
+        }
       }
       return result.build();
     }
   }
 
-  public DoubleVector eval(int positionCount, DoubleVector lhsVector, DoubleVector rhsVector) {
-    try(DoubleVector.Builder result = driverContext.blockFactory().newDoubleVectorBuilder(positionCount)) {
+  public DoubleBlock eval(int positionCount, DoubleVector lhsVector, DoubleVector rhsVector) {
+    try(DoubleBlock.Builder result = driverContext.blockFactory().newDoubleBlockBuilder(positionCount)) {
       position: for (int p = 0; p < positionCount; p++) {
-        result.appendDouble(Mod.processDoubles(lhsVector.getDouble(p), rhsVector.getDouble(p)));
+        try {
+          result.appendDouble(Mod.processDoubles(lhsVector.getDouble(p), rhsVector.getDouble(p)));
+        } catch (ArithmeticException e) {
+          warnings.registerException(e);
+          result.appendNull();
+        }
       }
       return result.build();
     }

diff --git a/.../main/java/org/elasticsearch/xpack/esql/expression/predicate/operator/arithmetic/Div.java b/.../main/java/org/elasticsearch/xpack/esql/expression/predicate/operator/arithmetic/Div.java
@@ -13,6 +13,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 static org.elasticsearch.xpack.esql.expression.predicate.operator.arithmetic.EsqlArithmeticOperation.OperationSymbol.DIV;
 import static org.elasticsearch.xpack.esql.type.EsqlDataTypeConverter.longToUnsignedLong;
@@ -63,21 +64,34 @@ public ArithmeticOperationFactory binaryComparisonInverse() {
 
     @Evaluator(extraName = "Ints", warnExceptions = { ArithmeticException.class })
     static int processInts(int lhs, int rhs) {
+        if (rhs == 0) {
+            throw new ArithmeticException("/ by zero");
+        }
         return lhs / rhs;
     }
 
     @Evaluator(extraName = "Longs", warnExceptions = { ArithmeticException.class })
     static long processLongs(long lhs, long rhs) {
+        if (rhs == 0L) {
+            throw new ArithmeticException("/ by zero");
+        }
         return lhs / rhs;
     }
 
     @Evaluator(extraName = "UnsignedLongs", warnExceptions = { ArithmeticException.class })
     static long processUnsignedLongs(long lhs, long rhs) {
+        if (rhs == NumericUtils.ZERO_AS_UNSIGNED_LONG) {
+            throw new ArithmeticException("/ by zero");
+        }
         return longToUnsignedLong(Long.divideUnsigned(longToUnsignedLong(lhs, true), longToUnsignedLong(rhs, true)), true);
     }
 
-    @Evaluator(extraName = "Doubles")
+    @Evaluator(extraName = "Doubles", warnExceptions = { ArithmeticException.class })
     static double processDoubles(double lhs, double rhs) {
-        return lhs / rhs;
+        double value = lhs / rhs;
+        if (Double.isNaN(value) || Double.isInfinite(value)) {
+            throw new ArithmeticException("/ by zero");
+        }
+        return value;
     }
 }
diff --git a/.../main/java/org/elasticsearch/xpack/esql/expression/predicate/operator/arithmetic/Mod.java b/.../main/java/org/elasticsearch/xpack/esql/expression/predicate/operator/arithmetic/Mod.java
@@ -11,6 +11,7 @@
 import org.elasticsearch.xpack.ql.expression.Expression;
 import org.elasticsearch.xpack.ql.tree.NodeInfo;
 import org.elasticsearch.xpack.ql.tree.Source;
+import org.elasticsearch.xpack.ql.util.NumericUtils;
 
 import static org.elasticsearch.xpack.esql.expression.predicate.operator.arithmetic.EsqlArithmeticOperation.OperationSymbol.MOD;
 import static org.elasticsearch.xpack.esql.type.EsqlDataTypeConverter.longToUnsignedLong;
@@ -42,21 +43,34 @@ protected Mod replaceChildren(Expression left, Expression right) {
 
     @Evaluator(extraName = "Ints", warnExceptions = { ArithmeticException.class })
     static int processInts(int lhs, int rhs) {
+        if (rhs == 0) {
+            throw new ArithmeticException("/ by zero");
+        }
         return lhs % rhs;
     }
 
     @Evaluator(extraName = "Longs", warnExceptions = { ArithmeticException.class })
     static long processLongs(long lhs, long rhs) {
+        if (rhs == 0L) {
+            throw new ArithmeticException("/ by zero");
+        }
         return lhs % rhs;
     }
 
     @Evaluator(extraName = "UnsignedLongs", warnExceptions = { ArithmeticException.class })
     static long processUnsignedLongs(long lhs, long rhs) {
+        if (rhs == NumericUtils.ZERO_AS_UNSIGNED_LONG) {
+            throw new ArithmeticException("/ by zero");
+        }
         return longToUnsignedLong(Long.remainderUnsigned(longToUnsignedLong(lhs, true), longToUnsignedLong(rhs, true)), true);
     }
 
-    @Evaluator(extraName = "Doubles")
+    @Evaluator(extraName = "Doubles", warnExceptions = { ArithmeticException.class })
     static double processDoubles(double lhs, double rhs) {
-        return lhs % rhs;
+        double value = lhs % rhs;
+        if (Double.isNaN(value) || Double.isInfinite(value)) {
+            throw new ArithmeticException("/ by zero");
+        }
+        return value;
     }
 }
diff --git a/.../test/java/org/elasticsearch/xpack/esql/expression/function/AbstractFunctionTestCase.java b/.../test/java/org/elasticsearch/xpack/esql/expression/function/AbstractFunctionTestCase.java
@@ -857,6 +857,7 @@ protected static String typeErrorMessage(boolean includeOrdinal, List<Set<DataTy
         Map.entry(Set.of(DataTypes.INTEGER, DataTypes.NULL), "integer"),
         Map.entry(Set.of(DataTypes.IP, DataTypes.NULL), "ip"),
         Map.entry(Set.of(DataTypes.LONG, DataTypes.INTEGER, DataTypes.UNSIGNED_LONG, DataTypes.DOUBLE, DataTypes.NULL), "numeric"),
+        Map.entry(Set.of(DataTypes.LONG, DataTypes.INTEGER, DataTypes.UNSIGNED_LONG, DataTypes.DOUBLE), "numeric"),
         Map.entry(Set.of(DataTypes.KEYWORD, DataTypes.TEXT, DataTypes.VERSION, DataTypes.NULL), "string or version"),
         Map.entry(Set.of(DataTypes.KEYWORD, DataTypes.TEXT, DataTypes.NULL), "string"),
         Map.entry(Set.of(DataTypes.IP, DataTypes.KEYWORD, DataTypes.TEXT, DataTypes.NULL), "ip or string"),