diff --git a/x-pack/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/expression/predicate/operator/comparison/InsensitiveBinaryComparison.java b/x-pack/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/expression/predicate/operator/comparison/InsensitiveBinaryComparison.java index ec6016d3b37dd..75a91dcaef359 100644 --- a/x-pack/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/expression/predicate/operator/comparison/InsensitiveBinaryComparison.java +++ b/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; diff --git a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/Attribute.java b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/Attribute.java index 2e74197db6e76..0eab3d834a905 100644 --- a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/Attribute.java +++ b/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); diff --git a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/Expression.java b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/Expression.java index 0d309a753f4b3..5f56694934f1b 100644 --- a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/Expression.java +++ b/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 canonicalChildren = Expressions.canonicalize(children()); + // check if replacement is really needed + if (children().equals(canonicalChildren)) { + return this; + } + return replaceChildrenSameSize(canonicalChildren); } public boolean semanticEquals(Expression other) { diff --git a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/Expressions.java b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/Expressions.java index b8e867653528d..2eee3fcd818e0 100644 --- a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/Expressions.java +++ b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/Expressions.java @@ -97,6 +97,14 @@ public static Nullability nullable(List exps) { return Nullability.and(exps.stream().map(Expression::nullable).toArray(Nullability[]::new)); } + public static List canonicalize(List exps) { + List canonical = new ArrayList<>(exps.size()); + for (Expression exp : exps) { + canonical.add(exp.canonical()); + } + return canonical; + } + public static boolean foldable(List exps) { for (Expression exp : exps) { if (exp.foldable() == false) { diff --git a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/LeafExpression.java b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/LeafExpression.java index 99ca23f4c07fe..df762d18ba9c7 100644 --- a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/LeafExpression.java +++ b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/LeafExpression.java @@ -26,4 +26,9 @@ public final Expression replaceChildren(List newChildren) { public AttributeSet references() { return AttributeSet.EMPTY; } + + @Override + protected Expression canonicalize() { + return this; + } } diff --git a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/Literal.java b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/Literal.java index e89076179c7f0..6acadee0e0b43 100644 --- a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/Literal.java +++ b/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 diff --git a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/BinaryOperator.java b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/BinaryOperator.java index aac195afb3497..44059928e6098 100644 --- a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/BinaryOperator.java +++ b/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 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 iterator = commutativeChildren.iterator(); + Expression last = iterator.next(); + while (iterator.hasNext()) { + last = replaceChildren(last, iterator.next()); + } + return last; + } + + protected void collectCommutative(List 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()); + } } } diff --git a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/Negatable.java b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/Negatable.java index c9495d1e5f673..b7305c281d7b5 100644 --- a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/Negatable.java +++ b/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 { +public interface Negatable { T negate(); diff --git a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/Predicates.java b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/Predicates.java index 47472318ac705..5c5883765873c 100644 --- a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/Predicates.java +++ b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/Predicates.java @@ -76,9 +76,11 @@ private static Expression combine(List exps, BiFunction { 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); + } } diff --git a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/logical/Or.java b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/logical/Or.java index d4d1d5e09a102..a7b027d6d1ae7 100644 --- a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/logical/Or.java +++ b/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())); } } diff --git a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/operator/arithmetic/Add.java b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/operator/arithmetic/Add.java index a075b951b7bb6..7a526136bd36a 100644 --- a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/operator/arithmetic/Add.java +++ b/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; + } } diff --git a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/operator/arithmetic/Mul.java b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/operator/arithmetic/Mul.java index 61c2946d7dd8e..3aaa860401788 100644 --- a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/operator/arithmetic/Mul.java +++ b/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; + } } diff --git a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/operator/comparison/Equals.java b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/operator/comparison/Equals.java index ff95ef6f446bd..88ccf4cf19baf 100644 --- a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/operator/comparison/Equals.java +++ b/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; + } } diff --git a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/operator/comparison/In.java b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/operator/comparison/In.java index 490021d7f8290..27311ef5d6f23 100644 --- a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/operator/comparison/In.java +++ b/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 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); diff --git a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/operator/comparison/NotEquals.java b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/operator/comparison/NotEquals.java index 777dc1056118c..e2605541e50f4 100644 --- a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/operator/comparison/NotEquals.java +++ b/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; + } } diff --git a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/operator/comparison/NullEquals.java b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/operator/comparison/NullEquals.java index 0767b31d7da53..2a3a989b7ba98 100644 --- a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/operator/comparison/NullEquals.java +++ b/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; + } } diff --git a/x-pack/plugin/ql/src/test/java/org/elasticsearch/xpack/ql/expression/CanonicalTests.java b/x-pack/plugin/ql/src/test/java/org/elasticsearch/xpack/ql/expression/CanonicalTests.java new file mode 100644 index 0000000000000..502746ae0775d --- /dev/null +++ b/x-pack/plugin/ql/src/test/java/org/elasticsearch/xpack/ql/expression/CanonicalTests.java @@ -0,0 +1,260 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0; you may not use this file except in compliance with the Elastic License + * 2.0. + */ + +package org.elasticsearch.xpack.ql.expression; + +import org.elasticsearch.test.ESTestCase; +import org.elasticsearch.xpack.ql.TestUtils; +import org.elasticsearch.xpack.ql.expression.predicate.BinaryOperator; +import org.elasticsearch.xpack.ql.expression.predicate.Predicates; +import org.elasticsearch.xpack.ql.expression.predicate.logical.And; +import org.elasticsearch.xpack.ql.expression.predicate.logical.Not; +import org.elasticsearch.xpack.ql.expression.predicate.logical.Or; +import org.elasticsearch.xpack.ql.expression.predicate.operator.arithmetic.Add; +import org.elasticsearch.xpack.ql.expression.predicate.operator.arithmetic.Div; +import org.elasticsearch.xpack.ql.expression.predicate.operator.arithmetic.Mod; +import org.elasticsearch.xpack.ql.expression.predicate.operator.arithmetic.Mul; +import org.elasticsearch.xpack.ql.expression.predicate.operator.arithmetic.Sub; +import org.elasticsearch.xpack.ql.expression.predicate.operator.comparison.Equals; +import org.elasticsearch.xpack.ql.expression.predicate.operator.comparison.GreaterThan; +import org.elasticsearch.xpack.ql.expression.predicate.operator.comparison.GreaterThanOrEqual; +import org.elasticsearch.xpack.ql.expression.predicate.operator.comparison.In; +import org.elasticsearch.xpack.ql.expression.predicate.operator.comparison.LessThan; +import org.elasticsearch.xpack.ql.expression.predicate.operator.comparison.LessThanOrEqual; +import org.elasticsearch.xpack.ql.expression.predicate.operator.comparison.NotEquals; +import org.elasticsearch.xpack.ql.tree.Source; +import org.elasticsearch.xpack.ql.type.DataType; +import org.elasticsearch.xpack.ql.type.DataTypes; + +import java.time.ZoneId; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collections; +import java.util.Comparator; +import java.util.List; +import java.util.function.Function; + +import static java.util.Arrays.asList; +import static org.elasticsearch.xpack.ql.TestUtils.equalsOf; +import static org.elasticsearch.xpack.ql.TestUtils.fieldAttribute; +import static org.elasticsearch.xpack.ql.TestUtils.greaterThanOf; +import static org.elasticsearch.xpack.ql.TestUtils.greaterThanOrEqualOf; +import static org.elasticsearch.xpack.ql.TestUtils.lessThanOf; +import static org.elasticsearch.xpack.ql.TestUtils.notEqualsOf; +import static org.elasticsearch.xpack.ql.TestUtils.of; +import static org.elasticsearch.xpack.ql.tree.Source.EMPTY; + +public class CanonicalTests extends ESTestCase { + + Comparator c = Comparator.comparingInt(Object::hashCode); + + public void testNonCommutativeBinary() throws Exception { + Div div = new Div(EMPTY, of(2), of(1)); + Sub sub = new Sub(EMPTY, of(2), of(1)); + Mod mod = new Mod(EMPTY, div, sub); + assertEquals(mod, mod.canonical()); + } + + public void testNonCommutativeMixedWithCommutative() throws Exception { + // (0+1) / 1 + Div div = new Div(EMPTY, new Add(EMPTY, of(0), of(1)), of(1)); + // 1*2 - 1+2 + Sub sub = new Sub(EMPTY, new Mul(EMPTY, of(1), new Add(EMPTY, of(1), of(2))), new Add(EMPTY, of(1), of(2))); + + Div shuffledDiv = new Div(EMPTY, new Add(EMPTY, of(1), of(0)), of(1)); + Sub shuffledSub = new Sub(EMPTY, new Mul(EMPTY, new Add(EMPTY, of(2), of(1)), of(1)), new Add(EMPTY, of(2), of(1))); + + And and = new And(EMPTY, div, sub); + And shuffledAnd = new And(EMPTY, shuffledDiv, shuffledSub); + + assertNotEquals(and, shuffledAnd); + assertEquals(and.canonical(), shuffledAnd.canonical()); + } + + public void testAndManually() throws Exception { + FieldAttribute a = fieldAttribute(); + FieldAttribute b = fieldAttribute(); + FieldAttribute c = fieldAttribute(); + FieldAttribute d = fieldAttribute(); + And one = new And(EMPTY, new And(EMPTY, a, b), new And(EMPTY, c, d)); + And two = new And(EMPTY, new And(EMPTY, c, a), new And(EMPTY, b, d)); + + assertEquals(one.canonical(), two.canonical()); + assertEquals(one.semanticHash(), two.semanticHash()); + } + + public void testBasicSymmetricalAdd() throws Exception { + Expression left = new Add(EMPTY, new Add(EMPTY, of(1), of(2)), new Add(EMPTY, of(3), of(4))); + Expression right = new Add(EMPTY, new Add(EMPTY, of(4), of(2)), new Add(EMPTY, of(1), of(3))); + + assertEquals(left.canonical(), right.canonical()); + assertEquals(left.semanticHash(), right.semanticHash()); + } + + public void testBasicASymmetricalAdd() throws Exception { + Expression left = new Add(EMPTY, new Add(EMPTY, of(1), of(2)), of(3)); + Expression right = new Add(EMPTY, of(1), new Add(EMPTY, of(2), of(3))); + + assertEquals(left.canonical(), right.canonical()); + assertEquals(left.semanticHash(), right.semanticHash()); + } + + public void testBasicAnd() throws Exception { + testBinaryLogic(Predicates::combineAnd); + } + + public void testBasicOr() throws Exception { + testBinaryLogic(Predicates::combineAnd); + } + + private void testBinaryLogic(Function, Expression> combiner) { + List children = randomList(2, 128, TestUtils::fieldAttribute); + Expression expression = combiner.apply(children); + Collections.shuffle(children, random()); + Expression shuffledExpression = combiner.apply(children); + assertTrue(expression.semanticEquals(shuffledExpression)); + assertEquals(expression.semanticHash(), shuffledExpression.semanticHash()); + } + + public void testBinaryOperatorCombinations() throws Exception { + FieldAttribute a = fieldAttribute(); + FieldAttribute b = fieldAttribute(); + FieldAttribute c = fieldAttribute(); + FieldAttribute d = fieldAttribute(); + + And ab = new And(EMPTY, greaterThanOf(a, of(1)), lessThanOf(b, of(2))); + And cd = new And(EMPTY, equalsOf(new Add(EMPTY, c, of(20)), of(3)), greaterThanOrEqualOf(d, of(4))); + + And and = new And(EMPTY, ab, cd); + + // swap d comparison + And db = new And(EMPTY, greaterThanOrEqualOf(d, of(4)).swapLeftAndRight(), lessThanOf(b, of(2))); + // swap order for c and swap a comparison + And ca = new And(EMPTY, equalsOf(new Add(EMPTY, of(20), c), of(3)), greaterThanOf(a, of(1))); + + And shuffleAnd = new And(EMPTY, db, ca); + + assertEquals(and.canonical(), shuffleAnd.canonical()); + } + + public void testNot() throws Exception { + FieldAttribute a = fieldAttribute(); + FieldAttribute b = fieldAttribute(); + FieldAttribute c = fieldAttribute(); + FieldAttribute d = fieldAttribute(); + + And ab = new And(EMPTY, greaterThanOf(a, of(1)), lessThanOf(b, of(2))); + And cd = new And(EMPTY, equalsOf(new Add(EMPTY, c, of(20)), of(3)), greaterThanOrEqualOf(d, of(4))); + And and = new And(EMPTY, ab, cd); + + // swap d comparison + Or db = new Or(EMPTY, new Not(EMPTY, greaterThanOrEqualOf(d, of(4))), lessThanOf(b, of(2)).negate()); + // swap order for c and swap a comparison + Or ca = new Or(EMPTY, notEqualsOf(new Add(EMPTY, of(20), c), of(3)), new Not(EMPTY, greaterThanOf(a, of(1)))); + + Not not = new Not(EMPTY, new Or(EMPTY, db, ca)); + + Expression expected = and.canonical(); + Expression actual = not.canonical(); + assertEquals(and.canonical(), not.canonical()); + } + + public void testLiteralHashSorting() throws Exception { + DataType type = randomFrom(DataTypes.types()); + List list = randomList(10, 1024, () -> new Literal(EMPTY, randomInt(), type)); + List shuffle = new ArrayList<>(list); + Collections.shuffle(shuffle, random()); + + assertNotEquals(list, shuffle); + + list.sort(c); + shuffle.sort(c); + + assertEquals(list, shuffle); + } + + public void testInManual() throws Exception { + FieldAttribute value = fieldAttribute(); + + Literal a = new Literal(EMPTY, 1, DataTypes.INTEGER); + Literal b = new Literal(EMPTY, 2, DataTypes.INTEGER); + Literal c = new Literal(EMPTY, 3, DataTypes.INTEGER); + + In in = new In(EMPTY, value, asList(a, b, c)); + In anotherIn = new In(EMPTY, value, asList(b, a, c)); + + assertTrue(in.semanticEquals(anotherIn)); + assertEquals(in.semanticHash(), anotherIn.semanticHash()); + } + + public void testIn() throws Exception { + FieldAttribute value = fieldAttribute(); + List list = randomList(randomInt(1024), () -> new Literal(EMPTY, randomInt(), DataTypes.INTEGER)); + In in = new In(EMPTY, value, list); + List shuffledList = new ArrayList<>(list); + Collections.shuffle(shuffledList, random()); + In shuffledIn = new In(EMPTY, value, shuffledList); + + assertEquals(in.semanticHash(), shuffledIn.semanticHash()); + assertTrue(in.semanticEquals(shuffledIn)); + } + + interface BinaryOperatorFactory { + BinaryOperator create(Source source, Expression left, Expression right); + } + + public void testBasicOperators() throws Exception { + List list = Arrays.asList( + // arithmetic + Add::new, + Mul::new, + // logical + Or::new, + And::new + ); + + for (BinaryOperatorFactory factory : list) { + Literal left = of(randomInt()); + Literal right = of(randomInt()); + + BinaryOperator first = factory.create(Source.EMPTY, left, right); + BinaryOperator second = factory.create(Source.EMPTY, right, left); + + assertNotEquals(first, second); + assertTrue(first.semanticEquals(second)); + assertEquals(first, second.swapLeftAndRight()); + assertEquals(second, first.swapLeftAndRight()); + } + } + + interface BinaryOperatorWithTzFactory { + BinaryOperator create(Source source, Expression left, Expression right, ZoneId zoneId); + } + + public void testTimeZoneOperators() throws Exception { + List list = Arrays.asList( + LessThan::new, + LessThanOrEqual::new, + Equals::new, + NotEquals::new, + GreaterThan::new, + GreaterThanOrEqual::new + ); + + for (BinaryOperatorWithTzFactory factory : list) { + Literal left = of(randomInt()); + Literal right = of(randomInt()); + ZoneId zoneId = randomZone(); + + BinaryOperator first = factory.create(Source.EMPTY, left, right, zoneId); + BinaryOperator swap = first.swapLeftAndRight(); + + assertNotEquals(first, swap); + assertTrue(first.semanticEquals(swap)); + } + } +} diff --git a/x-pack/plugin/ql/src/test/java/org/elasticsearch/xpack/ql/expression/predicate/CanonicalizeTests.java b/x-pack/plugin/ql/src/test/java/org/elasticsearch/xpack/ql/expression/predicate/CanonicalizeTests.java deleted file mode 100644 index 94558218b6752..0000000000000 --- a/x-pack/plugin/ql/src/test/java/org/elasticsearch/xpack/ql/expression/predicate/CanonicalizeTests.java +++ /dev/null @@ -1,83 +0,0 @@ -/* - * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one - * or more contributor license agreements. Licensed under the Elastic License - * 2.0; you may not use this file except in compliance with the Elastic License - * 2.0. - */ - -package org.elasticsearch.xpack.ql.expression.predicate; - -import org.elasticsearch.test.ESTestCase; -import org.elasticsearch.xpack.ql.expression.Expression; -import org.elasticsearch.xpack.ql.expression.Literal; -import org.elasticsearch.xpack.ql.expression.predicate.logical.And; -import org.elasticsearch.xpack.ql.expression.predicate.logical.Or; -import org.elasticsearch.xpack.ql.expression.predicate.operator.arithmetic.Add; -import org.elasticsearch.xpack.ql.expression.predicate.operator.arithmetic.Mul; -import org.elasticsearch.xpack.ql.expression.predicate.operator.comparison.Equals; -import org.elasticsearch.xpack.ql.expression.predicate.operator.comparison.GreaterThan; -import org.elasticsearch.xpack.ql.expression.predicate.operator.comparison.GreaterThanOrEqual; -import org.elasticsearch.xpack.ql.expression.predicate.operator.comparison.LessThan; -import org.elasticsearch.xpack.ql.expression.predicate.operator.comparison.LessThanOrEqual; -import org.elasticsearch.xpack.ql.expression.predicate.operator.comparison.NotEquals; -import org.elasticsearch.xpack.ql.tree.Source; - -import java.time.ZoneId; -import java.util.Arrays; -import java.util.List; - -import static org.elasticsearch.xpack.ql.TestUtils.of; - -public class CanonicalizeTests extends ESTestCase { - - interface BinaryOperatorFactory { - BinaryOperator create(Source source, Expression left, Expression right); - } - - public void testBasicOperators() throws Exception { - List list = Arrays.asList( - // arithmetic - Add::new, Mul::new, - // logical - Or::new, And::new - ); - - for (BinaryOperatorFactory factory : list) { - Literal left = of(randomInt()); - Literal right = of(randomInt()); - - BinaryOperator first = factory.create(Source.EMPTY, left, right); - BinaryOperator second = factory.create(Source.EMPTY, right, left); - - assertNotEquals(first, second); - assertTrue(first.semanticEquals(second)); - assertEquals(first, second.swapLeftAndRight()); - assertEquals(second, first.swapLeftAndRight()); - } - } - - interface BinaryOperatorWithTzFactory { - BinaryOperator create(Source source, Expression left, Expression right, ZoneId zoneId); - } - - public void testTimeZoneOperators() throws Exception { - List list = Arrays.asList( - LessThan::new, LessThanOrEqual::new, - Equals::new, NotEquals::new, - GreaterThan::new, GreaterThanOrEqual::new - ); - - for (BinaryOperatorWithTzFactory factory : list) { - Literal left = of(randomInt()); - Literal right = of(randomInt()); - ZoneId zoneId = randomZone(); - - - BinaryOperator first = factory.create(Source.EMPTY, left, right, zoneId); - BinaryOperator swap = first.swapLeftAndRight(); - - assertNotEquals(first, swap); - assertTrue(first.semanticEquals(swap)); - } - } -} diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/operator/arithmetic/Add.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/operator/arithmetic/Add.java index 0a799abb75941..c10859473c403 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/operator/arithmetic/Add.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/operator/arithmetic/Add.java @@ -38,4 +38,9 @@ public Add swapLeftAndRight() { public ArithmeticOperationFactory binaryComparisonInverse() { return Sub::new; } + + @Override + protected boolean isCommutative() { + return true; + } } diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/operator/arithmetic/Mul.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/operator/arithmetic/Mul.java index 1204e3f5cfddd..13cdc3831dd2f 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/operator/arithmetic/Mul.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/operator/arithmetic/Mul.java @@ -70,6 +70,7 @@ protected Mul replaceChildren(Expression newLeft, Expression newRight) { return new Mul(source(), newLeft, newRight); } + @Override public Mul swapLeftAndRight() { return new Mul(source(), right(), left()); } @@ -78,4 +79,9 @@ public Mul swapLeftAndRight() { public ArithmeticOperationFactory binaryComparisonInverse() { return Div::new; } + + @Override + protected boolean isCommutative() { + return true; + } }