vlsi commented on code in PR #5295:
URL: https://github.com/apache/calcite/pull/5295#discussion_r4219720981
##########
core/src/test/java/org/apache/calcite/rex/RexProgramTest.java:
##########
@@ -2472,6 +2472,61 @@ 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)" simplifies to "x" only if x is BOOLEAN;
+ // otherwise the AND would get a non-BOOLEAN operand
+ //
+ // Paranoid mode throws from RexSimplify.verify for these non-BOOLEAN
+ // inputs, so turn it off to let checkSimplifyUnchanged report the
+ // simplified expression instead
Review Comment:
Paranoid mode throws only when the simplified expression evaluates
differently from the original, not for these inputs as such. Suggest: `// With
paranoid mode on, a wrong simplification of these non-BOOLEAN inputs fails
inside RexSimplify.verify with an exception that hides the result; turn it off
so that checkSimplifyUnchanged prints the simplified expression`.
##########
core/src/main/java/org/apache/calcite/rex/RexSimplify.java:
##########
@@ -703,6 +703,18 @@ private static boolean
containsEquivalentComparison(List<RexNode> nodes,
return false;
}
+ /** Returns whether a comparison between {@code left} and {@code right}
+ * is well-formed, that is, its operands have the same type.
+ *
+ * <p>Comparisons whose operands have different types (e.g. a VARCHAR
+ * column compared with the BOOLEAN literal TRUE) never result from
+ * Calcite's own SQL translation, but clients that build Rex trees
+ * programmatically sometimes create them; such comparisons are left
+ * unsimplified. */
+ private static boolean isWellFormed(RexNode left, RexNode right) {
Review Comment:
The javadoc says that comparisons whose operands have different types "never
result from Calcite's own SQL translation". Nothing in `RexSimplify` or its
tests backs this, and no change to the converter would update it. Both call
sites need only "both operands are BOOLEAN": in `simplifyComparison` `o0` is
already BOOLEAN, and in the `AND` loop only a BOOLEAN node can be
`isAlwaysTrue()`. A helper named for that check, such as
`isBooleanComparison(left, right)` with the javadoc `Returns whether both
operands of a comparison have type BOOLEAN.`, behaves the same at both sites
and needs no claim about where such trees come from. `isWellFormed` names a
general property, so a later caller could use it to skip valid simplifications
such as INTEGER compared with BIGINT. The first sentence also says "the same
type" where the check compares `SqlTypeName`.
##########
core/src/test/java/org/apache/calcite/rex/RexProgramTest.java:
##########
@@ -2472,6 +2472,61 @@ 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)" simplifies to "x" only if x is BOOLEAN;
+ // otherwise the AND would get a non-BOOLEAN operand
+ //
+ // Paranoid mode throws from RexSimplify.verify for these non-BOOLEAN
+ // inputs, so turn it off to let checkSimplifyUnchanged report the
+ // simplified expression instead
+ simplify = this.simplify.withParanoid(false);
+ checkSimplifyUnchanged(
+ and(eq(vVarchar(0), trueLiteral),
+ eq(vVarchar(1), trueLiteral)));
+ checkSimplifyUnchanged(
+ and(eq(vInt(0), trueLiteral),
+ eq(vInt(1), trueLiteral)));
+ // same, with NOT NULL variables
+ checkSimplifyUnchanged(
+ and(eq(vVarcharNotNull(0), trueLiteral),
+ eq(vVarcharNotNull(1), trueLiteral)));
+ checkSimplifyUnchanged(
+ and(eq(vIntNotNull(0), trueLiteral),
+ eq(vIntNotNull(1), trueLiteral)));
+ // literal on the left (swapped) form
+ checkSimplifyUnchanged(eq(trueLiteral, vVarchar(0)));
+ checkSimplifyUnchanged(
+ and(eq(trueLiteral, vVarchar(0)),
+ eq(trueLiteral, vVarchar(1))));
+ checkSimplifyUnchanged(
+ and(eq(falseLiteral, vVarchar(0)),
+ eq(falseLiteral, vVarchar(1))));
+ checkSimplifyUnchanged(
+ and(ne(falseLiteral, vVarchar(0)),
+ ne(falseLiteral, vVarchar(1))));
+ // "=$0, true" where $0 has type ANY is no longer simplified to "$0";
+ // ITEM on a MAP<VARCHAR, ANY> column (e.g. MongoDB's _MAP) produces ANY,
+ // which may be BOOLEAN at runtime
Review Comment:
`"=$0, true"` is missing its parentheses. "is no longer simplified"
describes the change rather than the rule, and "which may be BOOLEAN at
runtime" gives the opposite reason: the term stays because a value of type
`ANY` need not be BOOLEAN. Suggest: `// "=($0, true)" stays when $0 has type
ANY, since its value need not be BOOLEAN; ITEM on a MAP<VARCHAR, ANY> column,
such as MongoDB's _MAP, has type ANY`.
##########
core/src/test/java/org/apache/calcite/rex/RexProgramTest.java:
##########
@@ -2472,6 +2472,61 @@ 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>. */
Review Comment:
The link text should match the JIRA summary, which is now `RexSimplify
replaces a comparison with a BOOLEAN literal by a non-BOOLEAN operand`.
--
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]
