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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
