You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by "thomasrebele (via GitHub)" <gi...@apache.org> on 2025/08/25 16:46:31 UTC
[PR] [CALCITE-7145] RexSimplify produces an incorrect result for IS NULL(10/0) [calcite]
thomasrebele opened a new pull request, #4505:
URL: https://github.com/apache/calcite/pull/4505
(no comment)
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7145] RexSimplify produces an incorrect result for IS NULL(10/0) [calcite]
Posted by "thomasrebele (via GitHub)" <gi...@apache.org>.
thomasrebele commented on code in PR #4505:
URL: https://github.com/apache/calcite/pull/4505#discussion_r2486683349
##########
core/src/main/java/org/apache/calcite/rex/RexSimplify.java:
##########
@@ -1189,6 +1102,23 @@ private RexNode simplifyIs(RexCall call, RexUnknownAs unknownAs) {
}
}
return RexUtil.composeDisjunction(rexBuilder, operands, false);
+ case CUSTOM:
+ if (a.getKind() == SqlKind.DIVIDE) {
+ RexNode op0 = ((RexCall) a).getOperands().get(0);
+ RexNode op1 = ((RexCall) a).getOperands().get(1);
+ if (RexUtil.isNull(op0) || RexUtil.isNull(op1)) {
+ return rexBuilder.makeLiteral(true);
+ }
+ if (!op1.isA(SqlKind.LITERAL)) {
+ return rexBuilder.makeCall(SqlStdOperatorTable.IS_NULL, simplifyGenericNode((RexCall) a));
+ }
+ if (checkLiteralValue(op1, BigDecimal.ZERO)) {
+ return rexBuilder.makeCall(SqlStdOperatorTable.IS_NULL, simplifyGenericNode((RexCall) a));
+ }
+ // op1 is a non-null and non-zero literal, so simplify op0
+ return simplifyIsNull(op0);
+ }
+ return null;
Review Comment:
It might indeed be possible. I would move this discussion to another ticket, though: [CALCITE-7263](https://issues.apache.org/jira/browse/CALCITE-7263).
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7145] RexSimplify produces an incorrect result for IS NULL(10/0) [calcite]
Posted by "thomasrebele (via GitHub)" <gi...@apache.org>.
thomasrebele commented on code in PR #4505:
URL: https://github.com/apache/calcite/pull/4505#discussion_r2298613378
##########
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:
If the second argument is not a literal, we might divide by 0. In that case, the nullability is not defined.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7145] RexSimplify produces an incorrect result for IS NULL(10/0) [calcite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #4505:
URL: https://github.com/apache/calcite/pull/4505#issuecomment-3457869389
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4505) **Quality Gate passed**
Issues
 [1 New issue](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4505&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4505&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=4505&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [86.1% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4505&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4505&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4505)
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7145] RexSimplify produces an incorrect result for IS NULL(10/0) [calcite]
Posted by "thomasrebele (via GitHub)" <gi...@apache.org>.
thomasrebele commented on code in PR #4505:
URL: https://github.com/apache/calcite/pull/4505#discussion_r2465247840
##########
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:
Paranoid evaluates the expression, resulting in "Division by zero" exceptions.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7145] RexSimplify produces an incorrect result for IS NULL(10/0) [calcite]
Posted by "thomasrebele (via GitHub)" <gi...@apache.org>.
thomasrebele commented on code in PR #4505:
URL: https://github.com/apache/calcite/pull/4505#discussion_r2485730768
##########
core/src/main/java/org/apache/calcite/rex/RexSimplify.java:
##########
@@ -1028,9 +1037,25 @@ private RexNode simplifyIs(RexCall call, RexUnknownAs unknownAs) {
switch (a.getKind()) {
case LITERAL:
return rexBuilder.makeLiteral(!((RexLiteral) a).isNull());
+ case DIVIDE: {
+ RexNode op0 = ((RexCall) a).getOperands().get(0);
+ RexNode op1 = ((RexCall) a).getOperands().get(1);
+ if (RexUtil.isNull(op0) || RexUtil.isNull(op1)) {
+ return rexBuilder.makeLiteral(false);
+ }
+ if (!op1.isA(SqlKind.LITERAL)) {
+ return rexBuilder.makeCall(
+ SqlStdOperatorTable.IS_NOT_NULL, simplifyGenericNode((RexCall) a));
+ }
+ if (checkLiteralValue(op1, BigDecimal.ZERO)) {
+ return rexBuilder.makeCall(
+ SqlStdOperatorTable.IS_NOT_NULL, simplifyGenericNode((RexCall) a));
+ }
+ // op1 is a non-null and non-zero literal, so simplify op0
+ return simplifyIsNotNull(op0);
+ }
default:
- throw new AssertionError("every CUSTOM policy needs a handler, "
- + a.getKind());
+ return null;
Review Comment:
@zabetak specifically asked for this in his [review](https://github.com/apache/calcite/pull/4505#discussion_r2435623000).
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7145] RexSimplify produces an incorrect result for IS NULL(10/0) [calcite]
Posted by "thomasrebele (via GitHub)" <gi...@apache.org>.
thomasrebele commented on code in PR #4505:
URL: https://github.com/apache/calcite/pull/4505#discussion_r2486626712
##########
core/src/main/java/org/apache/calcite/rex/RexSimplify.java:
##########
@@ -488,9 +493,13 @@ private RexNode simplifyMultiply(RexCall e) {
}
private RexNode simplifyDivide(RexCall e) {
- final int oneIndex = findLiteralIndex(e.operands, BigDecimal.ONE);
- if (oneIndex == 1) {
- RexNode leftOperand = e.getOperands().get(0);
+ RexNode leftOperand = e.getOperands().get(0);
+ RexNode rightOperand = e.getOperands().get(1);
+ if ((isSafeExpression(leftOperand) && STRONG.isNull(leftOperand))
Review Comment:
The simplification of arithmetic operations with nulls has already been implemented, see `RexProgramTest#testSimplifySimpleArithmetic`. The "new" simplification logic is needed to simplify `NULL / 0` to `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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7145] RexSimplify produces an incorrect result for IS NULL(10/0) [calcite]
Posted by "thomasrebele (via GitHub)" <gi...@apache.org>.
thomasrebele commented on code in PR #4505:
URL: https://github.com/apache/calcite/pull/4505#discussion_r2485946569
##########
core/src/main/java/org/apache/calcite/rex/RexSimplify.java:
##########
@@ -1028,9 +1037,25 @@ private RexNode simplifyIs(RexCall call, RexUnknownAs unknownAs) {
switch (a.getKind()) {
case LITERAL:
return rexBuilder.makeLiteral(!((RexLiteral) a).isNull());
+ case DIVIDE: {
Review Comment:
As far as I understand the SQL standard, floating point division by 0 should throw an exception as well. This is also what Calcite does. Here a test case for InterpreterTest that passes for 67e5bfcdc0151aa1926fa369382d12fc672ca5bf (shortly after the release of Calcite 1.41.0):
```
@Test void testDivideFloatByZero() {
sql("select 10.0/4.0").returnsRows("[2.5]");
try {
sql("select 1.0/0.0").returnsRows("[NaN]");
fail("expected an ArithmeticException");
}
catch (ArithmeticException ex) {
assertEquals("Division by zero", ex.getMessage());
}
}
```
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7145] RexSimplify produces an incorrect result for IS NULL(10/0) [calcite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #4505:
URL: https://github.com/apache/calcite/pull/4505#issuecomment-3481151552
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4505) **Quality Gate passed**
Issues
 [2 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4505&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4505&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=4505&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [93.2% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4505&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4505&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4505)
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7145] RexSimplify produces an incorrect result for IS NULL(10/0) [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #4505:
URL: https://github.com/apache/calcite/pull/4505#discussion_r2298622932
##########
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:
Julian left some comments in the JIRA about this choice.
But the fact that the literal is zero is irrelevant; the result is the same for all non-null literals.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7145] RexSimplify produces an incorrect result for IS NULL(10/0) [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #4505:
URL: https://github.com/apache/calcite/pull/4505#discussion_r2483816615
##########
core/src/main/java/org/apache/calcite/rex/RexSimplify.java:
##########
@@ -415,17 +415,22 @@ private RexNode simplifyGenericNode(RexCall e) {
*/
private static int findLiteralIndex(List<RexNode> operands, BigDecimal value) {
for (int i = 0; i < operands.size(); i++) {
- if (operands.get(i).isA(SqlKind.LITERAL)) {
- Comparable comparable = ((RexLiteral) operands.get(i)).getValue();
- if (comparable instanceof BigDecimal
- && value.compareTo((BigDecimal) comparable) == 0) {
- return i;
- }
+ if (checkLiteralValue(operands.get(i), value)) {
+ return i;
}
}
return -1;
}
+ /** Check whether the operand is a literal of the specified value. */
Review Comment:
This method is a conservative approximation only; the javadoc should say that.
It will not handle for example floating point literals, which can appear due to the constant folding of expressions.
##########
core/src/main/java/org/apache/calcite/rex/RexSimplify.java:
##########
@@ -1028,9 +1037,25 @@ private RexNode simplifyIs(RexCall call, RexUnknownAs unknownAs) {
switch (a.getKind()) {
case LITERAL:
return rexBuilder.makeLiteral(!((RexLiteral) a).isNull());
+ case DIVIDE: {
+ RexNode op0 = ((RexCall) a).getOperands().get(0);
+ RexNode op1 = ((RexCall) a).getOperands().get(1);
+ if (RexUtil.isNull(op0) || RexUtil.isNull(op1)) {
+ return rexBuilder.makeLiteral(false);
+ }
+ if (!op1.isA(SqlKind.LITERAL)) {
+ return rexBuilder.makeCall(
+ SqlStdOperatorTable.IS_NOT_NULL, simplifyGenericNode((RexCall) a));
Review Comment:
do you need the call to simplify a here and below?
if you do, maybe you can add a comment explaining why.
there was a call to simplify a above using a different method
##########
core/src/main/java/org/apache/calcite/rex/RexSimplify.java:
##########
@@ -488,9 +493,13 @@ private RexNode simplifyMultiply(RexCall e) {
}
private RexNode simplifyDivide(RexCall e) {
- final int oneIndex = findLiteralIndex(e.operands, BigDecimal.ONE);
- if (oneIndex == 1) {
- RexNode leftOperand = e.getOperands().get(0);
+ RexNode leftOperand = e.getOperands().get(0);
+ RexNode rightOperand = e.getOperands().get(1);
+ if ((isSafeExpression(leftOperand) && STRONG.isNull(leftOperand))
Review Comment:
is the 'isSafeExpression' necessary?
If the value is NULL, isn't it always safe?
##########
core/src/main/java/org/apache/calcite/plan/Strong.java:
##########
@@ -356,7 +358,8 @@ private static Map<SqlKind, Policy> createPolicyMap() {
map.put(SqlKind.CHECKED_TIMES, Policy.ANY);
map.put(SqlKind.CHECKED_DIVIDE, Policy.ANY);
- map.put(SqlKind.DIVIDE, Policy.ANY);
+ // DIVIDE behaves like ANY. However, (x/0) IS (NOT) NULL may not be simplified.
Review Comment:
This comment should say that in some dialects x/y may be NULL even when neither of x and y are NULL, e.g., y = 0.
it has nothing to do with simplification.
##########
core/src/main/java/org/apache/calcite/rex/RexSimplify.java:
##########
@@ -1028,9 +1037,25 @@ private RexNode simplifyIs(RexCall call, RexUnknownAs unknownAs) {
switch (a.getKind()) {
case LITERAL:
return rexBuilder.makeLiteral(!((RexLiteral) a).isNull());
+ case DIVIDE: {
Review Comment:
note that floating point division behaves like the STRONG policy, since division by 0 returns an infinity or NaN.
##########
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:
please add a link to the JIRA case
##########
core/src/main/java/org/apache/calcite/rex/RexSimplify.java:
##########
@@ -1028,9 +1037,25 @@ private RexNode simplifyIs(RexCall call, RexUnknownAs unknownAs) {
switch (a.getKind()) {
case LITERAL:
return rexBuilder.makeLiteral(!((RexLiteral) a).isNull());
+ case DIVIDE: {
+ RexNode op0 = ((RexCall) a).getOperands().get(0);
+ RexNode op1 = ((RexCall) a).getOperands().get(1);
+ if (RexUtil.isNull(op0) || RexUtil.isNull(op1)) {
+ return rexBuilder.makeLiteral(false);
+ }
+ if (!op1.isA(SqlKind.LITERAL)) {
+ return rexBuilder.makeCall(
+ SqlStdOperatorTable.IS_NOT_NULL, simplifyGenericNode((RexCall) a));
+ }
+ if (checkLiteralValue(op1, BigDecimal.ZERO)) {
+ return rexBuilder.makeCall(
+ SqlStdOperatorTable.IS_NOT_NULL, simplifyGenericNode((RexCall) a));
+ }
+ // op1 is a non-null and non-zero literal, so simplify op0
+ return simplifyIsNotNull(op0);
+ }
default:
- throw new AssertionError("every CUSTOM policy needs a handler, "
- + a.getKind());
+ return null;
Review Comment:
why change this?
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7145] RexSimplify produces an incorrect result for IS NULL(10/0) [calcite]
Posted by "thomasrebele (via GitHub)" <gi...@apache.org>.
thomasrebele commented on code in PR #4505:
URL: https://github.com/apache/calcite/pull/4505#discussion_r2485727973
##########
core/src/main/java/org/apache/calcite/rex/RexSimplify.java:
##########
@@ -488,9 +493,13 @@ private RexNode simplifyMultiply(RexCall e) {
}
private RexNode simplifyDivide(RexCall e) {
- final int oneIndex = findLiteralIndex(e.operands, BigDecimal.ONE);
- if (oneIndex == 1) {
- RexNode leftOperand = e.getOperands().get(0);
+ RexNode leftOperand = e.getOperands().get(0);
+ RexNode rightOperand = e.getOperands().get(1);
+ if ((isSafeExpression(leftOperand) && STRONG.isNull(leftOperand))
Review Comment:
The leftOperand might contain a sub-expression throwing an exception while `STRONG.isNull(leftOperand)` might return true. Compare `RexSimplify#simplify(RexNode, RexUnknownAs)`.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7145] RexSimplify produces an incorrect result for IS NULL(10/0) [calcite]
Posted by "thomasrebele (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7145] RexSimplify produces an incorrect result for IS NULL(10/0) [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #4505:
URL: https://github.com/apache/calcite/pull/4505#discussion_r2298609160
##########
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:
why does the nullability of the result depend on the argument being a literal?
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7145] RexSimplify produces an incorrect result for IS NULL(10/0) [calcite]
Posted by "thomasrebele (via GitHub)" <gi...@apache.org>.
thomasrebele commented on code in PR #4505:
URL: https://github.com/apache/calcite/pull/4505#discussion_r2298665691
##########
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:
If I understand Julian's comment correctly, division by 0 should throw an exception, so IS NULL(10/0) may not be simplified. However, IS NULL(10/1) may be simplified to false.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7145] RexSimplify produces an incorrect result for IS NULL(10/0) [calcite]
Posted by "thomasrebele (via GitHub)" <gi...@apache.org>.
thomasrebele closed pull request #4505: [CALCITE-7145] RexSimplify produces an incorrect result for IS NULL(10/0)
URL: https://github.com/apache/calcite/pull/4505
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7145] RexSimplify produces an incorrect result for IS NULL(10/0) [calcite]
Posted by "thomasrebele (via GitHub)" <gi...@apache.org>.
thomasrebele commented on PR #4505:
URL: https://github.com/apache/calcite/pull/4505#issuecomment-3541237955
Closing this in favor of https://github.com/apache/calcite/pull/4616.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7145] RexSimplify produces an incorrect result for IS NULL(10/0) [calcite]
Posted by "zabetak (via GitHub)" <gi...@apache.org>.
zabetak commented on code in PR #4505:
URL: https://github.com/apache/calcite/pull/4505#discussion_r2485912671
##########
core/src/main/java/org/apache/calcite/rex/RexSimplify.java:
##########
@@ -1189,6 +1102,23 @@ private RexNode simplifyIs(RexCall call, RexUnknownAs unknownAs) {
}
}
return RexUtil.composeDisjunction(rexBuilder, operands, false);
+ case CUSTOM:
+ if (a.getKind() == SqlKind.DIVIDE) {
+ RexNode op0 = ((RexCall) a).getOperands().get(0);
+ RexNode op1 = ((RexCall) a).getOperands().get(1);
+ if (RexUtil.isNull(op0) || RexUtil.isNull(op1)) {
+ return rexBuilder.makeLiteral(true);
+ }
+ if (!op1.isA(SqlKind.LITERAL)) {
+ return rexBuilder.makeCall(SqlStdOperatorTable.IS_NULL, simplifyGenericNode((RexCall) a));
+ }
+ if (checkLiteralValue(op1, BigDecimal.ZERO)) {
+ return rexBuilder.makeCall(SqlStdOperatorTable.IS_NULL, simplifyGenericNode((RexCall) a));
+ }
+ // op1 is a non-null and non-zero literal, so simplify op0
+ return simplifyIsNull(op0);
+ }
+ return null;
Review Comment:
The code handling the division here is very similar to the code inside `simplifyIsNotNull` so I am wondering if it's worth refactoring it into a separate private method.
##########
core/src/main/java/org/apache/calcite/rex/RexSimplify.java:
##########
@@ -1140,9 +1037,23 @@ private RexNode simplifyIs(RexCall call, RexUnknownAs unknownAs) {
switch (a.getKind()) {
case LITERAL:
return rexBuilder.makeLiteral(!((RexLiteral) a).isNull());
+ case DIVIDE: {
+ RexNode op0 = ((RexCall) a).getOperands().get(0);
+ RexNode op1 = ((RexCall) a).getOperands().get(1);
+ if (RexUtil.isNull(op0) || RexUtil.isNull(op1)) {
+ return rexBuilder.makeLiteral(false);
+ }
Review Comment:
If we handle simplifications of arithmetic operations with nulls this code seems redundant and it might be unreachable. For instance, if `simplifyDivide` is able to transform `x/null` and `null/y` to `null` then I guess we cannot hit this path.
##########
core/src/main/java/org/apache/calcite/plan/Strong.java:
##########
@@ -356,7 +358,8 @@ private static Map<SqlKind, Policy> createPolicyMap() {
map.put(SqlKind.CHECKED_TIMES, Policy.ANY);
map.put(SqlKind.CHECKED_DIVIDE, Policy.ANY);
Review Comment:
Wondering if we should also handle `CHECKED_DIVIDE` with the same custom policy.
##########
core/src/main/java/org/apache/calcite/rex/RexSimplify.java:
##########
@@ -488,9 +493,13 @@ private RexNode simplifyMultiply(RexCall e) {
}
private RexNode simplifyDivide(RexCall e) {
- final int oneIndex = findLiteralIndex(e.operands, BigDecimal.ONE);
- if (oneIndex == 1) {
- RexNode leftOperand = e.getOperands().get(0);
+ RexNode leftOperand = e.getOperands().get(0);
+ RexNode rightOperand = e.getOperands().get(1);
+ if ((isSafeExpression(leftOperand) && STRONG.isNull(leftOperand))
Review Comment:
This appears to be somewhat "new" simplification logic that was not there before. I get the feeling that simplifications of arithmetic operations with nulls is a problem that we should tackle in a more general fashion and not only for the division operator. From my point of view this deserves a separate JIRA ticket and PR dedicated to simplification of arithmetic operations with nulls. I see there is already some code doing this for comparison operators (`simplifyComparisonWithNull`).
##########
core/src/main/java/org/apache/calcite/rex/RexSimplify.java:
##########
@@ -1028,9 +1037,25 @@ private RexNode simplifyIs(RexCall call, RexUnknownAs unknownAs) {
switch (a.getKind()) {
case LITERAL:
return rexBuilder.makeLiteral(!((RexLiteral) a).isNull());
+ case DIVIDE: {
+ RexNode op0 = ((RexCall) a).getOperands().get(0);
+ RexNode op1 = ((RexCall) a).getOperands().get(1);
+ if (RexUtil.isNull(op0) || RexUtil.isNull(op1)) {
+ return rexBuilder.makeLiteral(false);
+ }
+ if (!op1.isA(SqlKind.LITERAL)) {
+ return rexBuilder.makeCall(
+ SqlStdOperatorTable.IS_NOT_NULL, simplifyGenericNode((RexCall) a));
+ }
+ if (checkLiteralValue(op1, BigDecimal.ZERO)) {
+ return rexBuilder.makeCall(
+ SqlStdOperatorTable.IS_NOT_NULL, simplifyGenericNode((RexCall) a));
+ }
+ // op1 is a non-null and non-zero literal, so simplify op0
+ return simplifyIsNotNull(op0);
+ }
default:
- throw new AssertionError("every CUSTOM policy needs a handler, "
- + a.getKind());
+ return null;
Review Comment:
Actually, I suggested to not add a **new** Assertion in `simplifyIsNull` method to avoid breaking changes. Removing the assertion from here for consistency purposes makes sense but I wouldn't push hard on it especially if others disagree.
##########
core/src/main/java/org/apache/calcite/rex/RexSimplify.java:
##########
@@ -415,17 +415,22 @@ private RexNode simplifyGenericNode(RexCall e) {
*/
private static int findLiteralIndex(List<RexNode> operands, BigDecimal value) {
for (int i = 0; i < operands.size(); i++) {
- if (operands.get(i).isA(SqlKind.LITERAL)) {
- Comparable comparable = ((RexLiteral) operands.get(i)).getValue();
- if (comparable instanceof BigDecimal
- && value.compareTo((BigDecimal) comparable) == 0) {
- return i;
- }
+ if (checkLiteralValue(operands.get(i), value)) {
+ return i;
}
}
return -1;
}
+ /** Check whether the operand is a literal of the specified value. */
Review Comment:
It seems that this method will always return false for approximate numeric (DOUBLE, FLOAT, REAL) literals since `comparable instanceof BigDecimal` is `false`.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7145] RexSimplify produces an incorrect result for IS NULL(10/0) [calcite]
Posted by "thomasrebele (via GitHub)" <gi...@apache.org>.
thomasrebele commented on code in PR #4505:
URL: https://github.com/apache/calcite/pull/4505#discussion_r2486635258
##########
core/src/main/java/org/apache/calcite/rex/RexSimplify.java:
##########
@@ -1140,9 +1037,23 @@ private RexNode simplifyIs(RexCall call, RexUnknownAs unknownAs) {
switch (a.getKind()) {
case LITERAL:
return rexBuilder.makeLiteral(!((RexLiteral) a).isNull());
+ case DIVIDE: {
+ RexNode op0 = ((RexCall) a).getOperands().get(0);
+ RexNode op1 = ((RexCall) a).getOperands().get(1);
+ if (RexUtil.isNull(op0) || RexUtil.isNull(op1)) {
+ return rexBuilder.makeLiteral(false);
+ }
Review Comment:
Indeed, my 2nd commit for this PR made the check superfluous. I'll remove it.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7145] RexSimplify produces an incorrect result for IS NULL(10/0) [calcite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #4505:
URL: https://github.com/apache/calcite/pull/4505#issuecomment-3221084827
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4505) **Quality Gate passed**
Issues
 [2 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4505&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4505&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=4505&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [84.1% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4505&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4505&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4505)
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7145] RexSimplify produces an incorrect result for IS NULL(10/0) [calcite]
Posted by "thomasrebele (via GitHub)" <gi...@apache.org>.
thomasrebele commented on code in PR #4505:
URL: https://github.com/apache/calcite/pull/4505#discussion_r2298725388
##########
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:
I've understood the "Returns whether an expression is definitely null." as "If the expression is always null, return true; if the expression might evaluate to any non-null value, return false".
The code that I've added here does consider a division to be null in certain cases: the second operand being null, or (the second operand being a non-zero literal and the first operand being null).
I've added the handling because of `checkSimplify(div(a, nullInt), "null:INTEGER")` (with a non-null integer `a`) in RexProgramTest#testSimplifySimpleArithmetic. It might make more sense to handle this in `org.apache.calcite.rex.RexSimplify#simplifyDivide`.
Assuming the Strong.Policy of the DIVIDE operator will be set to CUSTOM, as proposed in this PR: We could delete the DIVIDE block here, but then DIVIDE will fall-back to the default case and will never be considered null by Strong.isNull (even if the operands are 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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7145] RexSimplify produces an incorrect result for IS NULL(10/0) [calcite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #4505:
URL: https://github.com/apache/calcite/pull/4505#issuecomment-3480633456
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4505) **Quality Gate passed**
Issues
 [2 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4505&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4505&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=4505&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [85.7% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4505&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4505&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4505)
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7145] RexSimplify produces an incorrect result for IS NULL(10/0) [calcite]
Posted by "zabetak (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7145] RexSimplify produces an incorrect result for IS NULL(10/0) [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #4505:
URL: https://github.com/apache/calcite/pull/4505#discussion_r2298677785
##########
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:
The JavaDoc on this function says nothing about exceptions, so neither of them should be considered 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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7145] RexSimplify produces an incorrect result for IS NULL(10/0) [calcite]
Posted by "zabetak (via GitHub)" <gi...@apache.org>.
zabetak commented on code in PR #4505:
URL: https://github.com/apache/calcite/pull/4505#discussion_r2487071007
##########
core/src/main/java/org/apache/calcite/rex/RexSimplify.java:
##########
@@ -488,9 +493,13 @@ private RexNode simplifyMultiply(RexCall e) {
}
private RexNode simplifyDivide(RexCall e) {
- final int oneIndex = findLiteralIndex(e.operands, BigDecimal.ONE);
- if (oneIndex == 1) {
- RexNode leftOperand = e.getOperands().get(0);
+ RexNode leftOperand = e.getOperands().get(0);
+ RexNode rightOperand = e.getOperands().get(1);
+ if ((isSafeExpression(leftOperand) && STRONG.isNull(leftOperand))
Review Comment:
OK, I checked the code in `RexSimplify#simplify(RexNode, RexUnknownAs)` and I realized that we handle arithmetic with nulls there. However, I feel that `NULL / y` to `NULL` is generally a "missed" simplification and not really relevant to the denominator being zero or not.
I am a bit puzzled with the changes here cause there is a bit of overlap with the code in [SafeRexVisitor#visitCall](https://github.com/apache/calcite/blob/67e5bfcdc0151aa1926fa369382d12fc672ca5bf/core/src/main/java/org/apache/calcite/rex/RexSimplify.java#L1559). It seems that the case `X / NULL` is handled via the `SafeRexVisitor` while `NULL / X` is handled here. Shouldn't we handle both cases with the same code? Which is the best place to do it?
##########
core/src/main/java/org/apache/calcite/rex/RexSimplify.java:
##########
@@ -1189,6 +1102,23 @@ private RexNode simplifyIs(RexCall call, RexUnknownAs unknownAs) {
}
}
return RexUtil.composeDisjunction(rexBuilder, operands, false);
+ case CUSTOM:
+ if (a.getKind() == SqlKind.DIVIDE) {
+ RexNode op0 = ((RexCall) a).getOperands().get(0);
+ RexNode op1 = ((RexCall) a).getOperands().get(1);
+ if (RexUtil.isNull(op0) || RexUtil.isNull(op1)) {
+ return rexBuilder.makeLiteral(true);
+ }
+ if (!op1.isA(SqlKind.LITERAL)) {
+ return rexBuilder.makeCall(SqlStdOperatorTable.IS_NULL, simplifyGenericNode((RexCall) a));
+ }
+ if (checkLiteralValue(op1, BigDecimal.ZERO)) {
+ return rexBuilder.makeCall(SqlStdOperatorTable.IS_NULL, simplifyGenericNode((RexCall) a));
+ }
+ // op1 is a non-null and non-zero literal, so simplify op0
+ return simplifyIsNull(op0);
+ }
+ return null;
Review Comment:
I was mainly referring to the new code that is added but if we want to do something more ambitious indeed we can post-pone to CALCITE-7263.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7145] RexSimplify produces an incorrect result for IS NULL(10/0) [calcite]
Posted by "thomasrebele (via GitHub)" <gi...@apache.org>.
thomasrebele commented on code in PR #4505:
URL: https://github.com/apache/calcite/pull/4505#discussion_r2485708521
##########
core/src/main/java/org/apache/calcite/plan/Strong.java:
##########
@@ -356,7 +358,8 @@ private static Map<SqlKind, Policy> createPolicyMap() {
map.put(SqlKind.CHECKED_TIMES, Policy.ANY);
map.put(SqlKind.CHECKED_DIVIDE, Policy.ANY);
- map.put(SqlKind.DIVIDE, Policy.ANY);
+ // DIVIDE behaves like ANY. However, (x/0) IS (NOT) NULL may not be simplified.
Review Comment:
What I meant is this: DIVIDE is null iff any of its arguments is null. If the divisor is zero we get an exception. If we simplify an `IS NULL (x/0)` the simplified expression would not throw the exception anymore. The goal of the ticket was to fix this. It is not related to other dialects.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7145] RexSimplify produces an incorrect result for IS NULL(10/0) [calcite]
Posted by "xiedeyantu (via GitHub)" <gi...@apache.org>.
xiedeyantu commented on PR #4505:
URL: https://github.com/apache/calcite/pull/4505#issuecomment-3419751411
Hi @thomasrebele, @zabetak made some suggestions for revisions. Are you still working on this PR?
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7145] RexSimplify produces an incorrect result for IS NULL(10/0) [calcite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #4505:
URL: https://github.com/apache/calcite/pull/4505#issuecomment-3221084281
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4505) **Quality Gate passed**
Issues
 [2 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4505&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4505&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=4505&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [84.1% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4505&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4505&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4505)
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7145] RexSimplify produces an incorrect result for IS NULL(10/0) [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on PR #4505:
URL: https://github.com/apache/calcite/pull/4505#issuecomment-3481291326
I think the right way to solve this problem is to add a new operation SAFE_DIVIDE whose semantics is that a 0 denominator produces a NULL result. Whether / is compiled into DIVIDE or SAFE_DIVIDE could be a configuration option in the type system, similar to the CHECKED arithmetic option. This comment should probably be in the JIRA, I will paste it there too.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7145] RexSimplify produces an incorrect result for IS NULL(10/0) [calcite]
Posted by "thomasrebele (via GitHub)" <gi...@apache.org>.
thomasrebele commented on code in PR #4505:
URL: https://github.com/apache/calcite/pull/4505#discussion_r2486496978
##########
core/src/main/java/org/apache/calcite/rex/RexSimplify.java:
##########
@@ -415,17 +415,22 @@ private RexNode simplifyGenericNode(RexCall e) {
*/
private static int findLiteralIndex(List<RexNode> operands, BigDecimal value) {
for (int i = 0; i < operands.size(); i++) {
- if (operands.get(i).isA(SqlKind.LITERAL)) {
- Comparable comparable = ((RexLiteral) operands.get(i)).getValue();
- if (comparable instanceof BigDecimal
- && value.compareTo((BigDecimal) comparable) == 0) {
- return i;
- }
+ if (checkLiteralValue(operands.get(i), value)) {
+ return i;
}
}
return -1;
}
+ /** Check whether the operand is a literal of the specified value. */
Review Comment:
I've changed the javadoc to mention BigDecimal.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7145] RexSimplify produces an incorrect result for IS NULL(10/0) [calcite]
Posted by "thomasrebele (via GitHub)" <gi...@apache.org>.
thomasrebele commented on code in PR #4505:
URL: https://github.com/apache/calcite/pull/4505#discussion_r2486180717
##########
core/src/main/java/org/apache/calcite/rex/RexSimplify.java:
##########
@@ -1028,9 +1037,25 @@ private RexNode simplifyIs(RexCall call, RexUnknownAs unknownAs) {
switch (a.getKind()) {
case LITERAL:
return rexBuilder.makeLiteral(!((RexLiteral) a).isNull());
+ case DIVIDE: {
+ RexNode op0 = ((RexCall) a).getOperands().get(0);
+ RexNode op1 = ((RexCall) a).getOperands().get(1);
+ if (RexUtil.isNull(op0) || RexUtil.isNull(op1)) {
+ return rexBuilder.makeLiteral(false);
+ }
+ if (!op1.isA(SqlKind.LITERAL)) {
+ return rexBuilder.makeCall(
+ SqlStdOperatorTable.IS_NOT_NULL, simplifyGenericNode((RexCall) a));
Review Comment:
Indeed, `a` should already be simplified. I vaguely remember that I arrived at the makeCall with a not-simplified `a`. As I cannot reproduce it anymore, I'll remove the call to `simplifyGenericNode`.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7145] RexSimplify produces an incorrect result for IS NULL(10/0) [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #4505:
URL: https://github.com/apache/calcite/pull/4505#discussion_r2487181220
##########
core/src/main/java/org/apache/calcite/plan/Strong.java:
##########
@@ -356,7 +358,8 @@ private static Map<SqlKind, Policy> createPolicyMap() {
map.put(SqlKind.CHECKED_TIMES, Policy.ANY);
map.put(SqlKind.CHECKED_DIVIDE, Policy.ANY);
- map.put(SqlKind.DIVIDE, Policy.ANY);
+ // DIVIDE behaves like ANY. However, (x/0) IS (NOT) NULL may not be simplified.
Review Comment:
The change I am proposing is the most general explanation of what is happening here.
Code comments should generally not be related to bugs fixed; bugs go away, but the code stays.
Test comments can depend on bugs fixed.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7145] RexSimplify produces an incorrect result for IS NULL(10/0) [calcite]
Posted by "thomasrebele (via GitHub)" <gi...@apache.org>.
thomasrebele commented on PR #4505:
URL: https://github.com/apache/calcite/pull/4505#issuecomment-3435830560
Hi @xiedeyantu, currently I'm busy with another task. I try to pick up this PR soon again.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org