ES|QL: Disable optimizations that rely on Expression.nullable() (#105691

)
elastic · Mar 8, 2024 · dec76d4 · dec76d4
1 parent e1d91a2
commit dec76d4
Show file tree

Hide file tree

Showing 9 changed files with 543 additions and 32 deletions.
diff --git a/docs/changelog/105691.yaml b/docs/changelog/105691.yaml
@@ -0,0 +1,5 @@
+pr: 105691
+summary: "ES|QL: Disable optimizations that rely on Expression.nullable()"
+area: ES|QL
+type: bug
+issues: []
diff --git a/x-pack/plugin/esql/qa/testFixtures/src/main/resources/conditional.csv-spec b/x-pack/plugin/esql/qa/testFixtures/src/main/resources/conditional.csv-spec
@@ -127,3 +127,44 @@ error_rate:double | hour:date
 0.6            |2023-10-23T13:00:00.000Z
 // end::docsCaseHourlyErrorRate-result[]
 ;
+
+
+nullOnMultivaluesMathOperation#[skip:-8.13.99,reason:fixed in 8.14+]
+ROW a = 5, b = [ 1, 2 ]| EVAL sum = a + b| LIMIT 1 |  WHERE sum IS NULL;
+warning:Line 1:37: evaluation of [a + b] failed, treating result as null. Only first 20 failures recorded.
+warning:Line 1:37: java.lang.IllegalArgumentException: single-value function encountered multi-value
+
+a:integer | b:integer | sum:integer
+5         | [1, 2]    | null
+;
+
+
+notNullOnMultivaluesMathOperation#[skip:-8.13.99,reason:fixed in 8.14+]
+ROW a = 5, b = [ 1, 2 ]| EVAL sum = a + b| LIMIT 1 |  WHERE sum IS NOT NULL;
+warning:Line 1:37: evaluation of [a + b] failed, treating result as null. Only first 20 failures recorded.
+warning:Line 1:37: java.lang.IllegalArgumentException: single-value function encountered multi-value
+
+a:integer | b:integer | sum:integer
+;
+
+
+nullOnMultivaluesComparisonOperation#[skip:-8.13.99,reason:fixed in 8.14+]
+ROW a = 5, b = [ 1, 2 ]| EVAL same = a == b| LIMIT 1 |  WHERE same IS NULL;
+
+a:integer | b:integer | same:boolean
+5         | [1, 2]    | null
+;
+
+
+notNullOnMultivaluesComparisonOperation#[skip:-8.13.99,reason:fixed in 8.14+]
+ROW a = 5, b = [ 1, 2 ]| EVAL same = a == b| LIMIT 1 |  WHERE same IS NOT NULL;
+
+a:integer | b:integer | same:boolean
+;
+
+
+notNullOnMultivaluesComparisonOperationWithPartialMatch#[skip:-8.13.99,reason:fixed in 8.14+]
+ROW a = 5, b = [ 5, 2 ]| EVAL same = a == b| LIMIT 1 |  WHERE same IS NOT NULL;
+
+a:integer | b:integer | same:boolean
+;
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
@@ -200,14 +200,14 @@ Chirstian.     |Chirstian.Koblick|Chirstian.KoblickChirstian.|Chirstian
 Kyoichi.       |Kyoichi.Maliniak |Kyoichi.MaliniakKyoichi.   |Kyoichi
 ;
 
-roundArrays#[skip:-8.11.99, reason:Lucene multivalue warning introduced in 8.12 only]
+roundArrays#[skip:-8.13.99, reason:Alert order changed in 8.14]
 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]);
+warning:Line 1:56: evaluation of [round(b)] failed, treating result as null. Only first 20 failures recorded.
+warning:Line 1:56: java.lang.IllegalArgumentException: single-value function encountered multi-value
 warning:Line 1:88: evaluation of [round([1.2, 4.6])] failed, treating result as null. Only first 20 failures recorded.
 warning:Line 1:88: java.lang.IllegalArgumentException: single-value function encountered multi-value
 warning:Line 1:133: evaluation of [round([1.14], [1, 2])] failed, treating result as null. Only first 20 failures recorded.
 warning:Line 1:133: java.lang.IllegalArgumentException: single-value function encountered multi-value
