[CALCITE-7542] RexCall.isAlwaysTrue()/isAlwaysFalse() incorrectly returns true for CAST(boolean AS non-boolean)

[sbroeder]

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:

• testIsAlwaysTrueCastBooleanToInteger — CAST(TRUE AS INTEGER).isAlwaysTrue() is false. Fails on unfixed code.
• testIsAlwaysFalseCastBooleanToInteger — CAST(FALSE AS INTEGER).isAlwaysFalse() is false. Fails on unfixed code.
• testIsAlwaysTrueCastBooleanToBoolean — CAST(TRUE AS BOOLEAN).isAlwaysTrue() is still true (no regression for same-type casts).
• testIsAlwaysFalseCastBooleanToBoolean — CAST(FALSE AS BOOLEAN).isAlwaysFalse() is still true.

core/src/test/resources/sql/conditions.iq

• Add end-to-end probe for RexCall.isAlwaysTrue/False CAST recursion

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.

• core/src/main/java/org/apache/calcite/rex/RexCall.java

[mihaibudiu]

By adding an assertion that the type of the expression is Boolean you may catch other misuses.

[sbroeder]

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.

[mihaibudiu]

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.

[sbroeder]

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

[mihaibudiu]

Today this seems to be called only on predicates and join conditions.
These should all be Boolean.
I will review this version.

[mihaibudiu]

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.

[sbroeder]

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?

[mihaibudiu]

That was my question: which tests fail if you tighten the condition.
If many do, this means this is a genuine bug and not a precondtion, and your previous fix is the right one.

[sbroeder]

Yes, many fail. In RexProgramTest alone these fail, but there are others;
testSimplifyRecurseIntoArithmetics()
testSimplifyVarbinary()
testSimplifyNullCheckInFilter()
testSimplifyMeasure()
testSimplifyIsNullDivideWithNullArgument()
testSimplifyDivideSafe()
testSimplifyCondition2()
testSimplifyCondition()
testSimplifyCoalesce()
testSimplifyCastLiteral2()
testSimplifyCaseNullableVarChar()
testSimplifyCaseCompactionDiv()
testSimplifyCaseCompaction2()
testSimplifyCaseCompaction()
testIsAlwaysTrueFalseCastNonBooleanCallToBoolean()
testIsAlwaysTrueCastBooleanToInteger()

So are you happy for me to return false early in the top of isAlwaysTrue and isAlwaysFalse if the type is not a boolean?

[mihaibudiu]

yes, thank you

[mihaibudiu]

Please squash in preparation for commit; we'll probably merge after 1.43 is released

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud