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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
