[calcite] branch master updated: [CALCITE-4190] OR simplification incorrectly loses term

[jh...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

jhyde pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/calcite.git


The following commit(s) were added to refs/heads/master by this push:
     new 401b018  [CALCITE-4190] OR simplification incorrectly loses term
401b018 is described below

commit 401b01897b9a3b588d38acb6459411c5f7805776
Author: Julian Hyde <jh...@apache.org>
AuthorDate: Sun Aug 23 14:47:18 2020 -0700

    [CALCITE-4190] OR simplification incorrectly loses term
---
 .../java/org/apache/calcite/rex/RexSimplify.java   | 13 +++++--
 .../org/apache/calcite/rex/RexProgramTest.java     | 42 ++++++++++++++++++++++
 2 files changed, 53 insertions(+), 2 deletions(-)

diff --git a/core/src/main/java/org/apache/calcite/rex/RexSimplify.java b/core/src/main/java/org/apache/calcite/rex/RexSimplify.java
index 5e92098..b48fd7e 100644
--- a/core/src/main/java/org/apache/calcite/rex/RexSimplify.java
+++ b/core/src/main/java/org/apache/calcite/rex/RexSimplify.java
@@ -47,6 +47,7 @@ import com.google.common.collect.Sets;
 import com.google.common.collect.TreeRangeSet;
 
 import java.util.ArrayList;
+import java.util.BitSet;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.EnumSet;
@@ -526,11 +527,19 @@ public class RexSimplify {
     // may be unknown), because if either of them were true we would have
     // stopped.
     RexSimplify simplify = this;
+
+    // 'doneTerms' prevents us from visiting a term in both first and second
+    // loops. If we did this, the second visit would have a predicate saying
+    // that 'term' is false. Effectively, we sort terms: visiting
+    // 'allowedAsPredicate' terms in the first loop, and
+    // non-'allowedAsPredicate' in the second. Each term is visited once.
+    final BitSet doneTerms = new BitSet();
     for (int i = 0; i < terms.size(); i++) {
       final RexNode t = terms.get(i);
       if (!simplify.allowedAsPredicateDuringOrSimplification(t)) {
         continue;
       }
+      doneTerms.set(i);
       final RexNode t2 = simplify.simplify(t, unknownAs);
       terms.set(i, t2);
       final RexNode inverse =
@@ -542,8 +551,8 @@ public class RexSimplify {
     }
     for (int i = 0; i < terms.size(); i++) {
       final RexNode t = terms.get(i);
-      if (allowedAsPredicateDuringOrSimplification(t)) {
-        continue;
+      if (doneTerms.get(i)) {
+        continue; // we visited this term in the first loop
       }
       terms.set(i, simplify.simplify(t, unknownAs));
     }
diff --git a/core/src/test/java/org/apache/calcite/rex/RexProgramTest.java b/core/src/test/java/org/apache/calcite/rex/RexProgramTest.java
index dfd145c..92b6851 100644
--- a/core/src/test/java/org/apache/calcite/rex/RexProgramTest.java
+++ b/core/src/test/java/org/apache/calcite/rex/RexProgramTest.java
@@ -1562,6 +1562,48 @@ class RexProgramTest extends RexProgramTestBase {
         "true");
   }
 
+  @Test void testSimplifyRange() {
+    final RexNode aRef = input(tInt(), 0);
+    // ((0 < a and a <= 10) or a >= 15) and a <> 6 and a <> 12
+    RexNode expr = and(
+        or(
+            and(lt(literal(0), aRef),
+                le(aRef, literal(10))),
+            ge(aRef, literal(15))),
+        ne(aRef, literal(6)),
+        ne(aRef, literal(12)));
+    checkSimplifyUnchanged(expr);
+  }
+
+  @Test void testSimplifyRange2() {
+    final RexNode aRef = input(tInt(true), 0);
+    // a is null or a >= 15
+    RexNode expr = or(isNull(aRef),
+        ge(aRef, literal(15)));
+    checkSimplifyUnchanged(expr);
+  }
+
+  /** Unit test for
+   * <a href="https://issues.apache.org/jira/browse/CALCITE-4190">[CALCITE-4190]
+   * OR simplification incorrectly loses term</a>. */
+  @Test void testSimplifyRange3() {
+    final RexNode aRef = input(tInt(true), 0);
+    // (0 < a and a <= 10) or a is null or (8 < a and a < 12) or a >= 15
+    RexNode expr = or(
+        and(lt(literal(0), aRef),
+            le(aRef, literal(10))),
+        isNull(aRef),
+        and(lt(literal(8), aRef),
+            lt(aRef, literal(12))),
+        ge(aRef, literal(15)));
+    // [CALCITE-4190] causes "or a >= 15" to disappear from the simplified form.
+    final String expected = "OR(IS NULL($0),"
+        + " AND(<(0, $0), <=($0, 10)),"
+        + " AND(<(8, $0), <($0, 12)),"
+        + " >=($0, 15))";
+    checkSimplify(expr, expected);
+  }
+
   @Test void testSimplifyItemRangeTerms() {
     RexNode item = item(input(tArray(tInt()), 3), literal(1));
     // paranoid validation doesn't support array types, disable it for a moment