From d50774155e48f340e6f07d66b001e221ca57f21f Mon Sep 17 00:00:00 2001 From: Costin Leau Date: Thu, 26 Apr 2018 18:44:02 +0300 Subject: [PATCH 1/5] SQL: Reduce number of ranges generated for comparisons Rewrote optimization rule for combining ranges by improving the detection of binary comparisons in a tree to better combine them in a range, regardless of their place inside an expression. Additionally, improve the comparisons of Numbers of different types Also, improve reassembly of conjunction/disjunction into balanced trees. Fix #30017 --- .../predicate/BinaryComparison.java | 31 +- .../sql/expression/predicate/Predicates.java | 40 +- .../xpack/sql/expression/predicate/Range.java | 18 +- .../xpack/sql/optimizer/Optimizer.java | 363 ++++++++++++++-- .../xpack/sql/optimizer/OptimizerTests.java | 395 +++++++++++++++++- 5 files changed, 805 insertions(+), 42 deletions(-) diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/BinaryComparison.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/BinaryComparison.java index 0fe94feba16cc..b35f3cec9a02a 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/BinaryComparison.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/BinaryComparison.java @@ -9,7 +9,6 @@ import org.elasticsearch.xpack.sql.expression.Expression; import org.elasticsearch.xpack.sql.tree.Location; import org.elasticsearch.xpack.sql.type.DataType; -import org.elasticsearch.xpack.sql.type.DataTypes; // marker class to indicate operations that rely on values public abstract class BinaryComparison extends BinaryOperator { @@ -34,10 +33,34 @@ public DataType dataType() { } @SuppressWarnings({ "rawtypes", "unchecked" }) - static Integer compare(Object left, Object right) { - if (left instanceof Comparable && right instanceof Comparable) { - return Integer.valueOf(((Comparable) left).compareTo(right)); + public static Integer compare(Object l, Object r) { + // typical number comparison + if (l instanceof Number && r instanceof Number) { + return compare((Number) l, (Number) r); + } + + try { + if (l instanceof Comparable && r instanceof Comparable) { + return Integer.valueOf(((Comparable) l).compareTo(r)); + } + } catch (ClassCastException cce) { + // types are not compatible + // return null } return null; } + + static Integer compare(Number l, Number r) { + if (l instanceof Double || r instanceof Double) { + return Double.compare(l.doubleValue(), r.doubleValue()); + } + if (l instanceof Float || r instanceof Float) { + return Float.compare(l.floatValue(), r.floatValue()); + } + if (l instanceof Long || r instanceof Long) { + return Long.compare(l.longValue(), r.longValue()); + } + + return Integer.valueOf(Integer.compare(l.intValue(), r.intValue())); + } } diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/Predicates.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/Predicates.java index 7439f6def1453..755894c19718b 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/Predicates.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/Predicates.java @@ -9,8 +9,11 @@ import org.elasticsearch.xpack.sql.plan.logical.LogicalPlan; import java.util.ArrayList; -import java.util.Collections; import java.util.List; +import java.util.function.BiFunction; + +import static java.util.Collections.emptyList; +import static java.util.Collections.singletonList; public abstract class Predicates { @@ -22,7 +25,7 @@ public static List splitAnd(Expression exp) { list.addAll(splitAnd(and.right())); return list; } - return Collections.singletonList(exp); + return singletonList(exp); } public static List splitOr(Expression exp) { @@ -33,15 +36,38 @@ public static List splitOr(Expression exp) { list.addAll(splitOr(or.right())); return list; } - return Collections.singletonList(exp); + return singletonList(exp); } public static Expression combineOr(List exps) { - return exps.stream().reduce((l, r) -> new Or(l.location(), l, r)).orElse(null); + return combine(exps, (l, r) -> new Or(l.location(), l, r)); } public static Expression combineAnd(List exps) { - return exps.stream().reduce((l, r) -> new And(l.location(), l, r)).orElse(null); + return combine(exps, (l, r) -> new And(l.location(), l, r)); + } + + /** + * Build a binary 'pyramid' - while a bit longer this method creates a balanced tree as oppose to a plain + * recursive approach that creates an unbalanced one (either to the left or right). + */ + private static Expression combine(List exps, BiFunction combiner) { + if (exps.isEmpty()) { + return null; + } + + List result = new ArrayList<>(exps); + + while (result.size() > 1) { + // combine (in place) expressions in pairs + for (int i = 0; i < result.size() - 1; i++) { + Expression l = result.remove(i); + Expression r = result.remove(i); + result.add(i, combiner.apply(l, r)); + } + } + + return result.get(0); } public static List inCommon(List l, List r) { @@ -53,7 +79,7 @@ public static List inCommon(List l, List r) } } } - return common.isEmpty() ? Collections.emptyList() : common; + return common.isEmpty() ? emptyList() : common; } public static List subtract(List from, List r) { @@ -65,7 +91,7 @@ public static List subtract(List from, List } } } - return diff.isEmpty() ? Collections.emptyList() : diff; + return diff.isEmpty() ? emptyList() : diff; } diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/Range.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/Range.java index 96e427e90f166..07c4f6bd9a71d 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/Range.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/Range.java @@ -9,7 +9,6 @@ import org.elasticsearch.xpack.sql.tree.Location; import org.elasticsearch.xpack.sql.tree.NodeInfo; import org.elasticsearch.xpack.sql.type.DataType; -import org.elasticsearch.xpack.sql.type.DataTypes; import java.util.Arrays; import java.util.List; @@ -66,11 +65,19 @@ public boolean includeUpper() { @Override public boolean foldable() { - return value.foldable() && lower.foldable() && upper.foldable(); + if (lower.foldable() && upper.foldable()) { + return (excludingBoundaries() || value.foldable()); + } + + return false; } @Override public Object fold() { + if (excludingBoundaries()) { + return Boolean.FALSE; + } + Object val = value.fold(); Integer lowerCompare = BinaryComparison.compare(lower.fold(), val); Integer upperCompare = BinaryComparison.compare(val, upper().fold()); @@ -79,6 +86,13 @@ public Object fold() { return lowerComparsion && upperComparsion; } + private boolean excludingBoundaries() { + // if the boundaries exclude each other, the value doesn't need to be evaluated + Integer compare = BinaryComparison.compare(lower.fold(), upper.fold()); + // upper < lower OR upper == lower and the range doesn't contain any equals + return compare != null && (compare > 0 || (compare == 0 && (!includeLower || !includeUpper))); + } + @Override public boolean nullable() { return value.nullable() && lower.nullable() && upper.nullable(); 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 a56be01c95534..402d38af32dde 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 @@ -47,6 +47,7 @@ import org.elasticsearch.xpack.sql.expression.predicate.LessThanOrEqual; import org.elasticsearch.xpack.sql.expression.predicate.Not; import org.elasticsearch.xpack.sql.expression.predicate.Or; +import org.elasticsearch.xpack.sql.expression.predicate.Predicates; import org.elasticsearch.xpack.sql.expression.predicate.Range; import org.elasticsearch.xpack.sql.plan.logical.Aggregate; import org.elasticsearch.xpack.sql.plan.logical.Filter; @@ -60,6 +61,7 @@ import org.elasticsearch.xpack.sql.rule.RuleExecutor; import org.elasticsearch.xpack.sql.session.EmptyExecutable; import org.elasticsearch.xpack.sql.session.SingletonExecutable; +import org.elasticsearch.xpack.sql.util.CollectionUtils; import java.util.ArrayList; import java.util.Arrays; @@ -121,9 +123,11 @@ protected Iterable.Batch> batches() { new ConstantFolding(), // boolean new BooleanSimplification(), - new BinaryComparisonSimplification(), new BooleanLiteralsOnTheRight(), - new CombineComparisonsIntoRange(), + new BinaryComparisonSimplification(), + // needs to occur before BinaryComparison combinations (see class) + new PropagateEquals(), + new CombineBinaryComparisons(), // prune/elimination new PruneFilters(), new PruneOrderBy(), @@ -1231,7 +1235,7 @@ private Expression simplifyNot(Not n) { static class BinaryComparisonSimplification extends OptimizerExpressionRule { BinaryComparisonSimplification() { - super(TransformDirection.UP); + super(TransformDirection.DOWN); } @Override @@ -1277,48 +1281,353 @@ private Expression literalToTheRight(BinaryExpression be) { } } - static class CombineComparisonsIntoRange extends OptimizerExpressionRule { + /** + * Propagate Equals to eliminate conjuncted Ranges. + * When encountering a different Equals or exclusive {@link Range}, the conjunction becomes false. + * This rule doesn't perform any promotion of {@link BinaryComparison}s, that is handled by + * {@link CombineBinaryComparisons} on purpose as the resulting Range might be foldable + * (which is picked by the folding rule on the next run). + */ + static class PropagateEquals extends OptimizerExpressionRule { - CombineComparisonsIntoRange() { - super(TransformDirection.UP); + PropagateEquals() { + super(TransformDirection.DOWN); } @Override protected Expression rule(Expression e) { - return e instanceof And ? combine((And) e) : e; + if (e instanceof And) { + return propagate((And) e); + } + return e; } + // combine conjunction + private Expression propagate(And and) { + List ranges = new ArrayList<>(); + List equals = new ArrayList<>(); + List exps = new ArrayList<>(); + + boolean changed = false; + + for (Expression ex : Predicates.splitAnd(and)) { + if (ex instanceof Range) { + ranges.add((Range) ex); + } else if (ex instanceof Equals) { + Equals equ = (Equals) ex; + // equals on different values evaluate to FALSE + if (equ.right().foldable()) { + for (Equals eq : equals) { + // cannot evaluate equals so skip it + if (!eq.right().foldable()) { + continue; + } + if (equ.left().semanticEquals(eq.left())) { + if (eq.right().foldable() && equ.right().foldable()) { + Integer comp = BinaryComparison.compare(eq.right().fold(), equ.right().fold()); + if (comp != null) { + // var cannot be equal to two different values at the same time + if (comp != 0) { + return FALSE; + } + } + } + } + } + } + equals.add(equ); + } else { + exps.add(ex); + } + } + + // check + for (Equals eq : equals) { + // cannot evaluate equals so skip it + if (!eq.right().foldable()) { + continue; + } + Object eqValue = eq.right().fold(); + + for (int i = 0; i < ranges.size(); i++) { + Range range = ranges.get(i); + + if (range.value().semanticEquals(eq.left())) { + // if equals is outside the interval, evaluate the whole expression to FALSE + if (range.lower() != null && range.lower().foldable()) { + Integer compare = BinaryComparison.compare(range.lower().fold(), eqValue); + if (compare != null && ( + // eq outside the lower boundary + compare > 0 || + // eq matches the boundary but should not be included + (compare == 0 && !range.includeLower())) + ) { + return FALSE; + } + } + if (range.upper() != null && range.upper().foldable()) { + Integer compare = BinaryComparison.compare(range.upper().fold(), eqValue); + if (compare != null && ( + // eq outside the upper boundary + compare < 0 || + // eq matches the boundary but should not be included + (compare == 0 && !range.includeUpper())) + ) { + return FALSE; + } + } + + // it's in the range and thus, remove it + ranges.remove(i); + changed = true; + } + } + } + + return changed ? Predicates.combineAnd(CollectionUtils.combine(exps, equals, ranges)) : and; + } + } + + static class CombineBinaryComparisons extends OptimizerExpressionRule { + + CombineBinaryComparisons() { + super(TransformDirection.DOWN); + } + + @Override + protected Expression rule(Expression e) { + if (e instanceof And) { + return combine((And) e); + } else if (e instanceof Or) { + return combine((Or) e); + } + return e; + } + + // combine conjunction private Expression combine(And and) { - Expression l = and.left(); - Expression r = and.right(); + List ranges = new ArrayList<>(); + List exps = new ArrayList<>(); + + boolean changed = false; + + for (Expression ex : Predicates.splitAnd(and)) { + if (ex instanceof Range) { + changed |= combine((Range) ex, ranges); + } else if (ex instanceof BinaryComparison && !(ex instanceof Equals)) { + Range r = promoteToRange((BinaryComparison) ex); + combine(r, ranges); + changed = true; + } else { + exps.add(ex); + } + } + + return changed ? Predicates.combineAnd(CollectionUtils.combine(exps, ranges)) : and; + } + + private boolean combine(Range r, List ranges) { + for (int i = 0; i < ranges.size(); i++) { + Range range = ranges.get(i); - if (l instanceof BinaryComparison && r instanceof BinaryComparison) { - // if the same operator is used - BinaryComparison lb = (BinaryComparison) l; - BinaryComparison rb = (BinaryComparison) r; + if (r.value().semanticEquals(range.value())) { + boolean lower = false; + boolean upper = false; + // combine limits + + // if the lower is higher or if equal but is gt instead of gte or if a boundary is set where there was none + Integer low = null; + if (r.lower() != null && r.lower().foldable() && range.lower() != null && range.lower().foldable()) { + low = BinaryComparison.compare(r.lower().fold(), range.lower().fold()); + } + lower = low != null && (low > 0 || (low == 0 && !r.includeLower() && range.includeLower())) + || low == null && range.lower() == null && r.lower() != null; - if (lb.left().equals(((BinaryComparison) r).left()) && lb.right() instanceof Literal && rb.right() instanceof Literal) { - // >/>= AND />= - else if ((r instanceof GreaterThan || r instanceof GreaterThanOrEqual) - && (l instanceof LessThan || l instanceof LessThanOrEqual)) { - return new Range(and.location(), rb.left(), rb.right(), r instanceof GreaterThanOrEqual, lb.right(), - l instanceof LessThanOrEqual); + upper = up != null && (up < 0 || (up == 0 && !r.includeUpper() && range.includeUpper())) + || up == null && range.upper() == null && r.upper() != null; + + // if there's a difference, update the range in place + if (lower || upper) { + ranges.remove(i); + ranges.add(i, new Range(r.location(), r.value(), + lower ? r.lower() : range.lower(), + lower ? r.includeLower() : range.includeLower(), + upper ? r.upper() : range.upper(), + upper ? r.includeUpper() : range.includeUpper())); } + return true; } } + ranges.add(r); + return false; + } + + // promote comparison to Range as a common form + private static Range promoteToRange(BinaryComparison bc) { + Range promotedRange = null; + + if (bc instanceof GreaterThan) { + GreaterThan gt = (GreaterThan) bc; + promotedRange = new Range(gt.location(), gt.left(), gt.right(), false, null, false); + } else if (bc instanceof GreaterThanOrEqual) { + GreaterThanOrEqual gte = (GreaterThanOrEqual) bc; + promotedRange = new Range(gte.location(), gte.left(), gte.right(), true, null, false); + } else if (bc instanceof LessThan) { + LessThan lt = (LessThan) bc; + promotedRange = new Range(lt.location(), lt.left(), null, false, lt.right(), false); + } else if (bc instanceof LessThanOrEqual) { + LessThanOrEqual lte = (LessThanOrEqual) bc; + promotedRange = new Range(lte.location(), lte.left(), null, false, lte.right(), true); + } - return and; + return promotedRange; } - } + // combine disjunction + private Expression combine(Or or) { + List bcs = new ArrayList<>(); + List ranges = new ArrayList<>(); + List exps = new ArrayList<>(); + + for (Expression ex : Predicates.splitOr(or)) { + if (ex instanceof BinaryComparison) { + BinaryComparison bc = (BinaryComparison) ex; + // skip if cannot evaluate + if (bc.right() == null || !bc.right().foldable()) { + continue; + } + + if (!findExistingComparison(bc, bcs)) { + bcs.add(bc); + } + } else if (ex instanceof Range) { + Range r = (Range) ex; + + if (!findExistingRange(r, ranges)) { + ranges.add(r); + } + } else { + exps.add(ex); + } + } + + return Predicates.combineOr(CollectionUtils.combine(exps, bcs, ranges)); + } + + private static boolean findExistingComparison(BinaryComparison bc, List bcs) { + Object value = bc.right().fold(); + for (int i = 0; i < bcs.size(); i++) { + BinaryComparison c = bcs.get(i); + // skip if cannot evaluate + if (c.right() == null || !c.right().foldable()) { + continue; + } + // if bc is a lower value or gte vs gt, use it instead + if ((c instanceof GreaterThan || c instanceof GreaterThanOrEqual) && + (bc instanceof GreaterThan || bc instanceof GreaterThanOrEqual)) { + + if (bc.left().semanticEquals(c.left())) { + Integer compare = BinaryComparison.compare(value, c.right().fold()); + if (compare != null && + (compare < 0 || (compare == 0 && bc instanceof GreaterThanOrEqual && c instanceof GreaterThan))) { + bcs.remove(i); + bcs.add(i, bc); + } + + return true; + } + } + // if bc is a higher value or lte vs lt, use it instead + else if ((c instanceof LessThan || c instanceof LessThanOrEqual) && + (bc instanceof LessThan || bc instanceof LessThanOrEqual)) { + + if (bc.left().semanticEquals(c.left())) { + Integer compare = BinaryComparison.compare(value, c.right().fold()); + + if (compare != null && + (compare > 0 || (compare == 0 && bc instanceof LessThanOrEqual && c instanceof LessThan))) { + bcs.remove(i); + bcs.add(i, bc); + } + + return true; + } + } + } + return false; + } + + private static boolean findExistingRange(Range r, List ranges) { + if ((r.lower() == null || !r.lower().foldable()) && (r.upper() == null || !r.upper().foldable())) { + return false; + } + + for (int i = 0; i < ranges.size(); i++) { + Range other = ranges.get(i); + + if (r.value().semanticEquals(other.value())) { + + boolean lower = false; + boolean upper = false; + // boundary equality (useful to differentiate whether a range is included or not) + // and thus whether it should be preserved or ignored + boolean lowerEq = false; + boolean upperEq = false; + + // evaluate lower + if (r.lower() != null && other.lower() != null && r.lower().foldable() && other.lower().foldable()) { + Integer comp = BinaryComparison.compare(r.lower().fold(), other.lower().fold()); + // values are compareable + if (comp != null) { + // boundary equality + lowerEq = comp == 0 && r.includeLower() == other.includeLower(); + lower = comp < 0 || (comp == 0 && r.includeLower() && !other.includeLower()) || lowerEq; + } + } else { + lowerEq = r.lower() == null && other.lower() == null; + lower = lowerEq; + } + // evaluate upper + if (r.upper() != null && other.upper() != null && r.upper().foldable() && other.upper().foldable()) { + Integer comp = BinaryComparison.compare(r.upper().fold(), other.upper().fold()); + // values are compareable + if (comp != null) { + // if higher or equal (but at least includes it or is the same) + upperEq = comp == 0 && r.includeUpper() == other.includeUpper(); + upper = comp > 0 || (comp == 0 && r.includeUpper() && !other.includeUpper()) || upperEq; + } + } else { + upperEq = r.upper() == null && other.upper() == null; + upper = upperEq; + } + + // can loosen range + if (lower && upper) { + ranges.remove(i); + ranges.add(i, + new Range(r.location(), r.value(), + lower ? r.lower() : other.lower(), + lower ? r.includeLower() : other.includeLower(), + upper ? r.upper() : other.upper(), + upper ? r.includeUpper() : other.includeUpper())); + return true; + } + + // if the range in included, no need to add it + return !((lower && !lowerEq) || (upper && !upperEq)); + } + } + return false; + } + } + static class SkipQueryOnLimitZero extends OptimizerRule { @Override protected LogicalPlan rule(Limit limit) { @@ -1435,4 +1744,4 @@ protected LogicalPlan rule(LogicalPlan plan) { enum TransformDirection { UP, DOWN }; -} +} \ No newline at end of file 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 a49538c8d53f8..2d4a5df0ca88c 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 @@ -9,6 +9,7 @@ import org.elasticsearch.xpack.sql.expression.Alias; import org.elasticsearch.xpack.sql.expression.Expression; import org.elasticsearch.xpack.sql.expression.Expressions; +import org.elasticsearch.xpack.sql.expression.FieldAttribute; import org.elasticsearch.xpack.sql.expression.Literal; import org.elasticsearch.xpack.sql.expression.NamedExpression; import org.elasticsearch.xpack.sql.expression.Order; @@ -49,8 +50,10 @@ import org.elasticsearch.xpack.sql.optimizer.Optimizer.BinaryComparisonSimplification; import org.elasticsearch.xpack.sql.optimizer.Optimizer.BooleanLiteralsOnTheRight; import org.elasticsearch.xpack.sql.optimizer.Optimizer.BooleanSimplification; +import org.elasticsearch.xpack.sql.optimizer.Optimizer.CombineBinaryComparisons; import org.elasticsearch.xpack.sql.optimizer.Optimizer.CombineProjections; import org.elasticsearch.xpack.sql.optimizer.Optimizer.ConstantFolding; +import org.elasticsearch.xpack.sql.optimizer.Optimizer.PropagateEquals; import org.elasticsearch.xpack.sql.optimizer.Optimizer.PruneDuplicateFunctions; import org.elasticsearch.xpack.sql.optimizer.Optimizer.PruneSubqueryAliases; import org.elasticsearch.xpack.sql.optimizer.Optimizer.ReplaceFoldableAttributes; @@ -65,7 +68,7 @@ import org.elasticsearch.xpack.sql.tree.Location; import org.elasticsearch.xpack.sql.tree.NodeInfo; import org.elasticsearch.xpack.sql.type.DataType; -import org.joda.time.DateTimeZone; +import org.elasticsearch.xpack.sql.type.EsField; import java.util.Arrays; import java.util.Collections; @@ -74,6 +77,7 @@ import static java.util.Arrays.asList; import static java.util.Collections.emptyList; +import static java.util.Collections.emptyMap; import static java.util.Collections.singletonList; import static org.elasticsearch.xpack.sql.tree.Location.EMPTY; @@ -217,6 +221,10 @@ public void testReplaceFoldableAttributes() { assertEquals(10, lt.right().fold()); } + // + // Constant folding + // + public void testConstantFolding() { Expression exp = new Add(EMPTY, L(2), L(3)); @@ -314,6 +322,10 @@ private static Object unwrapAlias(Expression e) { return l.value(); } + // + // Logical simplifications + // + public void testBinaryComparisonSimplification() { assertEquals(Literal.TRUE, new BinaryComparisonSimplification().rule(new Equals(EMPTY, L(5), L(5)))); assertEquals(Literal.TRUE, new BinaryComparisonSimplification().rule(new GreaterThanOrEqual(EMPTY, L(5), L(5)))); @@ -369,4 +381,383 @@ public void testBoolCommonFactorExtraction() { assertEquals(expected, simplification.rule(actual)); } -} + + // + // Range optimization + // + + // 6 < a <= 5 -> FALSE + public void testFoldExcludingRangeToFalse() { + FieldAttribute fa = new FieldAttribute(EMPTY, "a", new EsField("af", DataType.INTEGER, emptyMap(), true)); + + Range r = new Range(EMPTY, fa, L(6), false, L(5), true); + assertTrue(r.foldable()); + assertEquals(Boolean.FALSE, r.fold()); + } + + // 6 < a <= 5.5 + public void testFoldExcludingRangeWithDifferentTypesToFalse() { + FieldAttribute fa = new FieldAttribute(EMPTY, "a", new EsField("af", DataType.INTEGER, emptyMap(), true)); + + Range r = new Range(EMPTY, fa, L(6), false, L(5.5d), true); + assertTrue(r.foldable()); + assertEquals(Boolean.FALSE, r.fold()); + } + + // Conjunction + + // a <= 6 AND a < 5 -> a < 5 + public void testCombineBinaryComparisonsUpper() { + FieldAttribute fa = new FieldAttribute(EMPTY, "a", new EsField("af", DataType.INTEGER, emptyMap(), true)); + LessThanOrEqual lte = new LessThanOrEqual(EMPTY, fa, L(6)); + LessThan lt = new LessThan(EMPTY, fa, L(5)); + + CombineBinaryComparisons rule = new CombineBinaryComparisons(); + + Expression exp = rule.rule(new And(EMPTY, lte, lt)); + assertEquals(Range.class, exp.getClass()); + Range r = (Range) exp; + assertNull(r.lower()); + assertEquals(L(5), r.upper()); + assertFalse(r.includeUpper()); + } + + // 6 <= a AND 5 < a -> 6 <= a + public void testCombineBinaryComparisonsLower() { + FieldAttribute fa = new FieldAttribute(EMPTY, "a", new EsField("af", DataType.INTEGER, emptyMap(), true)); + GreaterThanOrEqual gte = new GreaterThanOrEqual(EMPTY, fa, L(6)); + GreaterThan gt = new GreaterThan(EMPTY, fa, L(5)); + + CombineBinaryComparisons rule = new CombineBinaryComparisons(); + + Expression exp = rule.rule(new And(EMPTY, gte, gt)); + assertEquals(Range.class, exp.getClass()); + Range r = (Range) exp; + assertNull(r.upper()); + assertEquals(L(6), r.lower()); + assertTrue(r.includeLower()); + } + + // 5 <= a AND 5 < a -> 5 < a + public void testCombineBinaryComparisonsInclude() { + FieldAttribute fa = new FieldAttribute(EMPTY, "a", new EsField("af", DataType.INTEGER, emptyMap(), true)); + GreaterThanOrEqual gte = new GreaterThanOrEqual(EMPTY, fa, L(5)); + GreaterThan gt = new GreaterThan(EMPTY, fa, L(5)); + + CombineBinaryComparisons rule = new CombineBinaryComparisons(); + + Expression exp = rule.rule(new And(EMPTY, gte, gt)); + assertEquals(Range.class, exp.getClass()); + Range r = (Range) exp; + assertNull(r.upper()); + assertEquals(L(5), r.lower()); + assertFalse(r.includeLower()); + } + + // 3 <= a AND 4 < a AND a <= 7 AND a < 6 -> 4 < a < 6 + public void testCombineMultipleBinaryComparisons() { + FieldAttribute fa = new FieldAttribute(EMPTY, "a", new EsField("af", DataType.INTEGER, emptyMap(), true)); + GreaterThanOrEqual gte = new GreaterThanOrEqual(EMPTY, fa, L(3)); + GreaterThan gt = new GreaterThan(EMPTY, fa, L(4)); + LessThanOrEqual lte = new LessThanOrEqual(EMPTY, fa, L(7)); + LessThan lt = new LessThan(EMPTY, fa, L(6)); + + CombineBinaryComparisons rule = new CombineBinaryComparisons(); + + Expression exp = rule.rule(new And(EMPTY, gte, new And(EMPTY, gt, new And(EMPTY, lt, lte)))); + assertEquals(Range.class, exp.getClass()); + Range r = (Range) exp; + assertEquals(L(4), r.lower()); + assertFalse(r.includeLower()); + assertEquals(L(6), r.upper()); + assertFalse(r.includeUpper()); + } + + // 3 <= a AND TRUE AND 4 < a AND a != 5 AND a <= 7 -> 4 < a <= 7 AND a != 5 AND TRUE + public void testCombineMixedMultipleBinaryComparisons() { + FieldAttribute fa = new FieldAttribute(EMPTY, "a", new EsField("af", DataType.INTEGER, emptyMap(), true)); + GreaterThanOrEqual gte = new GreaterThanOrEqual(EMPTY, fa, L(3)); + GreaterThan gt = new GreaterThan(EMPTY, fa, L(4)); + LessThanOrEqual lte = new LessThanOrEqual(EMPTY, fa, L(7)); + Expression ne = new Not(EMPTY, new Equals(EMPTY, fa, L(5))); + + CombineBinaryComparisons rule = new CombineBinaryComparisons(); + + // TRUE AND a != 5 AND 4 < a <= 7 + Expression exp = rule.rule(new And(EMPTY, gte, new And(EMPTY, Literal.TRUE, new And(EMPTY, gt, new And(EMPTY, ne, lte))))); + assertEquals(And.class, exp.getClass()); + And and = ((And) exp); + assertEquals(Range.class, and.right().getClass()); + Range r = (Range) and.right(); + assertEquals(L(4), r.lower()); + assertFalse(r.includeLower()); + assertEquals(L(7), r.upper()); + assertTrue(r.includeUpper()); + } + + // 1 <= a AND a < 5 -> 1 <= a < 5 + public void testCombineComparisonsIntoRange() { + FieldAttribute fa = new FieldAttribute(EMPTY, "a", new EsField("af", DataType.INTEGER, emptyMap(), true)); + GreaterThanOrEqual gte = new GreaterThanOrEqual(EMPTY, fa, L(1)); + LessThan lt = new LessThan(EMPTY, fa, L(5)); + + CombineBinaryComparisons rule = new CombineBinaryComparisons(); + Expression exp = rule.rule(new And(EMPTY, gte, lt)); + assertEquals(Range.class, rule.rule(exp).getClass()); + + Range r = (Range) exp; + assertEquals(L(1), r.lower()); + assertTrue(r.includeLower()); + assertEquals(L(5), r.upper()); + assertFalse(r.includeUpper()); + } + + // a != NULL AND a > 1 AND a < 5 AND a == 10 -> (a != NULL AND a == 10) AND 1 <= a < 5 + public void testCombineUnbalancedComparisonsMixedWithEqualsIntoRange() { + FieldAttribute fa = new FieldAttribute(EMPTY, "a", new EsField("af", DataType.INTEGER, emptyMap(), true)); + IsNotNull isn = new IsNotNull(EMPTY, fa); + GreaterThanOrEqual gte = new GreaterThanOrEqual(EMPTY, fa, L(1)); + + Equals eq = new Equals(EMPTY, fa, L(10)); + LessThan lt = new LessThan(EMPTY, fa, L(5)); + + And and = new And(EMPTY, new And(EMPTY, isn, gte), new And(EMPTY, lt, eq)); + + CombineBinaryComparisons rule = new CombineBinaryComparisons(); + Expression exp = rule.rule(and); + assertEquals(And.class, exp.getClass()); + And a = (And) exp; + assertEquals(Range.class, a.right().getClass()); + + Range r = (Range) a.right(); + assertEquals(L(1), r.lower()); + assertTrue(r.includeLower()); + assertEquals(L(5), r.upper()); + assertFalse(r.includeUpper()); + } + + // Disjunction + + // 2 < a OR 1 < a OR 3 < a -> 1 < a + public void testCombineBinaryComparisonsDisjunctionLowerBound() { + FieldAttribute fa = new FieldAttribute(EMPTY, "a", new EsField("af", DataType.INTEGER, emptyMap(), true)); + + GreaterThan gt1 = new GreaterThan(EMPTY, fa, L(1)); + GreaterThan gt2 = new GreaterThan(EMPTY, fa, L(2)); + GreaterThan gt3 = new GreaterThan(EMPTY, fa, L(3)); + + Or or = new Or(EMPTY, gt1, new Or(EMPTY, gt2, gt3)); + + CombineBinaryComparisons rule = new CombineBinaryComparisons(); + Expression exp = rule.rule(or); + assertEquals(GreaterThan.class, exp.getClass()); + + GreaterThan gt = (GreaterThan) exp; + assertEquals(L(1), gt.right()); + } + + // 2 < a OR 1 < a OR 3 <= a -> 1 < a + public void testCombineBinaryComparisonsDisjunctionIncludeLowerBounds() { + FieldAttribute fa = new FieldAttribute(EMPTY, "a", new EsField("af", DataType.INTEGER, emptyMap(), true)); + + GreaterThan gt1 = new GreaterThan(EMPTY, fa, L(1)); + GreaterThan gt2 = new GreaterThan(EMPTY, fa, L(2)); + GreaterThanOrEqual gte3 = new GreaterThanOrEqual(EMPTY, fa, L(3)); + + Or or = new Or(EMPTY, new Or(EMPTY, gt1, gt2), gte3); + + CombineBinaryComparisons rule = new CombineBinaryComparisons(); + Expression exp = rule.rule(or); + assertEquals(GreaterThan.class, exp.getClass()); + + GreaterThan gt = (GreaterThan) exp; + assertEquals(L(1), gt.right()); + } + + // a < 1 OR a < 2 OR a < 3 -> a < 3 + public void testCombineBinaryComparisonsDisjunctionUpperBound() { + FieldAttribute fa = new FieldAttribute(EMPTY, "a", new EsField("af", DataType.INTEGER, emptyMap(), true)); + + LessThan lt1 = new LessThan(EMPTY, fa, L(1)); + LessThan lt2 = new LessThan(EMPTY, fa, L(2)); + LessThan lt3 = new LessThan(EMPTY, fa, L(3)); + + Or or = new Or(EMPTY, new Or(EMPTY, lt1, lt2), lt3); + + CombineBinaryComparisons rule = new CombineBinaryComparisons(); + Expression exp = rule.rule(or); + assertEquals(LessThan.class, exp.getClass()); + + LessThan lt = (LessThan) exp; + assertEquals(L(3), lt.right()); + } + + // a < 2 OR a <= 2 OR a < 1 -> null < a <= 2 + public void testCombineBinaryComparisonsDisjunctionIncludeUpperBounds() { + FieldAttribute fa = new FieldAttribute(EMPTY, "a", new EsField("af", DataType.INTEGER, emptyMap(), true)); + + LessThan lt1 = new LessThan(EMPTY, fa, L(1)); + LessThan lt2 = new LessThan(EMPTY, fa, L(2)); + LessThanOrEqual lte2 = new LessThanOrEqual(EMPTY, fa, L(2)); + + Or or = new Or(EMPTY, lt2, new Or(EMPTY, lte2, lt1)); + + CombineBinaryComparisons rule = new CombineBinaryComparisons(); + Expression exp = rule.rule(or); + assertEquals(LessThanOrEqual.class, exp.getClass()); + + LessThanOrEqual lte = (LessThanOrEqual) exp; + assertEquals(L(2), lte.right()); + } + + // a < 2 OR 3 < a OR a < 1 OR 4 < a -> a < 2 OR 3 < a + public void testCombineBinaryComparisonsDisjunctionOfLowerAndUpperBounds() { + FieldAttribute fa = new FieldAttribute(EMPTY, "a", new EsField("af", DataType.INTEGER, emptyMap(), true)); + + LessThan lt1 = new LessThan(EMPTY, fa, L(1)); + LessThan lt2 = new LessThan(EMPTY, fa, L(2)); + + GreaterThan gt3 = new GreaterThan(EMPTY, fa, L(3)); + GreaterThan gt4 = new GreaterThan(EMPTY, fa, L(4)); + + Or or = new Or(EMPTY, new Or(EMPTY, lt2, gt3), new Or(EMPTY, lt1, gt4)); + + CombineBinaryComparisons rule = new CombineBinaryComparisons(); + Expression exp = rule.rule(or); + assertEquals(Or.class, exp.getClass()); + + Or ro = (Or) exp; + + assertEquals(LessThan.class, ro.left().getClass()); + LessThan lt = (LessThan) ro.left(); + assertEquals(L(2), lt.right()); + assertEquals(GreaterThan.class, ro.right().getClass()); + GreaterThan gt = (GreaterThan) ro.right(); + assertEquals(L(3), gt.right()); + } + + // (2 < a < 3) OR (1 < a < 4) -> (1 < a < 4) + public void testCombineBinaryComparisonsDisjunctionOfIncludedRange() { + FieldAttribute fa = new FieldAttribute(EMPTY, "a", new EsField("af", DataType.INTEGER, emptyMap(), true)); + + Range r1 = new Range(EMPTY, fa, L(2), false, L(3), false); + Range r2 = new Range(EMPTY, fa, L(1), false, L(4), false); + + Or or = new Or(EMPTY, r1, r2); + + CombineBinaryComparisons rule = new CombineBinaryComparisons(); + Expression exp = rule.rule(or); + assertEquals(Range.class, exp.getClass()); + + Range r = (Range) exp; + assertEquals(L(1), r.lower()); + assertFalse(r.includeLower()); + assertEquals(L(4), r.upper()); + assertFalse(r.includeUpper()); + } + + // (2 < a < 3) OR (1 < a < 2) -> same + public void testCombineBinaryComparisonsDisjunctionOfNonOverlappingBoundaries() { + FieldAttribute fa = new FieldAttribute(EMPTY, "a", new EsField("af", DataType.INTEGER, emptyMap(), true)); + + Range r1 = new Range(EMPTY, fa, L(2), false, L(3), false); + Range r2 = new Range(EMPTY, fa, L(1), false, L(2), false); + + Or or = new Or(EMPTY, r1, r2); + + CombineBinaryComparisons rule = new CombineBinaryComparisons(); + Expression exp = rule.rule(or); + assertEquals(or, exp); + } + + // (2 < a < 3) OR (2 < a <= 3) -> 2 < a <= 3 + public void testCombineBinaryComparisonsDisjunctionOfUpperEqualsOverlappingBoundaries() { + FieldAttribute fa = new FieldAttribute(EMPTY, "a", new EsField("af", DataType.INTEGER, emptyMap(), true)); + + Range r1 = new Range(EMPTY, fa, L(2), false, L(3), false); + Range r2 = new Range(EMPTY, fa, L(2), false, L(3), true); + + Or or = new Or(EMPTY, r1, r2); + + CombineBinaryComparisons rule = new CombineBinaryComparisons(); + Expression exp = rule.rule(or); + assertEquals(r2, exp); + } + + // (2 < a < null) OR (1 < a < null) -> 1 < a < null + public void testCombineBinaryComparisonsOverlappingNoUpperBoundary() { + FieldAttribute fa = new FieldAttribute(EMPTY, "a", new EsField("af", DataType.INTEGER, emptyMap(), true)); + + Range r2 = new Range(EMPTY, fa, L(2), false, null, false); + Range r1 = new Range(EMPTY, fa, L(1), false, null, false); + + Or or = new Or(EMPTY, r1, r2); + + CombineBinaryComparisons rule = new CombineBinaryComparisons(); + Expression exp = rule.rule(or); + assertEquals(r1, exp); + } + + // (2 < a <= 3) OR (1 < a < 3) -> same (the <= prevents the ranges from being cumulated) + public void testCombineBinaryComparisonsWithDifferentUpperLimitInclusion() { + FieldAttribute fa = new FieldAttribute(EMPTY, "a", new EsField("af", DataType.INTEGER, emptyMap(), true)); + + Range r1 = new Range(EMPTY, fa, L(1), false, L(3), false); + Range r2 = new Range(EMPTY, fa, L(2), false, L(3), true); + + Or or = new Or(EMPTY, r1, r2); + + CombineBinaryComparisons rule = new CombineBinaryComparisons(); + Expression exp = rule.rule(or); + assertEquals(or, exp); + } + + // (null < a <= 1) OR (null < a < 2) -> null < a < 2 + public void testCombineBinaryComparisonsOverlappingNoLowerBoundary() { + FieldAttribute fa = new FieldAttribute(EMPTY, "a", new EsField("af", DataType.INTEGER, emptyMap(), true)); + + Range r2 = new Range(EMPTY, fa, null, false, L(2), false); + Range r1 = new Range(EMPTY, fa, null, false, L(1), true); + + Or or = new Or(EMPTY, r1, r2); + + CombineBinaryComparisons rule = new CombineBinaryComparisons(); + Expression exp = rule.rule(or); + assertEquals(r2, exp); + } + + // Equals + + // a == 1 AND a == 2 -> FALSE + public void testDualEqualsConjunction() { + FieldAttribute fa = new FieldAttribute(EMPTY, "a", new EsField("af", DataType.INTEGER, emptyMap(), true)); + Equals eq1 = new Equals(EMPTY, fa, L(1)); + Equals eq2 = new Equals(EMPTY, fa, L(2)); + + PropagateEquals rule = new PropagateEquals(); + Expression exp = rule.rule(new And(EMPTY, eq1, eq2)); + assertEquals(Literal.FALSE, rule.rule(exp)); + } + + // 1 <= a < 10 AND a == 1 -> a == 1 + public void testEliminateRangeByEqualsInInterval() { + FieldAttribute fa = new FieldAttribute(EMPTY, "a", new EsField("af", DataType.INTEGER, emptyMap(), true)); + Equals eq1 = new Equals(EMPTY, fa, L(1)); + Range r = new Range(EMPTY, fa, L(1), true, L(10), false); + + PropagateEquals rule = new PropagateEquals(); + Expression exp = rule.rule(new And(EMPTY, eq1, r)); + assertEquals(eq1, rule.rule(exp)); + } + + // 1 < a < 10 AND a == 10 -> FALSE + public void testEliminateRangeByEqualsOutsideInterval() { + FieldAttribute fa = new FieldAttribute(EMPTY, "a", new EsField("af", DataType.INTEGER, emptyMap(), true)); + Equals eq1 = new Equals(EMPTY, fa, L(10)); + Range r = new Range(EMPTY, fa, L(1), false, L(10), false); + + PropagateEquals rule = new PropagateEquals(); + Expression exp = rule.rule(new And(EMPTY, eq1, r)); + assertEquals(Literal.FALSE, rule.rule(exp)); + } +} \ No newline at end of file From 84872b4b04b74a2c1e78465c19b798ebaecdfddc Mon Sep 17 00:00:00 2001 From: Costin Leau Date: Mon, 30 Apr 2018 22:03:00 +0300 Subject: [PATCH 2/5] Address feedback --- .../expression/predicate/BinaryComparison.java | 17 ++++++++++++----- .../sql/expression/predicate/Predicates.java | 15 +++++++++++++-- .../xpack/sql/expression/predicate/Range.java | 11 +++++++---- .../xpack/sql/optimizer/Optimizer.java | 16 +++++++++------- 4 files changed, 41 insertions(+), 18 deletions(-) diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/BinaryComparison.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/BinaryComparison.java index b35f3cec9a02a..db1ba1d3cdf19 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/BinaryComparison.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/BinaryComparison.java @@ -32,6 +32,11 @@ public DataType dataType() { return DataType.BOOLEAN; } + /** + * Compares two expression arguments (typically Numbers), if possible. + * Otherwise returns null (the arguments are not comparable or at least + * one of them is null). + */ @SuppressWarnings({ "rawtypes", "unchecked" }) public static Integer compare(Object l, Object r) { // typical number comparison @@ -39,14 +44,16 @@ public static Integer compare(Object l, Object r) { return compare((Number) l, (Number) r); } - try { - if (l instanceof Comparable && r instanceof Comparable) { + if (l instanceof Comparable && r instanceof Comparable) { + try { return Integer.valueOf(((Comparable) l).compareTo(r)); + } catch (ClassCastException cce) { + // when types are not compatible, cce is thrown + // fall back to null + return null; } - } catch (ClassCastException cce) { - // types are not compatible - // return null } + return null; } diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/Predicates.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/Predicates.java index 755894c19718b..6bf59772e8ccb 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/Predicates.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/Predicates.java @@ -48,8 +48,19 @@ public static Expression combineAnd(List exps) { } /** - * Build a binary 'pyramid' - while a bit longer this method creates a balanced tree as oppose to a plain - * recursive approach that creates an unbalanced one (either to the left or right). + * Build a binary 'pyramid' from the given list: + * 
+     *       AND
+     *      /   \
+     *   AND     AND
+     *  /   \   /   \
+     * A     B C     D
+     *
+ * + * using the given combiner. + * + * While a bit longer, this method creates a balanced tree as oppose to a plain + * recursive approach which creates an unbalanced one (either to the left or right). */ private static Expression combine(List exps, BiFunction combiner) { if (exps.isEmpty()) { diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/Range.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/Range.java index 07c4f6bd9a71d..3a86fbad524b9 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/Range.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/Range.java @@ -66,7 +66,7 @@ public boolean includeUpper() { @Override public boolean foldable() { if (lower.foldable() && upper.foldable()) { - return (excludingBoundaries() || value.foldable()); + return areBoundariesInvalid() || value.foldable(); } return false; @@ -74,7 +74,7 @@ public boolean foldable() { @Override public Object fold() { - if (excludingBoundaries()) { + if (areBoundariesInvalid()) { return Boolean.FALSE; } @@ -86,8 +86,11 @@ public Object fold() { return lowerComparsion && upperComparsion; } - private boolean excludingBoundaries() { - // if the boundaries exclude each other, the value doesn't need to be evaluated + /** + * Check whether the boundaries are invalid ( upper < lower) or not. + * If they do, the value does not have to be evaluate. + */ + private boolean areBoundariesInvalid() { Integer compare = BinaryComparison.compare(lower.fold(), upper.fold()); // upper < lower OR upper == lower and the range doesn't contain any equals return compare != null && (compare > 0 || (compare == 0 && (!includeLower || !includeUpper))); 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 402d38af32dde..eac34849e4be4 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 @@ -1283,7 +1283,9 @@ private Expression literalToTheRight(BinaryExpression be) { /** * Propagate Equals to eliminate conjuncted Ranges. - * When encountering a different Equals or exclusive {@link Range}, the conjunction becomes false. + * When encountering a different Equals or non-containing {@link Range}, the conjunction becomes false. + * When encountering a containing {@link Range}, the range gets eliminated by the equality. + * * This rule doesn't perform any promotion of {@link BinaryComparison}s, that is handled by * {@link CombineBinaryComparisons} on purpose as the resulting Range might be foldable * (which is picked by the folding rule on the next run). @@ -1314,17 +1316,17 @@ private Expression propagate(And and) { if (ex instanceof Range) { ranges.add((Range) ex); } else if (ex instanceof Equals) { - Equals equ = (Equals) ex; + Equals otherEq = (Equals) ex; // equals on different values evaluate to FALSE - if (equ.right().foldable()) { + if (otherEq.right().foldable()) { for (Equals eq : equals) { // cannot evaluate equals so skip it if (!eq.right().foldable()) { continue; } - if (equ.left().semanticEquals(eq.left())) { - if (eq.right().foldable() && equ.right().foldable()) { - Integer comp = BinaryComparison.compare(eq.right().fold(), equ.right().fold()); + if (otherEq.left().semanticEquals(eq.left())) { + if (eq.right().foldable() && otherEq.right().foldable()) { + Integer comp = BinaryComparison.compare(eq.right().fold(), otherEq.right().fold()); if (comp != null) { // var cannot be equal to two different values at the same time if (comp != 0) { @@ -1335,7 +1337,7 @@ private Expression propagate(And and) { } } } - equals.add(equ); + equals.add(otherEq); } else { exps.add(ex); } From f0d7d4cc819e5ccee8bafe8b0ef7697252c36dd0 Mon Sep 17 00:00:00 2001 From: Costin Leau Date: Tue, 1 May 2018 16:27:32 +0300 Subject: [PATCH 3/5] Improve rule for optimizing binary comparison Do not promote BinaryComparisons to Ranges since it introduces NULL boundaries and thus a corner-case that needs too much handling Compare BinaryComparisons directly between themselves and to Ranges --- .../xpack/sql/expression/predicate/Range.java | 2 +- .../xpack/sql/optimizer/Optimizer.java | 439 +++++++++++------- .../elasticsearch/xpack/sql/tree/Node.java | 5 + .../sql/optimizer/OptimizerRunTests.java | 49 ++ .../xpack/sql/optimizer/OptimizerTests.java | 181 ++++++-- 5 files changed, 493 insertions(+), 183 deletions(-) create mode 100644 x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/optimizer/OptimizerRunTests.java diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/Range.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/Range.java index 3a86fbad524b9..54d541ab406b7 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/Range.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/Range.java @@ -139,4 +139,4 @@ public String toString() { sb.append(upper); return sb.toString(); } -} +} \ No newline at end of file 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 eac34849e4be4..28f54e5075a16 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 @@ -1356,7 +1356,7 @@ private Expression propagate(And and) { if (range.value().semanticEquals(eq.left())) { // if equals is outside the interval, evaluate the whole expression to FALSE - if (range.lower() != null && range.lower().foldable()) { + if (range.lower().foldable()) { Integer compare = BinaryComparison.compare(range.lower().fold(), eqValue); if (compare != null && ( // eq outside the lower boundary @@ -1367,7 +1367,7 @@ private Expression propagate(And and) { return FALSE; } } - if (range.upper() != null && range.upper().foldable()) { + if (range.upper().foldable()) { Integer compare = BinaryComparison.compare(range.upper().fold(), eqValue); if (compare != null && ( // eq outside the upper boundary @@ -1409,223 +1409,350 @@ protected Expression rule(Expression e) { // combine conjunction private Expression combine(And and) { List ranges = new ArrayList<>(); + List bcs = new ArrayList<>(); List exps = new ArrayList<>(); boolean changed = false; for (Expression ex : Predicates.splitAnd(and)) { if (ex instanceof Range) { - changed |= combine((Range) ex, ranges); + Range r = (Range) ex; + if (findExistingRange(r, ranges, true)) { + changed = true; + } else { + ranges.add(r); + } } else if (ex instanceof BinaryComparison && !(ex instanceof Equals)) { - Range r = promoteToRange((BinaryComparison) ex); - combine(r, ranges); - changed = true; + BinaryComparison bc = (BinaryComparison) ex; + + if (bc.right().foldable() && (findConjunctiveComparisonInRange(bc, ranges) || findExistingComparison(bc, bcs, true))) { + changed = true; + } else { + bcs.add(bc); + } } else { exps.add(ex); } } - return changed ? Predicates.combineAnd(CollectionUtils.combine(exps, ranges)) : and; - } + // finally try combining any left BinaryComparisons into possible Ranges + // this could be a different rule but it's clearer here wrt the order of comparisons - private boolean combine(Range r, List ranges) { - for (int i = 0; i < ranges.size(); i++) { - Range range = ranges.get(i); + for (int i = 0; i < bcs.size() - 1; i++) { + BinaryComparison main = bcs.get(i); - if (r.value().semanticEquals(range.value())) { - boolean lower = false; - boolean upper = false; - - // combine limits + for (int j = i + 1; j < bcs.size(); j++) { + BinaryComparison other = bcs.get(j); - // if the lower is higher or if equal but is gt instead of gte or if a boundary is set where there was none - Integer low = null; - if (r.lower() != null && r.lower().foldable() && range.lower() != null && range.lower().foldable()) { - low = BinaryComparison.compare(r.lower().fold(), range.lower().fold()); - } - lower = low != null && (low > 0 || (low == 0 && !r.includeLower() && range.includeLower())) - || low == null && range.lower() == null && r.lower() != null; + if (main.left().semanticEquals(other.left())) { + // >/>= AND />= + else if ((other instanceof GreaterThan || other instanceof GreaterThanOrEqual) + && (main instanceof LessThan || main instanceof LessThanOrEqual)) { + bcs.remove(j); + bcs.remove(i); + + ranges.add(new Range(and.location(), main.left(), + other.right(), other instanceof GreaterThanOrEqual, + main.right(), main instanceof LessThanOrEqual)); - // if there's a difference, update the range in place - if (lower || upper) { - ranges.remove(i); - ranges.add(i, new Range(r.location(), r.value(), - lower ? r.lower() : range.lower(), - lower ? r.includeLower() : range.includeLower(), - upper ? r.upper() : range.upper(), - upper ? r.includeUpper() : range.includeUpper())); + changed = true; + } } - return true; } } - ranges.add(r); - return false; - } - - // promote comparison to Range as a common form - private static Range promoteToRange(BinaryComparison bc) { - Range promotedRange = null; - - if (bc instanceof GreaterThan) { - GreaterThan gt = (GreaterThan) bc; - promotedRange = new Range(gt.location(), gt.left(), gt.right(), false, null, false); - } else if (bc instanceof GreaterThanOrEqual) { - GreaterThanOrEqual gte = (GreaterThanOrEqual) bc; - promotedRange = new Range(gte.location(), gte.left(), gte.right(), true, null, false); - } else if (bc instanceof LessThan) { - LessThan lt = (LessThan) bc; - promotedRange = new Range(lt.location(), lt.left(), null, false, lt.right(), false); - } else if (bc instanceof LessThanOrEqual) { - LessThanOrEqual lte = (LessThanOrEqual) bc; - promotedRange = new Range(lte.location(), lte.left(), null, false, lte.right(), true); - } - - return promotedRange; + + + return changed ? Predicates.combineAnd(CollectionUtils.combine(exps, bcs, ranges)) : and; } - // combine disjunction private Expression combine(Or or) { List bcs = new ArrayList<>(); List ranges = new ArrayList<>(); List exps = new ArrayList<>(); + boolean changed = false; + for (Expression ex : Predicates.splitOr(or)) { - if (ex instanceof BinaryComparison) { - BinaryComparison bc = (BinaryComparison) ex; - // skip if cannot evaluate - if (bc.right() == null || !bc.right().foldable()) { - continue; - } - - if (!findExistingComparison(bc, bcs)) { - bcs.add(bc); - } - } else if (ex instanceof Range) { + if (ex instanceof Range) { Range r = (Range) ex; - - if (!findExistingRange(r, ranges)) { + if (findExistingRange(r, ranges, false)) { + changed = true; + } else { ranges.add(r); } + } else if (ex instanceof BinaryComparison) { + BinaryComparison bc = (BinaryComparison) ex; + if (bc.right().foldable() && findExistingComparison(bc, bcs, false)) { + changed = true; + } else { + bcs.add(bc); + } } else { exps.add(ex); } } - - return Predicates.combineOr(CollectionUtils.combine(exps, bcs, ranges)); - } - - private static boolean findExistingComparison(BinaryComparison bc, List bcs) { - Object value = bc.right().fold(); - for (int i = 0; i < bcs.size(); i++) { - BinaryComparison c = bcs.get(i); - // skip if cannot evaluate - if (c.right() == null || !c.right().foldable()) { - continue; - } - // if bc is a lower value or gte vs gt, use it instead - if ((c instanceof GreaterThan || c instanceof GreaterThanOrEqual) && - (bc instanceof GreaterThan || bc instanceof GreaterThanOrEqual)) { - - if (bc.left().semanticEquals(c.left())) { - Integer compare = BinaryComparison.compare(value, c.right().fold()); - if (compare != null && - (compare < 0 || (compare == 0 && bc instanceof GreaterThanOrEqual && c instanceof GreaterThan))) { - bcs.remove(i); - bcs.add(i, bc); - } - - return true; - } - } - // if bc is a higher value or lte vs lt, use it instead - else if ((c instanceof LessThan || c instanceof LessThanOrEqual) && - (bc instanceof LessThan || bc instanceof LessThanOrEqual)) { - - if (bc.left().semanticEquals(c.left())) { - Integer compare = BinaryComparison.compare(value, c.right().fold()); - - if (compare != null && - (compare > 0 || (compare == 0 && bc instanceof LessThanOrEqual && c instanceof LessThan))) { - bcs.remove(i); - bcs.add(i, bc); - } - return true; - } - } - } - return false; + return changed ? Predicates.combineOr(CollectionUtils.combine(exps, bcs, ranges)) : or; } - - private static boolean findExistingRange(Range r, List ranges) { - if ((r.lower() == null || !r.lower().foldable()) && (r.upper() == null || !r.upper().foldable())) { + + private static boolean findExistingRange(Range main, List ranges, boolean conjunctive) { + if (!main.lower().foldable() && !main.upper().foldable()) { return false; } - + for (int i = 0; i < ranges.size(); i++) { Range other = ranges.get(i); - if (r.value().semanticEquals(other.value())) { - + if (main.value().semanticEquals(other.value())) { + + // make sure the comparison was done + boolean compared = false; + boolean lower = false; boolean upper = false; // boundary equality (useful to differentiate whether a range is included or not) // and thus whether it should be preserved or ignored boolean lowerEq = false; boolean upperEq = false; - + // evaluate lower - if (r.lower() != null && other.lower() != null && r.lower().foldable() && other.lower().foldable()) { - Integer comp = BinaryComparison.compare(r.lower().fold(), other.lower().fold()); - // values are compareable + if (main.lower().foldable() && other.lower().foldable()) { + compared = true; + + Integer comp = BinaryComparison.compare(main.lower().fold(), other.lower().fold()); + // values are comparable if (comp != null) { // boundary equality - lowerEq = comp == 0 && r.includeLower() == other.includeLower(); - lower = comp < 0 || (comp == 0 && r.includeLower() && !other.includeLower()) || lowerEq; + lowerEq = comp == 0 && main.includeLower() == other.includeLower(); + // AND + if (conjunctive) { + // (2 < a < 3) AND (1 < a < 3) -> (1 < a < 3) + lower = comp > 0 || + // (2 < a < 3) AND (2 < a <= 3) -> (2 < a < 3) + (comp == 0 && !main.includeLower() && other.includeLower()); + } + // OR + else { + // (1 < a < 3) OR (2 < a < 3) -> (1 < a < 3) + lower = comp < 0 || + // (2 <= a < 3) OR (2 < a < 3) -> (2 <= a < 3) + (comp == 0 && main.includeLower() && !other.includeLower()) || lowerEq; + } } - } else { - lowerEq = r.lower() == null && other.lower() == null; - lower = lowerEq; } // evaluate upper - if (r.upper() != null && other.upper() != null && r.upper().foldable() && other.upper().foldable()) { - Integer comp = BinaryComparison.compare(r.upper().fold(), other.upper().fold()); - // values are compareable + if (main.upper().foldable() && other.upper().foldable()) { + compared = true; + + Integer comp = BinaryComparison.compare(main.upper().fold(), other.upper().fold()); + // values are comparable if (comp != null) { - // if higher or equal (but at least includes it or is the same) - upperEq = comp == 0 && r.includeUpper() == other.includeUpper(); - upper = comp > 0 || (comp == 0 && r.includeUpper() && !other.includeUpper()) || upperEq; + // boundary equality + upperEq = comp == 0 && main.includeUpper() == other.includeUpper(); + + // AND + if (conjunctive) { + // (1 < a < 2) AND (1 < a < 3) -> (1 < a < 2) + upper = comp < 0 || + // (1 < a < 2) AND (1 < a <= 2) -> (1 < a < 2) + (comp == 0 && !main.includeUpper() && other.includeUpper()); + } + // OR + else { + // (1 < a < 3) OR (1 < a < 2) -> (1 < a < 3) + upper = comp > 0 || + // (1 < a <= 3) OR (1 < a < 3) -> (2 < a < 3) + (comp == 0 && main.includeUpper() && !other.includeUpper()) || upperEq; + } } - } else { - upperEq = r.upper() == null && other.upper() == null; - upper = upperEq; } + + // AND - at least one of lower or upper + if (conjunctive) { + // can tighten range + if (lower || upper) { + ranges.remove(i); + ranges.add(i, + new Range(main.location(), main.value(), + lower ? main.lower() : other.lower(), + lower ? main.includeLower() : other.includeLower(), + upper ? main.upper() : other.upper(), + upper ? main.includeUpper() : other.includeUpper())); + } + + // range was comparable + return compared; + } + // OR - needs both upper and lower to loosen range + else { + // can loosen range + if (lower && upper) { + ranges.remove(i); + ranges.add(i, + new Range(main.location(), main.value(), + lower ? main.lower() : other.lower(), + lower ? main.includeLower() : other.includeLower(), + upper ? main.upper() : other.upper(), + upper ? main.includeUpper() : other.includeUpper())); + return true; + } + + // if the range in included, no need to add it + return compared && (!((lower && !lowerEq) || (upper && !upperEq))); + } + } + } + return false; + } + + private boolean findConjunctiveComparisonInRange(BinaryComparison main, List ranges) { + Object value = main.right().fold(); + + for (int i = 0; i < ranges.size(); i++) { + Range other = ranges.get(i); + + if (main.left().semanticEquals(other.value())) { + + if (main instanceof GreaterThan || main instanceof GreaterThanOrEqual) { + if (other.lower().foldable()) { + Integer comp = BinaryComparison.compare(value, other.lower().fold()); + if (comp != null) { + // 2 < a AND (2 <= a < 3) -> 2 < a < 3 + boolean lowerEq = comp == 0 && other.includeLower() && main instanceof GreaterThan; + // 2 < a AND (1 < a < 3) -> 2 < a < 3 + boolean lower = comp > 0 || lowerEq; + + if (lower) { + ranges.remove(i); + ranges.add(i, + new Range(other.location(), other.value(), + main.right(), lowerEq ? true : other.includeLower(), + other.upper(), other.includeUpper())); + } + + // found a match + return true; + } + } + } else if (main instanceof LessThan || main instanceof LessThanOrEqual) { + if (other.lower().foldable()) { + Integer comp = BinaryComparison.compare(value, other.lower().fold()); + if (comp != null) { + // a < 2 AND (1 < a <= 2) -> 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; + + if (upper) { + ranges.remove(i); + ranges.add(i, new Range(other.location(), other.value(), + other.lower(), other.includeLower(), + main.right(), upperEq ? true : other.includeUpper())); + } + + // found a match + return true; + } + } + } + + return false; + } + } + return false; + } + + /** + * Find commonalities between the given comparison in the given list. + * The method can be applied both for conjunctive (AND) or disjunctive purposes (OR). + */ + private static boolean findExistingComparison(BinaryComparison main, List bcs, boolean conjunctive) { + Object value = main.right().fold(); + + for (int i = 0; i < bcs.size(); i++) { + BinaryComparison other = bcs.get(i); + // skip if cannot evaluate + if (!other.right().foldable()) { + continue; + } + // if bc is a higher/lower value or gte vs gt, use it instead + if ((other instanceof GreaterThan || other instanceof GreaterThanOrEqual) && + (main instanceof GreaterThan || main instanceof GreaterThanOrEqual)) { - // can loosen range - if (lower && upper) { - ranges.remove(i); - ranges.add(i, - new Range(r.location(), r.value(), - lower ? r.lower() : other.lower(), - lower ? r.includeLower() : other.includeLower(), - upper ? r.upper() : other.upper(), - upper ? r.includeUpper() : other.includeUpper())); - return true; + if (main.left().semanticEquals(other.left())) { + Integer compare = BinaryComparison.compare(value, other.right().fold()); + + if (compare != null) { + // AND + if ((conjunctive && + // a > 3 AND a > 2 -> a > 3 + (compare > 0 || + // a > 2 AND a >= 2 -> a > 2 + (compare == 0 && main instanceof GreaterThan && other instanceof GreaterThanOrEqual))) + || + // OR + (!conjunctive && + // a > 2 OR a > 3 -> a > 2 + (compare < 0 || + // a >= 2 OR a > 2 -> a >= 2 + (compare == 0 && main instanceof GreaterThanOrEqual && other instanceof GreaterThan)))) { + bcs.remove(i); + bcs.add(i, main); + } + // found a match + return true; + } + + return false; } + } + // if bc is a lower/higher value or lte vs lt, use it instead + else if ((other instanceof LessThan || other instanceof LessThanOrEqual) && + (main instanceof LessThan || main instanceof LessThanOrEqual)) { + + if (main.left().semanticEquals(other.left())) { + Integer compare = BinaryComparison.compare(value, other.right().fold()); + + if (compare != null) { + // AND + if ((conjunctive && + // a < 2 AND a < 3 -> a < 2 + (compare < 0 || + // a < 2 AND a <= 2 -> a < 2 + (compare == 0 && main instanceof LessThan && other instanceof LessThanOrEqual))) + || + // OR + (!conjunctive && + // a < 2 OR a < 3 -> a < 3 + (compare > 0 || + // a <= 2 OR a < 2 -> a <= 2 + (compare == 0 && main instanceof LessThanOrEqual && other instanceof LessThan)))) { + bcs.remove(i); + bcs.add(i, main); + + } + // found a match + return true; + } - // if the range in included, no need to add it - return !((lower && !lowerEq) || (upper && !upperEq)); + return false; + } } } + return false; } } diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/tree/Node.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/tree/Node.java index c0d885c6dcc4f..13b17791475ac 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/tree/Node.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/tree/Node.java @@ -5,6 +5,8 @@ */ package org.elasticsearch.xpack.sql.tree; +import org.elasticsearch.xpack.sql.SqlIllegalArgumentException; + import java.util.ArrayList; import java.util.BitSet; import java.util.List; @@ -37,6 +39,9 @@ public abstract class Node> { public Node(Location location, List children) { this.location = (location != null ? location : Location.EMPTY); + if (children.contains(null)) { + throw new SqlIllegalArgumentException("Null children are not allowed"); + } this.children = children; } 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 new file mode 100644 index 0000000000000..9e2d139460eea --- /dev/null +++ b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/optimizer/OptimizerRunTests.java @@ -0,0 +1,49 @@ +/* + * 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.sql.optimizer; + +import org.elasticsearch.test.ESTestCase; +import org.elasticsearch.xpack.sql.analysis.analyzer.Analyzer; +import org.elasticsearch.xpack.sql.analysis.index.EsIndex; +import org.elasticsearch.xpack.sql.analysis.index.IndexResolution; +import org.elasticsearch.xpack.sql.expression.function.FunctionRegistry; +import org.elasticsearch.xpack.sql.parser.SqlParser; +import org.elasticsearch.xpack.sql.plan.logical.LogicalPlan; +import org.elasticsearch.xpack.sql.type.EsField; +import org.elasticsearch.xpack.sql.type.TypesTests; + +import java.util.Map; +import java.util.TimeZone; + +public class OptimizerRunTests extends ESTestCase { + + private final SqlParser parser; + private final IndexResolution getIndexResult; + private final FunctionRegistry functionRegistry; + private final Analyzer analyzer; + private final Optimizer optimizer; + + public OptimizerRunTests() { + parser = new SqlParser(); + functionRegistry = new FunctionRegistry(); + + Map mapping = TypesTests.loadMapping("mapping-multi-field-variation.json"); + + EsIndex test = new EsIndex("test", mapping); + getIndexResult = IndexResolution.valid(test); + analyzer = new Analyzer(functionRegistry, getIndexResult, TimeZone.getTimeZone("UTC")); + optimizer = new Optimizer(); + } + + private LogicalPlan plan(String sql) { + return optimizer.optimize(analyzer.analyze(parser.createStatement(sql))); + } + + 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 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 2d4a5df0ca88c..c14184c82c5cc 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 @@ -395,7 +395,7 @@ public void testFoldExcludingRangeToFalse() { assertEquals(Boolean.FALSE, r.fold()); } - // 6 < a <= 5.5 + // 6 < a <= 5.5 -> FALSE public void testFoldExcludingRangeWithDifferentTypesToFalse() { FieldAttribute fa = new FieldAttribute(EMPTY, "a", new EsField("af", DataType.INTEGER, emptyMap(), true)); @@ -406,6 +406,17 @@ public void testFoldExcludingRangeWithDifferentTypesToFalse() { // Conjunction + public void testCombineBinaryComparisonsNotComparable() { + FieldAttribute fa = new FieldAttribute(EMPTY, "a", new EsField("af", DataType.INTEGER, emptyMap(), true)); + LessThanOrEqual lte = new LessThanOrEqual(EMPTY, fa, L(6)); + LessThan lt = new LessThan(EMPTY, fa, Literal.FALSE); + + CombineBinaryComparisons rule = new CombineBinaryComparisons(); + And and = new And(EMPTY, lte, lt); + Expression exp = rule.rule(and); + assertEquals(exp, and); + } + // a <= 6 AND a < 5 -> a < 5 public void testCombineBinaryComparisonsUpper() { FieldAttribute fa = new FieldAttribute(EMPTY, "a", new EsField("af", DataType.INTEGER, emptyMap(), true)); @@ -415,11 +426,9 @@ public void testCombineBinaryComparisonsUpper() { CombineBinaryComparisons rule = new CombineBinaryComparisons(); Expression exp = rule.rule(new And(EMPTY, lte, lt)); - assertEquals(Range.class, exp.getClass()); - Range r = (Range) exp; - assertNull(r.lower()); - assertEquals(L(5), r.upper()); - assertFalse(r.includeUpper()); + assertEquals(LessThan.class, exp.getClass()); + LessThan r = (LessThan) exp; + assertEquals(L(5), r.right()); } // 6 <= a AND 5 < a -> 6 <= a @@ -431,11 +440,9 @@ public void testCombineBinaryComparisonsLower() { CombineBinaryComparisons rule = new CombineBinaryComparisons(); Expression exp = rule.rule(new And(EMPTY, gte, gt)); - assertEquals(Range.class, exp.getClass()); - Range r = (Range) exp; - assertNull(r.upper()); - assertEquals(L(6), r.lower()); - assertTrue(r.includeLower()); + assertEquals(GreaterThanOrEqual.class, exp.getClass()); + GreaterThanOrEqual r = (GreaterThanOrEqual) exp; + assertEquals(L(6), r.right()); } // 5 <= a AND 5 < a -> 5 < a @@ -447,11 +454,9 @@ public void testCombineBinaryComparisonsInclude() { CombineBinaryComparisons rule = new CombineBinaryComparisons(); Expression exp = rule.rule(new And(EMPTY, gte, gt)); - assertEquals(Range.class, exp.getClass()); - Range r = (Range) exp; - assertNull(r.upper()); - assertEquals(L(5), r.lower()); - assertFalse(r.includeLower()); + assertEquals(GreaterThan.class, exp.getClass()); + GreaterThan r = (GreaterThan) exp; + assertEquals(L(5), r.right()); } // 3 <= a AND 4 < a AND a <= 7 AND a < 6 -> 4 < a < 6 @@ -536,8 +541,117 @@ public void testCombineUnbalancedComparisonsMixedWithEqualsIntoRange() { assertFalse(r.includeUpper()); } + // (2 < a < 3) AND (1 < a < 4) -> (2 < a < 3) + public void testCombineBinaryComparisonsConjunctionOfIncludedRange() { + FieldAttribute fa = new FieldAttribute(EMPTY, "a", new EsField("af", DataType.INTEGER, emptyMap(), true)); + + Range r1 = new Range(EMPTY, fa, L(2), false, L(3), false); + Range r2 = new Range(EMPTY, fa, L(1), false, L(4), false); + + And and = new And(EMPTY, r1, r2); + + CombineBinaryComparisons rule = new CombineBinaryComparisons(); + Expression exp = rule.rule(and); + assertEquals(r1, exp); + } + + // (2 < a < 3) AND a < 2 -> 2 < a < 2 + public void testCombineBinaryComparisonsConjunctionOfNonOverlappingBoundaries() { + FieldAttribute fa = new FieldAttribute(EMPTY, "a", new EsField("af", DataType.INTEGER, emptyMap(), true)); + + Range r1 = new Range(EMPTY, fa, L(2), false, L(3), false); + Range r2 = new Range(EMPTY, fa, L(1), false, L(2), false); + + And and = new And(EMPTY, r1, r2); + + CombineBinaryComparisons rule = new CombineBinaryComparisons(); + Expression exp = rule.rule(and); + assertEquals(Range.class, exp.getClass()); + Range r = (Range) exp; + assertEquals(L(2), r.lower()); + assertFalse(r.includeLower()); + assertEquals(L(2), r.upper()); + assertFalse(r.includeUpper()); + assertEquals(Literal.FALSE, r.fold()); + } + + // (2 < a < 3) AND (2 < a <= 3) -> 2 < a < 3 + public void testCombineBinaryComparisonsConjunctionOfUpperEqualsOverlappingBoundaries() { + FieldAttribute fa = new FieldAttribute(EMPTY, "a", new EsField("af", DataType.INTEGER, emptyMap(), true)); + + Range r1 = new Range(EMPTY, fa, L(2), false, L(3), false); + Range r2 = new Range(EMPTY, fa, L(2), false, L(3), true); + + And and = new And(EMPTY, r1, r2); + + CombineBinaryComparisons rule = new CombineBinaryComparisons(); + Expression exp = rule.rule(and); + assertEquals(r1, exp); + } + + // (2 < a < 3) AND (1 < a < 3) -> 2 < a < 3 + public void testCombineBinaryComparisonsConjunctionOverlappingUpperBoundary() { + FieldAttribute fa = new FieldAttribute(EMPTY, "a", new EsField("af", DataType.INTEGER, emptyMap(), true)); + + Range r2 = new Range(EMPTY, fa, L(2), false, L(3), false); + Range r1 = new Range(EMPTY, fa, L(1), false, L(3), false); + + And and = new And(EMPTY, r1, r2); + + CombineBinaryComparisons rule = new CombineBinaryComparisons(); + Expression exp = rule.rule(and); + assertEquals(r2, exp); + } + + // (2 < a <= 3) AND (1 < a < 3) -> 2 < a < 3 + public void testCombineBinaryComparisonsConjunctionWithDifferentUpperLimitInclusion() { + FieldAttribute fa = new FieldAttribute(EMPTY, "a", new EsField("af", DataType.INTEGER, emptyMap(), true)); + + Range r1 = new Range(EMPTY, fa, L(1), false, L(3), false); + Range r2 = new Range(EMPTY, fa, L(2), false, L(3), true); + + And and = new And(EMPTY, r1, r2); + + CombineBinaryComparisons rule = new CombineBinaryComparisons(); + Expression exp = rule.rule(and); + assertEquals(Range.class, exp.getClass()); + Range r = (Range) exp; + assertEquals(L(2), r.lower()); + assertFalse(r.includeLower()); + assertEquals(L(3), r.upper()); + assertFalse(r.includeUpper()); + } + + // (0 < a <= 1) AND (0 <= a < 2) -> 0 < a <= 1 + public void testRangesOverlappingConjunctionNoLowerBoundary() { + FieldAttribute fa = new FieldAttribute(EMPTY, "a", new EsField("af", DataType.INTEGER, emptyMap(), true)); + + Range r1 = new Range(EMPTY, fa, L(0), false, L(1), true); + Range r2 = new Range(EMPTY, fa, L(0), true, L(2), false); + + And and = new And(EMPTY, r1, r2); + + CombineBinaryComparisons rule = new CombineBinaryComparisons(); + Expression exp = rule.rule(and); + assertEquals(r1, exp); + } + // Disjunction + public void testCombineBinaryComparisonsDisjunctionNotComparable() { + FieldAttribute fa = new FieldAttribute(EMPTY, "a", new EsField("af", DataType.INTEGER, emptyMap(), true)); + + GreaterThan gt1 = new GreaterThan(EMPTY, fa, L(1)); + GreaterThan gt2 = new GreaterThan(EMPTY, fa, Literal.FALSE); + + Or or = new Or(EMPTY, gt1, gt2); + + CombineBinaryComparisons rule = new CombineBinaryComparisons(); + Expression exp = rule.rule(or); + assertEquals(exp, or); + } + + // 2 < a OR 1 < a OR 3 < a -> 1 < a public void testCombineBinaryComparisonsDisjunctionLowerBound() { FieldAttribute fa = new FieldAttribute(EMPTY, "a", new EsField("af", DataType.INTEGER, emptyMap(), true)); @@ -592,7 +706,7 @@ public void testCombineBinaryComparisonsDisjunctionUpperBound() { assertEquals(L(3), lt.right()); } - // a < 2 OR a <= 2 OR a < 1 -> null < a <= 2 + // a < 2 OR a <= 2 OR a < 1 -> a <= 2 public void testCombineBinaryComparisonsDisjunctionIncludeUpperBounds() { FieldAttribute fa = new FieldAttribute(EMPTY, "a", new EsField("af", DataType.INTEGER, emptyMap(), true)); @@ -636,6 +750,21 @@ public void testCombineBinaryComparisonsDisjunctionOfLowerAndUpperBounds() { assertEquals(L(3), gt.right()); } + // (2 < a < 3) OR (1 < a < 4) -> (1 < a < 4) + public void testCombineBinaryComparisonsDisjunctionOfIncludedRangeNotComparable() { + FieldAttribute fa = new FieldAttribute(EMPTY, "a", new EsField("af", DataType.INTEGER, emptyMap(), true)); + + Range r1 = new Range(EMPTY, fa, L(2), false, L(3), false); + Range r2 = new Range(EMPTY, fa, L(1), false, Literal.FALSE, false); + + Or or = new Or(EMPTY, r1, r2); + + CombineBinaryComparisons rule = new CombineBinaryComparisons(); + Expression exp = rule.rule(or); + assertEquals(or, exp); + } + + // (2 < a < 3) OR (1 < a < 4) -> (1 < a < 4) public void testCombineBinaryComparisonsDisjunctionOfIncludedRange() { FieldAttribute fa = new FieldAttribute(EMPTY, "a", new EsField("af", DataType.INTEGER, emptyMap(), true)); @@ -684,12 +813,12 @@ public void testCombineBinaryComparisonsDisjunctionOfUpperEqualsOverlappingBound assertEquals(r2, exp); } - // (2 < a < null) OR (1 < a < null) -> 1 < a < null - public void testCombineBinaryComparisonsOverlappingNoUpperBoundary() { + // (2 < a < 3) OR (1 < a < 3) -> 1 < a < 3 + public void testCombineBinaryComparisonsOverlappingUpperBoundary() { FieldAttribute fa = new FieldAttribute(EMPTY, "a", new EsField("af", DataType.INTEGER, emptyMap(), true)); - Range r2 = new Range(EMPTY, fa, L(2), false, null, false); - Range r1 = new Range(EMPTY, fa, L(1), false, null, false); + Range r2 = new Range(EMPTY, fa, L(2), false, L(3), false); + Range r1 = new Range(EMPTY, fa, L(1), false, L(3), false); Or or = new Or(EMPTY, r1, r2); @@ -698,7 +827,7 @@ public void testCombineBinaryComparisonsOverlappingNoUpperBoundary() { assertEquals(r1, exp); } - // (2 < a <= 3) OR (1 < a < 3) -> same (the <= prevents the ranges from being cumulated) + // (2 < a <= 3) OR (1 < a < 3) -> same (the <= prevents the ranges from being combined) public void testCombineBinaryComparisonsWithDifferentUpperLimitInclusion() { FieldAttribute fa = new FieldAttribute(EMPTY, "a", new EsField("af", DataType.INTEGER, emptyMap(), true)); @@ -712,12 +841,12 @@ public void testCombineBinaryComparisonsWithDifferentUpperLimitInclusion() { assertEquals(or, exp); } - // (null < a <= 1) OR (null < a < 2) -> null < a < 2 - public void testCombineBinaryComparisonsOverlappingNoLowerBoundary() { + // (0 < a <= 1) OR (0 < a < 2) -> 0 < a < 2 + public void testRangesOverlappingNoLowerBoundary() { FieldAttribute fa = new FieldAttribute(EMPTY, "a", new EsField("af", DataType.INTEGER, emptyMap(), true)); - Range r2 = new Range(EMPTY, fa, null, false, L(2), false); - Range r1 = new Range(EMPTY, fa, null, false, L(1), true); + Range r2 = new Range(EMPTY, fa, L(0), false, L(2), false); + Range r1 = new Range(EMPTY, fa, L(0), false, L(1), true); Or or = new Or(EMPTY, r1, r2); From 25aed2118dfb9f6696bbc712ae89dfa62683f4d6 Mon Sep 17 00:00:00 2001 From: Costin Leau Date: Wed, 2 May 2018 12:09:42 +0300 Subject: [PATCH 4/5] Fix tests --- .../xpack/sql/optimizer/OptimizerTests.java | 2 +- .../xpack/sql/querydsl/query/MatchQueryTests.java | 13 ++++++++++--- 2 files changed, 11 insertions(+), 4 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 c14184c82c5cc..9f95712b5d4c2 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 @@ -572,7 +572,7 @@ public void testCombineBinaryComparisonsConjunctionOfNonOverlappingBoundaries() assertFalse(r.includeLower()); assertEquals(L(2), r.upper()); assertFalse(r.includeUpper()); - assertEquals(Literal.FALSE, r.fold()); + assertEquals(Boolean.FALSE, r.fold()); } // (2 < a < 3) AND (2 < a <= 3) -> 2 < a < 3 diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/querydsl/query/MatchQueryTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/querydsl/query/MatchQueryTests.java index 431a6a146aee0..b8b4b2fb32bc5 100644 --- a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/querydsl/query/MatchQueryTests.java +++ b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/querydsl/query/MatchQueryTests.java @@ -8,16 +8,21 @@ import org.elasticsearch.index.query.MatchQueryBuilder; import org.elasticsearch.index.query.Operator; import org.elasticsearch.test.ESTestCase; +import org.elasticsearch.xpack.sql.expression.FieldAttribute; import org.elasticsearch.xpack.sql.expression.predicate.fulltext.MatchQueryPredicate; import org.elasticsearch.xpack.sql.tree.Location; import org.elasticsearch.xpack.sql.tree.LocationTests; +import org.elasticsearch.xpack.sql.type.DataType; +import org.elasticsearch.xpack.sql.type.EsField; import java.util.Arrays; import java.util.List; import java.util.function.Function; -import static org.hamcrest.Matchers.equalTo; +import static java.util.Collections.emptyMap; import static org.elasticsearch.test.EqualsHashCodeTestUtils.checkEqualsAndHashCode; +import static org.elasticsearch.xpack.sql.tree.Location.EMPTY; +import static org.hamcrest.Matchers.equalTo; public class MatchQueryTests extends ESTestCase { static MatchQuery randomMatchQuery() { @@ -62,14 +67,16 @@ public void testQueryBuilding() { private static MatchQueryBuilder getBuilder(String options) { final Location location = new Location(1, 1); - final MatchQueryPredicate mmqp = new MatchQueryPredicate(location, null, "eggplant", options); + FieldAttribute fa = new FieldAttribute(EMPTY, "a", new EsField("af", DataType.KEYWORD, emptyMap(), true)); + final MatchQueryPredicate mmqp = new MatchQueryPredicate(location, fa, "eggplant", options); final MatchQuery mmq = new MatchQuery(location, "eggplant", "foo", mmqp); return (MatchQueryBuilder) mmq.asBuilder(); } public void testToString() { final Location location = new Location(1, 1); - final MatchQueryPredicate mmqp = new MatchQueryPredicate(location, null, "eggplant", ""); + FieldAttribute fa = new FieldAttribute(EMPTY, "a", new EsField("af", DataType.KEYWORD, emptyMap(), true)); + final MatchQueryPredicate mmqp = new MatchQueryPredicate(location, fa, "eggplant", ""); final MatchQuery mmq = new MatchQuery(location, "eggplant", "foo", mmqp); assertEquals("MatchQuery@1:2[eggplant:foo]", mmq.toString()); } From a2fb0d2a98d71ae18f921aa7b9c800924c78845b Mon Sep 17 00:00:00 2001 From: Costin Leau Date: Wed, 2 May 2018 12:25:40 +0300 Subject: [PATCH 5/5] Add extra comments --- .../xpack/sql/expression/predicate/Predicates.java | 2 ++ .../java/org/elasticsearch/xpack/sql/optimizer/Optimizer.java | 4 +++- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/Predicates.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/Predicates.java index 6bf59772e8ccb..6fb26cb6dcc61 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/Predicates.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/Predicates.java @@ -67,10 +67,12 @@ private static Expression combine(List exps, BiFunction result = new ArrayList<>(exps); while (result.size() > 1) { // combine (in place) expressions in pairs + // NB: this loop modifies the list (just like an array) for (int i = 0; i < result.size() - 1; i++) { Expression l = result.remove(i); Expression r = result.remove(i); 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 28f54e5075a16..de4c0644b79b1 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 @@ -1512,7 +1512,7 @@ private static boolean findExistingRange(Range main, List ranges, boolean if (!main.lower().foldable() && !main.upper().foldable()) { return false; } - + // NB: the loop modifies the list (hence why the int is used) for (int i = 0; i < ranges.size(); i++) { Range other = ranges.get(i); @@ -1621,6 +1621,7 @@ private static boolean findExistingRange(Range main, List ranges, boolean private boolean findConjunctiveComparisonInRange(BinaryComparison main, List ranges) { Object value = main.right().fold(); + // NB: the loop modifies the list (hence why the int is used) for (int i = 0; i < ranges.size(); i++) { Range other = ranges.get(i); @@ -1682,6 +1683,7 @@ private boolean findConjunctiveComparisonInRange(BinaryComparison main, List bcs, boolean conjunctive) { Object value = main.right().fold(); + // NB: the loop modifies the list (hence why the int is used) for (int i = 0; i < bcs.size(); i++) { BinaryComparison other = bcs.get(i); // skip if cannot evaluate