[GitHub] [calcite] liyafan82 commented on a change in pull request #2238: [CALCITE-4364] `a in (1, 2) and a = 1` should be simplified to `a=1`

[GitBox <gi...@apache.org>]

liyafan82 commented on a change in pull request #2238:
URL: https://github.com/apache/calcite/pull/2238#discussion_r516407877



##########
File path: core/src/main/java/org/apache/calcite/rex/RexSimplify.java
##########
@@ -2710,6 +2710,16 @@ private boolean accept1(RexNode e, SqlKind kind,
       }
     }
 
+    /** Checks whether it is worth to fix and convert to {@code SEARCH} calls. */
+    boolean needToFix(int newTermsCnt) {
+      // Fix and converts to SEARCH if:
+      // 1. A Sarg has complexity greater than 1;
+      // 2. The terms are reduced as simpler Sarg points.
+      return map.values().stream().anyMatch(b -> b.complexity() > 1)

Review comment:
       + 1 for this suggestion.
   By using a simple loop, we can combine the two stream expressions?
   In addition, checking condition `newTermsCnt == 1` is cheap, can we move it forward?

##########
File path: core/src/main/java/org/apache/calcite/rex/RexSimplify.java
##########
@@ -2710,6 +2710,16 @@ private boolean accept1(RexNode e, SqlKind kind,
       }
     }
 
+    /** Checks whether it is worth to fix and convert to {@code SEARCH} calls. */
+    boolean needToFix(int newTermsCnt) {
+      // Fix and converts to SEARCH if:
+      // 1. A Sarg has complexity greater than 1;
+      // 2. The terms are reduced as simpler Sarg points.
+      return map.values().stream().anyMatch(b -> b.complexity() > 1)

Review comment:
       +1 for this suggestion.
   By using a simple loop, we can combine the two stream expressions?
   In addition, checking condition `newTermsCnt == 1` is cheap, can we move it forward?

##########
File path: core/src/test/java/org/apache/calcite/rex/RexProgramTest.java
##########
@@ -1774,6 +1770,17 @@ private void checkExponentialCnf(int n) {
     checkSimplify2(e, "SEARCH(?0.int0, Sarg[10])", "=(?0.int0, 10)");
   }
 
+  @Test void testSimplifyInAnd() {

Review comment:
       Maybe we need another test case: 
   
   deptno in (20, 10) and deptno = 30 
   ==> false?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org