zabetak commented on code in PR #4505:
URL: https://github.com/apache/calcite/pull/4505#discussion_r2435787917
##########
core/src/test/java/org/apache/calcite/rex/RexProgramTest.java:
##########
@@ -4230,6 +4146,8 @@ private SqlSpecialOperatorWithPolicy(String name, SqlKind
kind, int prec, boolea
checkSimplify(mul(nullInt, a), "null:INTEGER");
checkSimplify(div(a, one), "?0.notNullInt1");
+ checkSimplify(div(one, one), "1");
+ checkSimplify(div(nullInt, one), "null:INTEGER");
Review Comment:
Should we also add `div(zero, nullInt)` here given that we do some special
handling around the zero literals.
##########
core/src/test/java/org/apache/calcite/rex/RexProgramTest.java:
##########
@@ -2725,6 +2696,22 @@ trueLiteral, literal(1),
checkSimplifyUnchanged(divideNode4);
}
+ @Test void testSimplifyIsNullDivideByZero() {
+ simplify = simplify.withParanoid(false);
Review Comment:
Why is paranoid turned off?
##########
core/src/main/java/org/apache/calcite/plan/Strong.java:
##########
@@ -265,6 +266,22 @@ public boolean isNull(RexNode node) {
return anyNull(rexCall.getOperands());
}
return false;
+ case DIVIDE: {
Review Comment:
It seems that after the changes here `null / 0` will return `false` which
seems like a change in behavior.
Maybe we shouldn't bother with zero literals in this particular API. If we
have `null / X` or `X / null` the result should be null and it doesn't really
matter if X is zero. This is my interpretation of the SQL standard and probably
what the function used to do before the changes.
##########
core/src/main/java/org/apache/calcite/rex/RexSimplify.java:
##########
@@ -1189,6 +1092,23 @@ private RexNode simplifyIs(RexCall call, RexUnknownAs
unknownAs) {
}
}
return RexUtil.composeDisjunction(rexBuilder, operands, false);
+ case CUSTOM:
+ switch (a.getKind()) {
+ case DIVIDE: {
+ RexNode op1 = ((RexCall) a).getOperands().get(1);
+ if (!op1.isA(SqlKind.LITERAL)) {
+ return null;
+ }
+ if (checkLiteralValue(op1, BigDecimal.ZERO)) {
+ return null;
+ }
+ RexNode op0 = ((RexCall) a).getOperands().get(1);
+ return simplifyIsNull(op0);
+ }
+ default:
+ throw new AssertionError("every CUSTOM policy needs a handler, "
+ + a.getKind());
+ }
Review Comment:
Since the assertion error was not there before, I wouldn't add it to avoid
potential regressions. Before the changes if we encounter an operator with a
CUSTOM policy we will simply bail out; I think its better than throwing an
Assertion.
##########
core/src/test/java/org/apache/calcite/rex/RexProgramTest.java:
##########
@@ -2725,6 +2696,22 @@ trueLiteral, literal(1),
checkSimplifyUnchanged(divideNode4);
}
+ @Test void testSimplifyIsNullDivideByZero() {
+ simplify = simplify.withParanoid(false);
+
+ RelDataType intType =
+ typeFactory.createTypeWithNullability(
+ typeFactory.createSqlType(SqlTypeName.INTEGER), false);
+
+ checkSimplifyUnchanged(isNull(div(vIntNotNull(), literal(0))));
+ checkSimplifyUnchanged(isNull(div(vIntNotNull(), cast(literal(0),
intType))));
+ checkSimplify(isNull(div(vIntNotNull(), cast(literal(2), intType))),
"false");
Review Comment:
This is not a division by zero so it doesn't fit very well with the name of
the test.
##########
core/src/test/java/org/apache/calcite/rex/RexProgramTest.java:
##########
@@ -2725,6 +2696,22 @@ trueLiteral, literal(1),
checkSimplifyUnchanged(divideNode4);
}
+ @Test void testSimplifyIsNullDivideByZero() {
Review Comment:
How about covering the following cases as well:
* `div(cast(literal(2), intType), vIntNotNull())`
* `div(vIntNotNull(), vIntNotNull())`
* `div(null, 0)`
for `IS NULL` and `IS NOT NULL`.
--
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]
