sbroeder commented on code in PR #4959:
URL: https://github.com/apache/calcite/pull/4959#discussion_r3295770762
##########
core/src/main/java/org/apache/calcite/rex/RexCall.java:
##########
@@ -228,8 +228,13 @@ private boolean digestWithType() {
return operands.get(0).isAlwaysFalse();
case IS_NOT_FALSE:
case IS_TRUE:
- case CAST:
return operands.get(0).isAlwaysTrue();
+ case CAST:
Review Comment:
I believe
`with t(a) as (values (0), (1), (2))
select * from t where cast(a + 1 as boolean); `
would demonstrate the issue. However, I found just adding the assert causes
many existing tests to fail.
For example: `select * from "scott".emp where deptno = 25 and deptno <> 20;`
in conditions.iq results in this stack trace
Stack trace:
java.lang.AssertionError: isAlwaysTrue() called on non-boolean: INTEGER
at RexCall.isAlwaysTrue(RexCall.java:223)
at RexSimplify.simplifyAnd2ForUnknownAsFalse(RexSimplify.java:1889) ←
x = TRUE => x rewrite
at RexSimplify.simplifyAnd(RexSimplify.java:1766)
at RexSimplify.simplify(RexSimplify.java:287)
at RexSimplify.simplifyUnknownAs(RexSimplify.java:256)
at RexSimplify.simplifyPreservingType(RexSimplify.java:195)
at
ReduceExpressionsRule.reduceExpressions(ReduceExpressionsRule.java:729)
at
ReduceExpressionsRule$FilterReduceExpressionsRule.onMatch(ReduceExpressionsRule.java:159)
at VolcanoRuleCall.onMatch(VolcanoRuleCall.java:223)
at VolcanoPlanner.findBestExp(VolcanoPlanner.java:525)
Here RexSimplify.simplifyAnd2ForUnknownAsFalse calls isAlwaysTrue() on a
CAST(...):INTEGER and with the assert it fails.
In RexSimplify ~1889 we have a comment we want to simplify a boolean, again
without any checking the type:
` // Simplify BOOLEAN expressions if possible
while (term.getKind() == SqlKind.EQUALS) {
RexCall call = (RexCall) term;
if (call.getOperands().get(0).isAlwaysTrue()) {
`
I think this is trying to rewrite x = TRUE to x, which is valid for
booleans, but it's not validating the type first and just trusting
isAlwaysTrue.
There is a similar issue in RexSimplify.verify where we should wrap a guard
around the simplified.isAlwaysFalse simplified.isAlwaysTrue tests
```
// isAlwaysTrue/False sanity checks only apply to boolean expressions.
if (before.getType().getSqlTypeName() == SqlTypeName.BOOLEAN) {
```
But there are many more failures and I think fixing them all is beyond the
scope of 7542, which was targeted to RexCall.
I fear tightening this to 'callers must pass a boolean' might be fore of a
(breaking) API change than a bug fix.
If you agree, I would be happy to tackle all the remaining sites as a
follow-on API ticket. Perhaps something like "Tighten
RexCall.isAlwaysTrue/False() precondition to boolean-typed expressions only" ?
What are your thoughts?
--
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]
