[GitHub] [calcite] kgyrtkirk commented on a diff in pull request #3416: [CALCITE-4189] Simplify 'p OR (p IS NOT TRUE)' to 'TRUE'

["kgyrtkirk (via GitHub)" <gi...@apache.org>]

kgyrtkirk commented on code in PR #3416:
URL: https://github.com/apache/calcite/pull/3416#discussion_r1321568345


##########
core/src/main/java/org/apache/calcite/rex/RexSimplify.java:
##########
@@ -2060,6 +2060,25 @@ private RexNode simplifyOrs(List<RexNode> terms, RexUnknownAs unknownAs) {
           }
         }
         break;
+      case IS_NOT_TRUE:
+        if (terms.contains(((RexCall) term).getOperands().get(0))) {
+          return trueLiteral;

Review Comment:
   I'm not sure about this; I think safety should still be considered:
   ```
   1/0 or x or (x is not true) 
   ```
   I think even if its okay for `x` ; we can't replace it with `true` unless all other parts of the or passes `isSafeExpression` or something similar



##########
core/src/main/java/org/apache/calcite/rex/RexSimplify.java:
##########
@@ -2060,6 +2060,25 @@ private RexNode simplifyOrs(List<RexNode> terms, RexUnknownAs unknownAs) {
           }
         }
         break;
+      case IS_NOT_TRUE:
+        if (terms.contains(((RexCall) term).getOperands().get(0))) {
+          return trueLiteral;
+        }
+        break;
+      case NOT:
+        RexNode x = ((RexCall) term).getOperands().get(0);
+        if (terms.contains(x)) {

Review Comment:
   I think to proceed in both cases `x` must be `isSafeExpression` to remove them



##########
core/src/test/java/org/apache/calcite/rex/RexProgramTest.java:
##########
@@ -2318,7 +2318,7 @@ trueLiteral, literal(2),
             isTrue(vBool()), literal(1),
             isNotTrue(vBool()), literal(1),
             literal(2)),
-        "CASE(OR(?0.bool0, IS NOT TRUE(?0.bool0)), 1, 2)");
+        "1");

Review Comment:
   :cowboy_hat_face: 



##########
core/src/main/java/org/apache/calcite/rex/RexSimplify.java:
##########
@@ -2060,6 +2060,25 @@ private RexNode simplifyOrs(List<RexNode> terms, RexUnknownAs unknownAs) {
           }
         }
         break;
+      case IS_NOT_TRUE:
+        if (terms.contains(((RexCall) term).getOperands().get(0))) {
+          return trueLiteral;
+        }
+        break;
+      case NOT:
+        RexNode x = ((RexCall) term).getOperands().get(0);
+        if (terms.contains(x)) {
+          if (!x.getType().isNullable()) {
+            return trueLiteral;
+          }
+
+          final RexNode isNotNull =
+              rexBuilder.makeCall(SqlStdOperatorTable.IS_NOT_NULL, x);
+          terms.set(terms.indexOf(x), simplifyIs((RexCall) isNotNull, unknownAs));
+          terms.set(i, rexBuilder.makeNullLiteral(x.getType()));
+          i--;

Review Comment:
   I don't see the reason for this `i--` why its needed?



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

To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org

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