[CALCITE-4377] Fix the return nullability inference during rex simpli…#2246
[CALCITE-4377] Fix the return nullability inference during rex simpli…#2246danny0405 wants to merge 1 commit into
Conversation
e8e478e to
4eb6c60
Compare
|
Thanks, it looks ok. Would you please remove |
4eb6c60 to
84a9154
Compare
84a9154 to
53d54da
Compare
No, there are still some NPEs reported from the |
| // Ignores the nullability change when all the operands are literals | ||
| // which come from the simplification. | ||
| assert operands.stream().map(RexNode::getType).anyMatch(RelDataType::isNullable) | ||
| || operands.stream().allMatch(p -> RexUtil.isLiteral(p, false)); |
There was a problem hiding this comment.
This looks strange, and it contradicts Policy.ALL definition
This kind of expression is null if and only if at least one of its arguments is null
What do literals do here?
There was a problem hiding this comment.
During the simplification, we drop the useless cast cast(1 as nullable INT) which breaks the nullability. We have 2 choices:
- Do not make any simplification that breaks the nullability
- Make the validation strategy more relaxed
I choose the latter.
There was a problem hiding this comment.
See https://issues.apache.org/jira/browse/CALCITE-4387
Simplify should be allowed to both widen and narrowing nullability.
I believe we must not alter the definition of nullness policyhere.
There was a problem hiding this comment.
The nullness policy is always there, we should make the validation adapter to the simplification.
There was a problem hiding this comment.
case ANY is defined as return type is nullable if and only if one of the arguments is null.
It allows no exceptions like "ok, the return type might be null if one of the arguments is literal".
That is why I believe RexUtil.isLiteral must not be here.
There was a problem hiding this comment.
- Assertion at line 907 must be kept intact.
There was a problem hiding this comment.
Then we should never use ReduceExpressionsRule.Config.matchNullability, it should always return true.
I can not figure out a way to fix if we keep the assertion at line 907 as it is.
There was a problem hiding this comment.
rexBuilder.makeCall(e.getType(), e.getOperator(), operands); above must be updated to infer the proper type instead of e.getType().
PS. I have already done all of that in #2250 a week or two ago
There was a problem hiding this comment.
rexBuilder.makeCall(e.getType(), e.getOperator(), operands); above must be updated to infer the proper type instead of e.getType().
The problem is that it is hard to keep node type equivalence during simplification anymore, and the ReduceExpressionsRule.Config.matchNullability becomes meaningless.
There was a problem hiding this comment.
ReduceExpressionsRule.Config.matchNullability refers to the nullability of the overall expression rather than the nullability of the internal nodes.
3 participants
…fication