jaystarshot commented on a change in pull request #2598:
URL: https://github.com/apache/calcite/pull/2598#discussion_r744423629
##########
File path:
core/src/main/java/org/apache/calcite/sql2rel/StandardConvertletTable.java
##########
@@ -308,12 +308,20 @@ private static RexNode convertNvl(SqlRexContext cx,
SqlCall call) {
cx.convertExpression(call.getOperandList().get(1));
final RelDataType type =
cx.getValidator().getValidatedNodeType(call);
+ // Preserve Operand Nullability
return rexBuilder.makeCall(type, SqlStdOperatorTable.CASE,
ImmutableList.of(
rexBuilder.makeCall(SqlStdOperatorTable.IS_NOT_NULL,
operand0),
- rexBuilder.makeCast(type, operand0),
- rexBuilder.makeCast(type, operand1)));
+ rexBuilder.makeCast(
+ cx.getTypeFactory()
+ .createTypeWithNullability(type,
operand0.getType().isNullable()),
+ operand0),
+ rexBuilder.makeCast(
+ cx.getTypeFactory()
Review comment:
@chunweilei
1) Consider If that operand0 depends on an input reference(which is nullable)
2) If in future that operand pushed down like in
[ProjectJoinTransposeRule](https://github.com/apache/calcite/blob/bebe473fab2e242736614659ed6e5d04eeeb8bf5/core/src/main/java/org/apache/calcite/rel/rules/ProjectJoinTransposeRule.java),
there will be an issue as the operand0 and its reference have different
nullability.
3) The output of the NVL operation is Not Null but there is no need to
change the nullability of the operands as in calcite rules we might push the
operands down in some rules causing errors ( operand referenced input type is
nullable whereas we forced the operand to be not Null)
Also I agree that NVL output should be not Nullable but that does not mean
that operand0 should be not nullable as operand0 is not the output of the NVL
function (its just an operand).
TLDR: My point is to not force nullability on the input operands by the
output of the NVL operation as by definition the function NVL(a, b) only
outputs b if a is NULL, so it should handle output Nullability accordingly
however it should not change the nullability of a as a might be a reference
field which is indeed nullable.
Do you think hence it makes sense to let the operand nullability what it was
as the output is anyway not nullable?
--
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]
