Reduce creation of duplicates

costin · Dec 12, 2023 · d67c709 · d67c709
1 parent f57daef
commit d67c709
Show file tree

Hide file tree

Showing 2 changed files with 29 additions and 19 deletions.
diff --git a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/optimizer/OptimizerRules.java b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/optimizer/OptimizerRules.java
@@ -71,6 +71,7 @@
 
 import static java.lang.Math.signum;
 import static java.util.Arrays.asList;
+import static java.util.Collections.emptySet;
 import static org.elasticsearch.xpack.ql.expression.Literal.FALSE;
 import static org.elasticsearch.xpack.ql.expression.Literal.TRUE;
 import static org.elasticsearch.xpack.ql.expression.predicate.Predicates.combineAnd;
@@ -1798,8 +1799,8 @@ protected Expression nonNullify(Expression exp, Expression nonNullExp) {
      * This handles the case of fields nested inside functions or expressions in order to avoid:
      * - having to evaluate the whole expression
      * - not pushing down the filter due to expression evaluation
-     * IS NULL cannot be simplified since it leads to a disjunction which cannot prevents the simplified IS NULL
-     * to be pushed down:
+     * IS NULL cannot be simplified since it leads to a disjunction which prevents the filter to be
+     * pushed down:
      * (x + 1) IS NULL --> x IS NULL OR x + 1 IS NULL
      * and x IS NULL cannot be pushed down
      * <br/>
@@ -1843,30 +1844,30 @@ private Expression inferNotNullable(IsNotNull inn, AttributeMap<Expression> alia
          */
         protected Set<Expression> resolveExpressionAsRootAttributes(Expression exp, AttributeMap<Expression> aliases) {
             Set<Expression> resolvedExpressions = new LinkedHashSet<>();
-            doResolve(exp, aliases, resolvedExpressions);
-            return resolvedExpressions;
+            boolean changed = doResolve(exp, aliases, resolvedExpressions);
+            return changed ? resolvedExpressions : emptySet();
         }
 
-        private void doResolve(Expression exp, AttributeMap<Expression> aliases, Set<Expression> resolvedExpressions) {
-            if (skipExpression(exp)) {
+        private boolean doResolve(Expression exp, AttributeMap<Expression> aliases, Set<Expression> resolvedExpressions) {
+            boolean changed = false;
+            // check if the expression can be skipped or is not nullabe
+            if (skipExpression(exp) || exp.nullable() == Nullability.FALSE) {
                 resolvedExpressions.add(exp);
             } else {
-                // expression might be nullable, go after it
-                if (exp.nullable() != Nullability.FALSE) {
-                    for (Expression e : exp.references()) {
-                        Expression resolved = aliases.resolve(e, e);
-                        // found a root attribute, bail out
-                        if (resolved instanceof Attribute a && resolved == e) {
-                            resolvedExpressions.add(a);
-                        } else {
-                            // go further
-                            doResolve(resolved, aliases, resolvedExpressions);
-                        }
+                for (Expression e : exp.references()) {
+                    Expression resolved = aliases.resolve(e, e);
+                    // found a root attribute, bail out
+                    if (resolved instanceof Attribute a && resolved == e) {
+                        resolvedExpressions.add(a);
+                        // don't mark things as change if the original expression hasn't been broken down
+                        changed |= resolved != exp;
+                    } else {
+                        // go further
+                        changed |= doResolve(resolved, aliases, resolvedExpressions);
                     }
-                } else {
-                    resolvedExpressions.add(exp);
                 }
             }
+            return changed;
         }
 
         protected boolean skipExpression(Expression e) {

diff --git a/x-pack/plugin/ql/src/test/java/org/elasticsearch/xpack/ql/optimizer/OptimizerRulesTests.java b/x-pack/plugin/ql/src/test/java/org/elasticsearch/xpack/ql/optimizer/OptimizerRulesTests.java
@@ -1771,6 +1771,15 @@ public void testPushDownFilterThroughAgg() throws Exception {
         assertEquals(expected, new PushDownAndCombineFilters().apply(fb));
     }
 
+    public void testIsNotNullOnIsNullField() {
+        EsRelation relation = relation();
+        var fieldA = getFieldAttribute("a");
+        Expression inn = isNotNull(fieldA);
+        Filter f = new Filter(EMPTY, relation, inn);
+
+        assertEquals(f, new OptimizerRules.InferIsNotNull().apply(f));
+    }
+
     public void testIsNotNullOnOperatorWithOneField() {
         EsRelation relation = relation();
         var fieldA = getFieldAttribute("a");