SQL: Optimisation fixes for conjunction merges (#50703) (#50933)

* 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.

* adress review observation on anon. comparator

Replace anonymous comparator of split AND Expressions with a lambda.

(cherry picked from commit 9828cb1)

Author: bpintea

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.

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/optimizer/Optimizer.java

@@ -397,7 +397,7 @@ protected LogicalPlan rule(OrderBy ob) {
             return ob;
         }
     }
-    
+
     static class PruneOrderByForImplicitGrouping extends OptimizerRule<OrderBy> {
 
         @Override
@@ -1090,7 +1090,18 @@ private Expression combine(And and) {
 
             boolean changed = false;
 
-            for (Expression ex : Predicates.splitAnd(and)) {
+            List<Expression> 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((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) {
                 if (ex instanceof Range) {
                     Range r = (Range) ex;
                     if (findExistingRange(r, ranges, true)) {
@@ -1215,9 +1226,9 @@ private static boolean findExistingRange(Range main, List<Range> 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 +1327,7 @@ private boolean findConjunctiveComparisonInRange(BinaryComparison main, List<Ran
                                     ranges.remove(i);
                                     ranges.add(i,
                                             new Range(other.source(), other.value(),
-                                                    main.right(), lowerEq ? true : other.includeLower(),
+                                                    main.right(), lowerEq ? false : main instanceof GreaterThanOrEqual,
                                                     other.upper(), other.includeUpper()));
                                 }
 
@@ -1325,19 +1336,19 @@ private boolean findConjunctiveComparisonInRange(BinaryComparison main, List<Ran
                             }
                         }
                     } else if (main instanceof LessThan || main instanceof LessThanOrEqual) {
-                        if (other.lower().foldable()) {
-                            Integer comp = BinaryComparison.compare(value, other.lower().fold());
+                        if (other.upper().foldable()) {
+                            Integer comp = BinaryComparison.compare(value, other.upper().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;
+                                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

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();