Conversation
| RexCall call = (RexCall) term; | ||
| if (call.getOperands().get(0).isAlwaysTrue()) { | ||
| if (call.getOperands().get(0).isAlwaysTrue() | ||
| && call.getOperands().get(1).getType().getSqlTypeName() == SqlTypeName.BOOLEAN) { |
There was a problem hiding this comment.
I hope you have checked that this works for any nullability
There was a problem hiding this comment.
This is bullshit.
CALCITE-5907 is invalid, because you should never have "varchar-column = bool-literal" in a Rex node. If we see non-boolean terms in a boolean expression, we should throw.
There was a problem hiding this comment.
Let me share my understanding of this part—though I might be mistaken: I agree that a well-typed Rex tree should never contain =(varchar, bool), and that SqlValidator is the right place to enforce comparability. However, that invariant is not enforced at the Rex layer itself: RexBuilder.makeCall(EQUALS, ...) performs no operand-family check (EQUALS' return-type inference is unconditionally BOOLEAN), so downstream systems that construct expressions programmatically (e.g. Flink, the reporter of https://issues.apache.org/jira/browse/FLINK-27402) can and do produce such nodes.
The bug here is not the input's existence, but that the simplifier turned a valid BOOLEAN condition into a non-BOOLEAN one. A rewrite rule must never produce output less valid than its input; leaving the equality untouched is the minimal, safe behavior. Throwing from a simplification hot path would turn a silent robustness gap into planner crashes for rels that currently plan and execute fine, and conflates rewriting with validation, Calcite keeps those separate (cf. RexChecker/RexUtil.verify).
There was a problem hiding this comment.
Ah, it's a malformed =, not a malformed boolean term in AND or OR. That's not OK, but it's understandable. I think it would break a lot of clients it Calcite started to reject such = calls.
So I think you should make isAlwaysTrue more defensive - push the 'type is boolean' check into that method. Thus for the string literal 'true', isAlwaysTrue will return false.
Ditto isAlwaysFalse and any analogous methods.
There was a problem hiding this comment.
Thanks for the suggestion @julianhyde . isAlwaysTrue / isAlwaysFalse are already defensive: both RexLiteral and RexCall return false unless the expression has BOOLEAN type, so the string literal 'true' already returns false.The call-site check guards the other operand (the one that would replace the =` term), so it can't be pushed intoisAlwaysTrue. Without it,AND(=(x, TRUE), ...)with VARCHAR x would simplify toAND(x, ...) which is the CALCITE-5907 bug. Please correct me if I am wrong～
There was a problem hiding this comment.
I hope you have checked that this works for any nullability
Good point @mihaibudiu . The check only looks at the type name, not nullability, and it is safe for both cases: =(x, TRUE) → x with nullable x (if x is NULL, =(NULL, TRUE) and bare NULL are both UNKNOWN, i.e. FALSE under UNKNOWN_AS_FALSE) and with NOT NULL x (trivially equivalent).
I had added NOT NULL variants of VARCHAR, INTEGER and BOOLEAN to testSimplifyAndEqualityTrue to cover this.
There was a problem hiding this comment.
Maybe just check that the operands have the same type? Your change is making RexSimplify more complicated, because we are accommodating a client that is sending us garbage. That is tech debt right there.
There was a problem hiding this comment.
Or call a 'isWellFormed' method before entering the loop. It sends a strong message that "this code doesn't run on garbage".
There was a problem hiding this comment.
Good idea, done @julianhyde . The loop now starts with a well-formedness guard: unless both operands of = have the same type name (e.g.=(x, TRUE) with VARCHAR or INTEGER x is malformed), it breaks without simplifying. This is equivalent to the previous check, sinceisAlwaysTrue() can only be true for a BOOLEAN expression, but it states the precondition up front. I compare getSqlTypeName() rather than RelDataType.equals, because TRUE is NOT NULL and equals would compare nullability, blocking the valid simplification of=(nullable Bool X, TRUE).
|
|
Since this pr is approved and should had addressed comments, if no other objections I would merge it in few days. cc @julianhyde @mihaibudiu |
mihaibudiu
added
the
LGTM-will-merge-soon
3 participants
jira: https://issues.apache.org/jira/browse/CALCITE-5907