-warning:Line 1:56: evaluation of [round(b)] failed, treating result as null. Only first 20 failures recorded.
-warning:Line 1:56: java.lang.IllegalArgumentException: single-value function encountered multi-value
 
 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/...lugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizer.java b/...lugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizer.java
@@ -17,6 +17,7 @@
 import org.elasticsearch.xpack.esql.evaluator.predicate.operator.comparison.Equals;
 import org.elasticsearch.xpack.esql.expression.SurrogateExpression;
 import org.elasticsearch.xpack.esql.expression.function.aggregate.Count;
+import org.elasticsearch.xpack.esql.expression.function.scalar.nulls.Coalesce;
 import org.elasticsearch.xpack.esql.expression.predicate.operator.comparison.In;
 import org.elasticsearch.xpack.esql.plan.logical.Enrich;
 import org.elasticsearch.xpack.esql.plan.logical.Eval;
@@ -44,7 +45,6 @@
 import org.elasticsearch.xpack.ql.expression.predicate.logical.Or;
 import org.elasticsearch.xpack.ql.expression.predicate.regex.RegexMatch;
 import org.elasticsearch.xpack.ql.optimizer.OptimizerRules;
-import org.elasticsearch.xpack.ql.optimizer.OptimizerRules.BinaryComparisonSimplification;
 import org.elasticsearch.xpack.ql.optimizer.OptimizerRules.BooleanFunctionEqualsElimination;
 import org.elasticsearch.xpack.ql.optimizer.OptimizerRules.ConstantFolding;
 import org.elasticsearch.xpack.ql.optimizer.OptimizerRules.LiteralsOnTheRight;
@@ -83,9 +83,7 @@
 import static java.util.Collections.singleton;
 import static org.elasticsearch.xpack.esql.expression.NamedExpressions.mergeOutputExpressions;
 import static org.elasticsearch.xpack.ql.expression.Expressions.asAttributes;
-import static org.elasticsearch.xpack.ql.optimizer.OptimizerRules.FoldNull;
 import static org.elasticsearch.xpack.ql.optimizer.OptimizerRules.PropagateEquals;
-import static org.elasticsearch.xpack.ql.optimizer.OptimizerRules.PropagateNullable;
 import static org.elasticsearch.xpack.ql.optimizer.OptimizerRules.TransformDirection;
 
 public class LogicalPlanOptimizer extends ParameterizedRuleExecutor<LogicalPlan, LogicalOptimizerContext> {
@@ -121,12 +119,11 @@ protected static Batch<LogicalPlan> operators() {
             new ConvertStringToByteRef(),
             new FoldNull(),
             new SplitInWithFoldableValue(),
-            new ConstantFolding(),
             new PropagateEvalFoldables(),
+            new ConstantFolding(),
             // boolean
             new BooleanSimplification(),
             new LiteralsOnTheRight(),
-            new BinaryComparisonSimplification(),
             // needs to occur before BinaryComparison combinations (see class)
             new PropagateEquals(),
             new PropagateNullable(),
@@ -1588,4 +1585,26 @@ private static LogicalPlan normalize(Aggregate aggregate, AttributeMap<Expressio
             return changed.get() ? new Aggregate(aggregate.source(), aggregate.child(), aggregate.groupings(), newAggs) : aggregate;
         }
     }
+
+    public static class FoldNull extends OptimizerRules.FoldNull {
+
+        @Override
+        protected Expression tryReplaceIsNullIsNotNull(Expression e) {
+            return e;
+        }
+    }
+
+    public static class PropagateNullable extends OptimizerRules.PropagateNullable {
+
+        protected Expression nullify(Expression exp, Expression nullExp) {
+            if (exp instanceof Coalesce) {
+                List<Expression> newChildren = new ArrayList<>(exp.children());
+                newChildren.removeIf(e -> e.semanticEquals(nullExp));
+                if (newChildren.size() != exp.children().size() && newChildren.size() > 0) { // coalesce needs at least one input
+                    return exp.replaceChildren(newChildren);
+                }
+            }
+            return Literal.of(exp, null);
+        }
+    }
 }
diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/FoldNull.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/FoldNull.java
@@ -8,9 +8,8 @@
 package org.elasticsearch.xpack.esql.optimizer;
 
 import org.elasticsearch.xpack.ql.expression.Expression;
-import org.elasticsearch.xpack.ql.optimizer.OptimizerRules;
 
-public class FoldNull extends OptimizerRules.FoldNull {
+public class FoldNull extends LogicalPlanOptimizer.FoldNull {
     @Override
     public Expression rule(Expression e) {
         return super.rule(e);

diff --git a/.../src/test/java/org/elasticsearch/xpack/esql/optimizer/LocalLogicalPlanOptimizerTests.java b/.../src/test/java/org/elasticsearch/xpack/esql/optimizer/LocalLogicalPlanOptimizerTests.java
@@ -14,18 +14,22 @@
 import org.elasticsearch.xpack.esql.analysis.AnalyzerContext;
 import org.elasticsearch.xpack.esql.expression.function.EsqlFunctionRegistry;
 import org.elasticsearch.xpack.esql.expression.function.scalar.nulls.Coalesce;
+import org.elasticsearch.xpack.esql.expression.predicate.operator.arithmetic.Add;
 import org.elasticsearch.xpack.esql.parser.EsqlParser;
 import org.elasticsearch.xpack.esql.plan.logical.Eval;
 import org.elasticsearch.xpack.esql.plan.logical.local.LocalRelation;
 import org.elasticsearch.xpack.esql.plan.logical.local.LocalSupplier;
 import org.elasticsearch.xpack.esql.stats.SearchStats;
 import org.elasticsearch.xpack.ql.expression.Alias;
+import org.elasticsearch.xpack.ql.expression.Expression;
 import org.elasticsearch.xpack.ql.expression.Expressions;
 import org.elasticsearch.xpack.ql.expression.Literal;
 import org.elasticsearch.xpack.ql.expression.ReferenceAttribute;
+import org.elasticsearch.xpack.ql.expression.predicate.logical.And;
 import org.elasticsearch.xpack.ql.expression.predicate.nulls.IsNotNull;
 import org.elasticsearch.xpack.ql.index.EsIndex;
 import org.elasticsearch.xpack.ql.index.IndexResolution;
+import org.elasticsearch.xpack.ql.optimizer.OptimizerRulesTests;
 import org.elasticsearch.xpack.ql.plan.logical.EsRelation;
 import org.elasticsearch.xpack.ql.plan.logical.Filter;
 import org.elasticsearch.xpack.ql.plan.logical.Limit;
@@ -50,6 +54,10 @@
 import static org.elasticsearch.xpack.esql.EsqlTestUtils.statsForExistingField;
 import static org.elasticsearch.xpack.esql.EsqlTestUtils.statsForMissingField;
 import static org.elasticsearch.xpack.esql.EsqlTestUtils.withDefaultLimitWarning;
+import static org.elasticsearch.xpack.esql.optimizer.LogicalPlanOptimizerTests.getFieldAttribute;
+import static org.elasticsearch.xpack.esql.optimizer.LogicalPlanOptimizerTests.greaterThanOf;
+import static org.elasticsearch.xpack.ql.TestUtils.relation;
+import static org.elasticsearch.xpack.ql.tree.Source.EMPTY;
 import static org.hamcrest.Matchers.contains;
 import static org.hamcrest.Matchers.hasSize;
 import static org.hamcrest.Matchers.is;
@@ -64,6 +72,8 @@ public class LocalLogicalPlanOptimizerTests extends ESTestCase {
     private static Map<String, EsField> mapping;
 
     private static final Literal ONE = L(1);
+    private static final Literal TWO = L(2);
+    private static final Literal THREE = L(3);
 
     @BeforeClass
     public static void init() {
@@ -348,6 +358,73 @@ public void testSparseDocument() throws Exception {
         assertThat(Alias.unwrap(field).fold(), Matchers.nullValue());
     }
 
+    // InferIsNotNull
+
+    public void testIsNotNullOnIsNullField() {
+        EsRelation relation = relation();
+        var fieldA = getFieldAttribute("a");
+        Expression inn = isNotNull(fieldA);
+        Filter f = new Filter(EMPTY, relation, inn);
+
+        assertEquals(f, new LocalLogicalPlanOptimizer.InferIsNotNull().apply(f));
+    }
+
+    public void testIsNotNullOnOperatorWithOneField() {
+        EsRelation relation = relation();
+        var fieldA = getFieldAttribute("a");
+        Expression inn = isNotNull(new Add(EMPTY, fieldA, ONE));
+        Filter f = new Filter(EMPTY, relation, inn);
+        Filter expected = new Filter(EMPTY, relation, new And(EMPTY, isNotNull(fieldA), inn));
+
+        assertEquals(expected, new LocalLogicalPlanOptimizer.InferIsNotNull().apply(f));
+    }
+
+    public void testIsNotNullOnOperatorWithTwoFields() {
+        EsRelation relation = relation();
+        var fieldA = getFieldAttribute("a");
+        var fieldB = getFieldAttribute("b");
+        Expression inn = isNotNull(new Add(EMPTY, fieldA, fieldB));
+        Filter f = new Filter(EMPTY, relation, inn);
+        Filter expected = new Filter(EMPTY, relation, new And(EMPTY, new And(EMPTY, isNotNull(fieldA), isNotNull(fieldB)), inn));
+
+        assertEquals(expected, new LocalLogicalPlanOptimizer.InferIsNotNull().apply(f));
+    }
+
+    public void testIsNotNullOnFunctionWithOneField() {
+        EsRelation relation = relation();
+        var fieldA = getFieldAttribute("a");
+        var pattern = L("abc");
+        Expression inn = isNotNull(
+            new And(
+                EMPTY,
+                new OptimizerRulesTests.TestStartsWith(EMPTY, fieldA, pattern, false),
+                greaterThanOf(new Add(EMPTY, ONE, TWO), THREE)
+            )
+        );
+
+        Filter f = new Filter(EMPTY, relation, inn);
+        Filter expected = new Filter(EMPTY, relation, new And(EMPTY, isNotNull(fieldA), inn));
+
+        assertEquals(expected, new LocalLogicalPlanOptimizer.InferIsNotNull().apply(f));
+    }
+
+    public void testIsNotNullOnFunctionWithTwoFields() {
+        EsRelation relation = relation();
+        var fieldA = getFieldAttribute("a");
+        var fieldB = getFieldAttribute("b");
+        var pattern = L("abc");
+        Expression inn = isNotNull(new OptimizerRulesTests.TestStartsWith(EMPTY, fieldA, fieldB, false));
+
+        Filter f = new Filter(EMPTY, relation, inn);
+        Filter expected = new Filter(EMPTY, relation, new And(EMPTY, new And(EMPTY, isNotNull(fieldA), isNotNull(fieldB)), inn));
+
+        assertEquals(expected, new LocalLogicalPlanOptimizer.InferIsNotNull().apply(f));
+    }
+
+    private IsNotNull isNotNull(Expression field) {
+        return new IsNotNull(EMPTY, field);
+    }
+
     private LocalRelation asEmptyRelation(Object o) {
         var empty = as(o, LocalRelation.class);
         assertThat(empty.supplier(), is(LocalSupplier.EMPTY));