thomasrebele commented on code in PR #4505:
URL: https://github.com/apache/calcite/pull/4505#discussion_r2470532447
##########
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:
I've renamed the test method.
##########
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:
Ok, I've updated the PR to keep the previous behavior.
##########
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:
I've added the test case.
##########
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:
Indeed, `X / null` or `null / X` should be `null` (that's what Postgres
does). I've fixed it. I would still change the policy to CUSTOM, to keep the
changes to RexSimplify minimal.
##########
core/src/test/java/org/apache/calcite/rex/RexProgramTest.java:
##########
@@ -2725,6 +2696,22 @@ trueLiteral, literal(1),
checkSimplifyUnchanged(divideNode4);
}
+ @Test void testSimplifyIsNullDivideByZero() {
Review Comment:
Thanks for the test cases, they uncovered a problem with my proposal. I've
checked `div(null, 0)` with Postgres, and the result is `null`. As far as I
understand the SQL standard, this is the expected result.
I've added the test cases.
--
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]
