ESQL: Properly handle multi-values in fold() and date math (#100766) (#…

…100804)
elastic · Oct 13, 2023 · 7256954 · 7256954
1 parent bf64b80
commit 7256954
Show file tree

Hide file tree

Showing 7 changed files with 80 additions and 13 deletions.
diff --git a/docs/changelog/100766.yaml b/docs/changelog/100766.yaml
@@ -0,0 +1,6 @@
+pr: 100766
+summary: "ESQL: Properly handle multi-values in fold() and date math"
+area: ES|QL
+type: bug
+issues:
+ - 100497
diff --git a/x-pack/plugin/esql/qa/testFixtures/src/main/resources/eval.csv-spec b/x-pack/plugin/esql/qa/testFixtures/src/main/resources/eval.csv-spec
@@ -199,3 +199,10 @@ Parto.         |Parto.Bamford    |Parto.BamfordParto.        |Parto
 Chirstian.     |Chirstian.Koblick|Chirstian.KoblickChirstian.|Chirstian      
 Kyoichi.       |Kyoichi.Maliniak |Kyoichi.MaliniakKyoichi.   |Kyoichi
 ;
+
+roundArrays
+row a = [1.2], b = [2.4, 7.9] | eval c = round(a), d = round(b), e = round([1.2]), f = round([1.2, 4.6]), g = round([1.14], 1), h = round([1.14], [1, 2]);
+
+a:double | b:double   | c:double | d: double | e:double  | f:double  | g:double  | h:double 
+1.2      | [2.4, 7.9] | 1.0      | null      | 1.0       | null      | 1.1       | null
+;
diff --git a/...sql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/Round.java b/...sql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/Round.java
@@ -34,7 +34,6 @@
 import static org.elasticsearch.xpack.ql.expression.TypeResolutions.ParamOrdinal.SECOND;
 import static org.elasticsearch.xpack.ql.expression.TypeResolutions.isInteger;
 import static org.elasticsearch.xpack.ql.expression.TypeResolutions.isNumeric;
-import static org.elasticsearch.xpack.ql.type.DataTypeConverter.safeToLong;
 import static org.elasticsearch.xpack.ql.util.NumericUtils.asLongUnsigned;
 import static org.elasticsearch.xpack.ql.util.NumericUtils.asUnsignedLong;
 import static org.elasticsearch.xpack.ql.util.NumericUtils.unsignedLongAsNumber;
@@ -70,15 +69,7 @@ public boolean foldable() {
 
     @Override
     public Object fold() {
-        if (field.dataType() == DataTypes.UNSIGNED_LONG) {
-            return decimals == null
-                ? field.fold()
-                : processUnsignedLong(safeToLong((Number) field.fold()), safeToLong((Number) decimals.fold()));
-        }
-        if (decimals == null) {
-            return Maths.round((Number) field.fold(), 0L);
-        }
-        return Maths.round((Number) field.fold(), ((Number) decimals.fold()).longValue());
+        return EvaluatorMapper.super.fold();
     }
 
     @Evaluator(extraName = "DoubleNoDecimals")

diff --git a/...arch/xpack/esql/expression/predicate/operator/arithmetic/DateTimeArithmeticOperation.java b/...arch/xpack/esql/expression/predicate/operator/arithmetic/DateTimeArithmeticOperation.java
@@ -19,6 +19,7 @@
 import java.time.Duration;
 import java.time.Period;
 import java.time.temporal.TemporalAmount;
+import java.util.Collection;
 import java.util.function.Function;
 
 import static org.elasticsearch.common.logging.LoggerMessageFormat.format;
@@ -106,10 +107,13 @@ public final Object fold() {
         DataType rightDataType = right().dataType();
         if (leftDataType == DATE_PERIOD && rightDataType == DATE_PERIOD) {
             // Both left and right expressions are temporal amounts; we can assume they are both foldable.
-            Period l = (Period) left().fold();
-            Period r = (Period) right().fold();
+            var l = left().fold();
+            var r = right().fold();
+            if (l instanceof Collection<?> || r instanceof Collection<?>) {
+                return null;
+            }
             try {
-                return fold(l, r);
+                return fold((Period) l, (Period) r);
             } catch (ArithmeticException e) {
                 // Folding will be triggered before the plan is sent to the compute service, so we have to handle arithmetic exceptions
                 // manually and provide a user-friendly error message.

diff --git a/...rc/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/RoundTests.java b/...rc/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/RoundTests.java
@@ -24,6 +24,8 @@
 
 import static org.elasticsearch.compute.data.BlockUtils.toJavaObject;
 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.TestCase> testCaseSupplier) {
@@ -44,6 +46,31 @@ public static Iterable<Object[]> parameters() {
                 DataTypes.DOUBLE,
                 equalTo(Maths.round(number, precision))
             );
+        }), new TestCaseSupplier("round([<double>], <int>)", () -> {
+            double number = 1 / randomDouble();
+            int precision = between(-30, 30);
+            return new TestCaseSupplier.TestCase(
+                List.of(
+                    new TestCaseSupplier.TypedData(List.of(number), DataTypes.DOUBLE, "number"),
+                    new TestCaseSupplier.TypedData(precision, DataTypes.INTEGER, "precision")
+                ),
+                "RoundDoubleEvaluator[val=Attribute[channel=0], decimals=CastIntToLongEvaluator[v=Attribute[channel=1]]]",
+                DataTypes.DOUBLE,
+                equalTo(Maths.round(number, precision))
+            );
+        }), new TestCaseSupplier("round([<double>], <int>)", () -> {
+            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), DataTypes.DOUBLE, "number"),
+                    new TestCaseSupplier.TypedData(precision, DataTypes.INTEGER, "precision")
+                ),
+                "RoundDoubleEvaluator[val=Attribute[channel=0], decimals=CastIntToLongEvaluator[v=Attribute[channel=1]]]",
+                DataTypes.DOUBLE,
+                is(nullValue())
+            );
         })));
     }
 

