zabetak commented on code in PR #4616:
URL: https://github.com/apache/calcite/pull/4616#discussion_r2498433287
##########
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:
I checked a bit the doc of the `CHECKED_DIVIDE` and it seems that it can
throw an error differently than DIVIDE (on overflow) so it's probably incorrect
to use the same safety derivation logic. Let's consider this operator unsafe at
the moment and remove it from here.
##########
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 am not sure if the bail out condition due to safety should be put here or
deeper down this method.
Should the safety check prevail the following:
* losslessCast (CAST is unsafe but is this simplification wrong that we
should cancel it?)
* pulledUpPredicates (Similar question is it problematic to infer that an
unsafe expression is true if we already know that some predicates hold?)
* custom nullability rules (This is pretty cheap so should we prioritize it?)
##########
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 think that before claiming that the operation is safe we should ensure
that the other operand is safe.
Depending on the database we may not want to simplify the following to null:
```
null / CAST (fname AS INTEGER)
CAST (fname AS INTEGER) / null
```
In the above, I assume that CAST throws an error.
Can we add these test cases if they don't exist alread?
##########
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:
From a quick look this appears to be redundant. Isn't the logic in `RexNode
simplify(RexNode e, RexUnknownAs unknownAs)` sufficient to handle the
simplification?
##########
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:
How about the following and leaving the rest unchanged?
```suggestion
private Comparable evaluate(RexNode e, Map<RexNode, Comparable> map) {
try {
return RexInterpreter.evaluate(e, map);
} catch (RuntimeException ex) {
return ex.getClass().getSimpleName();
}
}
```
It's not super precise but since this mostly for testing purposes it might
be sufficient.
##########
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:
Are the changes here needed/used in this PR? If not we could move it to a
separate ticket and additionally check if we could change paranoid to true in
those test cases that it's disabled.
##########
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:
Is this used?
##########
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:
Instead of disabling, I would simply update the plan since this can be seen
as bug fix.
##########
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:
If we check the safety above we can remove the check from here since it will
always true at this stage.
##########
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:
Special case of the above so somewhat redundant. In addition, it appears
more like a test case for constant folding rather than simplifications.
##########
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:
Remove?
##########
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:
Let's consider this out-of-scope and remove the tests.
--
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]
