thomasrebele commented on code in PR #4616:
URL: https://github.com/apache/calcite/pull/4616#discussion_r2510472803
##########
core/src/main/java/org/apache/calcite/rex/RexSimplify.java:
##########
@@ -1142,6 +1151,9 @@ private RexNode simplifyIs(RexCall call, RexUnknownAs
unknownAs) {
// "(CASE WHEN FALSE THEN 1 ELSE 2) IS NOT NULL" we first simplify the
// argument to "2", and only then we can simplify "2 IS NOT NULL" to
"TRUE".
a = simplify(a, UNKNOWN);
+ if (!isSafeExpression(a)) {
+ return rexBuilder.makeCall(SqlStdOperatorTable.IS_NOT_NULL, a);
+ }
if (!a.getType().isNullable() && isSafeExpression(a)) {
Review Comment:
Indeed, fixed.
##########
core/src/main/java/org/apache/calcite/rex/RexSimplify.java:
##########
@@ -1549,17 +1563,23 @@ enum SafeRexVisitor implements RexVisitor<Boolean> {
SqlOperator sqlOperator = call.getOperator();
switch (sqlKind) {
+ case CHECKED_DIVIDE:
case DIVIDE:
case MOD:
List<RexNode> operands = call.getOperands();
- boolean isSafe = RexVisitorImpl.visitArrayAnd(this,
ImmutableList.of(operands.get(0)));
- if (!isSafe) {
+ boolean hasNullOperand = RexUtil.isNullLiteral(operands.get(0), true)
+ || RexUtil.isNullLiteral(operands.get(1), true);
+ if (hasNullOperand) {
+ return true;
Review Comment:
I've tried some popular DBMS with `null/CAST('a' as integer)` and
`null/(1/0)`:
It seems that
[Postgres](https://sqlfiddle.com/postgresql/online-compiler?id=30135a77-de99-4369-b222-58c563793ae9)
throws an exception for both.
[MySQL](https://sqlfiddle.com/mysql/online-compiler?id=d332d7f1-5fae-4d9c-a655-42a97af2900c)
behaves the same as Postgres. [SQL
Server](https://onecompiler.com/sqlserver/4448q6av9) as well. (To check the
other expression in the fiddle, the throwing one might need to be disabled by
commenting it.)
[SQLite](https://sqlfiddle.com/sqlite/online-compiler?id=15eb09f0-2ce0-4beb-8582-f9acf3199a4a)
throws an exception as well, but happily simplifies the similar `null/(1/0)`
to `null`.
[Oracle](https://sqlfiddle.com/oracle/online-compiler?id=91ac3bdd-58ca-409b-9efd-a4d41e0b96e8)
behaves the same as SQLite.
I guess we better be safe than sorry and only consider the division to be
safe if all operands are safe as well. I'll fix it.
##########
core/src/main/java/org/apache/calcite/rex/RexSimplify.java:
##########
@@ -1142,6 +1151,9 @@ private RexNode simplifyIs(RexCall call, RexUnknownAs
unknownAs) {
// "(CASE WHEN FALSE THEN 1 ELSE 2) IS NOT NULL" we first simplify the
// argument to "2", and only then we can simplify "2 IS NOT NULL" to
"TRUE".
a = simplify(a, UNKNOWN);
+ if (!isSafeExpression(a)) {
+ return rexBuilder.makeCall(SqlStdOperatorTable.IS_NOT_NULL, a);
+ }
Review Comment:
I'll rearrange the simplifyIsNotNull and simplifyIsNull methods.
##########
core/src/main/java/org/apache/calcite/rex/RexSimplify.java:
##########
@@ -1549,17 +1563,23 @@ enum SafeRexVisitor implements RexVisitor<Boolean> {
SqlOperator sqlOperator = call.getOperator();
switch (sqlKind) {
+ case CHECKED_DIVIDE:
Review Comment:
Seems that you've answered [your
question](https://github.com/apache/calcite/pull/4505#discussion_r2486530460)
whether we can handle CHECKED_DIVIDE the same way with "no". I thought as the
other checked operators have the same issue and are considered
["safe"](https://github.com/apache/calcite/blob/8b5c17e51e0c9c3f8e3db17c8d449e67e4e2974a/core/src/main/java/org/apache/calcite/rex/RexSimplify.java#L1503C8-L1506),
we could just do the same with CHECKED_DIVIDE. We could add another kind of
safeness, "simplifiable safe", where the simplification changes the behavior of
the expression (the original one throws, and the simplified one doesn't, and
vice versa).
I would suggest either we treat them all the same, and add CHECKED_DIVIDE
here, or we remove all the CHECKED operators in a later ticket.
It seems that you prefer the latter, so I'll just remove the CHECKED_DIVIDE.
##########
core/src/main/java/org/apache/calcite/rex/RexSimplify.java:
##########
@@ -2308,6 +2328,20 @@ private RexNode simplifyOrs(List<RexNode> terms,
RexUnknownAs unknownAs) {
return RexUtil.composeDisjunction(rexBuilder, terms);
}
+ private Pair<Comparable, RuntimeException> evaluate(RexNode e, Map<RexNode,
Comparable> map) {
+ Comparable c = null;
+ RuntimeException ex = null;
+ try {
+ c = RexInterpreter.evaluate(e, map);
+ } catch (RuntimeException exception) {
+ ex = exception;
+ }
+ if (c == null && ex == null) {
+ throw new AssertionError("interpreter returned null for " + e);
+ }
+ return Pair.of(c, ex);
+ }
Review Comment:
The code seems to be usable in production as well, so I prefer to keep the
longer but cleaner function.
##########
druid/src/test/java/org/apache/calcite/test/DruidAdapter2IT.java:
##########
@@ -2902,6 +2903,7 @@ private void testCountWithApproxDistinct(boolean approx,
String sql,
.returnsUnordered("EXPR$0=86829");
}
+ @Disabled("CALCITE-7271")
Review Comment:
As far as I understand Druid's SQL, `( cast(null as INTEGER) + cast("city"
as INTEGER)) IS NULL` should simplify to `true`. Changing the plan would
interfere with the intention of the test. I would expect Calcite to simplify
the expression using the Druid semantics. I would postpone the discussion to
CALCITE-7271 how this test case should be handled.
##########
core/src/test/java/org/apache/calcite/rex/RexProgramBuilderBase.java:
##########
@@ -336,6 +336,14 @@ protected RexNode div(RexNode n1, RexNode n2) {
return rexBuilder.makeCall(SqlStdOperatorTable.DIVIDE, n1, n2);
}
+ protected RexNode checkedDiv(RexNode n1, RexNode n2) {
+ return rexBuilder.makeCall(SqlStdOperatorTable.CHECKED_DIVIDE, n1, n2);
+ }
Review Comment:
Done.
##########
core/src/main/java/org/apache/calcite/rex/RexSimplify.java:
##########
@@ -490,9 +495,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))
+ || (isSafeExpression(rightOperand) && STRONG.isNull(rightOperand))) {
+ return rexBuilder.makeLiteral(null, e.getType());
+ }
Review Comment:
Indeed, I'll remove it.
##########
core/src/test/java/org/apache/calcite/rex/RexProgramTest.java:
##########
@@ -2715,42 +2715,94 @@ trueLiteral, literal(1),
* <a
href="https://issues.apache.org/jira/browse/CALCITE-7032";>[CALCITE-7032]
* Simplify 'NULL > ALL (ARRAY[1,2,NULL])' to 'NULL'</a>. */
@Test void testSimplifyDivideSafe() {
- // null + (a/0)/4
- // ==>
- // null + (a/0)/4
simplify = simplify.withParanoid(false);
+ // null + (a/0)/4 ==> null + (a/0)/4
+ // a/0 throws an exception so it may not be simplified
RexNode divideNode0 = plus(nullInt, div(div(vIntNotNull(), literal(0)),
literal(4)));
checkSimplifyUnchanged(divideNode0);
- // null + a/4
- // ==>
- // null + a/4
+ // null + a/4 ==> null
RexNode divideNode1 = plus(nullInt, div(vIntNotNull(), literal(4)));
- checkSimplifyUnchanged(divideNode1);
- // null + a/null
- // ==>
- // null
+ checkSimplify(divideNode1, "null:INTEGER");
+ // null + a/null ==> null
RexNode divideNode2 = plus(nullInt, div(vIntNotNull(), nullInt));
checkSimplify(divideNode2, "null:INTEGER");
- // null + null/0
- // ==>
- // null + null/0
- RexNode divideNode3 = plus(nullInt, div(vIntNotNull(), literal(0)));
- checkSimplifyUnchanged(divideNode3);
- // null + a/b
- // ==>
- // null + a/b
- RexNode divideNode4 = plus(nullInt, div(vIntNotNull(), vIntNotNull()));
+ // null + null/0 ==> null
+ // The SQL standard gives NULL a higher priority than division by 0.
+ // This is the same behavior as PostgreSQL, Oracle, SQLite, MariaDB, and
MySQL.
+ RexNode divideNode3 = plus(nullInt, div(nullInt, literal(0)));
+ checkSimplify(divideNode3, "null:INTEGER");
+ // null + a/0 ==> null + a/0
+ RexNode divideNode4 = plus(nullInt, div(vIntNotNull(), literal(0)));
checkSimplifyUnchanged(divideNode4);
+ // null + a/b ==> null + a/b
+ // b might be 0 and throw an exception, so a/b cannot be simplified.
+ // E.g., PostgreSQL throws a division-by-zero error for the following
query:
+ // SELECT NULL + (a/b) FROM (select 1 as a, 0 as b) t
+ RexNode divideNode5 = plus(nullInt, div(vIntNotNull(), vIntNotNull()));
+ checkSimplifyUnchanged(divideNode5);
+ }
+
+ /** Test cases for IS NULL(x/y).
+ * See <a
href="https://issues.apache.org/jira/browse/CALCITE-7145";>[CALCITE-7145]
+ * RexSimplify should not simplify IS NULL(10/0)</a>. */
+ @Test void testSimplifyIsNullDivide() {
+ 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");
+
+ checkSimplifyUnchanged(isNull(div(cast(literal(2), intType),
vIntNotNull())));
+ checkSimplifyUnchanged(isNull(div(vIntNotNull(), vIntNotNull())));
+ checkSimplify(isNull(div(nullInt, literal(0))), "true");
+ checkSimplify(isNull(div(literal(0), nullInt)), "true");
+
+ checkSimplifyUnchanged(isNotNull(div(vIntNotNull(), literal(0))));
+ checkSimplifyUnchanged(isNotNull(div(vIntNotNull(), cast(literal(0),
intType))));
+ checkSimplify(isNotNull(div(vIntNotNull(), cast(literal(2), intType))),
"true");
+
+ checkSimplifyUnchanged(isNotNull(div(cast(literal(2), intType),
vIntNotNull())));
+ checkSimplifyUnchanged(isNotNull(div(vIntNotNull(), vIntNotNull())));
+ checkSimplify(isNotNull(div(nullInt, literal(0))), "false");
+ checkSimplify(isNotNull(div(literal(0), nullInt)), "false");
+
+ checkSimplifyUnchanged(isNull(div(vDecimalNotNull(), literal(0))));
+ checkSimplify(
+ isNull(div(vDecimalNotNull(), cast(literal(BigDecimal.valueOf(2.5)),
intType))),
+ "false");
+ checkSimplify(isNull(div(nullDecimal, literal(BigDecimal.ZERO))), "true");
+
+ checkSimplifyUnchanged(isNotNull(div(vDecimalNotNull(), literal(0))));
+ checkSimplify(
+ isNotNull(div(vDecimalNotNull(),
cast(literal(BigDecimal.valueOf(2.5)), intType))),
+ "true");
+ checkSimplify(isNotNull(div(nullDecimal, literal(BigDecimal.ZERO))),
"false");
+
+ checkSimplifyUnchanged(isNull(checkedDiv(vDecimalNotNull(), literal(0))));
+ checkSimplify(
+ isNull(checkedDiv(vDecimalNotNull(),
cast(literal(BigDecimal.valueOf(2.5)), intType))),
+ "false");
+ checkSimplify(isNull(checkedDiv(nullDecimal, literal(BigDecimal.ZERO))),
"true");
+
+ checkSimplifyUnchanged(isNotNull(checkedDiv(vDecimalNotNull(),
literal(0))));
+ checkSimplify(
+ isNotNull(checkedDiv(vDecimalNotNull(),
cast(literal(BigDecimal.valueOf(2.5)), intType))),
+ "true");
+ checkSimplify(isNotNull(checkedDiv(nullDecimal,
literal(BigDecimal.ZERO))), "false");
Review Comment:
Done.
##########
core/src/main/java/org/apache/calcite/rex/RexSimplify.java:
##########
@@ -2338,14 +2372,28 @@ private void verify(RexNode before, RexNode simplified,
RexUnknownAs unknownAs)
continue assignment_loop;
}
}
- Comparable v0 = RexInterpreter.evaluate(foo0.e, map);
- if (v0 == null) {
- throw new AssertionError("interpreter returned null for " + foo0.e);
- }
- Comparable v1 = RexInterpreter.evaluate(foo1.e, map);
- if (v1 == null) {
- throw new AssertionError("interpreter returned null for " + foo1.e);
+ Pair<Comparable, RuntimeException> p0 = evaluate(foo0.e, map);
+ Pair<Comparable, RuntimeException> p1 = evaluate(foo1.e, map);
+ if (p0.right != null || p1.right != null) {
+ if (p0.right == null || p1.right == null) {
+ throw Util.first(p0.right, p1.right);
+ }
+ if (!p0.right.getClass().equals(p1.right.getClass())) {
+ AssertionError error = new AssertionError("exception class does not
match");
+ error.addSuppressed(p0.right);
+ error.addSuppressed(p1.right);
+ throw error;
+ }
+ if (!java.util.Objects.equals(p0.right.getMessage(),
p1.right.getMessage())) {
+ AssertionError error = new AssertionError("exception message does
not match");
+ error.addSuppressed(p0.right);
+ error.addSuppressed(p1.right);
+ throw error;
+ }
+ continue;
Review Comment:
In general `RexProgramTest` checks whether the simplified and original
expressions return the same value. I've modified
`RexProgramTest.testPushNotIntoCase()` so that the safety of the division is
the same as before. As this test uses the paranoid setting (default for all
tests in RexProgramTest), I've decided to expand the `verify` method to support
verifying the exceptions.
##########
core/src/test/java/org/apache/calcite/rex/RexProgramBuilderBase.java:
##########
@@ -336,6 +336,14 @@ protected RexNode div(RexNode n1, RexNode n2) {
return rexBuilder.makeCall(SqlStdOperatorTable.DIVIDE, n1, n2);
}
+ protected RexNode checkedDiv(RexNode n1, RexNode n2) {
+ return rexBuilder.makeCall(SqlStdOperatorTable.CHECKED_DIVIDE, n1, n2);
+ }
+
+ protected RexNode mod(RexNode n1, RexNode n2) {
+ return rexBuilder.makeCall(SqlStdOperatorTable.MOD, n1, n2);
+ }
Review Comment:
I've removed it.
##########
core/src/test/java/org/apache/calcite/rex/RexProgramTest.java:
##########
@@ -4248,7 +4300,11 @@ 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");
Review Comment:
I'll remove the `div(one, one)` check. FYI, [division by 1 is explicitly
handled by
simplification](https://github.com/apache/calcite/blob/8b5c17e51e0c9c3f8e3db17c8d449e67e4e2974a/core/src/main/java/org/apache/calcite/rex/RexSimplify.java#L493-L498).
--
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]