diff --git a/.../java/org/elasticsearch/xpack/esql/expression/predicate/operator/arithmetic/AddTests.java b/.../java/org/elasticsearch/xpack/esql/expression/predicate/operator/arithmetic/AddTests.java
@@ -32,6 +32,8 @@
 import static org.elasticsearch.xpack.ql.util.NumericUtils.asLongUnsigned;
 import static org.elasticsearch.xpack.ql.util.NumericUtils.unsignedLongAsBigInteger;
 import static org.hamcrest.Matchers.equalTo;
+import static org.hamcrest.Matchers.is;
+import static org.hamcrest.Matchers.nullValue;
 
 public class AddTests extends AbstractDateTimeArithmeticTestCase {
     public AddTests(@Name("TestCase") Supplier<TestCaseSupplier.TestCase> testCaseSupplier) {
@@ -164,6 +166,20 @@ public static Iterable<Object[]> parameters() {
                 EsqlDataTypes.TIME_DURATION,
                 equalTo(lhs.plus(rhs))
             );
+        }), new TestCaseSupplier("MV", () -> {
+            // Ensure we don't have an overflow
+            int rhs = randomIntBetween((Integer.MIN_VALUE >> 1) - 1, (Integer.MAX_VALUE >> 1) - 1);
+            int lhs = randomIntBetween((Integer.MIN_VALUE >> 1) - 1, (Integer.MAX_VALUE >> 1) - 1);
+            int lhs2 = randomIntBetween((Integer.MIN_VALUE >> 1) - 1, (Integer.MAX_VALUE >> 1) - 1);
+            return new TestCaseSupplier.TestCase(
+                List.of(
+                    new TestCaseSupplier.TypedData(List.of(lhs, lhs2), DataTypes.INTEGER, "lhs"),
+                    new TestCaseSupplier.TypedData(rhs, DataTypes.INTEGER, "rhs")
+                ),
+                "AddIntsEvaluator[lhs=Attribute[channel=0], rhs=Attribute[channel=1]]",
+                DataTypes.INTEGER,
+                is(nullValue())
+            );
         })));
     }
 

diff --git a/.../java/org/elasticsearch/xpack/esql/expression/predicate/operator/arithmetic/SubTests.java b/.../java/org/elasticsearch/xpack/esql/expression/predicate/operator/arithmetic/SubTests.java
@@ -32,6 +32,8 @@
 import static org.elasticsearch.xpack.ql.util.NumericUtils.asLongUnsigned;
 import static org.elasticsearch.xpack.ql.util.NumericUtils.unsignedLongAsBigInteger;
 import static org.hamcrest.Matchers.equalTo;
+import static org.hamcrest.Matchers.is;
+import static org.hamcrest.Matchers.nullValue;
 
 public class SubTests extends AbstractDateTimeArithmeticTestCase {
     public SubTests(@Name("TestCase") Supplier<TestCaseSupplier.TestCase> testCaseSupplier) {
@@ -140,6 +142,20 @@ public static Iterable<Object[]> parameters() {
                 EsqlDataTypes.TIME_DURATION,
                 equalTo(lhs.minus(rhs))
             );
+        }), new TestCaseSupplier("MV", () -> {
+            // Ensure we don't have an overflow
+            int rhs = randomIntBetween((Integer.MIN_VALUE >> 1) - 1, (Integer.MAX_VALUE >> 1) - 1);
+            int lhs = randomIntBetween((Integer.MIN_VALUE >> 1) - 1, (Integer.MAX_VALUE >> 1) - 1);
+            int lhs2 = randomIntBetween((Integer.MIN_VALUE >> 1) - 1, (Integer.MAX_VALUE >> 1) - 1);
+            return new TestCaseSupplier.TestCase(
+                List.of(
+                    new TestCaseSupplier.TypedData(List.of(lhs, lhs2), DataTypes.INTEGER, "lhs"),
+                    new TestCaseSupplier.TypedData(rhs, DataTypes.INTEGER, "rhs")
+                ),
+                "SubIntsEvaluator[lhs=Attribute[channel=0], rhs=Attribute[channel=1]]",
+                DataTypes.INTEGER,
+                is(nullValue())
+            );
         })));
     }