SQL: Predicate diff takes into account all values (#41346)

Fix bug in predicate subtraction that caused the evaluation to be
skipped on the first mismatch instead of evaluating the whole list. In
some cases this caused not only an incorrect result but one that kept on
growing causing the engine to bail

Fix #40835

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/capabilities/UnresolvedException.java

@@ -7,16 +7,12 @@ package org.elasticsearch.xpack.sql.capabilities;
 
 import org.elasticsearch.xpack.sql.ServerSqlException;
 
-import java.util.Locale;
-
-import static java.lang.String.format;
-
 /**
  * Thrown when we accidentally attempt to resolve something on on an unresolved entity. Throwing this
  * is always a bug.
  */
 public class UnresolvedException extends ServerSqlException {
     public UnresolvedException(String action, Object target) {
-        super(format(Locale.ROOT, "Invalid call to %s on an unresolved object %s", action, target));
+        super("Invalid call to {} on an unresolved object {}", action, target);
     }
 }

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/Predicates.java

@@ -97,14 +97,19 @@ public abstract class Predicates {
         return common.isEmpty() ? emptyList() : common;
     }
 
-    public static List<Expression> subtract(List<Expression> from, List<Expression> r) {
-        List<Expression> diff = new ArrayList<>(Math.min(from.size(), r.size()));
-        for (Expression lExp : from) {
-            for (Expression rExp : r) {
-                if (!lExp.semanticEquals(rExp)) {
-                    diff.add(lExp);
+    public static List<Expression> subtract(List<Expression> from, List<Expression> list) {
+        List<Expression> diff = new ArrayList<>(Math.min(from.size(), list.size()));
+        for (Expression f : from) {
+            boolean found = false;
+            for (Expression l : list) {
+                if (f.semanticEquals(l)) {
+                    found = true;
+                    break;
                 }
             }
+            if (found == false) {
+                diff.add(f);
+            }
         }
         return diff.isEmpty() ? emptyList() : diff;
     }

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

@@ -1236,7 +1236,7 @@ public class Optimizer extends RuleExecutor<LogicalPlan> {
 
         @Override
         protected Expression rule(Expression e) {
-            if (e instanceof BinaryPredicate) {
+            if (e instanceof And || e instanceof Or) {
                 return simplifyAndOr((BinaryPredicate<?, ?, ?, ?>) e);
             }
             if (e instanceof Not) {

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/rule/RuleExecutor.java

@@ -38,7 +38,7 @@ public abstract class RuleExecutor<TreeType extends Node<TreeType>> {
 
         boolean reached(int runs) {
             if (runs >= this.runs) {
-                throw new RuleExecutionException("Rule execution limit %d reached", runs);
+                throw new RuleExecutionException("Rule execution limit [{}] reached", runs);
             }
             return false;
         }
@@ -139,7 +139,7 @@ public abstract class RuleExecutor<TreeType extends Node<TreeType>> {
 
         for (Batch batch : batches) {
             int batchRuns = 0;
-            List<Transformation> tfs = new ArrayList<Transformation>();
+            List<Transformation> tfs = new ArrayList<>();
             transformations.put(batch, tfs);
 
             boolean hasChanged = false;

x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/optimizer/OptimizerTests.java

@@ -181,9 +181,12 @@ public class OptimizerTests extends ESTestCase {
     }
 
     private static FieldAttribute getFieldAttribute() {
-        return new FieldAttribute(EMPTY, "a", new EsField("af", DataType.INTEGER, emptyMap(), true));
+        return getFieldAttribute("a");
     }
 
+    private static FieldAttribute getFieldAttribute(String name) {
+        return new FieldAttribute(EMPTY, name, new EsField(name + "f", DataType.INTEGER, emptyMap(), true));
+    }
 
     public void testPruneSubqueryAliases() {
         ShowTables s = new ShowTables(EMPTY, null, null);
@@ -1145,6 +1148,23 @@ public class OptimizerTests extends ESTestCase {
         assertEquals(or, exp);
     }
 
+    // (a = 1 AND b = 3 AND c = 4) OR (a = 2 AND b = 3 AND c = 4) -> (b = 3 AND c = 4) AND (a = 1 OR a = 2)
+    public void testBooleanSimplificationCommonExpressionSubstraction() {
+        FieldAttribute fa = getFieldAttribute("a");
+        FieldAttribute fb = getFieldAttribute("b");
+        FieldAttribute fc = getFieldAttribute("c");
+
+        Expression a1 = new Equals(EMPTY, fa, ONE);
+        Expression a2 = new Equals(EMPTY, fa, TWO);
+        And common = new And(EMPTY, new Equals(EMPTY, fb, THREE), new Equals(EMPTY, fc, FOUR));
+        And left = new And(EMPTY, a1, common);
+        And right = new And(EMPTY, a2, common);
+        Or or = new Or(EMPTY, left, right);
+
+        Expression exp = new BooleanSimplification().rule(or);
+        assertEquals(new And(EMPTY, common, new Or(EMPTY, a1, a2)), exp);
+    }
+
     // (0 < a <= 1) OR (0 < a < 2) -> 0 < a < 2
     public void testRangesOverlappingNoLowerBoundary() {
         FieldAttribute fa = getFieldAttribute();
@@ -1289,7 +1309,7 @@ public class OptimizerTests extends ESTestCase {
         Order firstOrderBy = new Order(EMPTY, firstField, OrderDirection.ASC, Order.NullsPosition.LAST);
         Order secondOrderBy = new Order(EMPTY, secondField, OrderDirection.ASC, Order.NullsPosition.LAST);
         
-        OrderBy orderByPlan = new OrderBy(EMPTY, 
+        OrderBy orderByPlan = new OrderBy(EMPTY,
                 new Aggregate(EMPTY, FROM(), Arrays.asList(secondField, firstField), Arrays.asList(secondAlias, firstAlias)),
                 Arrays.asList(firstOrderBy, secondOrderBy));
         LogicalPlan result = new Optimizer.SortAggregateOnOrderBy().apply(orderByPlan);
@@ -1321,7 +1341,7 @@ public class OptimizerTests extends ESTestCase {
         Order firstOrderBy = new Order(EMPTY, firstAlias, OrderDirection.ASC, Order.NullsPosition.LAST);
         Order secondOrderBy = new Order(EMPTY, secondAlias, OrderDirection.ASC, Order.NullsPosition.LAST);
         
-        OrderBy orderByPlan = new OrderBy(EMPTY, 
+        OrderBy orderByPlan = new OrderBy(EMPTY,
                 new Aggregate(EMPTY, FROM(), Arrays.asList(secondAlias, firstAlias), Arrays.asList(secondAlias, firstAlias)),
                 Arrays.asList(firstOrderBy, secondOrderBy));
         LogicalPlan result = new Optimizer.SortAggregateOnOrderBy().apply(orderByPlan);