[7.x] QL: Make canonical form take into account children (#71266) (#71287)

Currently the canonical form takes into account only the node itself and
not its children.
For commutative cases this creates subtle issues in that the children
are swapped based on their canonical form but not canonicalize and thus
semantic comparison fail.

This PR fixes that by taking into account the canonical children and,
for commutative expressions, applies semantic ordering.
In the process, improve handling of nested negated expressions.

Author: costin

x-pack/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/expression/predicate/operator/comparison/InsensitiveBinaryComparison.java

@@ -52,11 +52,6 @@ protected TypeResolution resolveInputType(Expression e, ParamOrdinal paramOrdina
      */
     protected abstract String regularOperatorSymbol();
 
-    @Override
-    protected Expression canonicalize() {
-        return left().hashCode() > right().hashCode() ? swapLeftAndRight() : this;
-    }
-
     @Override
     public DataType dataType() {
         return DataTypes.BOOLEAN;

x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/Attribute.java

@@ -116,6 +116,11 @@ public boolean semanticEquals(Expression other) {
         return other instanceof Attribute ? id().equals(((Attribute) other).id()) : false;
     }
 
+    @Override
+    protected Expression canonicalize() {
+        return clone(Source.EMPTY, name(), dataType(), qualifier, nullability, id(), synthetic());
+    }
+
     @Override
     public int hashCode() {
         return Objects.hash(super.hashCode(), qualifier, nullability);

x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/Expression.java

@@ -114,7 +114,15 @@ public final Expression canonical() {
     }
 
     protected Expression canonicalize() {
-        return this;
+        if (children().isEmpty()) {
+            return this;
+        }
+        List<Expression> canonicalChildren = Expressions.canonicalize(children());
+        // check if replacement is really needed
+        if (children().equals(canonicalChildren)) {
+            return this;
+        }
+        return replaceChildrenSameSize(canonicalChildren);
     }
 
     public boolean semanticEquals(Expression other) {

x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/Expressions.java

@@ -118,6 +118,14 @@ public static Nullability nullable(List<? extends Expression> exps) {
         return value;
     }
 
+    public static List<Expression> canonicalize(List<? extends Expression> exps) {
+        List<Expression> canonical = new ArrayList<>(exps.size());
+        for (Expression exp : exps) {
+            canonical.add(exp.canonical());
+        }
+        return canonical;
+    }
+
     public static boolean foldable(List<? extends Expression> exps) {
         for (Expression exp : exps) {
             if (exp.foldable() == false) {

x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/LeafExpression.java

@@ -26,4 +26,9 @@ public final Expression replaceChildren(List<Expression> newChildren) {
     public AttributeSet references() {
         return AttributeSet.EMPTY;
     }
+
+    @Override
+    protected Expression canonicalize() {
+        return this;
+    }
 }

x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/Literal.java

@@ -68,7 +68,12 @@ public Object fold() {
 
     @Override
     public int hashCode() {
-        return Objects.hash(value, dataType);
+        return Objects.hash(dataType, value);
+    }
+
+    @Override
+    protected Expression canonicalize() {
+        return this;
     }
 
     @Override

x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/BinaryOperator.java

@@ -11,6 +11,10 @@
 import org.elasticsearch.xpack.ql.expression.Expressions.ParamOrdinal;
 import org.elasticsearch.xpack.ql.tree.Source;
 
+import java.util.ArrayList;
+import java.util.Iterator;
+import java.util.List;
+
 /**
  * Operator is a specialized binary predicate where both sides have the compatible types
  * (it's up to the analyzer to do any conversion if needed).
@@ -38,8 +42,56 @@ protected TypeResolution resolveType() {
         return resolveInputType(right(), ParamOrdinal.SECOND);
     }
 
+    protected boolean isCommutative() {
+        return false;
+    }
+
     @Override
     protected Expression canonicalize() {
-        return left().semanticHash() > right().semanticHash() ? swapLeftAndRight() : this;
+        // fast check
+        if (isCommutative() == false) {
+            Expression exp = left().semanticHash() > right().semanticHash() ? swapLeftAndRight() : this;
+            // swap is not guaranteed to return a different expression, in which case simply delegate to super to avoid a cycle
+            return exp != this ? exp.canonical() : super.canonicalize();
+        }
+        // break down all connected commutative operators
+        // in order to sort all their children at once
+        // then reassemble/reduce back the expression
+        List<Expression> commutativeChildren = new ArrayList<>(2);
+        collectCommutative(commutativeChildren, this);
+        // sort
+        commutativeChildren.sort((l, r) -> Integer.compare(l.semanticHash(), r.semanticHash()));
+
+        // reduce all children using the current operator - this method creates a balanced tree
+        while (commutativeChildren.size() > 1) {
+            // combine (in place) expressions in pairs
+            // NB: this loop modifies the list (just like an array)
+            for (int i = 0; i < commutativeChildren.size() - 1; i++) {
+                // reduce two children into one and moves to the next pair
+                Expression current = commutativeChildren.get(i);
+                Expression next = commutativeChildren.remove(i + 1);
+                // do the update in place to minimize the amount of array modifications
+                commutativeChildren.set(i, replaceChildren(current, next));
+
+            }
+        }
+        Iterator<Expression> iterator = commutativeChildren.iterator();
+        Expression last = iterator.next();
+        while (iterator.hasNext()) {
+            last = replaceChildren(last, iterator.next());
+        }
+        return last;
+    }
+
+    protected void collectCommutative(List<Expression> commutative, Expression expression) {
+        // keep digging for same binary operator
+        if (getClass() == expression.getClass()) {
+            BinaryOperator<?, ?, ?, ?> bi = (BinaryOperator<?, ?, ?, ?>) expression;
+            collectCommutative(commutative, bi.left());
+            collectCommutative(commutative, bi.right());
+        } else {
+            // not same operation - no ordering possible
+            commutative.add(expression.canonical());
+        }
     }
 }

x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/Negatable.java

@@ -6,9 +6,9 @@
  */
 package org.elasticsearch.xpack.ql.expression.predicate;
 
-import org.elasticsearch.xpack.ql.expression.function.scalar.ScalarFunction;
+import org.elasticsearch.xpack.ql.expression.Expression;
 
-public interface Negatable<T extends ScalarFunction> {
+public interface Negatable<T extends Expression> {
 
     T negate();
 

x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/Predicates.java

@@ -76,9 +76,11 @@ private static Expression combine(List<Expression> exps, BiFunction<Expression,
             // combine (in place) expressions in pairs
             // NB: this loop modifies the list (just like an array)
             for (int i = 0; i < result.size() - 1; i++) {
-                Expression l = result.remove(i);
-                Expression r = result.remove(i);
-                result.add(i, combiner.apply(l, r));
+                // keep the current element to update it in place
+                Expression l = result.get(i);
+                // remove the next element due to combining
+                Expression r = result.remove(i + 1);
+                result.set(i, combiner.apply(l, r));
             }
         }
 

x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/logical/And.java

@@ -36,7 +36,7 @@ public And swapLeftAndRight() {
 
     @Override
     public Or negate() {
-        return new Or(source(), new Not(source(), left()), new Not(source(), right()));
+        return new Or(source(), Not.negate(left()), Not.negate(right()));
     }
 
     @Override