zabetak commented on code in PR #4616:
URL: https://github.com/apache/calcite/pull/4616#discussion_r2522688958
##########
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 agree it's better to be more conservative.
##########
core/src/main/java/org/apache/calcite/rex/RexSimplify.java:
##########
@@ -1231,7 +1230,7 @@ private RexNode simplifyIs(RexCall call, RexUnknownAs
unknownAs) {
return RexUtil.composeDisjunction(rexBuilder, operands, false);
case AS_IS:
default:
- return null;
+ return rexBuilder.makeCall(SqlStdOperatorTable.IS_NULL, a);
Review Comment:
Instead of bailing out now the method now returns a different potentially
simplified expression. In addition, the way that we handle casts and perform
the simplifications has also changed and as observed by the tests it has impact
on expressions that have nothing to do with the division operator. The division
topic has captured lots of discussion so to keep the PR minimal I would suggest
to commit the improvements in `simplifyIsNotNull` and `simplifyIsNull` (that
are not strictly related to division) to another JIRA/PR.
##########
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:
Thanks for the explanation. It makes sense to extend the support for
verifying errors/exceptions but let's do it in a separate JIRA/PR. I guess we
may also be able to turn paranoid to `true` for various existing test cases so
that would be a more complete fix.
##########
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 performance impact of verifying every single expression is significant
so this "feature" is unlikely to be used in production. Moreover, production
code should never throw an `AssertionError` to begin with. Anyways, I don't
feel that strongly about it so we can keep the original version.
##########
core/src/test/resources/sql/sub-query.iq:
##########
@@ -1472,7 +1472,7 @@ EnumerableCalc(expr#0..3=[{inputs}],
expr#4=[null:BOOLEAN], expr#5=[IS NOT NULL(
# Test project null IN nullable
select sal,
cast(null as int) IN (
- select case when true then deptno else null end
+ select case when deptno>0 then deptno else null end
Review Comment:
When we are dealing with end-to-end tests (like .iq files) it's rather
hard/impossible to keep the original intent of the test intact across
revisions. Changing the query is also changing the test to some extend so in
many cases the most reasonable option is to just update the plan.
As far as I see the changes done here for the queries do not retain the
original plan but just lead to different variants. If we kept the original
queries the diff on this file would be probably smaller and the changes easier
to reason about in the context of this (or another) PR.
Personally, I would prefer to leave the queries in .iq files as is. If you
think that we may miss test coverage I would rather invest in adding unit tests
(or new end-to-end tests) around this area rather than modifying existing 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]
