ESQL: Fix a folding bug with MV_ZIP (elastic#109404)

In the process of migrating all tests off of the deprecated `AbstractScalarFunctionTestCase` I had to add some extra null tests to a few functions. This discovered a bug in MV_EXPAND where the explicit `MV_EXPAND(null, ["a", "b"])` would give different results then `a = null; MV_EXPAND(a, ["a", "b"])`. This fixes that and completes the migration off of `AbstractScalarFunctionTestCase`.
jimczi · Jun 5, 2024 · b19bab2 · b19bab2
1 parent f8291f8
commit b19bab2
Show file tree

Hide file tree

Showing 8 changed files with 121 additions and 286 deletions.
diff --git a/x-pack/plugin/esql/qa/testFixtures/src/main/resources/string.csv-spec b/x-pack/plugin/esql/qa/testFixtures/src/main/resources/string.csv-spec
@@ -839,6 +839,41 @@ emp_no:integer | full_name:keyword | full_name_2:keyword | job_positions:keyword
 10005          | Kyoichi Maliniak  | Maliniak,Kyoichi    | null                                                                   | [-2.14,13.07]                 | [-2.14,13.07]
 ;
 
+mvZipLiteralNullDelim
+required_capability: mv_sort
+
+FROM employees
+| EVAL full_name = mv_zip(first_name, last_name, null)
+| KEEP emp_no, full_name
+| SORT emp_no
+| LIMIT 5;
+
+emp_no:integer | full_name:keyword
+10001          | null
+10002          | null
+10003          | null
+10004          | null
+10005          | null
+;
+
+mvZipLiteralLongDelim
+required_capability: mv_sort
+
+FROM employees
+| EVAL full_name = mv_zip(first_name, last_name, " words words words ")
+| KEEP emp_no, full_name
+| SORT emp_no
+| LIMIT 5;
+
+emp_no:integer | full_name:keyword
+10001          | Georgi words words words Facello
+10002          | Bezalel words words words Simmel
+10003          | Parto words words words Bamford
+10004          | Chirstian words words words Koblick
+10005          | Kyoichi words words words Maliniak
+;
+
+
 showTextFields
 from hosts | sort description, card, ip0, ip1 | where host == "beta" | keep host, host_group, description;
 ignoreOrder:true

diff --git a/...c/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/multivalue/MvZip.java b/...c/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/multivalue/MvZip.java
@@ -14,6 +14,7 @@
 import org.elasticsearch.compute.operator.EvalOperator;
 import org.elasticsearch.xpack.esql.core.expression.Expression;
 import org.elasticsearch.xpack.esql.core.expression.Literal;
+import org.elasticsearch.xpack.esql.core.expression.Nullability;
 import org.elasticsearch.xpack.esql.core.expression.function.OptionalArgument;
 import org.elasticsearch.xpack.esql.core.tree.NodeInfo;
 import org.elasticsearch.xpack.esql.core.tree.Source;
@@ -93,6 +94,12 @@ public boolean foldable() {
         return mvLeft.foldable() && mvRight.foldable() && (delim == null || delim.foldable());
     }
 
+    @Override
+    public Nullability nullable() {
+        // Nullability.TRUE means if *any* parameter is null we return null. We're only null if the first two are null.
+        return Nullability.FALSE;
+    }
+
     @Override
     public EvalOperator.ExpressionEvaluator.Factory toEvaluator(
         Function<Expression, EvalOperator.ExpressionEvaluator.Factory> toEvaluator

diff --git a/...g/elasticsearch/xpack/esql/expression/function/scalar/AbstractScalarFunctionTestCase.java b/...g/elasticsearch/xpack/esql/expression/function/scalar/AbstractScalarFunctionTestCase.java
diff --git a/.../src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/AbsTests.java b/.../src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/AbsTests.java
@@ -13,8 +13,8 @@
 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.AbstractFunctionTestCase;
 import org.elasticsearch.xpack.esql.expression.function.TestCaseSupplier;
-import org.elasticsearch.xpack.esql.expression.function.scalar.AbstractScalarFunctionTestCase;
 
 import java.math.BigInteger;
 import java.util.ArrayList;
@@ -23,7 +23,7 @@
 
 import static org.hamcrest.Matchers.equalTo;
 
-public class AbsTests extends AbstractScalarFunctionTestCase {
+public class AbsTests extends AbstractFunctionTestCase {
     @ParametersFactory
     public static Iterable<Object[]> parameters() {
         List<TestCaseSupplier> suppliers = new ArrayList<>();
@@ -74,14 +74,4 @@ public AbsTests(@Name("TestCase") Supplier<TestCaseSupplier.TestCase> testCaseSu
     protected Expression build(Source source, List<Expression> args) {
         return new Abs(source, args.get(0));
     }
-
-    @Override
-    protected List<ArgumentSpec> argSpec() {
-        return List.of(required(numerics()));
-    }
-
-    @Override
-    protected DataType expectedType(List<DataType> argTypes) {
-        return argTypes.get(0);
-    }
 }
diff --git a/...src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/CeilTests.java b/...src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/CeilTests.java
@@ -13,8 +13,8 @@
 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.AbstractFunctionTestCase;
 import org.elasticsearch.xpack.esql.expression.function.TestCaseSupplier;
-import org.elasticsearch.xpack.esql.expression.function.scalar.AbstractScalarFunctionTestCase;
 
 import java.math.BigInteger;
 import java.util.ArrayList;
@@ -23,31 +23,31 @@
 
 import static org.hamcrest.Matchers.equalTo;
 
-public class CeilTests extends AbstractScalarFunctionTestCase {
+public class CeilTests extends AbstractFunctionTestCase {
     public CeilTests(@Name("TestCase") Supplier<TestCaseSupplier.TestCase> testCaseSupplier) {
         this.testCase = testCaseSupplier.get();
     }
 
     @ParametersFactory
     public static Iterable<Object[]> parameters() {
         List<TestCaseSupplier> suppliers = new ArrayList<>();
-        suppliers.addAll(List.of(new TestCaseSupplier("large double value", () -> {
+        suppliers.addAll(List.of(new TestCaseSupplier("large double value", List.of(DataType.DOUBLE), () -> {
             double arg = 1 / randomDouble();
             return new TestCaseSupplier.TestCase(
                 List.of(new TestCaseSupplier.TypedData(arg, DataType.DOUBLE, "arg")),
                 "CeilDoubleEvaluator[val=Attribute[channel=0]]",
                 DataType.DOUBLE,
                 equalTo(Math.ceil(arg))
             );
-        }), new TestCaseSupplier("integer value", () -> {
+        }), new TestCaseSupplier("integer value", List.of(DataType.INTEGER), () -> {
             int arg = randomInt();
             return new TestCaseSupplier.TestCase(
                 List.of(new TestCaseSupplier.TypedData(arg, DataType.INTEGER, "arg")),
                 "Attribute[channel=0]",
                 DataType.INTEGER,
                 equalTo(arg)
             );
-        }), new TestCaseSupplier("long value", () -> {
+        }), new TestCaseSupplier("long value", List.of(DataType.LONG), () -> {
             long arg = randomLong();
             return new TestCaseSupplier.TestCase(
                 List.of(new TestCaseSupplier.TypedData(arg, DataType.LONG, "arg")),
@@ -66,17 +66,7 @@ public static Iterable<Object[]> parameters() {
             UNSIGNED_LONG_MAX,
             List.of()
         );
-        return parameterSuppliersFromTypedData(suppliers);
-    }
-
-    @Override
-    protected DataType expectedType(List<DataType> argTypes) {
-        return argTypes.get(0);
-    }
-
-    @Override
-    protected List<ArgumentSpec> argSpec() {
-        return List.of(required(numerics()));
+        return parameterSuppliersFromTypedData(errorsForCasesWithoutExamples(anyNullIsNull(false, suppliers)));
     }
 
     @Override

diff --git a/.../src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/LogTests.java b/.../src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/LogTests.java
@@ -13,13 +13,13 @@
 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.AbstractFunctionTestCase;
 import org.elasticsearch.xpack.esql.expression.function.TestCaseSupplier;
-import org.elasticsearch.xpack.esql.expression.function.scalar.AbstractScalarFunctionTestCase;
 
 import java.util.List;
 import java.util.function.Supplier;
 
-public class LogTests extends AbstractScalarFunctionTestCase {
+public class LogTests extends AbstractFunctionTestCase {
     public LogTests(@Name("TestCase") Supplier<TestCaseSupplier.TestCase> testCaseSupplier) {
         this.testCase = testCaseSupplier.get();
     }
@@ -194,16 +194,6 @@ public static Iterable<Object[]> parameters() {
         return parameterSuppliersFromTypedData(errorsForCasesWithoutExamples(suppliers));
     }
 
-    @Override
-    protected DataType expectedType(List<DataType> argTypes) {
-        return DataType.DOUBLE;
-    }
-
-    @Override
-    protected List<ArgumentSpec> argSpec() {
-        return List.of(optional(numerics()), required(numerics()));
-    }
-
     @Override
     protected Expression build(Source source, List<Expression> args) {
         return new Log(source, args.get(0), args.size() > 1 ? args.get(1) : null);