From f8a73cd8b52838cac727aa4ff78812b51ed3f647 Mon Sep 17 00:00:00 2001 From: Bogdan Pintea Date: Tue, 7 Jan 2020 14:41:28 +0100 Subject: [PATCH 1/2] SQL: Optimisation fixes for conjunction merges This commit fixes the following issues around the way comparisions are merged with ranges in conjunctions: * the decision to include the equality of the lower limit is corrected; * the selection of the upper limit is corrected to use the upper bound of the range; * the list of terms in the conjunction is sorted to have the ranges at the bottom; this allows subsequent binary comarisions to find compatible ranges and potentially be merged away. The end guarantee being that the optimisation takes place irrespective of the order of the conjunction terms in the statement. Some comments are also corrected. --- .../xpack/sql/expression/Expression.java | 2 +- .../xpack/sql/optimizer/Optimizer.java | 33 ++++++++---- .../xpack/sql/optimizer/OptimizerTests.java | 51 +++++++++++++++++++ 3 files changed, 76 insertions(+), 10 deletions(-) diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/Expression.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/Expression.java index e2e3f99ca8790..166ccd72f711f 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/Expression.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/Expression.java @@ -19,7 +19,7 @@ * In a SQL statement, an Expression is whatever a user specifies inside an * action, so for instance: * - * {@code SELECT a, b, MAX(c, d) FROM i} + * {@code SELECT a, b, ABS(c) FROM i} * * a, b, ABS(c), and i are all Expressions, with ABS(c) being a Function * (which is a type of expression) with a single child, c. 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 0d7d9acc343d0..ab1b533fabaf7 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 @@ -92,6 +92,7 @@ import java.util.Map.Entry; import java.util.Set; import java.util.function.Consumer; +import java.util.Comparator; import static java.util.Collections.singletonList; import static org.elasticsearch.xpack.sql.expression.Expressions.equalsAsAttribute; @@ -397,7 +398,7 @@ protected LogicalPlan rule(OrderBy ob) { return ob; } } - + static class PruneOrderByForImplicitGrouping extends OptimizerRule { @Override @@ -1090,7 +1091,21 @@ private Expression combine(And and) { boolean changed = false; - for (Expression ex : Predicates.splitAnd(and)) { + List andExps = Predicates.splitAnd(and); + // Ranges need to show up before BinaryComparisons in list, to allow the latter be optimized away into a Range, if possible + andExps.sort(new Comparator() { + @Override + public int compare(Expression o1, Expression o2) { + if (o1 instanceof Range && o2 instanceof Range) { + return 0; // keep ranges' order + } else if (o1 instanceof Range || o2 instanceof Range) { + return o2 instanceof Range ? 1 : -1; + } else { + return 0; // keep non-ranges' order + } + } + }); + for (Expression ex : andExps) { if (ex instanceof Range) { Range r = (Range) ex; if (findExistingRange(r, ranges, true)) { @@ -1215,9 +1230,9 @@ private static boolean findExistingRange(Range main, List ranges, boolean lowerEq = comp == 0 && main.includeLower() == other.includeLower(); // AND if (conjunctive) { - // (2 < a < 3) AND (1 < a < 3) -> (1 < a < 3) + // (2 < a < 3) AND (1 < a < 3) -> (2 < a < 3) lower = comp > 0 || - // (2 < a < 3) AND (2 < a <= 3) -> (2 < a < 3) + // (2 < a < 3) AND (2 <= a < 3) -> (2 < a < 3) (comp == 0 && !main.includeLower() && other.includeLower()); } // OR @@ -1316,7 +1331,7 @@ private boolean findConjunctiveComparisonInRange(BinaryComparison main, List 1 < a < 2 boolean upperEq = comp == 0 && other.includeUpper() && main instanceof LessThan; // a < 2 AND (1 < a < 3) -> 1 < a < 2 - boolean upper = comp > 0 || upperEq; + boolean upper = comp < 0 || upperEq; if (upper) { ranges.remove(i); ranges.add(i, new Range(other.source(), other.value(), other.lower(), other.includeLower(), - main.right(), upperEq ? true : other.includeUpper())); + main.right(), upperEq ? false : main instanceof LessThanOrEqual)); } // found a match 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 1f45d917dabd2..2b6378e2ee13c 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 @@ -1012,6 +1012,57 @@ public void testCombineBinaryComparisonsInclude() { assertEquals(FIVE, r.right()); } + // 2 < a AND (2 <= a < 3) -> 2 < a < 3 + public void testCombineBinaryComparisonsAndRangeLower() { + FieldAttribute fa = getFieldAttribute(); + + GreaterThan gt = new GreaterThan(EMPTY, fa, TWO); + Range range = new Range(EMPTY, fa, TWO, true, THREE, false); + + CombineBinaryComparisons rule = new CombineBinaryComparisons(); + Expression exp = rule.rule(new And(EMPTY, gt, range)); + assertEquals(Range.class, exp.getClass()); + Range r = (Range)exp; + assertEquals(TWO, r.lower()); + assertFalse(r.includeLower()); + assertEquals(THREE, r.upper()); + assertFalse(r.includeUpper()); + } + + // a < 4 AND (1 < a < 3) -> 1 < a < 3 + public void testCombineBinaryComparisonsAndRangeUpper() { + FieldAttribute fa = getFieldAttribute(); + + LessThan lt = new LessThan(EMPTY, fa, FOUR); + Range range = new Range(EMPTY, fa, ONE, false, THREE, false); + + CombineBinaryComparisons rule = new CombineBinaryComparisons(); + Expression exp = rule.rule(new And(EMPTY, range, lt)); + assertEquals(Range.class, exp.getClass()); + Range r = (Range)exp; + assertEquals(ONE, r.lower()); + assertFalse(r.includeLower()); + assertEquals(THREE, r.upper()); + assertFalse(r.includeUpper()); + } + + // a <= 2 AND (1 < a < 3) -> 1 < a <= 2 + public void testCombineBinaryComparisonsAndRangeUpperEqual() { + FieldAttribute fa = getFieldAttribute(); + + LessThanOrEqual lte = new LessThanOrEqual(EMPTY, fa, TWO); + Range range = new Range(EMPTY, fa, ONE, false, THREE, false); + + CombineBinaryComparisons rule = new CombineBinaryComparisons(); + Expression exp = rule.rule(new And(EMPTY, lte, range)); + assertEquals(Range.class, exp.getClass()); + Range r = (Range)exp; + assertEquals(ONE, r.lower()); + assertFalse(r.includeLower()); + assertEquals(TWO, r.upper()); + assertTrue(r.includeUpper()); + } + // 3 <= a AND 4 < a AND a <= 7 AND a < 6 -> 4 < a < 6 public void testCombineMultipleBinaryComparisons() { FieldAttribute fa = getFieldAttribute(); From 9587bfe8d6d65af9b4768a7cfb05104d8402325a Mon Sep 17 00:00:00 2001 From: Bogdan Pintea Date: Wed, 8 Jan 2020 15:42:21 +0100 Subject: [PATCH 2/2] adress review observation on anon. comparator Replace anonymous comparator of split AND Expressions with a lambda. --- .../xpack/sql/optimizer/Optimizer.java | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-) 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 ab1b533fabaf7..ce23010c8bd98 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 @@ -92,7 +92,6 @@ import java.util.Map.Entry; import java.util.Set; import java.util.function.Consumer; -import java.util.Comparator; import static java.util.Collections.singletonList; import static org.elasticsearch.xpack.sql.expression.Expressions.equalsAsAttribute; @@ -1093,16 +1092,13 @@ private Expression combine(And and) { List andExps = Predicates.splitAnd(and); // Ranges need to show up before BinaryComparisons in list, to allow the latter be optimized away into a Range, if possible - andExps.sort(new Comparator() { - @Override - public int compare(Expression o1, Expression o2) { - if (o1 instanceof Range && o2 instanceof Range) { - return 0; // keep ranges' order - } else if (o1 instanceof Range || o2 instanceof Range) { - return o2 instanceof Range ? 1 : -1; - } else { - return 0; // keep non-ranges' order - } + andExps.sort((o1, o2) -> { + if (o1 instanceof Range && o2 instanceof Range) { + return 0; // keep ranges' order + } else if (o1 instanceof Range || o2 instanceof Range) { + return o2 instanceof Range ? 1 : -1; + } else { + return 0; // keep non-ranges' order } }); for (Expression ex : andExps) {