vlsi commented on code in PR #5295:
URL: https://github.com/apache/calcite/pull/5295#discussion_r4189685779
##########
core/src/main/java/org/apache/calcite/rex/RexSimplify.java:
##########
@@ -2013,6 +2013,10 @@ private <C extends Comparable<C>> RexNode
simplifyAnd2ForUnknownAsFalse(
// Simplify BOOLEAN expressions if possible
while (term.getKind() == SqlKind.EQUALS) {
RexCall call = (RexCall) term;
+ if (call.getOperands().get(0).getType().getSqlTypeName()
+ != call.getOperands().get(1).getType().getSqlTypeName()) {
+ break;
+ }
Review Comment:
Julian suggested an `isWellFormed` check
([comment](https://github.com/apache/calcite/pull/5295#discussion_r4124950334)).
A helper of that shape, called here and from `simplifyComparison` before L751,
would also stop the swapped-operand case from the review body, and its name
would say why the term is left alone. As written, the guard exists only in this
loop and carries no comment.
This also changes plans for `ANY`: under `UNKNOWN_AS_FALSE`, `AND(=($0,
true), ?0.bool1)` with `$0` of type `ANY` used to become `AND($0, ?0.bool1)`
and now stays unchanged. It needs a mention in the JIRA and a test, since
`ITEM` on a `MAP<VARCHAR, ANY>` column, such as MongoDB's `_MAP`, produces
`ANY`.
##########
core/src/test/java/org/apache/calcite/rex/RexProgramTest.java:
##########
@@ -2472,6 +2472,35 @@ private void checkExponentialCnf(int n) {
"=(?0.notNullInt0, ?0.int1)");
}
+ /** Test case for
+ * <a
href="https://issues.apache.org/jira/browse/CALCITE-5907";>[CALCITE-5907]
+ * Unexpected boolean expression simplification for And expression</a>. */
+ @Test void testSimplifyAndEqualityTrue() {
+ // "=(x, true)" can be simplified to "x" only if x has BOOLEAN type;
+ // there is no implicit cast from VARCHAR or INTEGER, so the equality
+ // must be retained
Review Comment:
The stated reason is not accurate: Calcite does coerce VARCHAR and numeric
types to BOOLEAN (`SqlTypeCoercionRule`). The equality has to stay because
replacing it with `x` would put a non-BOOLEAN operand into the AND. Suggest:
`"=(x, true)" simplifies to "x" only if x is BOOLEAN; otherwise the AND would
get a non-BOOLEAN operand.`
##########
core/src/test/java/org/apache/calcite/rex/RexProgramTest.java:
##########
@@ -2472,6 +2472,35 @@ private void checkExponentialCnf(int n) {
"=(?0.notNullInt0, ?0.int1)");
}
+ /** Test case for
+ * <a
href="https://issues.apache.org/jira/browse/CALCITE-5907";>[CALCITE-5907]
+ * Unexpected boolean expression simplification for And expression</a>. */
+ @Test void testSimplifyAndEqualityTrue() {
+ // "=(x, true)" can be simplified to "x" only if x has BOOLEAN type;
+ // there is no implicit cast from VARCHAR or INTEGER, so the equality
+ // must be retained
+ checkSimplifyUnchanged(
+ and(eq(vVarchar(0), trueLiteral),
+ eq(vVarchar(1), trueLiteral)));
Review Comment:
With paranoid mode on (`RexProgramBuilderBase` turns it on for every test),
a regression here throws `ClassCastException` from `RexInterpreter` inside
`RexSimplify.verify` and never shows the simplified expression. Turning
paranoid mode off for the non-BOOLEAN inputs, as `testSimplifyItemRangeTerms`
does with `simplify = simplify.withParanoid(false)`, makes
`checkSimplifyUnchanged` print the expression it got instead.
Nit: the second argument of `and(` is indented at the same level as `and(`
itself, so it reads like a second argument of `checkSimplifyUnchanged`; the
same applies at L2487, L2494, L2497, and L2500.
--
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]
