[CALCITE-7542] RexCall.isAlwaysTrue()/isAlwaysFalse() incorrectly returns true for CAST(boolean AS non-boolean)#4959
[CALCITE-7542] RexCall.isAlwaysTrue()/isAlwaysFalse() incorrectly returns true for CAST(boolean AS non-boolean)#4959sbroeder wants to merge 2 commits into
Conversation
| case IS_TRUE: | ||
| case CAST: | ||
| return operands.get(0).isAlwaysTrue(); | ||
| case CAST: |
There was a problem hiding this comment.
By adding an assertion that the type of the expression is Boolean you may catch other misuses.
There was a problem hiding this comment.
Great observation. I chose to use a test for BOOLEAN and return false because I am worried about assert not firing in production code. I think this is safe and covers the case where future cases might be added.
There was a problem hiding this comment.
If it's a bug to call this function with non-boolean expressions, it should be an assert or an explicit exception; by returning early you are hiding the bug.
Unfortunately there is no JavaDoc telling us what the precondtions are. I personally prefer as many assertions as possible, to find bugs early.
There was a problem hiding this comment.
I don't believe it is a bug to call this function on non-boolean expressions and there could be recursion in it related to casts from
return operands.get(0).isAlwaysTrue();
Consider the expression CAST(int_expr AS BOOLEAN) where int_expr is a RexCall (eg. 1 + 1), then with the assertion it will fail, but with the early return it works correctly.
I've added a new test to demonstrate this as well testIsAlwaysTrueFalseCastNonBooleanCallToBoolean
There was a problem hiding this comment.
Today this seems to be called only on predicates and join conditions.
These should all be Boolean.
I will review this version.
There was a problem hiding this comment.
If you make changes before approval you should generally use new commits, especially if the previous one have been reviewed. Makes it easier for reviewers to see what's new.
There was a problem hiding this comment.
yes, but previously your early return was only in the CAST branch.
What happens if you move it back there and change the top one with an assertion?
Does any test fail?
There was a problem hiding this comment.
My apologies and thank you for your patience. My attempt at efficiency failed miserably.
I've restored the previous logic that lived in the CAST case and all tests currently pass.
If I add a top level assert
222 + assert getType().getSqlTypeName() == SqlTypeName.BOOLEAN 223 + : "isAlwaysTrue() called on non-boolean: " + getType();
then these 3 tests will fail
- testIsAlwaysTrueCastBooleanToInteger
- testIsAlwaysFalseCastBooleanToInteger
- testIsAlwaysTrueFalseCastNonBooleanCallToBoolean
Perhaps you have a different idea of where to place the assert?
There was a problem hiding this comment.
This is because these test which you wrote do not respect the new precondition of the function. Do these tests correspond to a situation you have observed in practice? If so, can you supply an end-to-end test which illustrates this case?
If not, a solution is to simply delete these tests, since they do not exercise a "real" possible path.
…urns true for CAST(boolean AS non-boolean) RexCall.isAlwaysTrue() and isAlwaysFalse() group CAST with IS_TRUE/IS_NOT_FALSE (and IS_FALSE/IS_NOT_TRUE) and delegate to the operand. That is wrong when the cast changes the type: CAST(TRUE AS INTEGER) is an INTEGER expression that evaluates to 1, not a boolean, so it must report isAlwaysTrue() == false. Fix: Return early in RexCall and only delegate when the result type is BOOLEAN. Co-authored-by: Sean Broeder <sean@dremio.com>
Per review feedback: instead of an early-return at the top of isAlwaysTrue/False() that checks for BOOLEAN type, do the check only inside the CAST case where the type can diverge from the operand's. The other switch cases (IS_*, NOT, SEARCH) all produce BOOLEAN results by construction, so they do not need the guard.
|
3 participants
Summary
RexCall.isAlwaysTrue() and isAlwaysFalse() group CAST together with IS_TRUE / IS_NOT_FALSE (and IS_FALSE / IS_NOT_TRUE) and simply delegate to the operand. That is wrong when the cast changes the type: CAST(TRUE AS INTEGER) is an INTEGER expression that evaluates to 1, not a boolean, so it must report isAlwaysTrue() == false. Today it returns true.
This is an API-contract violation. RexSimplify happens not to mis-fire on this expression upstream (its Comparison helper requires a LITERAL-kind operand, which CAST is not), but any caller that consults isAlwaysTrue/False on an arbitrary RexNode and trusts the result to be boolean-valued can be misled.
Tests
core/src/test/java/org/apache/calcite/rex/RexProgramTest.java:
Repro (against unfixed code)
./gradlew :core:test
--tests "org.apache.calcite.rex.RexProgramTest.testIsAlwaysTrueCastBooleanToInteger"
--tests "org.apache.calcite.rex.RexProgramTest.testIsAlwaysFalseCastBooleanToInteger"
Both fail with Expected: is but: was . With the fix applied, all four tests pass.
CALCITE-7542
Fix
Return early in RexCall in both methods and only delegate to the operand when the result type is BOOLEAN.