QL: Make canonical form take into account children

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

@@ -97,6 +97,14 @@ public static Nullability nullable(List<? extends Expression> exps) {
         return Nullability.and(exps.stream().map(Expression::nullable).toArray(Nullability[]::new));
     }
 
+    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

@@ -35,6 +35,6 @@ 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()));
     }
 }

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

@@ -44,4 +44,9 @@ public Nullability nullable() {
         // Cannot fold null due to 3vl, constant folding will do any possible folding.
         return Nullability.UNKNOWN;
     }
+
+    @Override
+    protected boolean isCommutative() {
+        return true;
+    }
 }

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

@@ -19,7 +19,7 @@
 
 import static org.elasticsearch.xpack.ql.expression.TypeResolutions.isBoolean;
 
-public class Not extends UnaryScalarFunction {
+public class Not extends UnaryScalarFunction implements Negatable<Expression> {
 
     public Not(Source source, Expression child) {
         super(source, child);
@@ -60,15 +60,23 @@ public String processScript(String script) {
 
     @Override
     protected Expression canonicalize() {
-        Expression canonicalChild = field().canonical();
-        if (canonicalChild instanceof Negatable) {
-            return ((Negatable) canonicalChild).negate();
+        if (field() instanceof Negatable) {
+            return ((Negatable) field()).negate().canonical();
         }
-        return this;
+        return super.canonicalize();
+    }
+
+    @Override
+    public Expression negate() {
+        return field();
     }
 
     @Override
     public DataType dataType() {
         return DataTypes.BOOLEAN;
     }
+
+    static Expression negate(Expression exp) {
+        return exp instanceof Negatable ? ((Negatable) exp).negate() : new Not(exp.source(), exp);
+    }
 }

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

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

x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/operator/arithmetic/Add.java

@@ -28,6 +28,7 @@ protected Add replaceChildren(Expression left, Expression right) {
         return new Add(source(), left, right);
     }
 
+    @Override
     public Add swapLeftAndRight() {
         return new Add(source(), right(), left());
     }
@@ -36,4 +37,9 @@ public Add swapLeftAndRight() {
     public ArithmeticOperationFactory binaryComparisonInverse() {
         return Sub::new;
     }
+
+    @Override
+    protected boolean isCommutative() {
+        return true;
+    }
 }

x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/operator/arithmetic/Mul.java

@@ -50,6 +50,7 @@ protected Mul replaceChildren(Expression newLeft, Expression newRight) {
         return new Mul(source(), newLeft, newRight);
     }
 
+    @Override
     public Mul swapLeftAndRight() {
         return new Mul(source(), right(), left());
     }
@@ -58,4 +59,9 @@ public Mul swapLeftAndRight() {
     public ArithmeticOperationFactory binaryComparisonInverse() {
         return Div::new;
     }
+
+    @Override
+    protected boolean isCommutative() {
+        return true;
+    }
 }

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

@@ -48,4 +48,9 @@ public BinaryComparison negate() {
     public BinaryComparison reverse() {
         return this;
     }
+
+    @Override
+    protected boolean isCommutative() {
+        return true;
+    }
 }

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

@@ -23,6 +23,7 @@
 
 import java.time.ZoneId;
 import java.util.ArrayList;
+import java.util.Collections;
 import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Objects;
@@ -96,6 +97,14 @@ public Boolean fold() {
         return InProcessor.apply(value.fold(), foldAndConvertListOfValues(list, value.dataType()));
     }
 
+    @Override
+    protected Expression canonicalize() {
+        // order values for commutative operators
+        List<Expression> canonicalValues = Expressions.canonicalize(list);
+        Collections.sort(canonicalValues, (l, r) -> Integer.compare(l.hashCode(), r.hashCode()));
+        return new In(source(), value, canonicalValues, zoneId);
+    }
+
     @Override
     public ScriptTemplate asScript() {
         ScriptTemplate leftScript = asScript(value);

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

@@ -44,4 +44,9 @@ public BinaryComparison negate() {
     public BinaryComparison reverse() {
         return this;
     }
+
+    @Override
+    protected boolean isCommutative() {
+        return true;
+    }
 }

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

@@ -47,4 +47,9 @@ public Nullability nullable() {
     public BinaryComparison reverse() {
         return this;
     }
+
+    @Override
+    protected boolean isCommutative() {
+        return true;
+    }
 }