From 25ee71ae2d72defc54ef2e187821f2628fb0de69 Mon Sep 17 00:00:00 2001 From: Costin Leau Date: Thu, 1 Apr 2021 20:24:57 +0300 Subject: [PATCH 1/3] QL: Make canonical form take into account children 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. --- .../InsensitiveBinaryComparison.java | 5 - .../xpack/ql/expression/Attribute.java | 6 + .../xpack/ql/expression/Expression.java | 10 +- .../xpack/ql/expression/Expressions.java | 8 + .../xpack/ql/expression/LeafExpression.java | 5 + .../xpack/ql/expression/Literal.java | 7 +- .../expression/predicate/BinaryOperator.java | 53 +++- .../ql/expression/predicate/Negatable.java | 4 +- .../ql/expression/predicate/logical/And.java | 2 +- .../predicate/logical/BinaryLogic.java | 5 + .../ql/expression/predicate/logical/Not.java | 18 +- .../ql/expression/predicate/logical/Or.java | 2 +- .../predicate/operator/arithmetic/Add.java | 6 + .../predicate/operator/arithmetic/Mul.java | 6 + .../predicate/operator/comparison/Equals.java | 5 + .../predicate/operator/comparison/In.java | 9 + .../operator/comparison/NotEquals.java | 5 + .../operator/comparison/NullEquals.java | 5 + .../xpack/ql/expression/CanonicalTests.java | 260 ++++++++++++++++++ .../predicate/CanonicalizeTests.java | 83 ------ .../predicate/operator/arithmetic/Add.java | 5 + .../predicate/operator/arithmetic/Mul.java | 6 + 22 files changed, 415 insertions(+), 100 deletions(-) create mode 100644 x-pack/plugin/ql/src/test/java/org/elasticsearch/xpack/ql/expression/CanonicalTests.java delete mode 100644 x-pack/plugin/ql/src/test/java/org/elasticsearch/xpack/ql/expression/predicate/CanonicalizeTests.java 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..9ed9240edf90f 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 @@ -6,6 +6,7 @@ */ package org.elasticsearch.xpack.ql.expression; +import joptsimple.internal.Strings; import org.elasticsearch.xpack.ql.tree.Source; import org.elasticsearch.xpack.ql.type.DataType; @@ -116,6 +117,11 @@ public boolean semanticEquals(Expression other) { return other instanceof Attribute ? id().equals(((Attribute) other).id()) : false; } + @Override + protected Expression canonicalize() { + return clone(Source.EMPTY, Strings.EMPTY, dataType(), Strings.EMPTY, 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..c83fa2c87c901 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,55 @@ 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 next = commutativeChildren.remove(i + 1); + // do the update in place to minimize the amount of array modifications + commutativeChildren.set(i, replaceChildren(commutativeChildren.get(i), 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/logical/And.java b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/logical/And.java index e6e8f71c4b6a2..8cec2c5552876 100644 --- a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/logical/And.java +++ b/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())); } } diff --git a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/logical/BinaryLogic.java b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/logical/BinaryLogic.java index 3f806b98310d9..6cf9cdcf3b5c4 100644 --- a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/logical/BinaryLogic.java +++ b/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; + } } diff --git a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/logical/Not.java b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/logical/Not.java index 41f82ab7f9d9d..335f2f8456bd6 100644 --- a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/logical/Not.java +++ b/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 { 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; + } } From 102a86b77f3ca7c95686e6c2c4febc3041f352d8 Mon Sep 17 00:00:00 2001 From: Costin Leau Date: Sat, 3 Apr 2021 13:10:35 +0300 Subject: [PATCH 2/3] Improve code style --- .../xpack/ql/expression/predicate/BinaryOperator.java | 3 ++- .../xpack/ql/expression/predicate/Predicates.java | 8 +++++--- 2 files changed, 7 insertions(+), 4 deletions(-) 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 c83fa2c87c901..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 @@ -68,9 +68,10 @@ protected Expression canonicalize() { // 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(commutativeChildren.get(i), next)); + commutativeChildren.set(i, replaceChildren(current, next)); } } 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 Date: Sat, 3 Apr 2021 21:24:14 +0300 Subject: [PATCH 3/3] Fix Attribute canonical definition --- .../java/org/elasticsearch/xpack/ql/expression/Attribute.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) 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 9ed9240edf90f..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 @@ -6,7 +6,6 @@ */ package org.elasticsearch.xpack.ql.expression; -import joptsimple.internal.Strings; import org.elasticsearch.xpack.ql.tree.Source; import org.elasticsearch.xpack.ql.type.DataType; @@ -119,7 +118,7 @@ public boolean semanticEquals(Expression other) { @Override protected Expression canonicalize() { - return clone(Source.EMPTY, Strings.EMPTY, dataType(), Strings.EMPTY, nullability, id(), synthetic()); + return clone(Source.EMPTY, name(), dataType(), qualifier, nullability, id(), synthetic()); } @Override