From c931ef8dd4c54f34a6af260b9af440cbc0753826 Mon Sep 17 00:00:00 2001 From: Bogdan Pintea Date: Tue, 8 Dec 2020 13:47:17 +0100 Subject: [PATCH 01/14] Simplify arithmetic operations in binary comps This commit adds an optimizer rule to simplify the arithmetic operations in binary comparison expressions, which in turn will allow for further expression compounding by the optimiser. Only the negation and plus, minus, multiplication and division are currently considered and only when two of the operands are a literal. For instance `((a + 1) / 2 - 3) * 4 >= 14` becomes `a >= 12`. --- .../xpack/eql/optimizer/Optimizer.java | 2 + .../xpack/eql/optimizer/OptimizerTests.java | 23 +++++++ .../predicate/operator/arithmetic/Add.java | 5 ++ .../arithmetic/ArithmeticOperation.java | 4 +- .../predicate/operator/arithmetic/Div.java | 5 ++ .../predicate/operator/arithmetic/Mod.java | 8 ++- .../predicate/operator/arithmetic/Mul.java | 5 ++ .../predicate/operator/arithmetic/Sub.java | 5 ++ .../operator/comparison/BinaryComparison.java | 2 + .../predicate/operator/comparison/Equals.java | 5 ++ .../operator/comparison/GreaterThan.java | 5 ++ .../comparison/GreaterThanOrEqual.java | 5 ++ .../operator/comparison/LessThan.java | 5 ++ .../operator/comparison/LessThanOrEqual.java | 5 ++ .../operator/comparison/NotEquals.java | 5 ++ .../operator/comparison/NullEquals.java | 5 ++ .../xpack/ql/optimizer/OptimizerRules.java | 47 ++++++++++++++ .../ql/optimizer/OptimizerRulesTests.java | 6 +- .../predicate/operator/arithmetic/Add.java | 6 ++ .../predicate/operator/arithmetic/Div.java | 7 ++ .../predicate/operator/arithmetic/Mod.java | 9 ++- .../predicate/operator/arithmetic/Mul.java | 6 ++ .../predicate/operator/arithmetic/Sub.java | 6 ++ .../xpack/sql/optimizer/Optimizer.java | 10 +-- .../sql/optimizer/OptimizerRunTests.java | 64 ++++++++++++++++++- .../xpack/sql/optimizer/OptimizerTests.java | 29 ++++++--- 26 files changed, 263 insertions(+), 21 deletions(-) diff --git a/x-pack/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/optimizer/Optimizer.java b/x-pack/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/optimizer/Optimizer.java index 7a57dd6877af6..ed23cb12fd3e5 100644 --- a/x-pack/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/optimizer/Optimizer.java +++ b/x-pack/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/optimizer/Optimizer.java @@ -41,6 +41,7 @@ import org.elasticsearch.xpack.ql.optimizer.OptimizerRules.OptimizerRule; import org.elasticsearch.xpack.ql.optimizer.OptimizerRules.PropagateEquals; import org.elasticsearch.xpack.ql.optimizer.OptimizerRules.PruneLiteralsInOrderBy; +import org.elasticsearch.xpack.ql.optimizer.OptimizerRules.SimplifyArithmeticsInBinaryComparisons; import org.elasticsearch.xpack.ql.optimizer.OptimizerRules.ReplaceRegexMatch; import org.elasticsearch.xpack.ql.optimizer.OptimizerRules.ReplaceSurrogateFunction; import org.elasticsearch.xpack.ql.optimizer.OptimizerRules.SetAsOptimized; @@ -80,6 +81,7 @@ protected Iterable.Batch> batches() { new BooleanSimplification(), new BooleanLiteralsOnTheRight(), new BooleanFunctionEqualsElimination(), + new SimplifyArithmeticsInBinaryComparisons(), // needs to occur before BinaryComparison combinations new ReplaceNullChecks(), new PropagateEquals(), diff --git a/x-pack/plugin/eql/src/test/java/org/elasticsearch/xpack/eql/optimizer/OptimizerTests.java b/x-pack/plugin/eql/src/test/java/org/elasticsearch/xpack/eql/optimizer/OptimizerTests.java index a849f1ad875b7..93662d7f6ec4e 100644 --- a/x-pack/plugin/eql/src/test/java/org/elasticsearch/xpack/eql/optimizer/OptimizerTests.java +++ b/x-pack/plugin/eql/src/test/java/org/elasticsearch/xpack/eql/optimizer/OptimizerTests.java @@ -35,6 +35,7 @@ import org.elasticsearch.xpack.ql.expression.predicate.nulls.IsNull; 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.regex.Like; import org.elasticsearch.xpack.ql.index.EsIndex; @@ -665,6 +666,28 @@ public void testDifferentKeyFromDisjunction() { assertEquals(filter, filterCondition(child2.children().get(0))); } + // ((a + 1) / 2 - 3) * 4 >= 14 -> a > 12. + public void testReduceBinaryComparisons() { + LogicalPlan plan = accept("foo where ((pid + 1) / 2 - 3) * 4 >= 14"); + assertNotNull(plan); + List filters = plan.collectFirstChildren(x -> x instanceof Filter); + assertNotNull(filters); + assertEquals(1, filters.size()); + assertTrue(filters.get(0) instanceof Filter); + Filter filter = (Filter) filters.get(0); + + assertTrue(filter.condition() instanceof And); + And and = (And) filter.condition(); + assertTrue(and.right() instanceof GreaterThanOrEqual); + GreaterThanOrEqual gte = (GreaterThanOrEqual) and.right(); + + assertTrue(gte.left() instanceof FieldAttribute); + assertEquals("pid", ((FieldAttribute) gte.left()).name()); + + assertTrue(gte.right() instanceof Literal); + assertEquals(12d, ((Literal) gte.right()).value()); + } + private static Attribute timestamp() { return new FieldAttribute(EMPTY, "test", new EsField("field", INTEGER, emptyMap(), true)); } 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 cc57c6cd1bb31..38484059df9e6 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 @@ -30,4 +30,9 @@ protected Add replaceChildren(Expression left, Expression right) { public Add swapLeftAndRight() { return new Add(source(), right(), left()); } + + @Override + public ArithmeticOperation inverse(Source source, Expression left, Expression right) { + return new Sub(source, left, right); + } } diff --git a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/operator/arithmetic/ArithmeticOperation.java b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/operator/arithmetic/ArithmeticOperation.java index c2105333daf3a..719a6ee418465 100644 --- a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/operator/arithmetic/ArithmeticOperation.java +++ b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/operator/arithmetic/ArithmeticOperation.java @@ -22,7 +22,7 @@ public abstract class ArithmeticOperation extends BinaryOperatorModulo * function ({@code a % b}). - * + * * Note this operator is also registered as a function (needed for ODBC/SQL) purposes. */ public class Mod extends ArithmeticOperation { @@ -30,4 +30,10 @@ protected NodeInfo info() { protected Mod replaceChildren(Expression newLeft, Expression newRight) { return new Mod(source(), newLeft, newRight); } + + @Override + public ArithmeticOperation inverse(Source source, Expression left, Expression right) { + // TODO: Modular Multiplicative Inverse, if ever needed? + throw new UnsupportedOperationException("inverting modulo operation is not supported"); + } } 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 d78f1984cb8e0..c07ffe6a8c73c 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 @@ -52,4 +52,9 @@ protected Mul replaceChildren(Expression newLeft, Expression newRight) { public Mul swapLeftAndRight() { return new Mul(source(), right(), left()); } + + @Override + public ArithmeticOperation inverse(Source source, Expression left, Expression right) { + return new Div(source, left, right); + } } diff --git a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/operator/arithmetic/Sub.java b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/operator/arithmetic/Sub.java index c5d7fc920e843..6a3ec21426fb0 100644 --- a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/operator/arithmetic/Sub.java +++ b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/operator/arithmetic/Sub.java @@ -27,4 +27,9 @@ protected NodeInfo info() { protected Sub replaceChildren(Expression newLeft, Expression newRight) { return new Sub(source(), newLeft, newRight); } + + @Override + public ArithmeticOperation inverse(Source source, Expression left, Expression right) { + return new Add(source, left, right); + } } diff --git a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/operator/comparison/BinaryComparison.java b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/operator/comparison/BinaryComparison.java index 3774a14952077..ea294331c5aae 100644 --- a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/operator/comparison/BinaryComparison.java +++ b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/operator/comparison/BinaryComparison.java @@ -49,4 +49,6 @@ protected Pipe makePipe() { public static Integer compare(Object left, Object right) { return Comparisons.compare(left, right); } + + public abstract BinaryComparison reverse(); } 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 3f8fc3bf9283f..49e276bc60e1a 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 @@ -42,4 +42,9 @@ public Equals swapLeftAndRight() { public BinaryComparison negate() { return new NotEquals(source(), left(), right(), zoneId()); } + + @Override + public BinaryComparison reverse() { + return this; + } } diff --git a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/operator/comparison/GreaterThan.java b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/operator/comparison/GreaterThan.java index c9c7b6729f5d2..d20065302dbb8 100644 --- a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/operator/comparison/GreaterThan.java +++ b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/operator/comparison/GreaterThan.java @@ -38,4 +38,9 @@ public LessThan swapLeftAndRight() { public LessThanOrEqual negate() { return new LessThanOrEqual(source(), left(), right(), zoneId()); } + + @Override + public BinaryComparison reverse() { + return new LessThan(source(), left(), right(), zoneId()); + } } diff --git a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/operator/comparison/GreaterThanOrEqual.java b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/operator/comparison/GreaterThanOrEqual.java index f5da6e7349b69..c196ead8b0531 100644 --- a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/operator/comparison/GreaterThanOrEqual.java +++ b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/operator/comparison/GreaterThanOrEqual.java @@ -38,4 +38,9 @@ public LessThanOrEqual swapLeftAndRight() { public LessThan negate() { return new LessThan(source(), left(), right(), zoneId()); } + + @Override + public BinaryComparison reverse() { + return new LessThanOrEqual(source(), left(), right(), zoneId()); + } } diff --git a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/operator/comparison/LessThan.java b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/operator/comparison/LessThan.java index 4dfa00186bdcb..487910ac5c746 100644 --- a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/operator/comparison/LessThan.java +++ b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/operator/comparison/LessThan.java @@ -38,4 +38,9 @@ public GreaterThan swapLeftAndRight() { public GreaterThanOrEqual negate() { return new GreaterThanOrEqual(source(), left(), right(), zoneId()); } + + @Override + public BinaryComparison reverse() { + return new GreaterThan(source(), left(), right(), zoneId()); + } } diff --git a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/operator/comparison/LessThanOrEqual.java b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/operator/comparison/LessThanOrEqual.java index 71a963a2da5c3..c3a194a1353d0 100644 --- a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/operator/comparison/LessThanOrEqual.java +++ b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/operator/comparison/LessThanOrEqual.java @@ -38,4 +38,9 @@ public GreaterThanOrEqual swapLeftAndRight() { public GreaterThan negate() { return new GreaterThan(source(), left(), right(), zoneId()); } + + @Override + public BinaryComparison reverse() { + return new GreaterThanOrEqual(source(), left(), right(), zoneId()); + } } 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 ff0bb72255611..1e069ee3d8d91 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 @@ -38,4 +38,9 @@ public NotEquals swapLeftAndRight() { public BinaryComparison negate() { return new Equals(source(), left(), right(), zoneId()); } + + @Override + public BinaryComparison reverse() { + return this; + } } 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 8402e89fd31a9..5a1ba0bd7cd4e 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 @@ -41,4 +41,9 @@ public NullEquals swapLeftAndRight() { public Nullability nullable() { return Nullability.FALSE; } + + @Override + public BinaryComparison reverse() { + return this; + } } 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 index 1e10953919ec9..8a0efd58df64d 100644 --- 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 @@ -20,6 +20,9 @@ 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.nulls.IsNotNull; +import org.elasticsearch.xpack.ql.expression.predicate.operator.arithmetic.ArithmeticOperation; +import org.elasticsearch.xpack.ql.expression.predicate.operator.arithmetic.Mod; +import org.elasticsearch.xpack.ql.expression.predicate.operator.arithmetic.Neg; import org.elasticsearch.xpack.ql.expression.predicate.operator.comparison.BinaryComparison; import org.elasticsearch.xpack.ql.expression.predicate.operator.comparison.Equals; import org.elasticsearch.xpack.ql.expression.predicate.operator.comparison.GreaterThan; @@ -57,6 +60,7 @@ import static org.elasticsearch.xpack.ql.expression.predicate.Predicates.splitAnd; import static org.elasticsearch.xpack.ql.expression.predicate.Predicates.splitOr; import static org.elasticsearch.xpack.ql.expression.predicate.Predicates.subtract; +import static org.elasticsearch.xpack.ql.type.DataTypes.DOUBLE; import static org.elasticsearch.xpack.ql.util.CollectionUtils.combine; @@ -1137,6 +1141,49 @@ protected Expression rule(Expression e) { } } + public static final class SimplifyArithmeticsInBinaryComparisons extends OptimizerExpressionRule { + + public SimplifyArithmeticsInBinaryComparisons() { + super(TransformDirection.UP); + } + + @Override + protected Expression rule(Expression e) { + if (e instanceof BinaryComparison) { + return reduce((BinaryComparison) e); + } + return e; + } + + private Expression reduce(BinaryComparison bc) { + // optimize only once the expression looks like: AnyNode [ArithmOp] AnyNode [BiComp] Literal + if (bc.right() instanceof Literal == false) { + return bc; + } + Literal bcRight = (Literal) bc.right(); + + if (bc.left() instanceof ArithmeticOperation) { + ArithmeticOperation opLeft = (ArithmeticOperation) bc.left(); + if (opLeft instanceof Mod) { + return bc; // can't optimise Mods + } + + if (opLeft.left() instanceof Literal || opLeft.right() instanceof Literal) { // 5 [op] a >= 4 || a [op] 5 >= 4 + // force double division + if (opLeft.symbol().equals("*")) { // use symbol comp for SQL's Mul + bcRight = new Literal(bcRight.source(), ((Number) bcRight.value()).doubleValue(), DOUBLE); + } + return bc.replaceChildren(List.of(opLeft.left(), opLeft.inverse(bcRight.source(), bcRight, opLeft.right()))); + } + } else if (bc.left() instanceof Neg) { + return bc.reverse().replaceChildren(List.of(bc.left().children().get(0), new Neg(bcRight.source(), bcRight))); + } + + return bc; + } + + } + public abstract static class PruneFilters extends OptimizerRule { @Override 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 index 83c0d0fcd0dfe..12f45436bfe86 100644 --- 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 @@ -1027,7 +1027,7 @@ public void testRangesOverlappingNoLowerBoundary() { Expression exp = rule.rule(or); assertEquals(r2, exp); } - + public void testBinaryComparisonAndOutOfRangeNotEqualsDifferentFields() { FieldAttribute doubleOne = fieldAttribute("double", DOUBLE); FieldAttribute doubleTwo = fieldAttribute("double2", DOUBLE); @@ -1045,12 +1045,12 @@ public void testBinaryComparisonAndOutOfRangeNotEqualsDifferentFields() { new And(EMPTY, notEqualsOf(keywordOne, L("2021")), lessThanOrEqualOf(datetimeOne, L("2020-12-04T17:48:22.954240Z"))), // double > 10.1 AND double2 != -10.1 new And(EMPTY, greaterThanOf(doubleOne, L(10.1d)), notEqualsOf(doubleTwo, L(-10.1d)))); - + for (And and : testCases) { CombineBinaryComparisons rule = new CombineBinaryComparisons(); Expression exp = rule.rule(and); assertEquals("Rule should not have transformed [" + and.nodeString() + "]", and, exp); - } + } } // Equals & NullEquals 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 ef286d54a84a6..b60dc88c0c85b 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 @@ -6,6 +6,7 @@ package org.elasticsearch.xpack.sql.expression.predicate.operator.arithmetic; import org.elasticsearch.xpack.ql.expression.Expression; +import org.elasticsearch.xpack.ql.expression.predicate.operator.arithmetic.ArithmeticOperation; import org.elasticsearch.xpack.ql.tree.NodeInfo; import org.elasticsearch.xpack.ql.tree.Source; @@ -31,4 +32,9 @@ protected Add replaceChildren(Expression left, Expression right) { public Add swapLeftAndRight() { return new Add(source(), right(), left()); } + + @Override + public ArithmeticOperation inverse(Source source, Expression left, Expression right) { + return new Sub(source, left, right); + } } diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/operator/arithmetic/Div.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/operator/arithmetic/Div.java index 67da39f46eaf3..75c7f8b6b2dfa 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/operator/arithmetic/Div.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/operator/arithmetic/Div.java @@ -6,6 +6,8 @@ package org.elasticsearch.xpack.sql.expression.predicate.operator.arithmetic; import org.elasticsearch.xpack.ql.expression.Expression; +import org.elasticsearch.xpack.ql.expression.predicate.operator.arithmetic.ArithmeticOperation; +import org.elasticsearch.xpack.ql.expression.predicate.operator.arithmetic.Mul; import org.elasticsearch.xpack.ql.tree.NodeInfo; import org.elasticsearch.xpack.ql.tree.Source; import org.elasticsearch.xpack.ql.type.DataType; @@ -34,4 +36,9 @@ protected Div replaceChildren(Expression newLeft, Expression newRight) { public DataType dataType() { return SqlDataTypeConverter.commonType(left().dataType(), right().dataType()); } + + @Override + public ArithmeticOperation inverse(Source source, Expression left, Expression right) { + return new Mul(source, left, right); + } } diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/operator/arithmetic/Mod.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/operator/arithmetic/Mod.java index 1d828d51cb7c4..b41ef57511bea 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/operator/arithmetic/Mod.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/operator/arithmetic/Mod.java @@ -6,13 +6,14 @@ package org.elasticsearch.xpack.sql.expression.predicate.operator.arithmetic; import org.elasticsearch.xpack.ql.expression.Expression; +import org.elasticsearch.xpack.ql.expression.predicate.operator.arithmetic.ArithmeticOperation; import org.elasticsearch.xpack.ql.tree.NodeInfo; import org.elasticsearch.xpack.ql.tree.Source; /** * Modulo * function ({@code a % b}). - * + * * Note this operator is also registered as a function (needed for ODBC/SQL) purposes. */ public class Mod extends SqlArithmeticOperation { @@ -30,4 +31,10 @@ protected NodeInfo info() { protected Mod replaceChildren(Expression newLeft, Expression newRight) { return new Mod(source(), newLeft, newRight); } + + @Override + public ArithmeticOperation inverse(Source source, Expression left, Expression right) { + // TODO: Modular Multiplicative Inverse, if ever needed? + throw new UnsupportedOperationException("inverting modulo operation is not supported"); + } } 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 8827a42355cf9..a7c4e619328dc 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 @@ -6,6 +6,7 @@ package org.elasticsearch.xpack.sql.expression.predicate.operator.arithmetic; import org.elasticsearch.xpack.ql.expression.Expression; +import org.elasticsearch.xpack.ql.expression.predicate.operator.arithmetic.ArithmeticOperation; import org.elasticsearch.xpack.ql.tree.NodeInfo; import org.elasticsearch.xpack.ql.tree.Source; import org.elasticsearch.xpack.ql.type.DataType; @@ -58,6 +59,11 @@ public DataType dataType() { return dataType; } + @Override + public ArithmeticOperation inverse(Source source, Expression left, Expression right) { + return new Div(source, left, right); + } + @Override protected NodeInfo info() { return NodeInfo.create(this, Mul::new, left(), right()); diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/operator/arithmetic/Sub.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/operator/arithmetic/Sub.java index 77f8eed05afa3..2c702f0e2dbed 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/operator/arithmetic/Sub.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/operator/arithmetic/Sub.java @@ -6,6 +6,7 @@ package org.elasticsearch.xpack.sql.expression.predicate.operator.arithmetic; import org.elasticsearch.xpack.ql.expression.Expression; +import org.elasticsearch.xpack.ql.expression.predicate.operator.arithmetic.ArithmeticOperation; import org.elasticsearch.xpack.ql.tree.NodeInfo; import org.elasticsearch.xpack.ql.tree.Source; import org.elasticsearch.xpack.sql.type.SqlDataTypes; @@ -43,4 +44,9 @@ protected TypeResolution resolveWithIntervals() { } return TypeResolution.TYPE_RESOLVED; } + + @Override + public ArithmeticOperation inverse(Source source, Expression left, Expression right) { + return new Add(source, left, right); + } } diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/optimizer/Optimizer.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/optimizer/Optimizer.java index a0fdf5370f055..a865b38fe0893 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/optimizer/Optimizer.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/optimizer/Optimizer.java @@ -34,6 +34,7 @@ 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.expression.predicate.operator.comparison.NullEquals; +import org.elasticsearch.xpack.ql.optimizer.OptimizerRules.SimplifyArithmeticsInBinaryComparisons; import org.elasticsearch.xpack.ql.optimizer.OptimizerRules.BooleanLiteralsOnTheRight; import org.elasticsearch.xpack.ql.optimizer.OptimizerRules.BooleanSimplification; import org.elasticsearch.xpack.ql.optimizer.OptimizerRules.CombineBinaryComparisons; @@ -139,6 +140,7 @@ protected Iterable.Batch> batches() { new ConstantFolding(), new SimplifyConditional(), new SimplifyCase(), + new SimplifyArithmeticsInBinaryComparisons(), // boolean new BooleanSimplification(), new BooleanLiteralsOnTheRight(), @@ -1026,11 +1028,11 @@ private static class PercentileKey extends Tuple PercentileKey(PercentileRank per) { super(per.field(), per.percentilesConfig()); } - + private Expression field() { return v1(); } - + private PercentilesConfig percentilesConfig() { return v2(); } @@ -1054,7 +1056,7 @@ public LogicalPlan apply(LogicalPlan p) { Map percentilesPerAggKey = new LinkedHashMap<>(); percentsPerAggKey.forEach((aggKey, percents) -> percentilesPerAggKey.put( aggKey, - new Percentiles(percents.iterator().next().source(), aggKey.field(), new ArrayList<>(percents), + new Percentiles(percents.iterator().next().source(), aggKey.field(), new ArrayList<>(percents), aggKey.percentilesConfig()))); return p.transformExpressionsUp(e -> { @@ -1088,7 +1090,7 @@ public LogicalPlan apply(LogicalPlan p) { Map ranksPerAggKey = new LinkedHashMap<>(); valuesPerAggKey.forEach((aggKey, values) -> ranksPerAggKey.put( aggKey, - new PercentileRanks(values.iterator().next().source(), aggKey.field(), new ArrayList<>(values), + new PercentileRanks(values.iterator().next().source(), aggKey.field(), new ArrayList<>(values), aggKey.percentilesConfig()))); return p.transformExpressionsUp(e -> { diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/optimizer/OptimizerRunTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/optimizer/OptimizerRunTests.java index 4a54a1e48b78b..037035ec19a68 100644 --- a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/optimizer/OptimizerRunTests.java +++ b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/optimizer/OptimizerRunTests.java @@ -6,10 +6,19 @@ package org.elasticsearch.xpack.sql.optimizer; import org.elasticsearch.test.ESTestCase; +import org.elasticsearch.xpack.ql.expression.FieldAttribute; +import org.elasticsearch.xpack.ql.expression.Literal; import org.elasticsearch.xpack.ql.expression.function.FunctionRegistry; +import org.elasticsearch.xpack.ql.expression.predicate.operator.comparison.BinaryComparison; +import org.elasticsearch.xpack.ql.expression.predicate.operator.comparison.Equals; +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.NotEquals; import org.elasticsearch.xpack.ql.index.EsIndex; import org.elasticsearch.xpack.ql.index.IndexResolution; +import org.elasticsearch.xpack.ql.plan.logical.Filter; import org.elasticsearch.xpack.ql.plan.logical.LogicalPlan; +import org.elasticsearch.xpack.ql.plan.logical.UnaryPlan; import org.elasticsearch.xpack.ql.type.EsField; import org.elasticsearch.xpack.sql.SqlTestUtils; import org.elasticsearch.xpack.sql.analysis.analyzer.Analyzer; @@ -48,4 +57,57 @@ public void testWhereClause() { LogicalPlan p = plan("SELECT some.string l FROM test WHERE int IS NOT NULL AND int < 10005 ORDER BY int"); assertNotNull(p); } -} \ No newline at end of file + + public void testReducedBinaryComparisonGreaterThenOrEqual() { + // i >= 12 + doTestBinaryComparisonReduction("((int + 1) / 2 - 3) * 4 >= 14", GreaterThanOrEqual.class, 12d); + } + + public void testReducedBinaryComYparisonLessThen() { + // i < -5/6 + doTestBinaryComparisonReduction("12 * (-int / 5) > (8 + 12) / 10", LessThan.class, -5d / 6); + } + + public void testReducedBinaryComYparisonNotEquals() { + // i != 7000 + doTestBinaryComparisonReduction("-3600 != (int - 200) / 2", NotEquals.class, -7000); + } + + public void testReducedBinaryComparisonEquals() { + // i = -12 + doTestBinaryComparisonReduction("2 * 3 / (4 / -int) = 18", Equals.class, -12d); + } + + public void testReducedBinaryComparisonWithConjunction() { + doTestBinaryComparisonReduction("2 * 3 / (4 / -int) = 18 AND int >= -12", Equals.class, -12d); + } + + public void testReducedBinaryComparisonWithDisjunction() { + doTestBinaryComparisonReduction("2 * 3 / (4 / -int) = 18 OR int > -12", GreaterThanOrEqual.class, -12d); + } + + private void doTestBinaryComparisonReduction(String expression, Class binaryComparisonClass, + Number bound) { + LogicalPlan plan = plan("SELECT some.string FROM test WHERE " + expression); + + assertTrue(plan instanceof UnaryPlan); + UnaryPlan unaryPlan = (UnaryPlan) plan; + assertTrue(unaryPlan.child() instanceof Filter); + Filter filter = (Filter) unaryPlan.child(); + assertEquals(binaryComparisonClass, filter.condition().getClass()); + BinaryComparison bc = (BinaryComparison) filter.condition(); + + assertTrue(bc.left() instanceof FieldAttribute); + FieldAttribute attribute = (FieldAttribute) bc.left(); + assertEquals("int", attribute.name()); + + assertTrue(bc.right() instanceof Literal); + Literal literal = (Literal) bc.right(); + if (bound instanceof Double) { + assertTrue(literal.value() instanceof Number); + assertEquals(bound.doubleValue(), ((Number) literal.value()).doubleValue(), 1E-15); + } else { + assertEquals(bound, literal.value()); + } + } +} diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/optimizer/OptimizerTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/optimizer/OptimizerTests.java index 5c1248d2b7684..28c2d6b8e6fe1 100644 --- a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/optimizer/OptimizerTests.java +++ b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/optimizer/OptimizerTests.java @@ -35,6 +35,7 @@ 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.NullEquals; import org.elasticsearch.xpack.ql.expression.predicate.regex.RLike; import org.elasticsearch.xpack.ql.expression.predicate.regex.RLikePattern; @@ -705,7 +706,7 @@ public void testSimplifyIif_ConditionFalse_NonFoldableResult() { assertFalse(iif.foldable()); assertEquals("myField", Expressions.name(iif.elseResult())); } - + // // Logical simplifications // @@ -847,12 +848,12 @@ public void testSortAggregateOnOrderByWithTwoFields() { Alias secondAlias = new Alias(EMPTY, "second_alias", secondField); Order firstOrderBy = new Order(EMPTY, firstField, OrderDirection.ASC, Order.NullsPosition.LAST); Order secondOrderBy = new Order(EMPTY, secondField, OrderDirection.ASC, Order.NullsPosition.LAST); - + OrderBy orderByPlan = new OrderBy(EMPTY, new Aggregate(EMPTY, FROM(), Arrays.asList(secondField, firstField), Arrays.asList(secondAlias, firstAlias)), Arrays.asList(firstOrderBy, secondOrderBy)); LogicalPlan result = new SortAggregateOnOrderBy().apply(orderByPlan); - + assertTrue(result instanceof OrderBy); List order = ((OrderBy) result).order(); assertEquals(2, order.size()); @@ -860,7 +861,7 @@ public void testSortAggregateOnOrderByWithTwoFields() { assertTrue(order.get(1).child() instanceof FieldAttribute); assertEquals("first_field", ((FieldAttribute) order.get(0).child()).name()); assertEquals("second_field", ((FieldAttribute) order.get(1).child()).name()); - + assertTrue(((OrderBy) result).child() instanceof Aggregate); Aggregate agg = (Aggregate) ((OrderBy) result).child(); List groupings = agg.groupings(); @@ -879,12 +880,12 @@ public void testSortAggregateOnOrderByOnlyAliases() { Alias secondAlias = new Alias(EMPTY, "second_alias", secondField); Order firstOrderBy = new Order(EMPTY, firstAlias, OrderDirection.ASC, Order.NullsPosition.LAST); Order secondOrderBy = new Order(EMPTY, secondAlias, OrderDirection.ASC, Order.NullsPosition.LAST); - + OrderBy orderByPlan = new OrderBy(EMPTY, new Aggregate(EMPTY, FROM(), Arrays.asList(secondAlias, firstAlias), Arrays.asList(secondAlias, firstAlias)), Arrays.asList(firstOrderBy, secondOrderBy)); LogicalPlan result = new SortAggregateOnOrderBy().apply(orderByPlan); - + assertTrue(result instanceof OrderBy); List order = ((OrderBy) result).order(); assertEquals(2, order.size()); @@ -892,7 +893,7 @@ public void testSortAggregateOnOrderByOnlyAliases() { assertTrue(order.get(1).child() instanceof Alias); assertEquals("first_alias", ((Alias) order.get(0).child()).name()); assertEquals("second_alias", ((Alias) order.get(1).child()).name()); - + assertTrue(((OrderBy) result).child() instanceof Aggregate); Aggregate agg = (Aggregate) ((OrderBy) result).child(); List groupings = agg.groupings(); @@ -997,14 +998,14 @@ public void testAggregatesPromoteToStats_WithFullTextPredicatesConditions() { public void testReplaceAttributesWithTarget() { FieldAttribute a = getFieldAttribute("a"); FieldAttribute b = getFieldAttribute("b"); - + Alias aAlias = new Alias(EMPTY, "aAlias", a); Alias bAlias = new Alias(EMPTY, "bAlias", b); - + Project p = new Project(EMPTY, FROM(), Arrays.asList(aAlias, bAlias)); Filter f = new Filter(EMPTY, p, new And(EMPTY, greaterThanOf(aAlias.toAttribute(), L(1)), greaterThanOf(bAlias.toAttribute(), L(2)))); - + ReplaceReferenceAttributeWithSource rule = new ReplaceReferenceAttributeWithSource(); Expression condition = f.condition(); assertTrue(condition instanceof And); @@ -1023,4 +1024,12 @@ public void testReplaceAttributesWithTarget() { gt = (GreaterThan) and.left(); assertEquals(a, gt.left()); } + + public void testLiteralsToTheRight() { + FieldAttribute fieldAttribute = getFieldAttribute(); + + Expression left = new org.elasticsearch.xpack.ql.expression.predicate.operator.arithmetic.Add(EMPTY, ONE, TWO); + Expression right = new GreaterThan(EMPTY, THREE, getFieldAttribute(), null); + Expression lessThen = new LessThanOrEqual(EMPTY, left, right, null); + } } From 0db90e4457b32e85954126ec5a565850661b0535 Mon Sep 17 00:00:00 2001 From: Bogdan Pintea Date: Tue, 8 Dec 2020 14:00:30 +0100 Subject: [PATCH 02/14] Undo unintentional change Revert unintentional change of a source file. --- .../xpack/sql/optimizer/OptimizerTests.java | 29 +++++++------------ 1 file changed, 10 insertions(+), 19 deletions(-) diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/optimizer/OptimizerTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/optimizer/OptimizerTests.java index 28c2d6b8e6fe1..5c1248d2b7684 100644 --- a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/optimizer/OptimizerTests.java +++ b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/optimizer/OptimizerTests.java @@ -35,7 +35,6 @@ 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.NullEquals; import org.elasticsearch.xpack.ql.expression.predicate.regex.RLike; import org.elasticsearch.xpack.ql.expression.predicate.regex.RLikePattern; @@ -706,7 +705,7 @@ public void testSimplifyIif_ConditionFalse_NonFoldableResult() { assertFalse(iif.foldable()); assertEquals("myField", Expressions.name(iif.elseResult())); } - + // // Logical simplifications // @@ -848,12 +847,12 @@ public void testSortAggregateOnOrderByWithTwoFields() { Alias secondAlias = new Alias(EMPTY, "second_alias", secondField); Order firstOrderBy = new Order(EMPTY, firstField, OrderDirection.ASC, Order.NullsPosition.LAST); Order secondOrderBy = new Order(EMPTY, secondField, OrderDirection.ASC, Order.NullsPosition.LAST); - + OrderBy orderByPlan = new OrderBy(EMPTY, new Aggregate(EMPTY, FROM(), Arrays.asList(secondField, firstField), Arrays.asList(secondAlias, firstAlias)), Arrays.asList(firstOrderBy, secondOrderBy)); LogicalPlan result = new SortAggregateOnOrderBy().apply(orderByPlan); - + assertTrue(result instanceof OrderBy); List order = ((OrderBy) result).order(); assertEquals(2, order.size()); @@ -861,7 +860,7 @@ public void testSortAggregateOnOrderByWithTwoFields() { assertTrue(order.get(1).child() instanceof FieldAttribute); assertEquals("first_field", ((FieldAttribute) order.get(0).child()).name()); assertEquals("second_field", ((FieldAttribute) order.get(1).child()).name()); - + assertTrue(((OrderBy) result).child() instanceof Aggregate); Aggregate agg = (Aggregate) ((OrderBy) result).child(); List groupings = agg.groupings(); @@ -880,12 +879,12 @@ public void testSortAggregateOnOrderByOnlyAliases() { Alias secondAlias = new Alias(EMPTY, "second_alias", secondField); Order firstOrderBy = new Order(EMPTY, firstAlias, OrderDirection.ASC, Order.NullsPosition.LAST); Order secondOrderBy = new Order(EMPTY, secondAlias, OrderDirection.ASC, Order.NullsPosition.LAST); - + OrderBy orderByPlan = new OrderBy(EMPTY, new Aggregate(EMPTY, FROM(), Arrays.asList(secondAlias, firstAlias), Arrays.asList(secondAlias, firstAlias)), Arrays.asList(firstOrderBy, secondOrderBy)); LogicalPlan result = new SortAggregateOnOrderBy().apply(orderByPlan); - + assertTrue(result instanceof OrderBy); List order = ((OrderBy) result).order(); assertEquals(2, order.size()); @@ -893,7 +892,7 @@ public void testSortAggregateOnOrderByOnlyAliases() { assertTrue(order.get(1).child() instanceof Alias); assertEquals("first_alias", ((Alias) order.get(0).child()).name()); assertEquals("second_alias", ((Alias) order.get(1).child()).name()); - + assertTrue(((OrderBy) result).child() instanceof Aggregate); Aggregate agg = (Aggregate) ((OrderBy) result).child(); List groupings = agg.groupings(); @@ -998,14 +997,14 @@ public void testAggregatesPromoteToStats_WithFullTextPredicatesConditions() { public void testReplaceAttributesWithTarget() { FieldAttribute a = getFieldAttribute("a"); FieldAttribute b = getFieldAttribute("b"); - + Alias aAlias = new Alias(EMPTY, "aAlias", a); Alias bAlias = new Alias(EMPTY, "bAlias", b); - + Project p = new Project(EMPTY, FROM(), Arrays.asList(aAlias, bAlias)); Filter f = new Filter(EMPTY, p, new And(EMPTY, greaterThanOf(aAlias.toAttribute(), L(1)), greaterThanOf(bAlias.toAttribute(), L(2)))); - + ReplaceReferenceAttributeWithSource rule = new ReplaceReferenceAttributeWithSource(); Expression condition = f.condition(); assertTrue(condition instanceof And); @@ -1024,12 +1023,4 @@ public void testReplaceAttributesWithTarget() { gt = (GreaterThan) and.left(); assertEquals(a, gt.left()); } - - public void testLiteralsToTheRight() { - FieldAttribute fieldAttribute = getFieldAttribute(); - - Expression left = new org.elasticsearch.xpack.ql.expression.predicate.operator.arithmetic.Add(EMPTY, ONE, TWO); - Expression right = new GreaterThan(EMPTY, THREE, getFieldAttribute(), null); - Expression lessThen = new LessThanOrEqual(EMPTY, left, right, null); - } } From abc632d2f043832f3d0d63b1ea50906fc1b8fb31 Mon Sep 17 00:00:00 2001 From: Bogdan Pintea Date: Mon, 14 Dec 2020 10:14:20 +0100 Subject: [PATCH 03/14] Refactor the optimisation. Adress review comments The optimisation is now refactor splitting the logic by add-sub and mul-div operations. Further restictions are put in place, to prevent the optimiser from chaning the outcome of the filtering. More tests have been added. --- .../xpack/eql/optimizer/Optimizer.java | 6 +- .../src/test/resources/queryfolder_tests.txt | 32 +-- .../predicate/operator/arithmetic/Add.java | 2 +- .../arithmetic/ArithmeticOperation.java | 7 +- .../predicate/operator/arithmetic/Div.java | 2 +- .../predicate/operator/arithmetic/Mod.java | 6 - .../predicate/operator/arithmetic/Mul.java | 2 +- .../predicate/operator/arithmetic/Sub.java | 2 +- .../operator/comparison/BinaryComparison.java | 5 + .../xpack/ql/optimizer/OptimizerRules.java | 244 ++++++++++++++++-- .../ql/optimizer/OptimizerRulesTests.java | 24 ++ .../mapping-multi-field-variation.json | 1 + .../src/main/resources/arithmetic.csv-spec | 38 ++- .../src/main/resources/arithmetic.sql-spec | 146 +++++++++++ .../predicate/operator/arithmetic/Add.java | 3 +- .../predicate/operator/arithmetic/Div.java | 3 +- .../predicate/operator/arithmetic/Mod.java | 7 - .../predicate/operator/arithmetic/Mul.java | 3 +- .../SqlBinaryArithmeticOperation.java | 8 +- .../predicate/operator/arithmetic/Sub.java | 3 +- .../xpack/sql/optimizer/Optimizer.java | 9 +- .../xpack/sql/type/SqlDataTypes.java | 1 + .../analyzer/FieldAttributeTests.java | 2 +- .../arithmetic/SqlBinaryArithmeticTests.java | 14 +- .../sql/optimizer/OptimizerRunTests.java | 209 ++++++++++++--- .../logical/command/sys/SysColumnsTests.java | 116 +++++---- .../sql/planner/QueryTranslatorTests.java | 22 +- .../xpack/sql/type/SqlDataTypesTests.java | 22 ++ 28 files changed, 766 insertions(+), 173 deletions(-) diff --git a/x-pack/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/optimizer/Optimizer.java b/x-pack/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/optimizer/Optimizer.java index ed23cb12fd3e5..aafc85354a94e 100644 --- a/x-pack/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/optimizer/Optimizer.java +++ b/x-pack/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/optimizer/Optimizer.java @@ -35,13 +35,14 @@ import org.elasticsearch.xpack.ql.optimizer.OptimizerRules.BooleanFunctionEqualsElimination; import org.elasticsearch.xpack.ql.optimizer.OptimizerRules.BooleanLiteralsOnTheRight; import org.elasticsearch.xpack.ql.optimizer.OptimizerRules.BooleanSimplification; +import org.elasticsearch.xpack.ql.optimizer.OptimizerRules.BubbleUpNegations; import org.elasticsearch.xpack.ql.optimizer.OptimizerRules.CombineBinaryComparisons; import org.elasticsearch.xpack.ql.optimizer.OptimizerRules.CombineDisjunctionsToIn; import org.elasticsearch.xpack.ql.optimizer.OptimizerRules.ConstantFolding; import org.elasticsearch.xpack.ql.optimizer.OptimizerRules.OptimizerRule; import org.elasticsearch.xpack.ql.optimizer.OptimizerRules.PropagateEquals; import org.elasticsearch.xpack.ql.optimizer.OptimizerRules.PruneLiteralsInOrderBy; -import org.elasticsearch.xpack.ql.optimizer.OptimizerRules.SimplifyArithmeticsInBinaryComparisons; +import org.elasticsearch.xpack.ql.optimizer.OptimizerRules.SimplifyComparisonsArithmetics; import org.elasticsearch.xpack.ql.optimizer.OptimizerRules.ReplaceRegexMatch; import org.elasticsearch.xpack.ql.optimizer.OptimizerRules.ReplaceSurrogateFunction; import org.elasticsearch.xpack.ql.optimizer.OptimizerRules.SetAsOptimized; @@ -81,13 +82,14 @@ protected Iterable.Batch> batches() { new BooleanSimplification(), new BooleanLiteralsOnTheRight(), new BooleanFunctionEqualsElimination(), - new SimplifyArithmeticsInBinaryComparisons(), // needs to occur before BinaryComparison combinations new ReplaceNullChecks(), new PropagateEquals(), new CombineBinaryComparisons(), new CombineDisjunctionsToIn(), new PushDownAndCombineFilters(), + new BubbleUpNegations(), + new SimplifyComparisonsArithmetics(DataTypes::areCompatible), // prune/elimination new PruneFilters(), new PruneLiteralsInOrderBy(), diff --git a/x-pack/plugin/eql/src/test/resources/queryfolder_tests.txt b/x-pack/plugin/eql/src/test/resources/queryfolder_tests.txt index a9e3df3650c9b..da374a94650d3 100644 --- a/x-pack/plugin/eql/src/test/resources/queryfolder_tests.txt +++ b/x-pack/plugin/eql/src/test/resources/queryfolder_tests.txt @@ -454,35 +454,35 @@ process where wildcard(process_path, "*\\red_ttp\\wininit.*", "*\\abc\\*", "*def addOperator -process where serial_event_id + 2 == 41 +process where serial_event_id + 2 == -2147483647 ; "script":{"source":"InternalQlScriptUtils.nullSafeFilter(InternalQlScriptUtils.eq( InternalQlScriptUtils.add(InternalQlScriptUtils.docValue(doc,params.v0),params.v1),params.v2))", -"params":{"v0":"serial_event_id","v1":2,"v2":41} +"params":{"v0":"serial_event_id","v1":2,"v2":-2147483647} ; addOperatorReversed -process where 2 + serial_event_id == 41 +process where 2 + serial_event_id == -2147483647 ; "script":{"source":"InternalQlScriptUtils.nullSafeFilter(InternalQlScriptUtils.eq( InternalQlScriptUtils.add(InternalQlScriptUtils.docValue(doc,params.v0),params.v1),params.v2))", -"params":{"v0":"serial_event_id","v1":2,"v2":41} +"params":{"v0":"serial_event_id","v1":2,"v2":-2147483647} ; addFunction -process where add(serial_event_id, 2) == 41 +process where add(serial_event_id, 2) == -2147483647 ; "script":{"source":"InternalQlScriptUtils.nullSafeFilter(InternalQlScriptUtils.eq( InternalQlScriptUtils.add(InternalQlScriptUtils.docValue(doc,params.v0),params.v1),params.v2))", -"params":{"v0":"serial_event_id","v1":2,"v2":41} +"params":{"v0":"serial_event_id","v1":2,"v2":-2147483647} ; addFunctionReversed -process where add(2, serial_event_id) == 41 +process where add(2, serial_event_id) == -2147483647 ; "script":{"source":"InternalQlScriptUtils.nullSafeFilter(InternalQlScriptUtils.eq( InternalQlScriptUtils.add(InternalQlScriptUtils.docValue(doc,params.v0),params.v1),params.v2))", -"params":{"v0":"serial_event_id","v1":2,"v2":41} +"params":{"v0":"serial_event_id","v1":2,"v2":-2147483647} ; divideOperator @@ -582,35 +582,35 @@ InternalQlScriptUtils.mul(InternalQlScriptUtils.docValue(doc,params.v0),params.v ; subtractOperator -process where serial_event_id - 2 == 41 +process where serial_event_id - 2 == 2147483647 ; "script":{"source":"InternalQlScriptUtils.nullSafeFilter(InternalQlScriptUtils.eq( InternalQlScriptUtils.sub(InternalQlScriptUtils.docValue(doc,params.v0),params.v1),params.v2))", -"params":{"v0":"serial_event_id","v1":2,"v2":41} +"params":{"v0":"serial_event_id","v1":2,"v2":2147483647} ; subtractOperatorReversed -process where 43 - serial_event_id == 41 +process where 43 - serial_event_id == -2147483647 ; "script":{"source":"InternalQlScriptUtils.nullSafeFilter(InternalQlScriptUtils.eq( InternalQlScriptUtils.sub(params.v0,InternalQlScriptUtils.docValue(doc,params.v1)),params.v2))", -"params":{"v0":43,"v1":"serial_event_id","v2":41} +"params":{"v0":43,"v1":"serial_event_id","v2":-2147483647} ; subtractFunction -process where subtract(serial_event_id, 2) == 41 +process where subtract(serial_event_id, 2) == 2147483647 ; "script":{"source":"InternalQlScriptUtils.nullSafeFilter(InternalQlScriptUtils.eq( InternalQlScriptUtils.sub(InternalQlScriptUtils.docValue(doc,params.v0),params.v1),params.v2))", -"params":{"v0":"serial_event_id","v1":2,"v2":41} +"params":{"v0":"serial_event_id","v1":2,"v2":2147483647} ; subtractFunctionReversed -process where subtract(43, serial_event_id) == 41 +process where subtract(43, serial_event_id) == -2147483647 ; "script":{"source":"InternalQlScriptUtils.nullSafeFilter(InternalQlScriptUtils.eq( InternalQlScriptUtils.sub(params.v0,InternalQlScriptUtils.docValue(doc,params.v1)),params.v2))", -"params":{"v0":43,"v1":"serial_event_id","v2":41} +"params":{"v0":43,"v1":"serial_event_id","v2":-2147483647} ; eventQueryDefaultLimit 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 38484059df9e6..fc6362ddcd281 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 @@ -32,7 +32,7 @@ public Add swapLeftAndRight() { } @Override - public ArithmeticOperation inverse(Source source, Expression left, Expression right) { + public Sub inverse(Source source, Expression left, Expression right) { return new Sub(source, left, right); } } diff --git a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/operator/arithmetic/ArithmeticOperation.java b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/operator/arithmetic/ArithmeticOperation.java index 719a6ee418465..4d0124758665a 100644 --- a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/operator/arithmetic/ArithmeticOperation.java +++ b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/operator/arithmetic/ArithmeticOperation.java @@ -46,5 +46,10 @@ protected Pipe makePipe() { return new BinaryArithmeticPipe(source(), this, Expressions.pipe(left()), Expressions.pipe(right()), function()); } - public abstract ArithmeticOperation inverse(Source source, Expression left, Expression right); + /** + * Returns the opposite of this operation. + */ + public ArithmeticOperation inverse(Source source, Expression left, Expression right) { + return this; + } } diff --git a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/operator/arithmetic/Div.java b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/operator/arithmetic/Div.java index 096aed69fdb55..fc5704575b700 100644 --- a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/operator/arithmetic/Div.java +++ b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/operator/arithmetic/Div.java @@ -36,7 +36,7 @@ public DataType dataType() { } @Override - public ArithmeticOperation inverse(Source source, Expression left, Expression right) { + public Mul inverse(Source source, Expression left, Expression right) { return new Mul(source, left, right); } } diff --git a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/operator/arithmetic/Mod.java b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/operator/arithmetic/Mod.java index 111701666be12..37dcb47851bda 100644 --- a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/operator/arithmetic/Mod.java +++ b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/operator/arithmetic/Mod.java @@ -30,10 +30,4 @@ protected NodeInfo info() { protected Mod replaceChildren(Expression newLeft, Expression newRight) { return new Mod(source(), newLeft, newRight); } - - @Override - public ArithmeticOperation inverse(Source source, Expression left, Expression right) { - // TODO: Modular Multiplicative Inverse, if ever needed? - throw new UnsupportedOperationException("inverting modulo operation is not supported"); - } } 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 c07ffe6a8c73c..b537f1484aea4 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 @@ -54,7 +54,7 @@ public Mul swapLeftAndRight() { } @Override - public ArithmeticOperation inverse(Source source, Expression left, Expression right) { + public Div inverse(Source source, Expression left, Expression right) { return new Div(source, left, right); } } diff --git a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/operator/arithmetic/Sub.java b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/operator/arithmetic/Sub.java index 6a3ec21426fb0..e61eaf085e0af 100644 --- a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/operator/arithmetic/Sub.java +++ b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/operator/arithmetic/Sub.java @@ -29,7 +29,7 @@ protected Sub replaceChildren(Expression newLeft, Expression newRight) { } @Override - public ArithmeticOperation inverse(Source source, Expression left, Expression right) { + public Add inverse(Source source, Expression left, Expression right) { return new Add(source, left, right); } } diff --git a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/operator/comparison/BinaryComparison.java b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/operator/comparison/BinaryComparison.java index ea294331c5aae..225bd3854ee66 100644 --- a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/operator/comparison/BinaryComparison.java +++ b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/operator/comparison/BinaryComparison.java @@ -50,5 +50,10 @@ public static Integer compare(Object left, Object right) { return Comparisons.compare(left, right); } + /** + * Reverses the direction of this comparison on the comparison axis. + * Some operations like Greater/LessThan/OrEqual will behave as if the operands of a numerical comparison get multiplied with a + * negative number. Others like Not/Equal can be immutable to this operation. + */ public abstract BinaryComparison reverse(); } 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 index 8a0efd58df64d..982e4e3b2b4cc 100644 --- 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 @@ -21,8 +21,8 @@ import org.elasticsearch.xpack.ql.expression.predicate.logical.Or; import org.elasticsearch.xpack.ql.expression.predicate.nulls.IsNotNull; import org.elasticsearch.xpack.ql.expression.predicate.operator.arithmetic.ArithmeticOperation; -import org.elasticsearch.xpack.ql.expression.predicate.operator.arithmetic.Mod; import org.elasticsearch.xpack.ql.expression.predicate.operator.arithmetic.Neg; +import org.elasticsearch.xpack.ql.expression.predicate.operator.arithmetic.Sub; import org.elasticsearch.xpack.ql.expression.predicate.operator.comparison.BinaryComparison; import org.elasticsearch.xpack.ql.expression.predicate.operator.comparison.Equals; import org.elasticsearch.xpack.ql.expression.predicate.operator.comparison.GreaterThan; @@ -39,9 +39,11 @@ import org.elasticsearch.xpack.ql.plan.logical.LogicalPlan; import org.elasticsearch.xpack.ql.plan.logical.OrderBy; import org.elasticsearch.xpack.ql.rule.Rule; +import org.elasticsearch.xpack.ql.type.DataType; import org.elasticsearch.xpack.ql.type.DataTypes; import org.elasticsearch.xpack.ql.util.CollectionUtils; +import java.time.DateTimeException; import java.time.ZoneId; import java.util.ArrayList; import java.util.Iterator; @@ -51,6 +53,7 @@ import java.util.List; import java.util.Map; import java.util.Set; +import java.util.function.BiFunction; import static org.elasticsearch.xpack.ql.expression.Literal.FALSE; import static org.elasticsearch.xpack.ql.expression.Literal.TRUE; @@ -60,7 +63,11 @@ import static org.elasticsearch.xpack.ql.expression.predicate.Predicates.splitAnd; import static org.elasticsearch.xpack.ql.expression.predicate.Predicates.splitOr; import static org.elasticsearch.xpack.ql.expression.predicate.Predicates.subtract; -import static org.elasticsearch.xpack.ql.type.DataTypes.DOUBLE; +import static org.elasticsearch.xpack.ql.expression.predicate.operator.arithmetic.DefaultBinaryArithmeticOperation.DIV; +import static org.elasticsearch.xpack.ql.expression.predicate.operator.arithmetic.DefaultBinaryArithmeticOperation.MOD; +import static org.elasticsearch.xpack.ql.expression.predicate.operator.arithmetic.DefaultBinaryArithmeticOperation.MUL; +import static org.elasticsearch.xpack.ql.expression.predicate.operator.arithmetic.DefaultBinaryArithmeticOperation.SUB; +import static org.elasticsearch.xpack.ql.tree.Source.EMPTY; import static org.elasticsearch.xpack.ql.util.CollectionUtils.combine; @@ -230,6 +237,7 @@ private Expression simplifyNot(Not n) { } } + // TODO: should this be renamed to just `LiteralsOnTheRight`? It swaps all literals, not just booleans. Or `MaybeLiteralsOnTheRight`? public static final class BooleanLiteralsOnTheRight extends OptimizerExpressionRule { public BooleanLiteralsOnTheRight() { @@ -1141,47 +1149,233 @@ protected Expression rule(Expression e) { } } - public static final class SimplifyArithmeticsInBinaryComparisons extends OptimizerExpressionRule { + public static final class BubbleUpNegations extends OptimizerExpressionRule { - public SimplifyArithmeticsInBinaryComparisons() { - super(TransformDirection.UP); + public BubbleUpNegations() { + super(TransformDirection.DOWN); } @Override protected Expression rule(Expression e) { - if (e instanceof BinaryComparison) { - return reduce((BinaryComparison) e); + if (e instanceof Neg) { // cancels out NEG - MUL/DIV - NEG - x ==> MUL/DIV - x + Expression child = e.children().get(0); + if (child instanceof ArithmeticOperation) { + ArithmeticOperation operation = (ArithmeticOperation) child; + String opSymbol = operation.symbol(); + if (MUL.symbol().equals(opSymbol) || DIV.symbol().equals(opSymbol)) { + if (operation.left() instanceof Neg) { + return operation.replaceChildren(List.of(operation.left().children().get(0), operation.right())); + } + if (operation.right() instanceof Neg) { + return operation.replaceChildren(List.of(operation.left(), operation.right().children().get(0))); + } + } + } + } else if (e instanceof ArithmeticOperation) { // pulls up MUL/DIV - NEG - x ==> NEG - MUL/DIV - x + ArithmeticOperation operation = (ArithmeticOperation) e; + String opSymbol = operation.symbol(); + if (MUL.symbol().equals(opSymbol) || DIV.symbol().equals(opSymbol)) { + if (operation.left() instanceof Neg) { + Neg neg = (Neg) operation.left(); + return new Neg(operation.source(), operation.replaceChildren(List.of(neg.children().get(0), operation.right()))); + } + if (operation.right() instanceof Neg) { + Neg neg = (Neg) operation.right(); + return new Neg(operation.source(), operation.replaceChildren(List.of(operation.left(), neg.children().get(0)))); + } + } } return e; } + } - private Expression reduce(BinaryComparison bc) { - // optimize only once the expression looks like: AnyNode [ArithmOp] AnyNode [BiComp] Literal - if (bc.right() instanceof Literal == false) { - return bc; + // Simplifies arithmetic expressions with fixed point fields in BinaryComparisons. + public static final class SimplifyComparisonsArithmetics extends OptimizerExpressionRule { + BiFunction typesCompatible; + + public SimplifyComparisonsArithmetics(BiFunction typesCompatible) { + super(TransformDirection.UP); + this.typesCompatible = typesCompatible; + } + + @Override + protected Expression rule(Expression e) { + return (e instanceof BinaryComparison) ? simplify((BinaryComparison) e) : e; + } + + private Expression simplify(BinaryComparison bc) { + // optimize only once the expression has a literal on the right side of the binary comparison + if (bc.right() instanceof Literal) { + if (bc.left() instanceof ArithmeticOperation) { + return SimplifyOperation.simplify(bc, typesCompatible); + } else if (bc.left() instanceof Neg) { + return reduceNegation(bc); + } } - Literal bcRight = (Literal) bc.right(); + return bc; + } - if (bc.left() instanceof ArithmeticOperation) { - ArithmeticOperation opLeft = (ArithmeticOperation) bc.left(); - if (opLeft instanceof Mod) { - return bc; // can't optimise Mods + private static Expression reduceNegation(BinaryComparison bc) { + Literal bcLiteral = (Literal) bc.right(); + Expression neg = safeMaybeFold(new Neg(bcLiteral.source(), bcLiteral)); + return neg == null ? bc : bc.reverse().replaceChildren(List.of(bc.left().children().get(0), neg)); + } + + private static Expression safeMaybeFold(Expression expression) { + if (expression.foldable()) { + try { + expression = new Literal(expression.source(), expression.fold(), expression.dataType()); + } catch (Exception e) { + // could only catch (ArithmeticException | DateTimeException e), but safer to just turn off the optimisation + // should any exception arise. + expression = null; } + } + return expression; + } - if (opLeft.left() instanceof Literal || opLeft.right() instanceof Literal) { // 5 [op] a >= 4 || a [op] 5 >= 4 - // force double division - if (opLeft.symbol().equals("*")) { // use symbol comp for SQL's Mul - bcRight = new Literal(bcRight.source(), ((Number) bcRight.value()).doubleValue(), DOUBLE); - } - return bc.replaceChildren(List.of(opLeft.left(), opLeft.inverse(bcRight.source(), bcRight, opLeft.right()))); + private static class SimplifyOperation { + final BinaryComparison comparison; + final Literal bcLiteral; + final ArithmeticOperation operation; + final Expression opLeft; + final Expression opRight; + final Literal opLiteral; + + SimplifyOperation(BinaryComparison comparison) { + this.comparison = comparison; + operation = (ArithmeticOperation) comparison.left(); + bcLiteral = (Literal) comparison.right(); + + opLeft = operation.left(); + opRight = operation.right(); + + if (opLeft instanceof Literal) { + opLiteral = (Literal) opLeft; + } else if (opRight instanceof Literal) { + opLiteral = (Literal) opRight; + } else { + opLiteral = null; } - } else if (bc.left() instanceof Neg) { - return bc.reverse().replaceChildren(List.of(bc.left().children().get(0), new Neg(bcRight.source(), bcRight))); } - return bc; + // can it be quickly fast-tracked that the operation can't be reduced? + boolean cannotSimplify(BiFunction typesCompatible) { + if (opLiteral == null) { + // one of the arithm. operands must be a literal, otherwise the operation wouldn't simplify anything + return true; + } + + // Only operations on fixed point literals are supported, since optimizing float point operations can also change the + // outcome of the filtering: + // x + 1e18 > 1e18::long will yield different results with a field value in [-2^6, 2^6], optimised vs original; + // x * (1 + 1e-15d) > 1 : same with a field value of (1 - 1e-15d) + // so consequently, int fields optimisation requiring FP arithmetic isn't possible either: (x - 1e-15) * (1 + 1e-15) > 1. + if (opLiteral.dataType().isRational() || bcLiteral.dataType().isRational()) { + return true; + } + + // the Literal will be moved to the right of the comparison, but only if data-compatible with what's there + return typesCompatible.apply(bcLiteral.dataType(), opLiteral.dataType()) == false; + } + + Expression doSimplify() { + // force float point folding for FlP field + Literal bcl = operation.dataType().isRational() + ? Literal.of(bcLiteral, ((Number) bcLiteral.value()).doubleValue()) + : bcLiteral; + + Expression bcRightExpression = safeMaybeFold(operation.inverse(bcl.source(), bcl, opRight)); + return bcRightExpression == null ? comparison : comparison.replaceChildren(List.of(opLeft, bcRightExpression)); + } + + static Expression simplify(BinaryComparison comparison, BiFunction typesCompatible) { + ArithmeticOperation operation = (ArithmeticOperation) comparison.left(); + // Use symbol comp: SQL operations aren't available in this package (as dependencies) + String opSymbol = operation.symbol(); + // Modulo can't be simplified. + if (MOD.symbol().equals(opSymbol)) { + return comparison; + } + SimplifyOperation simplification = (MUL.symbol().equals(opSymbol) || DIV.symbol().equals(opSymbol)) + ? new SimplifyMulDiv(comparison) + : new SimplifyAddSub(comparison); + + return (simplification.cannotSimplify(typesCompatible) == false) ? simplification.doSimplify() : comparison; + } + } + + private static class SimplifyAddSub extends SimplifyOperation { + + SimplifyAddSub(BinaryComparison comparison) { + super(comparison); + } + + @Override + boolean cannotSimplify(BiFunction typesCompatible) { + // no ADD/SUB with floating fields + return operation.dataType().isRational() || super.cannotSimplify(typesCompatible); + } + + @Override + Expression doSimplify() { + if (SUB.symbol().equals(operation.symbol()) && opRight instanceof Literal == false) { + // if next simplification step would fail on overflow anyways, skip the optimisation already + if (safeMaybeFold(new Sub(EMPTY, opLeft, bcLiteral)) == null) { + return comparison; + } + } + return super.doSimplify(); + } } + private static class SimplifyMulDiv extends SimplifyOperation { + + private final boolean isDiv; // and not MUL. + + SimplifyMulDiv(BinaryComparison comparison) { + super(comparison); + isDiv = DIV.symbol().equals(operation.symbol()); + } + + @Override + boolean cannotSimplify(BiFunction typesCompatible) { + // Integer divisions are not safe to optimise: x / 5 > 1 <=/=> x > 5 for x in [6, 9]; same for the `==` comp + if (operation.dataType().isInteger()) { + if (isDiv) { + return true; + } + } else if (operation.dataType().isRational()) { + Expression opNonLiteral = opLiteral == opLeft ? opRight : opLeft; + if (opNonLiteral instanceof Neg) { + // wait until Neg bubbles all the way up + return true; + } + } + return super.cannotSimplify(typesCompatible); + } + + @Override + Expression doSimplify() { + // If current operation is a multiplication, it's inverse will be a division: safe only if outcome is still integral. + if (isDiv == false && opLeft.dataType().isInteger()) { + if (((Number) bcLiteral.value()).longValue() % ((Number) opLiteral.value()).longValue() != 0) { + return comparison; + } + } + BinaryComparison bc = (BinaryComparison) super.doSimplify(); + // negative multiplication/division changes the direction of the comparison + if (opRight instanceof Literal) { + long expLiteralValue = ((Number) ((Literal) opRight).value()).longValue(); + if (expLiteralValue < 0) { + bc = bc.reverse(); + } else if (expLiteralValue == 0) { + bc = comparison; // keep the original + } + } + return bc; + } + } } public abstract static class PruneFilters extends OptimizerRule { 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 index 12f45436bfe86..a3197dcdac0f5 100644 --- 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 @@ -20,9 +20,11 @@ import org.elasticsearch.xpack.ql.expression.predicate.logical.Or; import org.elasticsearch.xpack.ql.expression.predicate.nulls.IsNotNull; import org.elasticsearch.xpack.ql.expression.predicate.operator.arithmetic.Add; +import org.elasticsearch.xpack.ql.expression.predicate.operator.arithmetic.ArithmeticOperation; 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.Neg; import org.elasticsearch.xpack.ql.expression.predicate.operator.arithmetic.Sub; import org.elasticsearch.xpack.ql.expression.predicate.operator.comparison.BinaryComparison; import org.elasticsearch.xpack.ql.expression.predicate.operator.comparison.Equals; @@ -52,6 +54,7 @@ import java.time.ZoneId; import java.util.Collections; import java.util.List; +import java.util.function.Function; import static java.util.Arrays.asList; import static java.util.Collections.emptyMap; @@ -69,6 +72,7 @@ import static org.elasticsearch.xpack.ql.expression.Literal.FALSE; import static org.elasticsearch.xpack.ql.expression.Literal.NULL; import static org.elasticsearch.xpack.ql.expression.Literal.TRUE; +import static org.elasticsearch.xpack.ql.optimizer.OptimizerRules.BubbleUpNegations; import static org.elasticsearch.xpack.ql.optimizer.OptimizerRules.CombineDisjunctionsToIn; import static org.elasticsearch.xpack.ql.optimizer.OptimizerRules.ReplaceRegexMatch; import static org.elasticsearch.xpack.ql.tree.Source.EMPTY; @@ -1537,4 +1541,24 @@ public void testOrWithNonCombinableExpressions() throws Exception { assertEquals(fa, in.value()); assertThat(in.list(), contains(one, three)); } + + public void testBubbleUpNegation() { + for (Function f : List.>of(x -> new Div(EMPTY, TWO, x), + x -> new Mul(EMPTY, TWO, x))) { + FieldAttribute fa = getFieldAttribute(); + Neg neg = new Neg(fa.source(), fa); + Expression op = f.apply(neg); + + Expression e = new BubbleUpNegations().rule(op); + assertEquals(Neg.class, e.getClass()); + Neg upperNeg = (Neg) e; + assertEquals(1, upperNeg.children().size()); + assertEquals(op.getClass(), upperNeg.children().get(0).getClass()); + ArithmeticOperation newOp = (ArithmeticOperation) upperNeg.children().get(0); + assertEquals(TWO, newOp.left()); + assertEquals(FieldAttribute.class, newOp.right().getClass()); + FieldAttribute divFa = (FieldAttribute) newOp.right(); + assertEquals(fa, divFa); + } + } } diff --git a/x-pack/plugin/ql/src/test/resources/mapping-multi-field-variation.json b/x-pack/plugin/ql/src/test/resources/mapping-multi-field-variation.json index c75ecfdc845c0..6021efc1f026e 100644 --- a/x-pack/plugin/ql/src/test/resources/mapping-multi-field-variation.json +++ b/x-pack/plugin/ql/src/test/resources/mapping-multi-field-variation.json @@ -2,6 +2,7 @@ "properties" : { "bool" : { "type" : "boolean" }, "int" : { "type" : "integer" }, + "float" : { "type" : "float" }, "text" : { "type" : "text" }, "keyword" : { "type" : "keyword" }, "date" : { "type" : "date" }, diff --git a/x-pack/plugin/sql/qa/server/src/main/resources/arithmetic.csv-spec b/x-pack/plugin/sql/qa/server/src/main/resources/arithmetic.csv-spec index 31b2417932aa4..e66f90e5f6ab3 100644 --- a/x-pack/plugin/sql/qa/server/src/main/resources/arithmetic.csv-spec +++ b/x-pack/plugin/sql/qa/server/src/main/resources/arithmetic.csv-spec @@ -23,7 +23,41 @@ nullArithmetics schema::a:i|b:d|c:s|d:s|e:l|f:i|g:i|h:i|i:i|j:i|k:d SELECT null + 2 AS a, null * 1.5 AS b, null + null AS c, null - null AS d, null - 1234567890123 AS e, 123 - null AS f, null / 5 AS g, 5 / null AS h, null % 5 AS i, 5 % null AS j, null + 5.5 - (null * (null * 3)) AS k; - a | b | c | d | e | f | g | h | i | j | k + a | b | c | d | e | f | g | h | i | j | k ---------------+---------------+---------------+---------------+---------------+---------------+---------------+---------------+---------------+---------------+--------------- -null |null |null |null |null |null |null |null |null |null |null +null |null |null |null |null |null |null |null |null |null |null +; + +optimizedIntervalFilterPlus +SELECT emp_no x FROM test_emp WHERE hire_date + INTERVAL 20 YEAR > CAST('2010-01-01T00:00:00' AS TIMESTAMP) LIMIT 10; + + x:i +--------------- +10008 +10011 +10012 +10016 +10017 +10019 +10020 +10022 +10024 +10026 +; + +optimizedIntervalFilterMinus +SELECT emp_no x FROM test_emp WHERE hire_date - INTERVAL 10 YEAR > CAST('1980-01-01T00:00:00' AS TIMESTAMP) LIMIT 10; + + x:i +--------------- +10008 +10011 +10012 +10016 +10017 +10019 +10020 +10022 +10024 +10026 ; diff --git a/x-pack/plugin/sql/qa/server/src/main/resources/arithmetic.sql-spec b/x-pack/plugin/sql/qa/server/src/main/resources/arithmetic.sql-spec index d94123ea0642f..3687cea3235f7 100644 --- a/x-pack/plugin/sql/qa/server/src/main/resources/arithmetic.sql-spec +++ b/x-pack/plugin/sql/qa/server/src/main/resources/arithmetic.sql-spec @@ -87,3 +87,149 @@ orderByModulo SELECT emp_no FROM test_emp ORDER BY emp_no % 10000 LIMIT 10; orderByMul SELECT emp_no FROM test_emp ORDER BY emp_no * 2 LIMIT 10; + +// arithmetic optimiser +plusMulIntFieldPlus +SELECT emp_no FROM test_emp WHERE 1000 + 2 * (salary + 50000) > 201000; +mulIntFieldPlus +SELECT emp_no FROM test_emp WHERE -2 * (salary + 50000) - 1000 <= -201000; +plusIntFieldPlusDiv +SELECT emp_no FROM test_emp WHERE 1000 + (salary + 50000) / 2 > 101000; +intFieldPlusDiv +SELECT emp_no FROM test_emp WHERE (salary + 50000) / 2 + 1000 > 101000; +plusMulPlusIntField +SELECT emp_no FROM test_emp WHERE 1000 + 2 * (50000 + salary) > 201000; +mulPlusIntFieldPlus +SELECT emp_no FROM test_emp WHERE -2 * (50000 + salary) - 1000 <= -201000; +plusPlusIntFieldDiv +SELECT emp_no FROM test_emp WHERE 1000 + (50000 + salary) / 2 > 51000; +plusIntFieldDivPlus +SELECT emp_no FROM test_emp WHERE (50000 + salary) / 2 + 1000 > 51000; +plusMulIntFieldMinus +SELECT emp_no FROM test_emp WHERE 1000 + 2 * (salary - 10000) > 60000; +mulIntFieldMinusPlus +SELECT emp_no FROM test_emp WHERE -2 * (salary - 10000) - 1000 <= -61000; +plusIntFieldMinusDiv +SELECT emp_no FROM test_emp WHERE 1000 + (salary - 10000) / 2 > 16000; +intFieldMinusDivPlus +SELECT emp_no FROM test_emp WHERE (salary - 10000) / 2 + 1000 > 16000; +plusMulMinusIntField +SELECT emp_no FROM test_emp WHERE 1000 + 2 * (100000 - salary) > 101000; +mulMinusIntFieldPlus +SELECT emp_no FROM test_emp WHERE -2 * (100000 - salary) - 1000 <= -101000; +plusMinusIntFieldDiv +SELECT emp_no FROM test_emp WHERE 1000 + (100000 - salary) / 2 > 26000; +minusIntFieldDivPlus +SELECT emp_no FROM test_emp WHERE (100000 - salary) / 2 + 1000 > 25000; +plusMulIntFieldMul +SELECT emp_no FROM test_emp WHERE 1000 + 2 * (salary * 2) > 201000; +mulIntFieldMulPlus +SELECT emp_no FROM test_emp WHERE 2 * (-salary * 2) - 1000 <= -200000; +plusIntFieldMulDiv +SELECT emp_no FROM test_emp WHERE 1000 + (salary * 2) / 2 > 50100; +intFieldMulDivPlus +SELECT emp_no FROM test_emp WHERE (salary * 2) / 2 + 1000 > 50100; +plusMulMulIntField +SELECT emp_no FROM test_emp WHERE 1000 + 2 * (2 * salary) > 201000; +mulMulIntFieldPlus +SELECT emp_no FROM test_emp WHERE 2 * (2 * -salary) - 1000 <= -201000; +plusMulIntFieldDiv +SELECT emp_no FROM test_emp WHERE 1000 + (2 * salary) / 2 > 51000; +mulIntFieldDivPlus +SELECT emp_no FROM test_emp WHERE (2 * salary) / 2 + 1000 > 51000; +plusMulIntFieldDiv +SELECT emp_no FROM test_emp WHERE 1000 + 2 * (salary / 2) > 61000; +mulIntFieldDivPlus +SELECT emp_no FROM test_emp WHERE 2 * (-salary / 2) - 1000 <= -61000; +plusIntFieldDivDiv +SELECT emp_no FROM test_emp WHERE 1000 + (salary / 2) / 2 > 16000; +intFieldDivDivPlus +SELECT emp_no FROM test_emp WHERE (salary / 2) / 2 + 1000 > 16000; +plusMulDivIntField +SELECT emp_no FROM test_emp WHERE 1000 + 2 * (1000000000 / salary) > 61000; +mulDivIntFieldPlus +SELECT emp_no FROM test_emp WHERE 2 * (1000000000 / -salary) - 1000 <= -61000; +plusDivIntFieldDiv +SELECT emp_no FROM test_emp WHERE 1000 + (1000000000 / salary) / 2 > 16000; +divIntFieldDivPlus +SELECT emp_no FROM test_emp WHERE (1000000000 / salary) / 2 + 1000 > 16000; + +plusMulFloatFieldPlus +SELECT emp_no FROM test_emp WHERE 1000 + 2 * (salary::FLOAT + 50000) > 201000; +mulFloatFieldPlus +SELECT emp_no FROM test_emp WHERE -2 * (salary::FLOAT + 50000) - 1000 <= -201000; +plusFloatFieldPlusDiv +SELECT emp_no FROM test_emp WHERE 1000 + (salary::FLOAT + 50000) / 2 > 101000; +floatFieldPlusDiv +SELECT emp_no FROM test_emp WHERE (salary::FLOAT + 50000) / 2 + 1000 > 101000; +plusMulPlusFloatField +SELECT emp_no FROM test_emp WHERE 1000 + 2 * (50000 + salary::FLOAT) > 201000; +mulPlusFloatFieldPlus +SELECT emp_no FROM test_emp WHERE -2 * (50000 + salary::FLOAT) - 1000 <= -201000; +plusPlusFloatFieldDiv +SELECT emp_no FROM test_emp WHERE 1000 + (50000 + salary::FLOAT) / 2 > 51000; +plusFloatFieldDivPlus +SELECT emp_no FROM test_emp WHERE (50000 + salary::FLOAT) / 2 + 1000 > 51000; +plusMulFloatFieldMinus +SELECT emp_no FROM test_emp WHERE 1000 + 2 * (salary::FLOAT - 10000) > 60000; +mulFloatFieldMinusPlus +SELECT emp_no FROM test_emp WHERE -2 * (salary::FLOAT - 10000) - 1000 <= -61000; +plusFloatFieldMinusDiv +SELECT emp_no FROM test_emp WHERE 1000 + (salary::FLOAT - 10000) / 2 > 16000; +floatFieldMinusDivPlus +SELECT emp_no FROM test_emp WHERE (salary::FLOAT - 10000) / 2 + 1000 > 16000; +plusMulMinusFloatField +SELECT emp_no FROM test_emp WHERE 1000 + 2 * (100000 - salary::FLOAT) > 101000; +mulMinusFloatFieldPlus +SELECT emp_no FROM test_emp WHERE -2 * (100000 - salary::FLOAT) - 1000 <= -101000; +plusMinusFloatFieldDiv +SELECT emp_no FROM test_emp WHERE 1000 + (100000 - salary::FLOAT) / 2 > 26000; +minusFloatFieldDivPlus +SELECT emp_no FROM test_emp WHERE (100000 - salary::FLOAT) / 2 + 1000 > 25000; +plusMulFloatFieldMul +SELECT emp_no FROM test_emp WHERE 1000 + 2 * (salary::FLOAT * 2) > 201000; +mulFloatFieldMulPlus +SELECT emp_no FROM test_emp WHERE 2 * (-salary::FLOAT * 2) - 1000 <= -200000; +plusFloatFieldMulDiv +SELECT emp_no FROM test_emp WHERE 1000 + (salary::FLOAT * 2) / 2 > 50100; +floatFieldMulDivPlus +SELECT emp_no FROM test_emp WHERE (salary::FLOAT * 2) / 2 + 1000 > 50100; +plusMulMulFloatField +SELECT emp_no FROM test_emp WHERE 1000 + 2 * (2 * salary::FLOAT) > 201000; +mulMulFloatFieldPlus +SELECT emp_no FROM test_emp WHERE 2 * (2 * -salary::FLOAT) - 1000 <= -201000; +plusMulFloatFieldDiv +SELECT emp_no FROM test_emp WHERE 1000 + (2 * salary::FLOAT) / 2 > 51000; +mulFloatFieldDivPlus +SELECT emp_no FROM test_emp WHERE (2 * salary::FLOAT) / 2 + 1000 > 51000; +plusMulFloatFieldDiv +SELECT emp_no FROM test_emp WHERE 1000 + 2 * (salary::FLOAT / 2) > 61000; +mulFloatFieldDivPlus +SELECT emp_no FROM test_emp WHERE 2 * (-salary::FLOAT / 2) - 1000 <= -61000; +plusFloatFieldDivDiv +SELECT emp_no FROM test_emp WHERE 1000 + (salary::FLOAT / 2) / 2 > 16000; +floatFieldDivDivPlus +SELECT emp_no FROM test_emp WHERE (salary::FLOAT / 2) / 2 + 1000 > 16000; +plusMulDivFloatField +SELECT emp_no FROM test_emp WHERE 1000 + 2 * (1000000000 / salary::FLOAT) > 61000; +mulDivFloatFieldPlus +SELECT emp_no FROM test_emp WHERE 2 * (1000000000 / -salary::FLOAT) - 1000 <= -61000; +plusDivFloatFieldDiv +SELECT emp_no FROM test_emp WHERE 1000 + (1000000000 / salary::FLOAT) / 2 > 16000; +divFloatFieldDivPlus +SELECT emp_no FROM test_emp WHERE (1000000000 / salary::FLOAT) / 2 + 1000 > 16000; + +noOptimisationOnOverflowAdd +SELECT emp_no FROM test_emp WHERE salary - 2 < 9223372036854775807; +noOptimisationOnUnderflowSub +SELECT emp_no FROM test_emp WHERE -salary + 2 < -9223372036854775807; +noOptimisationOnOverflowMul +SELECT emp_no FROM test_emp WHERE salary / 10 < 1.7976931348623157E308; +noOptimisationOnPrecisionLossOnFloatFieldAdd +SELECT emp_no FROM test_emp WHERE 1 - salary::FLOAT/1E21 < 1; +noOptimisationOnPrecisionLossOnFloatFieldDiv +SELECT emp_no FROM test_emp WHERE (1 - salary::FLOAT / 1E21) * (1 + 1E-15) > 1; + +multipleNegations +SELECT emp_no FROM test_emp WHERE ((1000000000 / -salary) / 2) * (-1 / (2 / -emp_no::FLOAT)) / -1000 >= 50000; + 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 b60dc88c0c85b..09eff19ee1853 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 @@ -6,7 +6,6 @@ package org.elasticsearch.xpack.sql.expression.predicate.operator.arithmetic; import org.elasticsearch.xpack.ql.expression.Expression; -import org.elasticsearch.xpack.ql.expression.predicate.operator.arithmetic.ArithmeticOperation; import org.elasticsearch.xpack.ql.tree.NodeInfo; import org.elasticsearch.xpack.ql.tree.Source; @@ -34,7 +33,7 @@ public Add swapLeftAndRight() { } @Override - public ArithmeticOperation inverse(Source source, Expression left, Expression right) { + public Sub inverse(Source source, Expression left, Expression right) { return new Sub(source, left, right); } } diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/operator/arithmetic/Div.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/operator/arithmetic/Div.java index 75c7f8b6b2dfa..1492d5d55dae6 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/operator/arithmetic/Div.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/operator/arithmetic/Div.java @@ -6,7 +6,6 @@ package org.elasticsearch.xpack.sql.expression.predicate.operator.arithmetic; import org.elasticsearch.xpack.ql.expression.Expression; -import org.elasticsearch.xpack.ql.expression.predicate.operator.arithmetic.ArithmeticOperation; import org.elasticsearch.xpack.ql.expression.predicate.operator.arithmetic.Mul; import org.elasticsearch.xpack.ql.tree.NodeInfo; import org.elasticsearch.xpack.ql.tree.Source; @@ -38,7 +37,7 @@ public DataType dataType() { } @Override - public ArithmeticOperation inverse(Source source, Expression left, Expression right) { + public Mul inverse(Source source, Expression left, Expression right) { return new Mul(source, left, right); } } diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/operator/arithmetic/Mod.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/operator/arithmetic/Mod.java index b41ef57511bea..2924e6e468162 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/operator/arithmetic/Mod.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/operator/arithmetic/Mod.java @@ -6,7 +6,6 @@ package org.elasticsearch.xpack.sql.expression.predicate.operator.arithmetic; import org.elasticsearch.xpack.ql.expression.Expression; -import org.elasticsearch.xpack.ql.expression.predicate.operator.arithmetic.ArithmeticOperation; import org.elasticsearch.xpack.ql.tree.NodeInfo; import org.elasticsearch.xpack.ql.tree.Source; @@ -31,10 +30,4 @@ protected NodeInfo info() { protected Mod replaceChildren(Expression newLeft, Expression newRight) { return new Mod(source(), newLeft, newRight); } - - @Override - public ArithmeticOperation inverse(Source source, Expression left, Expression right) { - // TODO: Modular Multiplicative Inverse, if ever needed? - throw new UnsupportedOperationException("inverting modulo operation is not supported"); - } } 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 a7c4e619328dc..72b15c65d6434 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 @@ -6,7 +6,6 @@ package org.elasticsearch.xpack.sql.expression.predicate.operator.arithmetic; import org.elasticsearch.xpack.ql.expression.Expression; -import org.elasticsearch.xpack.ql.expression.predicate.operator.arithmetic.ArithmeticOperation; import org.elasticsearch.xpack.ql.tree.NodeInfo; import org.elasticsearch.xpack.ql.tree.Source; import org.elasticsearch.xpack.ql.type.DataType; @@ -60,7 +59,7 @@ public DataType dataType() { } @Override - public ArithmeticOperation inverse(Source source, Expression left, Expression right) { + public Div inverse(Source source, Expression left, Expression right) { return new Div(source, left, right); } diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/operator/arithmetic/SqlBinaryArithmeticOperation.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/operator/arithmetic/SqlBinaryArithmeticOperation.java index 04be1399a1115..661a7c3f6500f 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/operator/arithmetic/SqlBinaryArithmeticOperation.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/operator/arithmetic/SqlBinaryArithmeticOperation.java @@ -52,7 +52,7 @@ public enum SqlBinaryArithmeticOperation implements BinaryArithmeticOperation { return IntervalArithmetics.add((Temporal) r, ((IntervalDayTime) l).interval()); } - throw new QlIllegalArgumentException("Cannot compute [+] between [{}] [{}]", l.getClass().getSimpleName(), + throw new QlIllegalArgumentException("Cannot compute [+] between [{}] and [{}]", l.getClass().getSimpleName(), r.getClass().getSimpleName()); }, "+"), SUB((Object l, Object r) -> { @@ -77,7 +77,7 @@ public enum SqlBinaryArithmeticOperation implements BinaryArithmeticOperation { throw new QlIllegalArgumentException("Cannot subtract a date from an interval; do you mean the reverse?"); } - throw new QlIllegalArgumentException("Cannot compute [-] between [{}] [{}]", l.getClass().getSimpleName(), + throw new QlIllegalArgumentException("Cannot compute [-] between [{}] and [{}]", l.getClass().getSimpleName(), r.getClass().getSimpleName()); }, "-"), MUL((Object l, Object r) -> { @@ -99,7 +99,7 @@ public enum SqlBinaryArithmeticOperation implements BinaryArithmeticOperation { return ((IntervalDayTime) l).mul(((Number) r).longValue()); } - throw new QlIllegalArgumentException("Cannot compute [*] between [{}] [{}]", l.getClass().getSimpleName(), + throw new QlIllegalArgumentException("Cannot compute [*] between [{}] and [{}]", l.getClass().getSimpleName(), r.getClass().getSimpleName()); }, "*"), DIV(Arithmetics::div, "/"), @@ -151,4 +151,4 @@ public static SqlBinaryArithmeticOperation read(StreamInput in) throws IOExcepti private static Object unwrapJodaTime(Object o) { return o instanceof JodaCompatibleZonedDateTime ? ((JodaCompatibleZonedDateTime) o).getZonedDateTime() : o; } -} \ No newline at end of file +} diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/operator/arithmetic/Sub.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/operator/arithmetic/Sub.java index 2c702f0e2dbed..bda9855d3eeb8 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/operator/arithmetic/Sub.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/operator/arithmetic/Sub.java @@ -6,7 +6,6 @@ package org.elasticsearch.xpack.sql.expression.predicate.operator.arithmetic; import org.elasticsearch.xpack.ql.expression.Expression; -import org.elasticsearch.xpack.ql.expression.predicate.operator.arithmetic.ArithmeticOperation; import org.elasticsearch.xpack.ql.tree.NodeInfo; import org.elasticsearch.xpack.ql.tree.Source; import org.elasticsearch.xpack.sql.type.SqlDataTypes; @@ -46,7 +45,7 @@ protected TypeResolution resolveWithIntervals() { } @Override - public ArithmeticOperation inverse(Source source, Expression left, Expression right) { + public Add inverse(Source source, Expression left, Expression right) { return new Add(source, left, right); } } diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/optimizer/Optimizer.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/optimizer/Optimizer.java index a865b38fe0893..690c8135c1e65 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/optimizer/Optimizer.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/optimizer/Optimizer.java @@ -34,9 +34,9 @@ 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.expression.predicate.operator.comparison.NullEquals; -import org.elasticsearch.xpack.ql.optimizer.OptimizerRules.SimplifyArithmeticsInBinaryComparisons; import org.elasticsearch.xpack.ql.optimizer.OptimizerRules.BooleanLiteralsOnTheRight; import org.elasticsearch.xpack.ql.optimizer.OptimizerRules.BooleanSimplification; +import org.elasticsearch.xpack.ql.optimizer.OptimizerRules.BubbleUpNegations; import org.elasticsearch.xpack.ql.optimizer.OptimizerRules.CombineBinaryComparisons; import org.elasticsearch.xpack.ql.optimizer.OptimizerRules.ConstantFolding; import org.elasticsearch.xpack.ql.optimizer.OptimizerRules.OptimizerExpressionRule; @@ -45,6 +45,7 @@ import org.elasticsearch.xpack.ql.optimizer.OptimizerRules.PruneLiteralsInOrderBy; import org.elasticsearch.xpack.ql.optimizer.OptimizerRules.ReplaceRegexMatch; import org.elasticsearch.xpack.ql.optimizer.OptimizerRules.SetAsOptimized; +import org.elasticsearch.xpack.ql.optimizer.OptimizerRules.SimplifyComparisonsArithmetics; import org.elasticsearch.xpack.ql.optimizer.OptimizerRules.TransformDirection; import org.elasticsearch.xpack.ql.plan.logical.Aggregate; import org.elasticsearch.xpack.ql.plan.logical.EsRelation; @@ -90,6 +91,7 @@ import org.elasticsearch.xpack.sql.plan.logical.SubQueryAlias; import org.elasticsearch.xpack.sql.session.EmptyExecutable; import org.elasticsearch.xpack.sql.session.SingletonExecutable; +import org.elasticsearch.xpack.sql.type.SqlDataTypes; import java.time.ZoneId; import java.util.ArrayList; @@ -140,7 +142,6 @@ protected Iterable.Batch> batches() { new ConstantFolding(), new SimplifyConditional(), new SimplifyCase(), - new SimplifyArithmeticsInBinaryComparisons(), // boolean new BooleanSimplification(), new BooleanLiteralsOnTheRight(), @@ -149,6 +150,8 @@ protected Iterable.Batch> batches() { new PropagateEquals(), new CombineBinaryComparisons(), new CombineDisjunctionsToIn(), + new BubbleUpNegations(), + new SimplifyComparisonsArithmetics(SqlDataTypes::areCompatible), // prune/elimination new PruneLiteralsInGroupBy(), new PruneDuplicatesInGroupBy(), @@ -159,7 +162,7 @@ protected Iterable.Batch> batches() { new PruneCast(), // order by alignment of the aggs new SortAggregateOnOrderBy() - ); + ); Batch aggregate = new Batch("Aggregation Rewrite", new ReplaceMinMaxWithTopHits(), diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/type/SqlDataTypes.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/type/SqlDataTypes.java index 9b0ed868174b9..a7420222141ca 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/type/SqlDataTypes.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/type/SqlDataTypes.java @@ -283,6 +283,7 @@ public static boolean areCompatible(DataType left, DataType right) { || (DataTypes.isString(left) && DataTypes.isString(right)) || (left.isNumeric() && right.isNumeric()) || (isDateBased(left) && isDateBased(right)) + || (isInterval(left) && isDateBased(right)) || (isDateBased(left) && isInterval(right)) || (isInterval(left) && isInterval(right) && Intervals.compatibleInterval(left, right) != null); } } diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/analysis/analyzer/FieldAttributeTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/analysis/analyzer/FieldAttributeTests.java index 7b8582dcbc3a0..f93417c6ff95b 100644 --- a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/analysis/analyzer/FieldAttributeTests.java +++ b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/analysis/analyzer/FieldAttributeTests.java @@ -165,7 +165,7 @@ public void testDottedFieldPathTypo() { public void testStarExpansionExcludesObjectAndUnsupportedTypes() { LogicalPlan plan = plan("SELECT * FROM test"); List list = ((Project) plan).projections(); - assertThat(list, hasSize(10)); + assertThat(list, hasSize(11)); List names = Expressions.names(list); assertThat(names, not(hasItem("some"))); assertThat(names, not(hasItem("some.dotted"))); diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/expression/predicate/operator/arithmetic/SqlBinaryArithmeticTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/expression/predicate/operator/arithmetic/SqlBinaryArithmeticTests.java index 3b3f6c003732f..909de206e0472 100644 --- a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/expression/predicate/operator/arithmetic/SqlBinaryArithmeticTests.java +++ b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/expression/predicate/operator/arithmetic/SqlBinaryArithmeticTests.java @@ -136,7 +136,7 @@ public void testAddDayTimeIntervalToTimeReverse() { public void testAddNumberToIntervalIllegal() { Literal r = interval(Duration.ofHours(2), INTERVAL_HOUR); QlIllegalArgumentException expect = expectThrows(QlIllegalArgumentException.class, () -> add(r, L(1))); - assertEquals("Cannot compute [+] between [IntervalDayTime] [Integer]", expect.getMessage()); + assertEquals("Cannot compute [+] between [IntervalDayTime] and [Integer]", expect.getMessage()); } public void testSubYearMonthIntervals() { @@ -210,7 +210,7 @@ public void testSubDayTimeIntervalToTime() { public void testSubNumberFromIntervalIllegal() { Literal r = interval(Duration.ofHours(2), INTERVAL_HOUR); QlIllegalArgumentException expect = expectThrows(QlIllegalArgumentException.class, () -> sub(r, L(1))); - assertEquals("Cannot compute [-] between [IntervalDayTime] [Integer]", expect.getMessage()); + assertEquals("Cannot compute [-] between [IntervalDayTime] and [Integer]", expect.getMessage()); } public void testMulIntervalNumber() { @@ -228,14 +228,14 @@ public void testMulNumberInterval() { Period p = interval.interval(); assertEquals(Period.ofYears(2).negated(), p); } - + public void testMulNullInterval() { Literal literal = interval(Period.ofMonths(1), INTERVAL_MONTH); Mul result = new Mul(EMPTY, L(null), literal); assertTrue(result.foldable()); assertNull(result.fold()); assertEquals(INTERVAL_MONTH, result.dataType()); - + result = new Mul(EMPTY, literal, L(null)); assertTrue(result.foldable()); assertNull(result.fold()); @@ -248,7 +248,7 @@ public void testAddNullInterval() { assertTrue(result.foldable()); assertNull(result.fold()); assertEquals(INTERVAL_MONTH, result.dataType()); - + result = new Add(EMPTY, literal, L(null)); assertTrue(result.foldable()); assertNull(result.fold()); @@ -261,7 +261,7 @@ public void testSubNullInterval() { assertTrue(result.foldable()); assertNull(result.fold()); assertEquals(INTERVAL_MONTH, result.dataType()); - + result = new Sub(EMPTY, literal, L(null)); assertTrue(result.foldable()); assertNull(result.fold()); @@ -298,4 +298,4 @@ private static Literal interval(TemporalAmount value, DataType intervalType) { : new IntervalDayTime((Duration) value, intervalType); return new Literal(EMPTY, i, SqlDataTypes.fromJava(i)); } -} \ No newline at end of file +} diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/optimizer/OptimizerRunTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/optimizer/OptimizerRunTests.java index 037035ec19a68..aea636c3b196f 100644 --- a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/optimizer/OptimizerRunTests.java +++ b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/optimizer/OptimizerRunTests.java @@ -5,17 +5,24 @@ */ package org.elasticsearch.xpack.sql.optimizer; +import org.elasticsearch.common.collect.Tuple; import org.elasticsearch.test.ESTestCase; +import org.elasticsearch.xpack.ql.expression.Expression; import org.elasticsearch.xpack.ql.expression.FieldAttribute; import org.elasticsearch.xpack.ql.expression.Literal; +import org.elasticsearch.xpack.ql.expression.UnresolvedAttribute; import org.elasticsearch.xpack.ql.expression.function.FunctionRegistry; import org.elasticsearch.xpack.ql.expression.predicate.operator.comparison.BinaryComparison; 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.expression.predicate.operator.comparison.NullEquals; import org.elasticsearch.xpack.ql.index.EsIndex; import org.elasticsearch.xpack.ql.index.IndexResolution; +import org.elasticsearch.xpack.ql.optimizer.OptimizerRules.BooleanLiteralsOnTheRight; import org.elasticsearch.xpack.ql.plan.logical.Filter; import org.elasticsearch.xpack.ql.plan.logical.LogicalPlan; import org.elasticsearch.xpack.ql.plan.logical.UnaryPlan; @@ -27,7 +34,19 @@ import org.elasticsearch.xpack.sql.stats.Metrics; import org.elasticsearch.xpack.sql.types.SqlTypesTests; +import java.time.ZonedDateTime; +import java.util.HashMap; +import java.util.List; import java.util.Map; +import java.util.stream.Collectors; + +import static org.elasticsearch.xpack.ql.expression.predicate.operator.comparison.BinaryComparisonProcessor.BinaryComparisonOperation.EQ; +import static org.elasticsearch.xpack.ql.expression.predicate.operator.comparison.BinaryComparisonProcessor.BinaryComparisonOperation.GT; +import static org.elasticsearch.xpack.ql.expression.predicate.operator.comparison.BinaryComparisonProcessor.BinaryComparisonOperation.GTE; +import static org.elasticsearch.xpack.ql.expression.predicate.operator.comparison.BinaryComparisonProcessor.BinaryComparisonOperation.LT; +import static org.elasticsearch.xpack.ql.expression.predicate.operator.comparison.BinaryComparisonProcessor.BinaryComparisonOperation.LTE; +import static org.elasticsearch.xpack.ql.expression.predicate.operator.comparison.BinaryComparisonProcessor.BinaryComparisonOperation.NEQ; +import static org.elasticsearch.xpack.ql.expression.predicate.operator.comparison.BinaryComparisonProcessor.BinaryComparisonOperation.NULLEQ; public class OptimizerRunTests extends ESTestCase { @@ -36,6 +55,18 @@ public class OptimizerRunTests extends ESTestCase { private final FunctionRegistry functionRegistry; private final Analyzer analyzer; private final Optimizer optimizer; + private static final Map> COMPARISONS = new HashMap<>() { + { + put(EQ.symbol(), Equals.class); + put(NULLEQ.symbol(), NullEquals.class); + put(NEQ.symbol(), NotEquals.class); + put(GT.symbol(), GreaterThan.class); + put(GTE.symbol(), GreaterThanOrEqual.class); + put(LT.symbol(), LessThan.class); + put(LTE.symbol(), LessThanOrEqual.class); + } + }; + private static final BooleanLiteralsOnTheRight LITERALS_ON_THE_RIGHT = new BooleanLiteralsOnTheRight(); public OptimizerRunTests() { parser = new SqlParser(); @@ -58,56 +89,172 @@ public void testWhereClause() { assertNotNull(p); } - public void testReducedBinaryComparisonGreaterThenOrEqual() { - // i >= 12 - doTestBinaryComparisonReduction("((int + 1) / 2 - 3) * 4 >= 14", GreaterThanOrEqual.class, 12d); + public void testSimplifyComparisonArithmeticCommutativeVsNonCommutativeOps() { + doTestSimplifyComparisonArithmetics("int + 2 > 3", "int", ">", 1); + doTestSimplifyComparisonArithmetics("2 + int > 3", "int", ">", 1); + doTestSimplifyComparisonArithmetics("int - 2 > 3", "int", ">", 5); + doTestSimplifyComparisonArithmetics("2 - int > 3", "int", "<", -1); + doTestSimplifyComparisonArithmetics("int * 2 > 4", "int", ">", 2); + doTestSimplifyComparisonArithmetics("2 * int > 4", "int", ">", 2); + doTestSimplifyComparisonArithmetics("float / 2 > 4", "float", ">", 8d); + doTestSimplifyComparisonArithmetics("2 / float < 4", "float", ">", .5); } - public void testReducedBinaryComYparisonLessThen() { - // i < -5/6 - doTestBinaryComparisonReduction("12 * (-int / 5) > (8 + 12) / 10", LessThan.class, -5d / 6); + public void testSimplifyComparisonArithmeticWithMultipleOps() { + // i >= 3 + doTestSimplifyComparisonArithmetics("((int + 1) * 2 - 4) * 4 >= 16", "int", ">=", 3); } - public void testReducedBinaryComYparisonNotEquals() { - // i != 7000 - doTestBinaryComparisonReduction("-3600 != (int - 200) / 2", NotEquals.class, -7000); + public void testSimplifyComparisonArithmeticWithFieldNegation() { + doTestSimplifyComparisonArithmetics("12 * (-int - 5) >= -120", "int", "<=", 5); } - public void testReducedBinaryComparisonEquals() { - // i = -12 - doTestBinaryComparisonReduction("2 * 3 / (4 / -int) = 18", Equals.class, -12d); + public void testSimplifyComparisonArithmeticWithFieldDoubleNegation() { + doTestSimplifyComparisonArithmetics("12 * -(-int - 5) <= 120", "int", "<=", 5); } - public void testReducedBinaryComparisonWithConjunction() { - doTestBinaryComparisonReduction("2 * 3 / (4 / -int) = 18 AND int >= -12", Equals.class, -12d); + public void testSimplifyComparisonArithmeticWithConjunction() { + doTestSimplifyComparisonArithmetics("12 * (-int - 5) = -120 AND int < 6 ", "int", "==", 5); } - public void testReducedBinaryComparisonWithDisjunction() { - doTestBinaryComparisonReduction("2 * 3 / (4 / -int) = 18 OR int > -12", GreaterThanOrEqual.class, -12d); + public void testSimplifyComparisonArithmeticWithDisjunction() { + doTestSimplifyComparisonArithmetics("12 * (-int - 5) >= -120 OR int < 5", "int", "<=", 5); } - private void doTestBinaryComparisonReduction(String expression, Class binaryComparisonClass, - Number bound) { - LogicalPlan plan = plan("SELECT some.string FROM test WHERE " + expression); + public void testSimplifyComparisonArithmeticWithFloatsAndDirectionChange() { + doTestSimplifyComparisonArithmetics("float / -2 < 4", "float", ">", -8d); + doTestSimplifyComparisonArithmetics("float * -2 < 4", "float", ">", -2d); + } - assertTrue(plan instanceof UnaryPlan); - UnaryPlan unaryPlan = (UnaryPlan) plan; - assertTrue(unaryPlan.child() instanceof Filter); - Filter filter = (Filter) unaryPlan.child(); - assertEquals(binaryComparisonClass, filter.condition().getClass()); - BinaryComparison bc = (BinaryComparison) filter.condition(); + public void testSimplifyComparisonArithmeticSkippedOnIntegerArithmeticalOverflow() { + assertNotSimplified("int - 1 " + randomBinaryComparison() + " " + Long.MAX_VALUE); + assertNotSimplified("1 - int " + randomBinaryComparison() + " " + Long.MIN_VALUE); + assertNotSimplified("int - 1 " + randomBinaryComparison() + " " + Integer.MAX_VALUE); + assertNotSimplified("1 - int " + randomBinaryComparison() + " " + Integer.MIN_VALUE); + } + + public void testSimplifyComparisonArithmeticSkippedOnFloatingPointArithmeticalOverflow() { + assertNotSimplified("float / 10 " + randomBinaryComparison() + " " + Float.MAX_VALUE); + assertNotSimplified("float / " + Float.MAX_VALUE +" " + randomBinaryComparison() + " 10"); + assertNotSimplified("float / 10 " + randomBinaryComparison() + " " + Double.MAX_VALUE); + assertNotSimplified("float / " + Double.MAX_VALUE + " " + randomBinaryComparison() + " 10"); + // note: the "reversed" test (i.e.: MAX_VALUE / float < literal) would require a floating literal, which is skipped for other + // reason (see testSimplifyComparisonArithmeticSkippedOnFloats()) + } + + public void testSimplifyComparisonArithmeticSkippedOnNegatingOverflow() { + assertNotSimplified("-int " + randomBinaryComparison() + " " + Long.MIN_VALUE); + assertNotSimplified("-int " + randomBinaryComparison() + " " + Integer.MIN_VALUE); + } + + public void testSimplifyComparisonArithmeticSkippedOnDateOverflow() { + assertNotSimplified("date - INTERVAL 999999999 YEAR > '2010-01-01T01:01:01'::DATETIME"); + assertNotSimplified("date + INTERVAL -999999999 YEAR > '2010-01-01T01:01:01'::DATETIME"); + } + + public void testSimplifyComparisonArithmeticSkippedOnMulDivByZero() { + assertNotSimplified("float / 0 " + randomBinaryComparison() + " 1"); + assertNotSimplified("float * 0 " + randomBinaryComparison() + " 1"); + } + + public void testSimplifyComparisonArithmeticSkippedOnDiv() { + assertNotSimplified("int / 4 " + randomBinaryComparison() + " 1"); + assertNotSimplified("4 / int " + randomBinaryComparison() + " 1"); + } + + public void testSimplifyComparisonArithmeticSkippedOnResultingFloatLiteral() { + assertNotSimplified("int * 2 " + randomBinaryComparison() + " 3"); + } + + public void testSimplifyComparisonArithmeticSkippedOnFloatFieldWithPlusMinus() { + assertNotSimplified("float + 4 " + randomBinaryComparison() + " 1"); + assertNotSimplified("4 + float " + randomBinaryComparison() + " 1"); + assertNotSimplified("float - 4 " + randomBinaryComparison() + " 1"); + assertNotSimplified("4 - float " + randomBinaryComparison() + " 1"); + } + + public void testSimplifyComparisonArithmeticSkippedOnFloats() { + for (String field : List.of("int", "float")) { + for (Tuple nr : List.of(new Tuple<>(.4, 1), new Tuple<>(1, .4))) { + assertNotSimplified(field + " + " + nr.v1() + " " + randomBinaryComparison() + " " + nr.v2()); + assertNotSimplified(field + " - " + nr.v1() + " " + randomBinaryComparison() + " " + nr.v2()); + assertNotSimplified(nr.v1()+ " + " + field + " " + randomBinaryComparison() + " " + nr.v2()); + assertNotSimplified(nr.v1()+ " - " + field + " " + randomBinaryComparison() + " " + nr.v2()); + } + } + } + + public void testBubbleUpNegation() { + String provided = "(1 / -int) * (-float) * (-float / (2 / -int)) * -2 * (-float - int) * (-(-int)) * (-float / (-int + 1)) > 1"; + String expected = "(1/int) * float * (float / (2 / int)) * -2 * (-float - int) * int * (float / (-int + 1)) < -1"; + BinaryComparison bc = extractBinaryComparison(provided); + Expression exp = parser.createExpression(expected); + assertSemanticMatching(bc, exp); + } + + public void testSimplifyComparisonArithmeticWithDateTime() { + doTestSimplifyComparisonArithmetics("date - INTERVAL 1 MONTH > '2010-01-01T01:01:01'::DATETIME", "date", ">", + ZonedDateTime.parse("2010-02-01T01:01:01Z")); + } + + public void testSimplifyComparisonArithmeticWithDate() { + doTestSimplifyComparisonArithmetics("date + INTERVAL 1 YEAR <= '2011-01-01T00:00:00'::DATE", "date", "<=", + ZonedDateTime.parse("2010-01-01T00:00:00Z")); + } + + public void testSimplifyComparisonArithmeticWithDateAndMultiplication() { + // the multiplication should be folded, but check + doTestSimplifyComparisonArithmetics("date + 2 * INTERVAL 1 YEAR <= '2012-01-01T00:00:00'::DATE", "date", "<=", + ZonedDateTime.parse("2010-01-01T00:00:00Z")); + } + + private void doTestSimplifyComparisonArithmetics(String expression, String fieldName, String compSymbol, Object bound) { + BinaryComparison bc = extractBinaryComparison(expression); + assertTrue(COMPARISONS.get(compSymbol).isInstance(bc)); assertTrue(bc.left() instanceof FieldAttribute); FieldAttribute attribute = (FieldAttribute) bc.left(); - assertEquals("int", attribute.name()); + assertEquals(fieldName, attribute.name()); assertTrue(bc.right() instanceof Literal); Literal literal = (Literal) bc.right(); - if (bound instanceof Double) { - assertTrue(literal.value() instanceof Number); - assertEquals(bound.doubleValue(), ((Number) literal.value()).doubleValue(), 1E-15); - } else { - assertEquals(bound, literal.value()); + assertEquals(bound, literal.value()); + } + + private void assertNotSimplified(String condition) { + assertSemanticMatching(extractBinaryComparison(condition), parser.createExpression(condition)); + } + + private BinaryComparison extractBinaryComparison(String expression) { + LogicalPlan plan = planWithArithmeticCondition(expression); + + assertTrue(plan instanceof UnaryPlan); + UnaryPlan unaryPlan = (UnaryPlan) plan; + assertTrue(unaryPlan.child() instanceof Filter); + Filter filter = (Filter) unaryPlan.child(); + assertTrue(filter.condition() instanceof BinaryComparison); + return (BinaryComparison) filter.condition(); + } + + private LogicalPlan planWithArithmeticCondition(String condition) { + return plan("SELECT some.string FROM test WHERE " + condition); + } + + private static void assertSemanticMatching(Expression fieldAttributeExp, Expression unresolvedAttributeExp) { + Expression unresolvedUpdated = unresolvedAttributeExp + .transformUp(LITERALS_ON_THE_RIGHT::rule) + .transformUp(x -> x.foldable() ? new Literal(x.source(), x.fold(), x.dataType()) : x); + + List resolvedFields = fieldAttributeExp.collectFirstChildren(x -> x instanceof FieldAttribute); + for (Expression field : resolvedFields) { + FieldAttribute fa = (FieldAttribute) field; + unresolvedUpdated = unresolvedUpdated.transformDown(x -> x.name().equals(fa.name()) ? fa : x, UnresolvedAttribute.class); } + + assertTrue(unresolvedUpdated.semanticEquals(fieldAttributeExp)); + } + + private static String randomBinaryComparison() { + return randomFrom(COMPARISONS.keySet().stream().map(x -> EQ.symbol().equals(x) ? "=" : x).collect(Collectors.toSet())); } } diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/plan/logical/command/sys/SysColumnsTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/plan/logical/command/sys/SysColumnsTests.java index 61743900c8266..73bfe838f3503 100644 --- a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/plan/logical/command/sys/SysColumnsTests.java +++ b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/plan/logical/command/sys/SysColumnsTests.java @@ -58,7 +58,7 @@ public void testSysColumns() { SysColumns.fillInRows("test", "index", loadMapping("mapping-multi-field-variation.json", true), null, rows, null, randomValueOtherThanMany(Mode::isDriver, () -> randomFrom(Mode.values()))); // nested fields are ignored - assertEquals(15, rows.size()); + assertEquals(16, rows.size()); assertEquals(24, rows.get(0).size()); List row = rows.get(0); @@ -74,67 +74,73 @@ public void testSysColumns() { assertEquals(4, bufferLength(row)); row = rows.get(2); + assertEquals("float", name(row)); + assertEquals(Types.REAL, sqlType(row)); + assertEquals(2, radix(row)); + assertEquals(4, bufferLength(row)); + + row = rows.get(3); assertEquals("text", name(row)); assertEquals(Types.VARCHAR, sqlType(row)); assertEquals(null, radix(row)); assertEquals(Integer.MAX_VALUE, bufferLength(row)); - - row = rows.get(3); + + row = rows.get(4); assertEquals("keyword", name(row)); assertEquals(Types.VARCHAR, sqlType(row)); assertEquals(null, radix(row)); assertEquals(Integer.MAX_VALUE, bufferLength(row)); - row = rows.get(4); + row = rows.get(5); assertEquals("date", name(row)); assertEquals(Types.TIMESTAMP, sqlType(row)); assertEquals(null, radix(row)); assertEquals(29, precision(row)); assertEquals(8, bufferLength(row)); - row = rows.get(5); + row = rows.get(6); assertEquals("some.dotted.field", name(row)); assertEquals(Types.VARCHAR, sqlType(row)); assertEquals(null, radix(row)); assertEquals(Integer.MAX_VALUE, bufferLength(row)); - row = rows.get(6); + row = rows.get(7); assertEquals("some.string", name(row)); assertEquals(Types.VARCHAR, sqlType(row)); assertEquals(null, radix(row)); assertEquals(Integer.MAX_VALUE, bufferLength(row)); - - row = rows.get(7); + + row = rows.get(8); assertEquals("some.string.normalized", name(row)); assertEquals(Types.VARCHAR, sqlType(row)); assertEquals(null, radix(row)); assertEquals(Integer.MAX_VALUE, bufferLength(row)); - - row = rows.get(8); + + row = rows.get(9); assertEquals("some.string.typical", name(row)); assertEquals(Types.VARCHAR, sqlType(row)); assertEquals(null, radix(row)); assertEquals(Integer.MAX_VALUE, bufferLength(row)); - - row = rows.get(9); + + row = rows.get(10); assertEquals("some.ambiguous", name(row)); assertEquals(Types.VARCHAR, sqlType(row)); assertEquals(null, radix(row)); assertEquals(Integer.MAX_VALUE, bufferLength(row)); - - row = rows.get(10); + + row = rows.get(11); assertEquals("some.ambiguous.one", name(row)); assertEquals(Types.VARCHAR, sqlType(row)); assertEquals(null, radix(row)); assertEquals(Integer.MAX_VALUE, bufferLength(row)); - - row = rows.get(11); + + row = rows.get(12); assertEquals("some.ambiguous.two", name(row)); assertEquals(Types.VARCHAR, sqlType(row)); assertEquals(null, radix(row)); assertEquals(Integer.MAX_VALUE, bufferLength(row)); - row = rows.get(12); + row = rows.get(13); assertEquals("some.ambiguous.normalized", name(row)); assertEquals(Types.VARCHAR, sqlType(row)); assertEquals(null, radix(row)); @@ -145,7 +151,7 @@ public void testSysColumnsInOdbcMode() { List> rows = new ArrayList<>(); SysColumns.fillInRows("test", "index", loadMapping("mapping-multi-field-variation.json", true), null, rows, null, Mode.ODBC); - assertEquals(15, rows.size()); + assertEquals(16, rows.size()); assertEquals(24, rows.get(0).size()); List row = rows.get(0); @@ -165,6 +171,16 @@ public void testSysColumnsInOdbcMode() { assertEquals(Short.class, sqlDataTypeSub(row).getClass()); row = rows.get(2); + assertEquals("float", name(row)); + assertEquals((short) Types.REAL, sqlType(row)); + assertEquals(Short.class, radix(row).getClass()); + assertEquals(4, bufferLength(row)); + assertNull(decimalPrecision(row)); + assertEquals(Short.class, nullable(row).getClass()); + assertEquals(Short.class, sqlDataType(row).getClass()); + assertEquals(Short.class, sqlDataTypeSub(row).getClass()); + + row = rows.get(3); assertEquals("text", name(row)); assertEquals((short) Types.VARCHAR, sqlType(row)); assertEquals(null, radix(row)); @@ -174,7 +190,7 @@ public void testSysColumnsInOdbcMode() { assertEquals(Short.class, sqlDataType(row).getClass()); assertEquals(Short.class, sqlDataTypeSub(row).getClass()); - row = rows.get(3); + row = rows.get(4); assertEquals("keyword", name(row)); assertEquals((short) Types.VARCHAR, sqlType(row)); assertEquals(null, radix(row)); @@ -184,7 +200,7 @@ public void testSysColumnsInOdbcMode() { assertEquals(Short.class, sqlDataType(row).getClass()); assertEquals(Short.class, sqlDataTypeSub(row).getClass()); - row = rows.get(4); + row = rows.get(5); assertEquals("date", name(row)); assertEquals((short) Types.TIMESTAMP, sqlType(row)); assertEquals(null, radix(row)); @@ -195,7 +211,7 @@ public void testSysColumnsInOdbcMode() { assertEquals(Short.class, sqlDataType(row).getClass()); assertEquals(Short.class, sqlDataTypeSub(row).getClass()); - row = rows.get(5); + row = rows.get(6); assertEquals("some.dotted.field", name(row)); assertEquals((short) Types.VARCHAR, sqlType(row)); assertEquals(null, radix(row)); @@ -205,7 +221,7 @@ public void testSysColumnsInOdbcMode() { assertEquals(Short.class, sqlDataType(row).getClass()); assertEquals(Short.class, sqlDataTypeSub(row).getClass()); - row = rows.get(6); + row = rows.get(7); assertEquals("some.string", name(row)); assertEquals((short) Types.VARCHAR, sqlType(row)); assertEquals(null, radix(row)); @@ -215,7 +231,7 @@ public void testSysColumnsInOdbcMode() { assertEquals(Short.class, sqlDataType(row).getClass()); assertEquals(Short.class, sqlDataTypeSub(row).getClass()); - row = rows.get(7); + row = rows.get(8); assertEquals("some.string.normalized", name(row)); assertEquals((short) Types.VARCHAR, sqlType(row)); assertEquals(null, radix(row)); @@ -225,7 +241,7 @@ public void testSysColumnsInOdbcMode() { assertEquals(Short.class, sqlDataType(row).getClass()); assertEquals(Short.class, sqlDataTypeSub(row).getClass()); - row = rows.get(8); + row = rows.get(9); assertEquals("some.string.typical", name(row)); assertEquals((short) Types.VARCHAR, sqlType(row)); assertEquals(null, radix(row)); @@ -235,7 +251,7 @@ public void testSysColumnsInOdbcMode() { assertEquals(Short.class, sqlDataType(row).getClass()); assertEquals(Short.class, sqlDataTypeSub(row).getClass()); - row = rows.get(9); + row = rows.get(10); assertEquals("some.ambiguous", name(row)); assertEquals((short) Types.VARCHAR, sqlType(row)); assertEquals(null, radix(row)); @@ -244,8 +260,8 @@ public void testSysColumnsInOdbcMode() { assertEquals(Short.class, nullable(row).getClass()); assertEquals(Short.class, sqlDataType(row).getClass()); assertEquals(Short.class, sqlDataTypeSub(row).getClass()); - - row = rows.get(10); + + row = rows.get(11); assertEquals("some.ambiguous.one", name(row)); assertEquals((short) Types.VARCHAR, sqlType(row)); assertEquals(null, radix(row)); @@ -254,8 +270,8 @@ public void testSysColumnsInOdbcMode() { assertEquals(Short.class, nullable(row).getClass()); assertEquals(Short.class, sqlDataType(row).getClass()); assertEquals(Short.class, sqlDataTypeSub(row).getClass()); - - row = rows.get(11); + + row = rows.get(12); assertEquals("some.ambiguous.two", name(row)); assertEquals((short) Types.VARCHAR, sqlType(row)); assertEquals(null, radix(row)); @@ -265,7 +281,7 @@ public void testSysColumnsInOdbcMode() { assertEquals(Short.class, sqlDataType(row).getClass()); assertEquals(Short.class, sqlDataTypeSub(row).getClass()); - row = rows.get(12); + row = rows.get(13); assertEquals("some.ambiguous.normalized", name(row)); assertEquals((short) Types.VARCHAR, sqlType(row)); assertEquals(null, radix(row)); @@ -275,12 +291,12 @@ public void testSysColumnsInOdbcMode() { assertEquals(Short.class, sqlDataType(row).getClass()); assertEquals(Short.class, sqlDataTypeSub(row).getClass()); } - + public void testSysColumnsInJdbcMode() { List> rows = new ArrayList<>(); SysColumns.fillInRows("test", "index", loadMapping("mapping-multi-field-variation.json", true), null, rows, null, Mode.JDBC); - assertEquals(15, rows.size()); + assertEquals(16, rows.size()); assertEquals(24, rows.get(0).size()); List row = rows.get(0); @@ -300,6 +316,16 @@ public void testSysColumnsInJdbcMode() { assertEquals(Integer.class, sqlDataTypeSub(row).getClass()); row = rows.get(2); + assertEquals("float", name(row)); + assertEquals(Types.REAL, sqlType(row)); + assertEquals(Integer.class, radix(row).getClass()); + assertEquals(4, bufferLength(row)); + assertNull(decimalPrecision(row)); + assertEquals(Integer.class, nullable(row).getClass()); + assertEquals(Integer.class, sqlDataType(row).getClass()); + assertEquals(Integer.class, sqlDataTypeSub(row).getClass()); + + row = rows.get(3); assertEquals("text", name(row)); assertEquals(Types.VARCHAR, sqlType(row)); assertEquals(null, radix(row)); @@ -309,7 +335,7 @@ public void testSysColumnsInJdbcMode() { assertEquals(Integer.class, sqlDataType(row).getClass()); assertEquals(Integer.class, sqlDataTypeSub(row).getClass()); - row = rows.get(3); + row = rows.get(4); assertEquals("keyword", name(row)); assertEquals(Types.VARCHAR, sqlType(row)); assertEquals(null, radix(row)); @@ -319,7 +345,7 @@ public void testSysColumnsInJdbcMode() { assertEquals(Integer.class, sqlDataType(row).getClass()); assertEquals(Integer.class, sqlDataTypeSub(row).getClass()); - row = rows.get(4); + row = rows.get(5); assertEquals("date", name(row)); assertEquals(Types.TIMESTAMP, sqlType(row)); assertEquals(null, radix(row)); @@ -330,7 +356,7 @@ public void testSysColumnsInJdbcMode() { assertEquals(Integer.class, sqlDataType(row).getClass()); assertEquals(Integer.class, sqlDataTypeSub(row).getClass()); - row = rows.get(5); + row = rows.get(6); assertEquals("some.dotted.field", name(row)); assertEquals(Types.VARCHAR, sqlType(row)); assertEquals(null, radix(row)); @@ -340,7 +366,7 @@ public void testSysColumnsInJdbcMode() { assertEquals(Integer.class, sqlDataType(row).getClass()); assertEquals(Integer.class, sqlDataTypeSub(row).getClass()); - row = rows.get(6); + row = rows.get(7); assertEquals("some.string", name(row)); assertEquals(Types.VARCHAR, sqlType(row)); assertEquals(null, radix(row)); @@ -350,7 +376,7 @@ public void testSysColumnsInJdbcMode() { assertEquals(Integer.class, sqlDataType(row).getClass()); assertEquals(Integer.class, sqlDataTypeSub(row).getClass()); - row = rows.get(7); + row = rows.get(8); assertEquals("some.string.normalized", name(row)); assertEquals(Types.VARCHAR, sqlType(row)); assertEquals(null, radix(row)); @@ -360,7 +386,7 @@ public void testSysColumnsInJdbcMode() { assertEquals(Integer.class, sqlDataType(row).getClass()); assertEquals(Integer.class, sqlDataTypeSub(row).getClass()); - row = rows.get(8); + row = rows.get(9); assertEquals("some.string.typical", name(row)); assertEquals(Types.VARCHAR, sqlType(row)); assertEquals(null, radix(row)); @@ -369,8 +395,8 @@ public void testSysColumnsInJdbcMode() { assertEquals(Integer.class, nullable(row).getClass()); assertEquals(Integer.class, sqlDataType(row).getClass()); assertEquals(Integer.class, sqlDataTypeSub(row).getClass()); - - row = rows.get(9); + + row = rows.get(10); assertEquals("some.ambiguous", name(row)); assertEquals(Types.VARCHAR, sqlType(row)); assertEquals(null, radix(row)); @@ -379,8 +405,8 @@ public void testSysColumnsInJdbcMode() { assertEquals(Integer.class, nullable(row).getClass()); assertEquals(Integer.class, sqlDataType(row).getClass()); assertEquals(Integer.class, sqlDataTypeSub(row).getClass()); - - row = rows.get(10); + + row = rows.get(11); assertEquals("some.ambiguous.one", name(row)); assertEquals(Types.VARCHAR, sqlType(row)); assertEquals(null, radix(row)); @@ -389,8 +415,8 @@ public void testSysColumnsInJdbcMode() { assertEquals(Integer.class, nullable(row).getClass()); assertEquals(Integer.class, sqlDataType(row).getClass()); assertEquals(Integer.class, sqlDataTypeSub(row).getClass()); - - row = rows.get(11); + + row = rows.get(12); assertEquals("some.ambiguous.two", name(row)); assertEquals(Types.VARCHAR, sqlType(row)); assertEquals(null, radix(row)); @@ -400,7 +426,7 @@ public void testSysColumnsInJdbcMode() { assertEquals(Integer.class, sqlDataType(row).getClass()); assertEquals(Integer.class, sqlDataTypeSub(row).getClass()); - row = rows.get(12); + row = rows.get(13); assertEquals("some.ambiguous.normalized", name(row)); assertEquals(Types.VARCHAR, sqlType(row)); assertEquals(null, radix(row)); diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/QueryTranslatorTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/QueryTranslatorTests.java index bac97756d9520..de60ea47a0eda 100644 --- a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/QueryTranslatorTests.java +++ b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/QueryTranslatorTests.java @@ -2074,15 +2074,15 @@ public void testNoCountDoesNotTrackHits() { public void testZonedDateTimeInScripts() { PhysicalPlan p = optimizeAndPlan( - "SELECT date FROM test WHERE date + INTERVAL 1 YEAR > CAST('2019-03-11T12:34:56.000Z' AS DATETIME)"); + "SELECT date FROM test WHERE date - INTERVAL 999999999 YEAR > CAST('2019-03-11T12:34:56.000Z' AS DATETIME)"); assertEquals(EsQueryExec.class, p.getClass()); EsQueryExec eqe = (EsQueryExec) p; assertThat(eqe.queryContainer().toString().replaceAll("\\s+", ""), containsString( "\"script\":{\"script\":{\"source\":\"InternalQlScriptUtils.nullSafeFilter(" - + "InternalQlScriptUtils.gt(InternalSqlScriptUtils.add(InternalQlScriptUtils.docValue(doc,params.v0)," + + "InternalQlScriptUtils.gt(InternalSqlScriptUtils.sub(InternalQlScriptUtils.docValue(doc,params.v0)," + "InternalSqlScriptUtils.intervalYearMonth(params.v1,params.v2)),InternalSqlScriptUtils.asDateTime(params.v3)))\"," + "\"lang\":\"painless\"," - + "\"params\":{\"v0\":\"date\",\"v1\":\"P1Y\",\"v2\":\"INTERVAL_YEAR\",\"v3\":\"2019-03-11T12:34:56.000Z\"}},")); + + "\"params\":{\"v0\":\"date\",\"v1\":\"P999999999Y\",\"v2\":\"INTERVAL_YEAR\",\"v3\":\"2019-03-11T12:34:56.000Z\"}},")); } public void testChronoFieldBasedDateTimeFunctionsWithMathIntervalAndGroupBy() { @@ -2385,11 +2385,11 @@ public void testPercentileMethodParametersSameAsDefault() { " PERCENTILE(int, 50), " + // 4: this has a different method parameter // just to make sure we don't fold everything to default - " PERCENTILE(int, 50, 'tdigest', 22) " + " PERCENTILE(int, 50, 'tdigest', 22) " + "FROM test").replaceAll("PERCENTILE", fnName); - + List aggs = percentilesAggsByField(optimizeAndPlan(sql), fieldCount); - + // 0-3 assertEquals(aggs.get(0), aggs.get(1)); assertEquals(aggs.get(0), aggs.get(2)); @@ -2401,7 +2401,7 @@ public void testPercentileMethodParametersSameAsDefault() { assertEquals(new PercentilesConfig.TDigest(22), aggs.get(4).percentilesConfig()); assertArrayEquals(new double[] { 50 }, pctOrValFn.apply(aggs.get(4)), 0); }; - + test.accept("PERCENTILE", p -> ((PercentilesAggregationBuilder)p).percentiles()); test.accept("PERCENTILE_RANK", p -> ((PercentileRanksAggregationBuilder)p).values()); } @@ -2414,17 +2414,17 @@ public void testPercentileOptimization() { // 0-1: fold into the same aggregation " PERCENTILE(int, 50, 'tdigest'), " + " PERCENTILE(int, 60, 'tdigest'), " + - + // 2-3: fold into one aggregation " PERCENTILE(int, 50, 'hdr'), " + " PERCENTILE(int, 60, 'hdr', 3), " + - + // 4: folds into a separate aggregation " PERCENTILE(int, 60, 'hdr', 4)" + "FROM test").replaceAll("PERCENTILE", fnName); List aggs = percentilesAggsByField(optimizeAndPlan(sql), fieldCount); - + // 0-1 assertEquals(aggs.get(0), aggs.get(1)); assertEquals(new PercentilesConfig.TDigest(), aggs.get(0).percentilesConfig()); @@ -2434,7 +2434,7 @@ public void testPercentileOptimization() { assertEquals(aggs.get(2), aggs.get(3)); assertEquals(new PercentilesConfig.Hdr(), aggs.get(2).percentilesConfig()); assertArrayEquals(new double[]{50, 60}, pctOrValFn.apply(aggs.get(2)), 0); - + // 4 assertEquals(new PercentilesConfig.Hdr(4), aggs.get(4).percentilesConfig()); assertArrayEquals(new double[]{60}, pctOrValFn.apply(aggs.get(4)), 0); diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/type/SqlDataTypesTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/type/SqlDataTypesTests.java index b238a7e9fc5c3..68144dfb46a6c 100644 --- a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/type/SqlDataTypesTests.java +++ b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/type/SqlDataTypesTests.java @@ -35,6 +35,7 @@ import static org.elasticsearch.xpack.sql.type.SqlDataTypes.INTERVAL_YEAR; import static org.elasticsearch.xpack.sql.type.SqlDataTypes.INTERVAL_YEAR_TO_MONTH; import static org.elasticsearch.xpack.sql.type.SqlDataTypes.TIME; +import static org.elasticsearch.xpack.sql.type.SqlDataTypes.areCompatible; import static org.elasticsearch.xpack.sql.type.SqlDataTypes.defaultPrecision; import static org.elasticsearch.xpack.sql.type.SqlDataTypes.isInterval; import static org.elasticsearch.xpack.sql.type.SqlDataTypes.metaSqlDataType; @@ -138,6 +139,27 @@ public void testIncompatibleInterval() { assertNull(compatibleInterval(INTERVAL_MINUTE_TO_SECOND, INTERVAL_MONTH)); } + public void testIntervalCompabitilityWithDateTimes() { + for (DataType intervalType : asList(INTERVAL_YEAR, + INTERVAL_MONTH, + INTERVAL_DAY, + INTERVAL_HOUR, + INTERVAL_MINUTE, + INTERVAL_SECOND, + INTERVAL_YEAR_TO_MONTH, + INTERVAL_DAY_TO_HOUR, + INTERVAL_DAY_TO_MINUTE, + INTERVAL_DAY_TO_SECOND, + INTERVAL_HOUR_TO_MINUTE, + INTERVAL_HOUR_TO_SECOND, + INTERVAL_MINUTE_TO_SECOND)) { + for (DataType dateTimeType: asList(DATE, DATETIME)) { + assertTrue(areCompatible(intervalType, dateTimeType)); + assertTrue(areCompatible(dateTimeType, intervalType)); + } + } + } + public void testEsToDataType() { List types = new ArrayList<>(Arrays.asList("null", "boolean", "bool", "byte", "tinyint", From 4d641e45fe7faea9d2aaa635fec272521788bb1e Mon Sep 17 00:00:00 2001 From: Bogdan Pintea Date: Mon, 14 Dec 2020 10:36:09 +0100 Subject: [PATCH 04/14] Style fix Removed now unused import. --- .../org/elasticsearch/xpack/ql/optimizer/OptimizerRules.java | 1 - 1 file changed, 1 deletion(-) 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 index 982e4e3b2b4cc..c0ba02319af9f 100644 --- 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 @@ -43,7 +43,6 @@ import org.elasticsearch.xpack.ql.type.DataTypes; import org.elasticsearch.xpack.ql.util.CollectionUtils; -import java.time.DateTimeException; import java.time.ZoneId; import java.util.ArrayList; import java.util.Iterator; From 1a28179fd76bae83d23c56ef566ae006ff7a7a2a Mon Sep 17 00:00:00 2001 From: Bogdan Pintea Date: Mon, 14 Dec 2020 11:32:44 +0100 Subject: [PATCH 05/14] Test fix - update test condition to remove (non-optimisable) integer division. --- .../elasticsearch/xpack/eql/optimizer/OptimizerTests.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/x-pack/plugin/eql/src/test/java/org/elasticsearch/xpack/eql/optimizer/OptimizerTests.java b/x-pack/plugin/eql/src/test/java/org/elasticsearch/xpack/eql/optimizer/OptimizerTests.java index 93662d7f6ec4e..5531d8fe238d3 100644 --- a/x-pack/plugin/eql/src/test/java/org/elasticsearch/xpack/eql/optimizer/OptimizerTests.java +++ b/x-pack/plugin/eql/src/test/java/org/elasticsearch/xpack/eql/optimizer/OptimizerTests.java @@ -666,9 +666,9 @@ public void testDifferentKeyFromDisjunction() { assertEquals(filter, filterCondition(child2.children().get(0))); } - // ((a + 1) / 2 - 3) * 4 >= 14 -> a > 12. + // ((a + 1) - 3) * 4 >= 16 -> a >= 6. public void testReduceBinaryComparisons() { - LogicalPlan plan = accept("foo where ((pid + 1) / 2 - 3) * 4 >= 14"); + LogicalPlan plan = accept("foo where ((pid + 1) - 3) * 4 >= 16"); assertNotNull(plan); List filters = plan.collectFirstChildren(x -> x instanceof Filter); assertNotNull(filters); @@ -685,7 +685,7 @@ public void testReduceBinaryComparisons() { assertEquals("pid", ((FieldAttribute) gte.left()).name()); assertTrue(gte.right() instanceof Literal); - assertEquals(12d, ((Literal) gte.right()).value()); + assertEquals(6, ((Literal) gte.right()).value()); } private static Attribute timestamp() { From e74b697a5aefb1050961cd28b016577c14af471d Mon Sep 17 00:00:00 2001 From: Bogdan Pintea Date: Thu, 7 Jan 2021 16:42:47 +0100 Subject: [PATCH 06/14] Address review comments - add isMulOrDiv and isAddOrSub methods; - add a new factory if'ace for the arithmetic operations invertible in relation to a binary comparison; - simplify NEG - NEG tree structures; - more tests. --- .../predicate/operator/arithmetic/Add.java | 6 +- .../arithmetic/ArithmeticOperation.java | 7 -- .../BinaryComparisonInvertible.java | 24 +++++++ .../predicate/operator/arithmetic/Div.java | 6 +- .../predicate/operator/arithmetic/Mul.java | 6 +- .../predicate/operator/arithmetic/Sub.java | 6 +- .../xpack/ql/optimizer/OptimizerRules.java | 64 ++++++++++++------- .../ql/optimizer/OptimizerRulesTests.java | 15 ++++- .../src/main/resources/arithmetic.sql-spec | 13 +++- .../predicate/operator/arithmetic/Add.java | 7 +- .../predicate/operator/arithmetic/Div.java | 7 +- .../predicate/operator/arithmetic/Mul.java | 13 ++-- .../predicate/operator/arithmetic/Sub.java | 7 +- .../sql/optimizer/OptimizerRunTests.java | 2 + 14 files changed, 122 insertions(+), 61 deletions(-) create mode 100644 x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/operator/arithmetic/BinaryComparisonInvertible.java 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 fc6362ddcd281..9358d89d2d7ad 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 @@ -12,7 +12,7 @@ /** * Addition function ({@code a + b}). */ -public class Add extends DateTimeArithmeticOperation { +public class Add extends DateTimeArithmeticOperation implements BinaryComparisonInvertible { public Add(Source source, Expression left, Expression right) { super(source, left, right, DefaultBinaryArithmeticOperation.ADD); } @@ -32,7 +32,7 @@ public Add swapLeftAndRight() { } @Override - public Sub inverse(Source source, Expression left, Expression right) { - return new Sub(source, left, right); + public ArithmeticOperationFactory binaryComparisonInverse() { + return Sub::new; } } diff --git a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/operator/arithmetic/ArithmeticOperation.java b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/operator/arithmetic/ArithmeticOperation.java index 4d0124758665a..eaad5463a47bc 100644 --- a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/operator/arithmetic/ArithmeticOperation.java +++ b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/operator/arithmetic/ArithmeticOperation.java @@ -45,11 +45,4 @@ public DataType dataType() { protected Pipe makePipe() { return new BinaryArithmeticPipe(source(), this, Expressions.pipe(left()), Expressions.pipe(right()), function()); } - - /** - * Returns the opposite of this operation. - */ - public ArithmeticOperation inverse(Source source, Expression left, Expression right) { - return this; - } } diff --git a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/operator/arithmetic/BinaryComparisonInvertible.java b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/operator/arithmetic/BinaryComparisonInvertible.java new file mode 100644 index 0000000000000..da6b8443dc694 --- /dev/null +++ b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/operator/arithmetic/BinaryComparisonInvertible.java @@ -0,0 +1,24 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License; + * you may not use this file except in compliance with the Elastic License. + */ + +package org.elasticsearch.xpack.ql.expression.predicate.operator.arithmetic; + +import org.elasticsearch.xpack.ql.expression.Expression; +import org.elasticsearch.xpack.ql.tree.Source; + +/* + * Factory interface for arithmetic operations that have an inverse in reference to a binary comparison. + * For instance the division is multiplication's inverse, substitution addition's, log exponentiation's a.s.o. + * Not all operations - like modulo - are invertible. + */ +public interface BinaryComparisonInvertible { + + interface ArithmeticOperationFactory { + ArithmeticOperation create(Source source, Expression left, Expression right); + } + + ArithmeticOperationFactory binaryComparisonInverse(); +} diff --git a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/operator/arithmetic/Div.java b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/operator/arithmetic/Div.java index fc5704575b700..2910714a05883 100644 --- a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/operator/arithmetic/Div.java +++ b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/operator/arithmetic/Div.java @@ -14,7 +14,7 @@ /** * Division function ({@code a / b}). */ -public class Div extends ArithmeticOperation { +public class Div extends ArithmeticOperation implements BinaryComparisonInvertible { public Div(Source source, Expression left, Expression right) { super(source, left, right, DefaultBinaryArithmeticOperation.DIV); @@ -36,7 +36,7 @@ public DataType dataType() { } @Override - public Mul inverse(Source source, Expression left, Expression right) { - return new Mul(source, left, right); + public ArithmeticOperationFactory binaryComparisonInverse() { + return Mul::new; } } 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 b537f1484aea4..b5a79538fb537 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 @@ -16,7 +16,7 @@ /** * Multiplication function ({@code a * b}). */ -public class Mul extends ArithmeticOperation { +public class Mul extends ArithmeticOperation implements BinaryComparisonInvertible { public Mul(Source source, Expression left, Expression right) { super(source, left, right, DefaultBinaryArithmeticOperation.MUL); @@ -54,7 +54,7 @@ public Mul swapLeftAndRight() { } @Override - public Div inverse(Source source, Expression left, Expression right) { - return new Div(source, left, right); + public ArithmeticOperationFactory binaryComparisonInverse() { + return Div::new; } } diff --git a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/operator/arithmetic/Sub.java b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/operator/arithmetic/Sub.java index e61eaf085e0af..068688544d0e5 100644 --- a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/operator/arithmetic/Sub.java +++ b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/operator/arithmetic/Sub.java @@ -12,7 +12,7 @@ /** * Subtraction function ({@code a - b}). */ -public class Sub extends DateTimeArithmeticOperation { +public class Sub extends DateTimeArithmeticOperation implements BinaryComparisonInvertible { public Sub(Source source, Expression left, Expression right) { super(source, left, right, DefaultBinaryArithmeticOperation.SUB); @@ -29,7 +29,7 @@ protected Sub replaceChildren(Expression newLeft, Expression newRight) { } @Override - public Add inverse(Source source, Expression left, Expression right) { - return new Add(source, left, right); + public ArithmeticOperationFactory binaryComparisonInverse() { + return Add::new; } } 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 index c0ba02319af9f..2c8a0e22e48a4 100644 --- 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 @@ -5,6 +5,7 @@ */ package org.elasticsearch.xpack.ql.optimizer; +import org.elasticsearch.xpack.ql.QlIllegalArgumentException; import org.elasticsearch.xpack.ql.expression.Expression; import org.elasticsearch.xpack.ql.expression.Expressions; import org.elasticsearch.xpack.ql.expression.Literal; @@ -21,6 +22,7 @@ import org.elasticsearch.xpack.ql.expression.predicate.logical.Or; import org.elasticsearch.xpack.ql.expression.predicate.nulls.IsNotNull; import org.elasticsearch.xpack.ql.expression.predicate.operator.arithmetic.ArithmeticOperation; +import org.elasticsearch.xpack.ql.expression.predicate.operator.arithmetic.BinaryComparisonInvertible; import org.elasticsearch.xpack.ql.expression.predicate.operator.arithmetic.Neg; import org.elasticsearch.xpack.ql.expression.predicate.operator.arithmetic.Sub; import org.elasticsearch.xpack.ql.expression.predicate.operator.comparison.BinaryComparison; @@ -43,6 +45,7 @@ import org.elasticsearch.xpack.ql.type.DataTypes; import org.elasticsearch.xpack.ql.util.CollectionUtils; +import java.time.DateTimeException; import java.time.ZoneId; import java.util.ArrayList; import java.util.Iterator; @@ -62,6 +65,7 @@ import static org.elasticsearch.xpack.ql.expression.predicate.Predicates.splitAnd; import static org.elasticsearch.xpack.ql.expression.predicate.Predicates.splitOr; import static org.elasticsearch.xpack.ql.expression.predicate.Predicates.subtract; +import static org.elasticsearch.xpack.ql.expression.predicate.operator.arithmetic.DefaultBinaryArithmeticOperation.ADD; import static org.elasticsearch.xpack.ql.expression.predicate.operator.arithmetic.DefaultBinaryArithmeticOperation.DIV; import static org.elasticsearch.xpack.ql.expression.predicate.operator.arithmetic.DefaultBinaryArithmeticOperation.MOD; import static org.elasticsearch.xpack.ql.expression.predicate.operator.arithmetic.DefaultBinaryArithmeticOperation.MUL; @@ -1156,36 +1160,41 @@ public BubbleUpNegations() { @Override protected Expression rule(Expression e) { - if (e instanceof Neg) { // cancels out NEG - MUL/DIV - NEG - x ==> MUL/DIV - x - Expression child = e.children().get(0); - if (child instanceof ArithmeticOperation) { + if (e instanceof Neg) { + Expression child = ((Neg) e).field(); + if (child instanceof ArithmeticOperation) { // cancels out NEG - MUL/DIV - NEG - x ==> MUL/DIV - x ArithmeticOperation operation = (ArithmeticOperation) child; - String opSymbol = operation.symbol(); - if (MUL.symbol().equals(opSymbol) || DIV.symbol().equals(opSymbol)) { + if (isMulOrDiv(operation)) { if (operation.left() instanceof Neg) { - return operation.replaceChildren(List.of(operation.left().children().get(0), operation.right())); + return operation.replaceChildren(List.of(((Neg) operation.left()).field(), operation.right())); } if (operation.right() instanceof Neg) { - return operation.replaceChildren(List.of(operation.left(), operation.right().children().get(0))); + return operation.replaceChildren(List.of(operation.left(), ((Neg) operation.right()).field())); } } + } else if (child instanceof Neg) { // cancels out NEG - NEG - x ==> x + return ((Neg) child).field(); } } else if (e instanceof ArithmeticOperation) { // pulls up MUL/DIV - NEG - x ==> NEG - MUL/DIV - x ArithmeticOperation operation = (ArithmeticOperation) e; - String opSymbol = operation.symbol(); - if (MUL.symbol().equals(opSymbol) || DIV.symbol().equals(opSymbol)) { + if (isMulOrDiv(operation)) { if (operation.left() instanceof Neg) { Neg neg = (Neg) operation.left(); - return new Neg(operation.source(), operation.replaceChildren(List.of(neg.children().get(0), operation.right()))); + return new Neg(operation.source(), operation.replaceChildren(List.of(neg.field(), operation.right()))); } if (operation.right() instanceof Neg) { Neg neg = (Neg) operation.right(); - return new Neg(operation.source(), operation.replaceChildren(List.of(operation.left(), neg.children().get(0)))); + return new Neg(operation.source(), operation.replaceChildren(List.of(operation.left(), neg.field()))); } } } return e; } + + public static boolean isMulOrDiv(ArithmeticOperation operation) { + String opSymbol = operation.symbol(); + return MUL.symbol().equals(opSymbol) || DIV.symbol().equals(opSymbol); + } } // Simplifies arithmetic expressions with fixed point fields in BinaryComparisons. @@ -1216,17 +1225,15 @@ private Expression simplify(BinaryComparison bc) { private static Expression reduceNegation(BinaryComparison bc) { Literal bcLiteral = (Literal) bc.right(); - Expression neg = safeMaybeFold(new Neg(bcLiteral.source(), bcLiteral)); - return neg == null ? bc : bc.reverse().replaceChildren(List.of(bc.left().children().get(0), neg)); + Expression literalNeg = safeMaybeFold(new Neg(bcLiteral.source(), bcLiteral)); + return literalNeg == null ? bc : bc.reverse().replaceChildren(List.of(((Neg) bc.left()).field(), literalNeg)); } private static Expression safeMaybeFold(Expression expression) { if (expression.foldable()) { try { expression = new Literal(expression.source(), expression.fold(), expression.dataType()); - } catch (Exception e) { - // could only catch (ArithmeticException | DateTimeException e), but safer to just turn off the optimisation - // should any exception arise. + } catch (ArithmeticException | DateTimeException e) { expression = null; } } @@ -1284,7 +1291,12 @@ Expression doSimplify() { ? Literal.of(bcLiteral, ((Number) bcLiteral.value()).doubleValue()) : bcLiteral; - Expression bcRightExpression = safeMaybeFold(operation.inverse(bcl.source(), bcl, opRight)); + if (operation instanceof BinaryComparisonInvertible == false) { // should be an assertion? + throw new QlIllegalArgumentException("Unexpected [" + operation.symbol() + "] operation object"); + } + Expression bcRightExpression = ((BinaryComparisonInvertible) operation).binaryComparisonInverse() + .create(bcl.source(), bcl, opRight); + bcRightExpression = safeMaybeFold(bcRightExpression); return bcRightExpression == null ? comparison : comparison.replaceChildren(List.of(opLeft, bcRightExpression)); } @@ -1296,11 +1308,18 @@ static Expression simplify(BinaryComparison comparison, BiFunction typesCompatible) Expression doSimplify() { // If current operation is a multiplication, it's inverse will be a division: safe only if outcome is still integral. if (isDiv == false && opLeft.dataType().isInteger()) { - if (((Number) bcLiteral.value()).longValue() % ((Number) opLiteral.value()).longValue() != 0) { + long opLiteralValue = ((Number) opLiteral.value()).longValue(); + if (opLiteralValue == 0 || ((Number) bcLiteral.value()).longValue() % opLiteralValue != 0) { return comparison; } } 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 index a3197dcdac0f5..350d113fd8c41 100644 --- 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 @@ -1552,13 +1552,22 @@ public void testBubbleUpNegation() { Expression e = new BubbleUpNegations().rule(op); assertEquals(Neg.class, e.getClass()); Neg upperNeg = (Neg) e; - assertEquals(1, upperNeg.children().size()); - assertEquals(op.getClass(), upperNeg.children().get(0).getClass()); - ArithmeticOperation newOp = (ArithmeticOperation) upperNeg.children().get(0); + assertEquals(op.getClass(), upperNeg.field().getClass()); + ArithmeticOperation newOp = (ArithmeticOperation) upperNeg.field(); assertEquals(TWO, newOp.left()); assertEquals(FieldAttribute.class, newOp.right().getClass()); FieldAttribute divFa = (FieldAttribute) newOp.right(); assertEquals(fa, divFa); } } + + public void testCancelDoubleNegation() { + FieldAttribute fa = getFieldAttribute(); + Neg innerNeg = new Neg(fa.source(), fa); + Neg outerNeg = new Neg(innerNeg.source(), innerNeg); + + Expression e = new BubbleUpNegations().rule(outerNeg); + assertEquals(FieldAttribute.class, e.getClass()); + assertEquals(fa, e); + } } diff --git a/x-pack/plugin/sql/qa/server/src/main/resources/arithmetic.sql-spec b/x-pack/plugin/sql/qa/server/src/main/resources/arithmetic.sql-spec index 3687cea3235f7..4bfd3ec42fea0 100644 --- a/x-pack/plugin/sql/qa/server/src/main/resources/arithmetic.sql-spec +++ b/x-pack/plugin/sql/qa/server/src/main/resources/arithmetic.sql-spec @@ -219,17 +219,26 @@ SELECT emp_no FROM test_emp WHERE 1000 + (1000000000 / salary::FLOAT) / 2 > 1600 divFloatFieldDivPlus SELECT emp_no FROM test_emp WHERE (1000000000 / salary::FLOAT) / 2 + 1000 > 16000; -noOptimisationOnOverflowAdd +noOptimisationOnLongOverflowAdd SELECT emp_no FROM test_emp WHERE salary - 2 < 9223372036854775807; -noOptimisationOnUnderflowSub +noOptimisationOnLongUnderflowSub SELECT emp_no FROM test_emp WHERE -salary + 2 < -9223372036854775807; +noOptimisationOnIntOverflowAdd +SELECT emp_no FROM test_emp WHERE salary::INT - 2 < 2147483647; +noOptimisationOnIntUnderflowSub +SELECT emp_no FROM test_emp WHERE -salary::INT + 2 < -2147483648; noOptimisationOnOverflowMul SELECT emp_no FROM test_emp WHERE salary / 10 < 1.7976931348623157E308; noOptimisationOnPrecisionLossOnFloatFieldAdd SELECT emp_no FROM test_emp WHERE 1 - salary::FLOAT/1E21 < 1; noOptimisationOnPrecisionLossOnFloatFieldDiv SELECT emp_no FROM test_emp WHERE (1 - salary::FLOAT / 1E21) * (1 + 1E-15) > 1; +noOptimisationOnIntegralDivByZero +SELECT emp_no FROM test_emp WHERE (5/4 - 1) * salary > 1; +noOptimisationOnFloatDivByZero +SELECT emp_no FROM test_emp WHERE (5/4 - 1) * salary::FLOAT > 1; +// negations optimiser multipleNegations SELECT emp_no FROM test_emp WHERE ((1000000000 / -salary) / 2) * (-1 / (2 / -emp_no::FLOAT)) / -1000 >= 50000; 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 09eff19ee1853..36444f3cb8fa9 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 @@ -6,13 +6,14 @@ package org.elasticsearch.xpack.sql.expression.predicate.operator.arithmetic; import org.elasticsearch.xpack.ql.expression.Expression; +import org.elasticsearch.xpack.ql.expression.predicate.operator.arithmetic.BinaryComparisonInvertible; import org.elasticsearch.xpack.ql.tree.NodeInfo; import org.elasticsearch.xpack.ql.tree.Source; /** * Addition function ({@code a + b}). */ -public class Add extends DateTimeArithmeticOperation { +public class Add extends DateTimeArithmeticOperation implements BinaryComparisonInvertible { public Add(Source source, Expression left, Expression right) { super(source, left, right, SqlBinaryArithmeticOperation.ADD); } @@ -33,7 +34,7 @@ public Add swapLeftAndRight() { } @Override - public Sub inverse(Source source, Expression left, Expression right) { - return new Sub(source, left, right); + public ArithmeticOperationFactory binaryComparisonInverse() { + return Sub::new; } } diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/operator/arithmetic/Div.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/operator/arithmetic/Div.java index 1492d5d55dae6..10830d4cec3e7 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/operator/arithmetic/Div.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/operator/arithmetic/Div.java @@ -6,6 +6,7 @@ package org.elasticsearch.xpack.sql.expression.predicate.operator.arithmetic; import org.elasticsearch.xpack.ql.expression.Expression; +import org.elasticsearch.xpack.ql.expression.predicate.operator.arithmetic.BinaryComparisonInvertible; import org.elasticsearch.xpack.ql.expression.predicate.operator.arithmetic.Mul; import org.elasticsearch.xpack.ql.tree.NodeInfo; import org.elasticsearch.xpack.ql.tree.Source; @@ -15,7 +16,7 @@ /** * Division function ({@code a / b}). */ -public class Div extends SqlArithmeticOperation { +public class Div extends SqlArithmeticOperation implements BinaryComparisonInvertible { public Div(Source source, Expression left, Expression right) { super(source, left, right, SqlBinaryArithmeticOperation.DIV); @@ -37,7 +38,7 @@ public DataType dataType() { } @Override - public Mul inverse(Source source, Expression left, Expression right) { - return new Mul(source, left, right); + public ArithmeticOperationFactory binaryComparisonInverse() { + return Mul::new; } } 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 72b15c65d6434..0a7b14a75e73c 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 @@ -6,6 +6,7 @@ package org.elasticsearch.xpack.sql.expression.predicate.operator.arithmetic; import org.elasticsearch.xpack.ql.expression.Expression; +import org.elasticsearch.xpack.ql.expression.predicate.operator.arithmetic.BinaryComparisonInvertible; import org.elasticsearch.xpack.ql.tree.NodeInfo; import org.elasticsearch.xpack.ql.tree.Source; import org.elasticsearch.xpack.ql.type.DataType; @@ -17,7 +18,7 @@ /** * Multiplication function ({@code a * b}). */ -public class Mul extends SqlArithmeticOperation { +public class Mul extends SqlArithmeticOperation implements BinaryComparisonInvertible { private DataType dataType; @@ -58,11 +59,6 @@ public DataType dataType() { return dataType; } - @Override - public Div inverse(Source source, Expression left, Expression right) { - return new Div(source, left, right); - } - @Override protected NodeInfo info() { return NodeInfo.create(this, Mul::new, left(), right()); @@ -76,4 +72,9 @@ protected Mul replaceChildren(Expression newLeft, Expression newRight) { public Mul swapLeftAndRight() { return new Mul(source(), right(), left()); } + + @Override + public ArithmeticOperationFactory binaryComparisonInverse() { + return Div::new; + } } diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/operator/arithmetic/Sub.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/operator/arithmetic/Sub.java index bda9855d3eeb8..4ca90f08a9621 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/operator/arithmetic/Sub.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/operator/arithmetic/Sub.java @@ -6,6 +6,7 @@ package org.elasticsearch.xpack.sql.expression.predicate.operator.arithmetic; import org.elasticsearch.xpack.ql.expression.Expression; +import org.elasticsearch.xpack.ql.expression.predicate.operator.arithmetic.BinaryComparisonInvertible; import org.elasticsearch.xpack.ql.tree.NodeInfo; import org.elasticsearch.xpack.ql.tree.Source; import org.elasticsearch.xpack.sql.type.SqlDataTypes; @@ -15,7 +16,7 @@ /** * Subtraction function ({@code a - b}). */ -public class Sub extends DateTimeArithmeticOperation { +public class Sub extends DateTimeArithmeticOperation implements BinaryComparisonInvertible { public Sub(Source source, Expression left, Expression right) { super(source, left, right, SqlBinaryArithmeticOperation.SUB); @@ -45,7 +46,7 @@ protected TypeResolution resolveWithIntervals() { } @Override - public Add inverse(Source source, Expression left, Expression right) { - return new Add(source, left, right); + public ArithmeticOperationFactory binaryComparisonInverse() { + return Add::new; } } diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/optimizer/OptimizerRunTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/optimizer/OptimizerRunTests.java index aea636c3b196f..83ce88821ef85 100644 --- a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/optimizer/OptimizerRunTests.java +++ b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/optimizer/OptimizerRunTests.java @@ -155,6 +155,8 @@ public void testSimplifyComparisonArithmeticSkippedOnDateOverflow() { public void testSimplifyComparisonArithmeticSkippedOnMulDivByZero() { assertNotSimplified("float / 0 " + randomBinaryComparison() + " 1"); assertNotSimplified("float * 0 " + randomBinaryComparison() + " 1"); + assertNotSimplified("int / 0 " + randomBinaryComparison() + " 1"); + assertNotSimplified("int * 0 " + randomBinaryComparison() + " 1"); } public void testSimplifyComparisonArithmeticSkippedOnDiv() { From 06fea9ca1e0d45fbad6bf731fe4fd0d36a7dfa8b Mon Sep 17 00:00:00 2001 From: Bogdan Pintea Date: Tue, 12 Jan 2021 13:03:05 +0100 Subject: [PATCH 07/14] Remove negation optmisations These are unsafe, see https://github.com/elastic/elasticsearch/pull/67278 --- .../xpack/eql/optimizer/Optimizer.java | 2 - .../xpack/ql/optimizer/OptimizerRules.java | 127 ++++++------------ .../ql/optimizer/OptimizerRulesTests.java | 33 ----- .../xpack/sql/optimizer/Optimizer.java | 2 - .../sql/optimizer/OptimizerRunTests.java | 28 ++-- 5 files changed, 56 insertions(+), 136 deletions(-) diff --git a/x-pack/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/optimizer/Optimizer.java b/x-pack/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/optimizer/Optimizer.java index b62c982cc98a9..5bc6d746be7a0 100644 --- a/x-pack/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/optimizer/Optimizer.java +++ b/x-pack/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/optimizer/Optimizer.java @@ -35,7 +35,6 @@ import org.elasticsearch.xpack.ql.optimizer.OptimizerRules.BooleanFunctionEqualsElimination; import org.elasticsearch.xpack.ql.optimizer.OptimizerRules.BooleanLiteralsOnTheRight; import org.elasticsearch.xpack.ql.optimizer.OptimizerRules.BooleanSimplification; -import org.elasticsearch.xpack.ql.optimizer.OptimizerRules.BubbleUpNegations; import org.elasticsearch.xpack.ql.optimizer.OptimizerRules.CombineBinaryComparisons; import org.elasticsearch.xpack.ql.optimizer.OptimizerRules.CombineDisjunctionsToIn; import org.elasticsearch.xpack.ql.optimizer.OptimizerRules.ConstantFolding; @@ -88,7 +87,6 @@ protected Iterable.Batch> batches() { new CombineBinaryComparisons(), new CombineDisjunctionsToIn(), new PushDownAndCombineFilters(), - new BubbleUpNegations(), new SimplifyComparisonsArithmetics(DataTypes::areCompatible), // prune/elimination new PruneFilters(), 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 index eabdc95c16269..e07b5ea34a558 100644 --- 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 @@ -1152,51 +1152,6 @@ protected Expression rule(Expression e) { } } - public static final class BubbleUpNegations extends OptimizerExpressionRule { - - public BubbleUpNegations() { - super(TransformDirection.DOWN); - } - - @Override - protected Expression rule(Expression e) { - if (e instanceof Neg) { - Expression child = ((Neg) e).field(); - if (child instanceof ArithmeticOperation) { // cancels out NEG - MUL/DIV - NEG - x ==> MUL/DIV - x - ArithmeticOperation operation = (ArithmeticOperation) child; - if (isMulOrDiv(operation)) { - if (operation.left() instanceof Neg) { - return operation.replaceChildren(List.of(((Neg) operation.left()).field(), operation.right())); - } - if (operation.right() instanceof Neg) { - return operation.replaceChildren(List.of(operation.left(), ((Neg) operation.right()).field())); - } - } - } else if (child instanceof Neg) { // cancels out NEG - NEG - x ==> x - return ((Neg) child).field(); - } - } else if (e instanceof ArithmeticOperation) { // pulls up MUL/DIV - NEG - x ==> NEG - MUL/DIV - x - ArithmeticOperation operation = (ArithmeticOperation) e; - if (isMulOrDiv(operation)) { - if (operation.left() instanceof Neg) { - Neg neg = (Neg) operation.left(); - return new Neg(operation.source(), operation.replaceChildren(List.of(neg.field(), operation.right()))); - } - if (operation.right() instanceof Neg) { - Neg neg = (Neg) operation.right(); - return new Neg(operation.source(), operation.replaceChildren(List.of(operation.left(), neg.field()))); - } - } - } - return e; - } - - public static boolean isMulOrDiv(ArithmeticOperation operation) { - String opSymbol = operation.symbol(); - return MUL.symbol().equals(opSymbol) || DIV.symbol().equals(opSymbol); - } - } - // Simplifies arithmetic expressions with fixed point fields in BinaryComparisons. public static final class SimplifyComparisonsArithmetics extends OptimizerExpressionRule { BiFunction typesCompatible; @@ -1208,36 +1163,15 @@ public SimplifyComparisonsArithmetics(BiFunction ty @Override protected Expression rule(Expression e) { - return (e instanceof BinaryComparison) ? simplify((BinaryComparison) e) : e; - } - - private Expression simplify(BinaryComparison bc) { - // optimize only once the expression has a literal on the right side of the binary comparison - if (bc.right() instanceof Literal) { - if (bc.left() instanceof ArithmeticOperation) { + if (e instanceof BinaryComparison) { + BinaryComparison bc = (BinaryComparison) e; + // optimize only once the expression has a literal on the right side of the binary comparison + if (bc.left() instanceof ArithmeticOperation && bc.right() instanceof Literal) { return SimplifyOperation.simplify(bc, typesCompatible); - } else if (bc.left() instanceof Neg) { - return reduceNegation(bc); } + // Note: negations can't be optimized: `-int > 4` would fail for Integer.MIN_VALUE unoptimized, but succeed optimized. } - return bc; - } - - private static Expression reduceNegation(BinaryComparison bc) { - Literal bcLiteral = (Literal) bc.right(); - Expression literalNeg = safeMaybeFold(new Neg(bcLiteral.source(), bcLiteral)); - return literalNeg == null ? bc : bc.reverse().replaceChildren(List.of(((Neg) bc.left()).field(), literalNeg)); - } - - private static Expression safeMaybeFold(Expression expression) { - if (expression.foldable()) { - try { - expression = new Literal(expression.source(), expression.fold(), expression.dataType()); - } catch (ArithmeticException | DateTimeException e) { - expression = null; - } - } - return expression; + return e; } private static class SimplifyOperation { @@ -1309,17 +1243,28 @@ static Expression simplify(BinaryComparison comparison, BiFunction f : List.>of(x -> new Div(EMPTY, TWO, x), - x -> new Mul(EMPTY, TWO, x))) { - FieldAttribute fa = getFieldAttribute(); - Neg neg = new Neg(fa.source(), fa); - Expression op = f.apply(neg); - - Expression e = new BubbleUpNegations().rule(op); - assertEquals(Neg.class, e.getClass()); - Neg upperNeg = (Neg) e; - assertEquals(op.getClass(), upperNeg.field().getClass()); - ArithmeticOperation newOp = (ArithmeticOperation) upperNeg.field(); - assertEquals(TWO, newOp.left()); - assertEquals(FieldAttribute.class, newOp.right().getClass()); - FieldAttribute divFa = (FieldAttribute) newOp.right(); - assertEquals(fa, divFa); - } - } - - public void testCancelDoubleNegation() { - FieldAttribute fa = getFieldAttribute(); - Neg innerNeg = new Neg(fa.source(), fa); - Neg outerNeg = new Neg(innerNeg.source(), innerNeg); - - Expression e = new BubbleUpNegations().rule(outerNeg); - assertEquals(FieldAttribute.class, e.getClass()); - assertEquals(fa, e); - } } diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/optimizer/Optimizer.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/optimizer/Optimizer.java index 20a737c232780..038cc1c481418 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/optimizer/Optimizer.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/optimizer/Optimizer.java @@ -36,7 +36,6 @@ import org.elasticsearch.xpack.ql.expression.predicate.operator.comparison.NullEquals; import org.elasticsearch.xpack.ql.optimizer.OptimizerRules.BooleanLiteralsOnTheRight; import org.elasticsearch.xpack.ql.optimizer.OptimizerRules.BooleanSimplification; -import org.elasticsearch.xpack.ql.optimizer.OptimizerRules.BubbleUpNegations; import org.elasticsearch.xpack.ql.optimizer.OptimizerRules.CombineBinaryComparisons; import org.elasticsearch.xpack.ql.optimizer.OptimizerRules.ConstantFolding; import org.elasticsearch.xpack.ql.optimizer.OptimizerRules.OptimizerExpressionRule; @@ -151,7 +150,6 @@ protected Iterable.Batch> batches() { new PropagateEquals(), new CombineBinaryComparisons(), new CombineDisjunctionsToIn(), - new BubbleUpNegations(), new SimplifyComparisonsArithmetics(SqlDataTypes::areCompatible), // prune/elimination new PruneLiteralsInGroupBy(), diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/optimizer/OptimizerRunTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/optimizer/OptimizerRunTests.java index f45066477756f..4c0c44b18fa70 100644 --- a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/optimizer/OptimizerRunTests.java +++ b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/optimizer/OptimizerRunTests.java @@ -106,19 +106,19 @@ public void testSimplifyComparisonArithmeticWithMultipleOps() { } public void testSimplifyComparisonArithmeticWithFieldNegation() { - doTestSimplifyComparisonArithmetics("12 * (-int - 5) >= -120", "int", "<=", 5); + assertSemanticMatching("-int >= -5", "12 * (-int - 5) >= -120"); } public void testSimplifyComparisonArithmeticWithFieldDoubleNegation() { - doTestSimplifyComparisonArithmetics("12 * -(-int - 5) <= 120", "int", "<=", 5); + assertSemanticMatching("-(-int - 5) <= 10", "12 * -(-int - 5) <= 120"); } public void testSimplifyComparisonArithmeticWithConjunction() { - doTestSimplifyComparisonArithmetics("12 * (-int - 5) = -120 AND int < 6 ", "int", "==", 5); + assertSemanticMatching("-int = -5", "12 * (-int - 5) = -120 AND -int < 6 "); } public void testSimplifyComparisonArithmeticWithDisjunction() { - doTestSimplifyComparisonArithmetics("12 * (-int - 5) >= -120 OR int < 5", "int", "<=", 5); + assertSemanticMatching("-int >= -5", "12 * (-int - 5) >= -120 OR -int > -5"); } public void testSimplifyComparisonArithmeticWithFloatsAndDirectionChange() { @@ -186,14 +186,6 @@ public void testSimplifyComparisonArithmeticSkippedOnFloats() { } } - public void testBubbleUpNegation() { - String provided = "(1 / -int) * (-float) * (-float / (2 / -int)) * -2 * (-float - int) * (-(-int)) * (-float / (-int + 1)) > 1"; - String expected = "(1/int) * float * (float / (2 / int)) * -2 * (-float - int) * int * (float / (-int + 1)) < -1"; - BinaryComparison bc = extractBinaryComparison(provided); - Expression exp = parser.createExpression(expected); - assertSemanticMatching(bc, exp); - } - public void testSimplifyComparisonArithmeticWithDateTime() { doTestSimplifyComparisonArithmetics("date - INTERVAL 1 MONTH > '2010-01-01T01:01:01'::DATETIME", "date", ">", ZonedDateTime.parse("2010-02-01T01:01:01Z")); @@ -211,7 +203,7 @@ public void testSimplifyComparisonArithmeticWithDateAndMultiplication() { } private void doTestSimplifyComparisonArithmetics(String expression, String fieldName, String compSymbol, Object bound) { - BinaryComparison bc = extractBinaryComparison(expression); + BinaryComparison bc = extractPlannedBinaryComparison(expression); assertTrue(COMPARISONS.get(compSymbol).isInstance(bc)); assertTrue(bc.left() instanceof FieldAttribute); @@ -224,10 +216,16 @@ private void doTestSimplifyComparisonArithmetics(String expression, String field } private void assertNotSimplified(String condition) { - assertSemanticMatching(extractBinaryComparison(condition), parser.createExpression(condition)); + assertSemanticMatching(extractPlannedBinaryComparison(condition), parser.createExpression(condition)); + } + + private void assertSemanticMatching(String expected, String provided) { + BinaryComparison bc = extractPlannedBinaryComparison(provided); + Expression exp = parser.createExpression(expected); + assertSemanticMatching(bc, exp); } - private BinaryComparison extractBinaryComparison(String expression) { + private BinaryComparison extractPlannedBinaryComparison(String expression) { LogicalPlan plan = planWithArithmeticCondition(expression); assertTrue(plan instanceof UnaryPlan); From 6e9d04b63c4012dcd44dece07482e8dc5ff1440c Mon Sep 17 00:00:00 2001 From: Bogdan Pintea Date: Tue, 12 Jan 2021 16:56:15 +0100 Subject: [PATCH 08/14] Reintroduce negation reduction * `NEG - x < Literal` becomes `x > -Literal` if folding can be done. * add more tests --- .../xpack/ql/optimizer/OptimizerRules.java | 57 +++++++++---------- .../src/main/resources/arithmetic.sql-spec | 12 +++- .../sql/optimizer/OptimizerRunTests.java | 17 ++++-- 3 files changed, 51 insertions(+), 35 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 index e07b5ea34a558..35e56f08a717b 100644 --- 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 @@ -1163,15 +1163,36 @@ public SimplifyComparisonsArithmetics(BiFunction ty @Override protected Expression rule(Expression e) { - if (e instanceof BinaryComparison) { - BinaryComparison bc = (BinaryComparison) e; - // optimize only once the expression has a literal on the right side of the binary comparison - if (bc.left() instanceof ArithmeticOperation && bc.right() instanceof Literal) { + return (e instanceof BinaryComparison) ? simplify((BinaryComparison) e) : e; + } + + private Expression simplify(BinaryComparison bc) { + // optimize only once the expression has a literal on the right side of the binary comparison + if (bc.right() instanceof Literal) { + if (bc.left() instanceof ArithmeticOperation) { return SimplifyOperation.simplify(bc, typesCompatible); + } else if (bc.left() instanceof Neg) { + return reduceNegation(bc); } - // Note: negations can't be optimized: `-int > 4` would fail for Integer.MIN_VALUE unoptimized, but succeed optimized. } - return e; + return bc; + } + + private static Expression reduceNegation(BinaryComparison bc) { + Literal bcLiteral = (Literal) bc.right(); + Expression literalNeg = safeMaybeFold(new Neg(bcLiteral.source(), bcLiteral)); + return literalNeg == null ? bc : bc.reverse().replaceChildren(List.of(((Neg) bc.left()).field(), literalNeg)); + } + + static Expression safeMaybeFold(Expression expression) { + if (expression.foldable()) { + try { + expression = new Literal(expression.source(), expression.fold(), expression.dataType()); + } catch (ArithmeticException | DateTimeException e) { + expression = null; + } + } + return expression; } private static class SimplifyOperation { @@ -1255,17 +1276,6 @@ static Expression simplify(BinaryComparison comparison, BiFunction typesCompatible) { // Integer divisions are not safe to optimise: x / 5 > 1 <=/=> x > 5 for x in [6, 9]; same for the `==` comp - if (operation.dataType().isInteger()) { - if (isDiv) { - return true; - } - } else if (operation.dataType().isRational()) { - Expression opNonLiteral = opLiteral == opLeft ? opRight : opLeft; - if (opNonLiteral instanceof Neg) { - // wait until Neg bubbles all the way up - return true; - } - } - return super.cannotSimplify(typesCompatible); + return operation.dataType().isInteger() && isDiv || super.cannotSimplify(typesCompatible); } @Override diff --git a/x-pack/plugin/sql/qa/server/src/main/resources/arithmetic.sql-spec b/x-pack/plugin/sql/qa/server/src/main/resources/arithmetic.sql-spec index 4bfd3ec42fea0..5eb07570b74ea 100644 --- a/x-pack/plugin/sql/qa/server/src/main/resources/arithmetic.sql-spec +++ b/x-pack/plugin/sql/qa/server/src/main/resources/arithmetic.sql-spec @@ -238,7 +238,15 @@ SELECT emp_no FROM test_emp WHERE (5/4 - 1) * salary > 1; noOptimisationOnFloatDivByZero SELECT emp_no FROM test_emp WHERE (5/4 - 1) * salary::FLOAT > 1; -// negations optimiser -multipleNegations +// negations +negationDenominator +SELECT emp_no FROM test_emp WHERE 1./-salary > 1/-6E4; +chainedNegationDenominator +SELECT emp_no FROM test_emp WHERE 1./(-(-(-salary)) * -1) < 1/6E4; +negationProductDenominator +SELECT emp_no FROM test_emp WHERE 1/(-salary::FLOAT * -emp_no) > 1/5E8; +negationNumeratorAndDenominator +SELECT emp_no FROM test_emp WHERE 3 * (-languages)/(-salary::FLOAT * -emp_no) > -3/6E7; +negationsDoubleDenominator SELECT emp_no FROM test_emp WHERE ((1000000000 / -salary) / 2) * (-1 / (2 / -emp_no::FLOAT)) / -1000 >= 50000; diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/optimizer/OptimizerRunTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/optimizer/OptimizerRunTests.java index 4c0c44b18fa70..a8d1832f92b5a 100644 --- a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/optimizer/OptimizerRunTests.java +++ b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/optimizer/OptimizerRunTests.java @@ -106,19 +106,19 @@ public void testSimplifyComparisonArithmeticWithMultipleOps() { } public void testSimplifyComparisonArithmeticWithFieldNegation() { - assertSemanticMatching("-int >= -5", "12 * (-int - 5) >= -120"); + doTestSimplifyComparisonArithmetics("12 * (-int - 5) >= -120", "int", "<=", 5); } public void testSimplifyComparisonArithmeticWithFieldDoubleNegation() { - assertSemanticMatching("-(-int - 5) <= 10", "12 * -(-int - 5) <= 120"); + doTestSimplifyComparisonArithmetics("12 * -(-int - 5) <= 120", "int", "<=", 5); } public void testSimplifyComparisonArithmeticWithConjunction() { - assertSemanticMatching("-int = -5", "12 * (-int - 5) = -120 AND -int < 6 "); + doTestSimplifyComparisonArithmetics("12 * (-int - 5) = -120 AND int < 6 ", "int", "==", 5); } public void testSimplifyComparisonArithmeticWithDisjunction() { - assertSemanticMatching("-int >= -5", "12 * (-int - 5) >= -120 OR -int > -5"); + doTestSimplifyComparisonArithmetics("12 * (-int - 5) >= -120 OR int < 5", "int", "<=", 5); } public void testSimplifyComparisonArithmeticWithFloatsAndDirectionChange() { @@ -126,6 +126,10 @@ public void testSimplifyComparisonArithmeticWithFloatsAndDirectionChange() { doTestSimplifyComparisonArithmetics("float * -2 < 4", "float", ">", -2d); } + public void testSimplyComparisonArithmeticWithUnfoldedProd() { + assertSemanticMatching("int * int >= 3", "((int * int + 1) * 2 - 4) * 4 >= 16"); + } + public void testSimplifyComparisonArithmeticSkippedOnIntegerArithmeticalOverflow() { assertNotSimplified("int - 1 " + randomBinaryComparison() + " " + Long.MAX_VALUE); assertNotSimplified("1 - int " + randomBinaryComparison() + " " + Long.MIN_VALUE); @@ -133,6 +137,11 @@ public void testSimplifyComparisonArithmeticSkippedOnIntegerArithmeticalOverflow assertNotSimplified("1 - int " + randomBinaryComparison() + " " + Integer.MIN_VALUE); } + public void testSimplifyComparisonArithmeticSkippedOnIntegerArithmeticalOverflowOnNegation() { + assertNotSimplified("-int " + randomBinaryComparison() + " " + Long.MIN_VALUE); + assertNotSimplified("-int " + randomBinaryComparison() + " " + Integer.MIN_VALUE); + } + public void testSimplifyComparisonArithmeticSkippedOnFloatingPointArithmeticalOverflow() { assertNotSimplified("float / 10 " + randomBinaryComparison() + " " + Float.MAX_VALUE); assertNotSimplified("float / " + Float.MAX_VALUE +" " + randomBinaryComparison() + " 10"); From 0c9c088e024e0a7fff43678dfd2b773683c373a4 Mon Sep 17 00:00:00 2001 From: Bogdan Pintea Date: Tue, 12 Jan 2021 16:59:50 +0100 Subject: [PATCH 09/14] Revert WS-only changes Revert changes in files where only WS was updated. --- .../operator/arithmetic/ArithmeticOperation.java | 2 +- .../expression/predicate/operator/arithmetic/Mod.java | 2 +- .../xpack/ql/optimizer/OptimizerRulesTests.java | 6 +++--- .../expression/predicate/operator/arithmetic/Mod.java | 2 +- .../operator/arithmetic/SqlBinaryArithmeticTests.java | 10 +++++----- 5 files changed, 11 insertions(+), 11 deletions(-) diff --git a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/operator/arithmetic/ArithmeticOperation.java b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/operator/arithmetic/ArithmeticOperation.java index eaad5463a47bc..c2105333daf3a 100644 --- a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/operator/arithmetic/ArithmeticOperation.java +++ b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/operator/arithmetic/ArithmeticOperation.java @@ -22,7 +22,7 @@ public abstract class ArithmeticOperation extends BinaryOperatorModulo * function ({@code a % b}). - * + * * Note this operator is also registered as a function (needed for ODBC/SQL) purposes. */ public class Mod extends ArithmeticOperation { 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 index 12f45436bfe86..83c0d0fcd0dfe 100644 --- 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 @@ -1027,7 +1027,7 @@ public void testRangesOverlappingNoLowerBoundary() { Expression exp = rule.rule(or); assertEquals(r2, exp); } - + public void testBinaryComparisonAndOutOfRangeNotEqualsDifferentFields() { FieldAttribute doubleOne = fieldAttribute("double", DOUBLE); FieldAttribute doubleTwo = fieldAttribute("double2", DOUBLE); @@ -1045,12 +1045,12 @@ public void testBinaryComparisonAndOutOfRangeNotEqualsDifferentFields() { new And(EMPTY, notEqualsOf(keywordOne, L("2021")), lessThanOrEqualOf(datetimeOne, L("2020-12-04T17:48:22.954240Z"))), // double > 10.1 AND double2 != -10.1 new And(EMPTY, greaterThanOf(doubleOne, L(10.1d)), notEqualsOf(doubleTwo, L(-10.1d)))); - + for (And and : testCases) { CombineBinaryComparisons rule = new CombineBinaryComparisons(); Expression exp = rule.rule(and); assertEquals("Rule should not have transformed [" + and.nodeString() + "]", and, exp); - } + } } // Equals & NullEquals diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/operator/arithmetic/Mod.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/operator/arithmetic/Mod.java index 2924e6e468162..1d828d51cb7c4 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/operator/arithmetic/Mod.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/operator/arithmetic/Mod.java @@ -12,7 +12,7 @@ /** * Modulo * function ({@code a % b}). - * + * * Note this operator is also registered as a function (needed for ODBC/SQL) purposes. */ public class Mod extends SqlArithmeticOperation { diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/expression/predicate/operator/arithmetic/SqlBinaryArithmeticTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/expression/predicate/operator/arithmetic/SqlBinaryArithmeticTests.java index 909de206e0472..238330918173c 100644 --- a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/expression/predicate/operator/arithmetic/SqlBinaryArithmeticTests.java +++ b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/expression/predicate/operator/arithmetic/SqlBinaryArithmeticTests.java @@ -228,14 +228,14 @@ public void testMulNumberInterval() { Period p = interval.interval(); assertEquals(Period.ofYears(2).negated(), p); } - + public void testMulNullInterval() { Literal literal = interval(Period.ofMonths(1), INTERVAL_MONTH); Mul result = new Mul(EMPTY, L(null), literal); assertTrue(result.foldable()); assertNull(result.fold()); assertEquals(INTERVAL_MONTH, result.dataType()); - + result = new Mul(EMPTY, literal, L(null)); assertTrue(result.foldable()); assertNull(result.fold()); @@ -248,7 +248,7 @@ public void testAddNullInterval() { assertTrue(result.foldable()); assertNull(result.fold()); assertEquals(INTERVAL_MONTH, result.dataType()); - + result = new Add(EMPTY, literal, L(null)); assertTrue(result.foldable()); assertNull(result.fold()); @@ -261,7 +261,7 @@ public void testSubNullInterval() { assertTrue(result.foldable()); assertNull(result.fold()); assertEquals(INTERVAL_MONTH, result.dataType()); - + result = new Sub(EMPTY, literal, L(null)); assertTrue(result.foldable()); assertNull(result.fold()); @@ -298,4 +298,4 @@ private static Literal interval(TemporalAmount value, DataType intervalType) { : new IntervalDayTime((Duration) value, intervalType); return new Literal(EMPTY, i, SqlDataTypes.fromJava(i)); } -} +} \ No newline at end of file From 32cefb8a296a57a9e9aeccee05fef27cce5432a7 Mon Sep 17 00:00:00 2001 From: Bogdan Pintea Date: Tue, 12 Jan 2021 19:27:27 +0100 Subject: [PATCH 10/14] CSV spec test - add test on negation handling with integer limit value. --- .../qa/server/src/main/resources/arithmetic.csv-spec | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/x-pack/plugin/sql/qa/server/src/main/resources/arithmetic.csv-spec b/x-pack/plugin/sql/qa/server/src/main/resources/arithmetic.csv-spec index e66f90e5f6ab3..f89604a8cff27 100644 --- a/x-pack/plugin/sql/qa/server/src/main/resources/arithmetic.csv-spec +++ b/x-pack/plugin/sql/qa/server/src/main/resources/arithmetic.csv-spec @@ -61,3 +61,13 @@ SELECT emp_no x FROM test_emp WHERE hire_date - INTERVAL 10 YEAR > CAST('1980-01 10024 10026 ; + +optimizedBinaryCompArithmeticWithNegationOfIntMinVal +SELECT IIF(languages < 4, -2147483645 - languages, 0) AS I FROM test_emp WHERE -I * 3 > 6 GROUP BY I; + + I:i +--------------- +-2147483648 +-2147483647 +-2147483646 +; From 70df9813a1eff54774acc46a66d0e4bfd4de6862 Mon Sep 17 00:00:00 2001 From: Bogdan Pintea Date: Wed, 27 Jan 2021 11:46:02 +0100 Subject: [PATCH 11/14] Address feedback - rename methods for more clarify; - regroup simplification checks; - eliminate calling super class' methods; - use == for static string comps; - use Math.signum instead of inlining the calculation. --- .../predicate/operator/arithmetic/Add.java | 2 +- ...e.java => BinaryComparisonInversible.java} | 2 +- .../predicate/operator/arithmetic/Div.java | 2 +- .../predicate/operator/arithmetic/Mul.java | 2 +- .../predicate/operator/arithmetic/Sub.java | 2 +- .../xpack/ql/optimizer/OptimizerRules.java | 156 ++++++++++-------- .../predicate/operator/arithmetic/Add.java | 4 +- .../predicate/operator/arithmetic/Div.java | 4 +- .../predicate/operator/arithmetic/Mul.java | 4 +- .../predicate/operator/arithmetic/Sub.java | 4 +- 10 files changed, 97 insertions(+), 85 deletions(-) rename x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/operator/arithmetic/{BinaryComparisonInvertible.java => BinaryComparisonInversible.java} (95%) 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 9358d89d2d7ad..6e1c64907c0f3 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 @@ -12,7 +12,7 @@ /** * Addition function ({@code a + b}). */ -public class Add extends DateTimeArithmeticOperation implements BinaryComparisonInvertible { +public class Add extends DateTimeArithmeticOperation implements BinaryComparisonInversible { public Add(Source source, Expression left, Expression right) { super(source, left, right, DefaultBinaryArithmeticOperation.ADD); } diff --git a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/operator/arithmetic/BinaryComparisonInvertible.java b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/operator/arithmetic/BinaryComparisonInversible.java similarity index 95% rename from x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/operator/arithmetic/BinaryComparisonInvertible.java rename to x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/operator/arithmetic/BinaryComparisonInversible.java index da6b8443dc694..8771ea3e77b77 100644 --- a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/operator/arithmetic/BinaryComparisonInvertible.java +++ b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/operator/arithmetic/BinaryComparisonInversible.java @@ -14,7 +14,7 @@ * For instance the division is multiplication's inverse, substitution addition's, log exponentiation's a.s.o. * Not all operations - like modulo - are invertible. */ -public interface BinaryComparisonInvertible { +public interface BinaryComparisonInversible { interface ArithmeticOperationFactory { ArithmeticOperation create(Source source, Expression left, Expression right); diff --git a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/operator/arithmetic/Div.java b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/operator/arithmetic/Div.java index 2910714a05883..ba1e74dc8f9e0 100644 --- a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/operator/arithmetic/Div.java +++ b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/operator/arithmetic/Div.java @@ -14,7 +14,7 @@ /** * Division function ({@code a / b}). */ -public class Div extends ArithmeticOperation implements BinaryComparisonInvertible { +public class Div extends ArithmeticOperation implements BinaryComparisonInversible { public Div(Source source, Expression left, Expression right) { super(source, left, right, DefaultBinaryArithmeticOperation.DIV); 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 b5a79538fb537..a9156471faa4d 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 @@ -16,7 +16,7 @@ /** * Multiplication function ({@code a * b}). */ -public class Mul extends ArithmeticOperation implements BinaryComparisonInvertible { +public class Mul extends ArithmeticOperation implements BinaryComparisonInversible { public Mul(Source source, Expression left, Expression right) { super(source, left, right, DefaultBinaryArithmeticOperation.MUL); diff --git a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/operator/arithmetic/Sub.java b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/operator/arithmetic/Sub.java index 068688544d0e5..f1338e47d8d33 100644 --- a/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/operator/arithmetic/Sub.java +++ b/x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/predicate/operator/arithmetic/Sub.java @@ -12,7 +12,7 @@ /** * Subtraction function ({@code a - b}). */ -public class Sub extends DateTimeArithmeticOperation implements BinaryComparisonInvertible { +public class Sub extends DateTimeArithmeticOperation implements BinaryComparisonInversible { public Sub(Source source, Expression left, Expression right) { super(source, left, right, DefaultBinaryArithmeticOperation.SUB); 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 index 35e56f08a717b..383286007eeca 100644 --- 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 @@ -5,7 +5,6 @@ */ package org.elasticsearch.xpack.ql.optimizer; -import org.elasticsearch.xpack.ql.QlIllegalArgumentException; import org.elasticsearch.xpack.ql.expression.Expression; import org.elasticsearch.xpack.ql.expression.Expressions; import org.elasticsearch.xpack.ql.expression.Literal; @@ -22,7 +21,7 @@ import org.elasticsearch.xpack.ql.expression.predicate.logical.Or; import org.elasticsearch.xpack.ql.expression.predicate.nulls.IsNotNull; import org.elasticsearch.xpack.ql.expression.predicate.operator.arithmetic.ArithmeticOperation; -import org.elasticsearch.xpack.ql.expression.predicate.operator.arithmetic.BinaryComparisonInvertible; +import org.elasticsearch.xpack.ql.expression.predicate.operator.arithmetic.BinaryComparisonInversible; import org.elasticsearch.xpack.ql.expression.predicate.operator.arithmetic.Neg; import org.elasticsearch.xpack.ql.expression.predicate.operator.arithmetic.Sub; import org.elasticsearch.xpack.ql.expression.predicate.operator.comparison.BinaryComparison; @@ -57,6 +56,8 @@ import java.util.Set; import java.util.function.BiFunction; +import static java.lang.Math.signum; +import static java.util.Arrays.asList; 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; @@ -1152,7 +1153,7 @@ protected Expression rule(Expression e) { } } - // Simplifies arithmetic expressions with fixed point fields in BinaryComparisons. + // Simplifies arithmetic expressions with BinaryComparisons and fixed point fields, such as: (int + 2) / 3 > 4 => int > 10 public static final class SimplifyComparisonsArithmetics extends OptimizerExpressionRule { BiFunction typesCompatible; @@ -1170,21 +1171,44 @@ private Expression simplify(BinaryComparison bc) { // optimize only once the expression has a literal on the right side of the binary comparison if (bc.right() instanceof Literal) { if (bc.left() instanceof ArithmeticOperation) { - return SimplifyOperation.simplify(bc, typesCompatible); - } else if (bc.left() instanceof Neg) { - return reduceNegation(bc); + return simplifyBinaryComparison(bc); + } + if (bc.left() instanceof Neg) { + return foldNegation(bc); } } return bc; } - private static Expression reduceNegation(BinaryComparison bc) { + private Expression simplifyBinaryComparison(BinaryComparison comparison) { + ArithmeticOperation operation = (ArithmeticOperation) comparison.left(); + // Use symbol comp: SQL operations aren't available in this package (as dependencies) + String opSymbol = operation.symbol(); + // Modulo can't be simplified. + if (opSymbol == MOD.symbol()) { + return comparison; + } + OperationSimplifier simplification = null; + if (isMulOrDiv(opSymbol)) { + simplification = new MulDivSimplifier(comparison); + } else if (opSymbol == ADD.symbol() || opSymbol == SUB.symbol()) { + simplification = new AddSubSimplifier(comparison); + } + + return (simplification == null || simplification.isUnsafe(typesCompatible)) ? comparison : simplification.apply(); + } + + private static boolean isMulOrDiv(String opSymbol) { + return opSymbol == MUL.symbol() || opSymbol == DIV.symbol(); + } + + private static Expression foldNegation(BinaryComparison bc) { Literal bcLiteral = (Literal) bc.right(); - Expression literalNeg = safeMaybeFold(new Neg(bcLiteral.source(), bcLiteral)); - return literalNeg == null ? bc : bc.reverse().replaceChildren(List.of(((Neg) bc.left()).field(), literalNeg)); + Expression literalNeg = tryFolding(new Neg(bcLiteral.source(), bcLiteral)); + return literalNeg == null ? bc : bc.reverse().replaceChildren(asList(((Neg) bc.left()).field(), literalNeg)); } - static Expression safeMaybeFold(Expression expression) { + private static Expression tryFolding(Expression expression) { if (expression.foldable()) { try { expression = new Literal(expression.source(), expression.fold(), expression.dataType()); @@ -1195,7 +1219,7 @@ static Expression safeMaybeFold(Expression expression) { return expression; } - private static class SimplifyOperation { + private abstract static class OperationSimplifier { final BinaryComparison comparison; final Literal bcLiteral; final ArithmeticOperation operation; @@ -1203,7 +1227,7 @@ private static class SimplifyOperation { final Expression opRight; final Literal opLiteral; - SimplifyOperation(BinaryComparison comparison) { + OperationSimplifier(BinaryComparison comparison) { this.comparison = comparison; operation = (ArithmeticOperation) comparison.left(); bcLiteral = (Literal) comparison.right(); @@ -1221,7 +1245,7 @@ private static class SimplifyOperation { } // can it be quickly fast-tracked that the operation can't be reduced? - boolean cannotSimplify(BiFunction typesCompatible) { + boolean isUnsafe(BiFunction typesCompatible) { if (opLiteral == null) { // one of the arithm. operands must be a literal, otherwise the operation wouldn't simplify anything return true; @@ -1237,109 +1261,97 @@ boolean cannotSimplify(BiFunction typesCompatible) } // the Literal will be moved to the right of the comparison, but only if data-compatible with what's there - return typesCompatible.apply(bcLiteral.dataType(), opLiteral.dataType()) == false; + if (typesCompatible.apply(bcLiteral.dataType(), opLiteral.dataType()) == false) { + return true; + } + + return opSpecificIsUnsafe(); } - Expression doSimplify() { + Expression apply() { // force float point folding for FlP field Literal bcl = operation.dataType().isRational() ? Literal.of(bcLiteral, ((Number) bcLiteral.value()).doubleValue()) : bcLiteral; - if (operation instanceof BinaryComparisonInvertible == false) { // should be an assertion? - throw new QlIllegalArgumentException("Unexpected [" + operation.symbol() + "] operation object"); - } - Expression bcRightExpression = ((BinaryComparisonInvertible) operation).binaryComparisonInverse() + Expression bcRightExpression = ((BinaryComparisonInversible) operation).binaryComparisonInverse() .create(bcl.source(), bcl, opRight); - bcRightExpression = safeMaybeFold(bcRightExpression); - return bcRightExpression == null ? comparison : comparison.replaceChildren(List.of(opLeft, bcRightExpression)); + bcRightExpression = tryFolding(bcRightExpression); + return bcRightExpression != null + ? opSpecificPostApply((BinaryComparison) comparison.replaceChildren(List.of(opLeft, bcRightExpression))) + : comparison; } - static Expression simplify(BinaryComparison comparison, BiFunction typesCompatible) { - ArithmeticOperation operation = (ArithmeticOperation) comparison.left(); - // Use symbol comp: SQL operations aren't available in this package (as dependencies) - String opSymbol = operation.symbol(); - // Modulo can't be simplified. - if (MOD.symbol().equals(opSymbol)) { - return comparison; - } - SimplifyOperation simplify = null; - if (isMulOrDiv(opSymbol)) { - simplify = new SimplifyMulDiv(comparison); - } else if (ADD.symbol().equals(opSymbol) || SUB.symbol().equals(opSymbol)) { - simplify = new SimplifyAddSub(comparison); - } - - return (simplify == null || simplify.cannotSimplify(typesCompatible)) ? comparison : simplify.doSimplify(); - } + // operation-specific operations: + // - fast-tracking of simplification unsafety + abstract boolean opSpecificIsUnsafe(); - static boolean isMulOrDiv(String opSymbol) { - return MUL.symbol().equals(opSymbol) || DIV.symbol().equals(opSymbol); + // - post adjustments + Expression opSpecificPostApply(BinaryComparison binaryComparison) { + return binaryComparison; } } - private static class SimplifyAddSub extends SimplifyOperation { + private static class AddSubSimplifier extends OperationSimplifier { - SimplifyAddSub(BinaryComparison comparison) { + AddSubSimplifier(BinaryComparison comparison) { super(comparison); } @Override - boolean cannotSimplify(BiFunction typesCompatible) { + boolean opSpecificIsUnsafe() { // no ADD/SUB with floating fields - return operation.dataType().isRational() || super.cannotSimplify(typesCompatible); - } + if (operation.dataType().isRational()) { + return true; + } - @Override - Expression doSimplify() { - if (SUB.symbol().equals(operation.symbol()) && opRight instanceof Literal == false) { + if (operation.symbol() == SUB.symbol() && opRight instanceof Literal == false) { // such as: 1 - x > -MAX // if next simplification step would fail on overflow anyways, skip the optimisation already - if (safeMaybeFold(new Sub(EMPTY, opLeft, bcLiteral)) == null) { - return comparison; - } + return tryFolding(new Sub(EMPTY, opLeft, bcLiteral)) == null; } - return super.doSimplify(); + + return false; } } - private static class SimplifyMulDiv extends SimplifyOperation { + private static class MulDivSimplifier extends OperationSimplifier { private final boolean isDiv; // and not MUL. + private final int opRightSign; // sign of the right operand in: (left) (op) (right) (comp) (literal) - SimplifyMulDiv(BinaryComparison comparison) { + MulDivSimplifier(BinaryComparison comparison) { super(comparison); - isDiv = DIV.symbol().equals(operation.symbol()); + isDiv = operation.symbol() == DIV.symbol(); + opRightSign = sign(opRight); } @Override - boolean cannotSimplify(BiFunction typesCompatible) { + boolean opSpecificIsUnsafe() { // Integer divisions are not safe to optimise: x / 5 > 1 <=/=> x > 5 for x in [6, 9]; same for the `==` comp - return operation.dataType().isInteger() && isDiv || super.cannotSimplify(typesCompatible); - } + if (operation.dataType().isInteger() && isDiv) { + return true; + } - @Override - Expression doSimplify() { // If current operation is a multiplication, it's inverse will be a division: safe only if outcome is still integral. if (isDiv == false && opLeft.dataType().isInteger()) { long opLiteralValue = ((Number) opLiteral.value()).longValue(); - if (opLiteralValue == 0 || ((Number) bcLiteral.value()).longValue() % opLiteralValue != 0) { - return comparison; - } + return opLiteralValue == 0 || ((Number) bcLiteral.value()).longValue() % opLiteralValue != 0; } + + // can't move a 0 in Mul/Div comparisons + return opRightSign == 0; + } + + @Override + Expression opSpecificPostApply(BinaryComparison binaryComparison) { // negative multiplication/division changes the direction of the comparison - int sign = sign(opRight); - if (sign == 0) { - return comparison; - } - BinaryComparison bc = (BinaryComparison) super.doSimplify(); - return sign < 0 ? bc.reverse() : bc; + return opRightSign < 0 ? binaryComparison.reverse() : binaryComparison; } - private int sign(Object obj) { + private static int sign(Object obj) { int sign = 1; if (obj instanceof Number) { - double d = ((Number) obj).doubleValue(); - sign = d == 0 ? 0 : (d < 0 ? -1 : 1); + sign = (int) signum(((Number) obj).doubleValue()); } else if (obj instanceof Literal) { sign = sign(((Literal) obj).value()); } else if (obj instanceof Neg) { 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 36444f3cb8fa9..433647043724c 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 @@ -6,14 +6,14 @@ package org.elasticsearch.xpack.sql.expression.predicate.operator.arithmetic; import org.elasticsearch.xpack.ql.expression.Expression; -import org.elasticsearch.xpack.ql.expression.predicate.operator.arithmetic.BinaryComparisonInvertible; +import org.elasticsearch.xpack.ql.expression.predicate.operator.arithmetic.BinaryComparisonInversible; import org.elasticsearch.xpack.ql.tree.NodeInfo; import org.elasticsearch.xpack.ql.tree.Source; /** * Addition function ({@code a + b}). */ -public class Add extends DateTimeArithmeticOperation implements BinaryComparisonInvertible { +public class Add extends DateTimeArithmeticOperation implements BinaryComparisonInversible { public Add(Source source, Expression left, Expression right) { super(source, left, right, SqlBinaryArithmeticOperation.ADD); } diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/operator/arithmetic/Div.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/operator/arithmetic/Div.java index 10830d4cec3e7..1de9fc741973a 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/operator/arithmetic/Div.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/operator/arithmetic/Div.java @@ -6,7 +6,7 @@ package org.elasticsearch.xpack.sql.expression.predicate.operator.arithmetic; import org.elasticsearch.xpack.ql.expression.Expression; -import org.elasticsearch.xpack.ql.expression.predicate.operator.arithmetic.BinaryComparisonInvertible; +import org.elasticsearch.xpack.ql.expression.predicate.operator.arithmetic.BinaryComparisonInversible; import org.elasticsearch.xpack.ql.expression.predicate.operator.arithmetic.Mul; import org.elasticsearch.xpack.ql.tree.NodeInfo; import org.elasticsearch.xpack.ql.tree.Source; @@ -16,7 +16,7 @@ /** * Division function ({@code a / b}). */ -public class Div extends SqlArithmeticOperation implements BinaryComparisonInvertible { +public class Div extends SqlArithmeticOperation implements BinaryComparisonInversible { public Div(Source source, Expression left, Expression right) { super(source, left, right, SqlBinaryArithmeticOperation.DIV); 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 0a7b14a75e73c..f26eb168c2810 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 @@ -6,7 +6,7 @@ package org.elasticsearch.xpack.sql.expression.predicate.operator.arithmetic; import org.elasticsearch.xpack.ql.expression.Expression; -import org.elasticsearch.xpack.ql.expression.predicate.operator.arithmetic.BinaryComparisonInvertible; +import org.elasticsearch.xpack.ql.expression.predicate.operator.arithmetic.BinaryComparisonInversible; import org.elasticsearch.xpack.ql.tree.NodeInfo; import org.elasticsearch.xpack.ql.tree.Source; import org.elasticsearch.xpack.ql.type.DataType; @@ -18,7 +18,7 @@ /** * Multiplication function ({@code a * b}). */ -public class Mul extends SqlArithmeticOperation implements BinaryComparisonInvertible { +public class Mul extends SqlArithmeticOperation implements BinaryComparisonInversible { private DataType dataType; diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/operator/arithmetic/Sub.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/operator/arithmetic/Sub.java index 4ca90f08a9621..7ced4a81eb46d 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/operator/arithmetic/Sub.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/operator/arithmetic/Sub.java @@ -6,7 +6,7 @@ package org.elasticsearch.xpack.sql.expression.predicate.operator.arithmetic; import org.elasticsearch.xpack.ql.expression.Expression; -import org.elasticsearch.xpack.ql.expression.predicate.operator.arithmetic.BinaryComparisonInvertible; +import org.elasticsearch.xpack.ql.expression.predicate.operator.arithmetic.BinaryComparisonInversible; import org.elasticsearch.xpack.ql.tree.NodeInfo; import org.elasticsearch.xpack.ql.tree.Source; import org.elasticsearch.xpack.sql.type.SqlDataTypes; @@ -16,7 +16,7 @@ /** * Subtraction function ({@code a - b}). */ -public class Sub extends DateTimeArithmeticOperation implements BinaryComparisonInvertible { +public class Sub extends DateTimeArithmeticOperation implements BinaryComparisonInversible { public Sub(Source source, Expression left, Expression right) { super(source, left, right, SqlBinaryArithmeticOperation.SUB); From 99f08c1f30aafe63f8e03237c3197510d4c58eb6 Mon Sep 17 00:00:00 2001 From: Bogdan Pintea Date: Wed, 27 Jan 2021 12:19:34 +0100 Subject: [PATCH 12/14] Address review comments Update QA test. --- .../src/main/resources/arithmetic.csv-spec | 52 +++++++++---------- 1 file changed, 26 insertions(+), 26 deletions(-) diff --git a/x-pack/plugin/sql/qa/server/src/main/resources/arithmetic.csv-spec b/x-pack/plugin/sql/qa/server/src/main/resources/arithmetic.csv-spec index f89604a8cff27..9550091f3df37 100644 --- a/x-pack/plugin/sql/qa/server/src/main/resources/arithmetic.csv-spec +++ b/x-pack/plugin/sql/qa/server/src/main/resources/arithmetic.csv-spec @@ -29,37 +29,37 @@ null |null |null |null |null | ; optimizedIntervalFilterPlus -SELECT emp_no x FROM test_emp WHERE hire_date + INTERVAL 20 YEAR > CAST('2010-01-01T00:00:00' AS TIMESTAMP) LIMIT 10; +SELECT emp_no x, hire_date h FROM test_emp WHERE hire_date + INTERVAL 20 YEAR > CAST('2010-01-01T00:00:00' AS TIMESTAMP) LIMIT 10; - x:i ---------------- -10008 -10011 -10012 -10016 -10017 -10019 -10020 -10022 -10024 -10026 + x | h +---------------+------------------------ +10008 |1994-09-15T00:00:00.000Z +10011 |1990-01-22T00:00:00.000Z +10012 |1992-12-18T00:00:00.000Z +10016 |1995-01-27T00:00:00.000Z +10017 |1993-08-03T00:00:00.000Z +10019 |1999-04-30T00:00:00.000Z +10020 |1991-01-26T00:00:00.000Z +10022 |1995-08-22T00:00:00.000Z +10024 |1997-05-19T00:00:00.000Z +10026 |1995-03-20T00:00:00.000Z ; optimizedIntervalFilterMinus -SELECT emp_no x FROM test_emp WHERE hire_date - INTERVAL 10 YEAR > CAST('1980-01-01T00:00:00' AS TIMESTAMP) LIMIT 10; +SELECT emp_no x, hire_date h FROM test_emp WHERE hire_date - INTERVAL 10 YEAR > CAST('1980-01-01T00:00:00' AS TIMESTAMP) LIMIT 10; - x:i ---------------- -10008 -10011 -10012 -10016 -10017 -10019 -10020 -10022 -10024 -10026 + x | h +---------------+------------------------ +10008 |1994-09-15T00:00:00.000Z +10011 |1990-01-22T00:00:00.000Z +10012 |1992-12-18T00:00:00.000Z +10016 |1995-01-27T00:00:00.000Z +10017 |1993-08-03T00:00:00.000Z +10019 |1999-04-30T00:00:00.000Z +10020 |1991-01-26T00:00:00.000Z +10022 |1995-08-22T00:00:00.000Z +10024 |1997-05-19T00:00:00.000Z +10026 |1995-03-20T00:00:00.000Z ; optimizedBinaryCompArithmeticWithNegationOfIntMinVal From 8eee4eab81b3a49a39ba31319e2b75169e5892e9 Mon Sep 17 00:00:00 2001 From: Bogdan Pintea Date: Thu, 28 Jan 2021 20:21:23 +0100 Subject: [PATCH 13/14] Address review comments - rename methods; - add comments. --- .../xpack/ql/optimizer/OptimizerRules.java | 21 ++++++++++--------- 1 file changed, 11 insertions(+), 10 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 index 383286007eeca..b2d55fe151a7f 100644 --- 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 @@ -1213,6 +1213,7 @@ private static Expression tryFolding(Expression expression) { try { expression = new Literal(expression.source(), expression.fold(), expression.dataType()); } catch (ArithmeticException | DateTimeException e) { + // null signals that folding would result in an over-/underflow (such as Long.MAX_VALUE+1); the optimisation is skipped. expression = null; } } @@ -1245,7 +1246,7 @@ private abstract static class OperationSimplifier { } // can it be quickly fast-tracked that the operation can't be reduced? - boolean isUnsafe(BiFunction typesCompatible) { + final boolean isUnsafe(BiFunction typesCompatible) { if (opLiteral == null) { // one of the arithm. operands must be a literal, otherwise the operation wouldn't simplify anything return true; @@ -1265,10 +1266,10 @@ boolean isUnsafe(BiFunction typesCompatible) { return true; } - return opSpecificIsUnsafe(); + return isOpUnsafe(); } - Expression apply() { + final Expression apply() { // force float point folding for FlP field Literal bcl = operation.dataType().isRational() ? Literal.of(bcLiteral, ((Number) bcLiteral.value()).doubleValue()) @@ -1278,16 +1279,16 @@ Expression apply() { .create(bcl.source(), bcl, opRight); bcRightExpression = tryFolding(bcRightExpression); return bcRightExpression != null - ? opSpecificPostApply((BinaryComparison) comparison.replaceChildren(List.of(opLeft, bcRightExpression))) + ? postProcess((BinaryComparison) comparison.replaceChildren(List.of(opLeft, bcRightExpression))) : comparison; } // operation-specific operations: // - fast-tracking of simplification unsafety - abstract boolean opSpecificIsUnsafe(); + abstract boolean isOpUnsafe(); - // - post adjustments - Expression opSpecificPostApply(BinaryComparison binaryComparison) { + // - post optimisation adjustments + Expression postProcess(BinaryComparison binaryComparison) { return binaryComparison; } } @@ -1299,7 +1300,7 @@ private static class AddSubSimplifier extends OperationSimplifier { } @Override - boolean opSpecificIsUnsafe() { + boolean isOpUnsafe() { // no ADD/SUB with floating fields if (operation.dataType().isRational()) { return true; @@ -1326,7 +1327,7 @@ private static class MulDivSimplifier extends OperationSimplifier { } @Override - boolean opSpecificIsUnsafe() { + boolean isOpUnsafe() { // Integer divisions are not safe to optimise: x / 5 > 1 <=/=> x > 5 for x in [6, 9]; same for the `==` comp if (operation.dataType().isInteger() && isDiv) { return true; @@ -1343,7 +1344,7 @@ boolean opSpecificIsUnsafe() { } @Override - Expression opSpecificPostApply(BinaryComparison binaryComparison) { + Expression postProcess(BinaryComparison binaryComparison) { // negative multiplication/division changes the direction of the comparison return opRightSign < 0 ? binaryComparison.reverse() : binaryComparison; } From 28ba3e53c69ac9f3ab2e5d68f7e682d580c91f35 Mon Sep 17 00:00:00 2001 From: Bogdan Pintea Date: Mon, 1 Feb 2021 18:33:10 +0100 Subject: [PATCH 14/14] Adjust merging outcome for one test Both upstream and current branch updated line in the same way (incrementing a count), so merging left it unchanged. However the net result needs to reflect both modifications (i.e. inc by two). --- .../xpack/sql/analysis/analyzer/FieldAttributeTests.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/analysis/analyzer/FieldAttributeTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/analysis/analyzer/FieldAttributeTests.java index f93417c6ff95b..db5472424dc55 100644 --- a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/analysis/analyzer/FieldAttributeTests.java +++ b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/analysis/analyzer/FieldAttributeTests.java @@ -165,7 +165,7 @@ public void testDottedFieldPathTypo() { public void testStarExpansionExcludesObjectAndUnsupportedTypes() { LogicalPlan plan = plan("SELECT * FROM test"); List list = ((Project) plan).projections(); - assertThat(list, hasSize(11)); + assertThat(list, hasSize(12)); List names = Expressions.names(list); assertThat(names, not(hasItem("some"))); assertThat(names, not(hasItem("some.dotted")));