[CALCITE-6534] Adjust type when pulling up Calc in JoinUnifyRule#3921
[CALCITE-6534] Adjust type when pulling up Calc in JoinUnifyRule#3921mihaibudiu merged 1 commit intoapache:mainfrom
Conversation
suibianwanwank
force-pushed
the
6528-null-if
branch
from
4b0f825 to
2233a96
Compare
mihaibudiu
left a comment
mihaibudiu
left a comment
There was a problem hiding this comment.
I personally I am not very familiar with this part of the codebase so I am not confident enough to approve. Based on the tests, this seems to work very well, though.
The code looks generally fine, I left most comments that could improve readability.
| @Test void testLeftProjectOnRightJoinToJoinFailed() { | ||
| * Assertion Error in JoinUnifyRule Due to Type Mismatch</a>. | ||
| * | ||
| * <p>Originally, this test was noMat due to a type mismatch. |
There was a problem hiding this comment.
I don't think this kind of comment is useful long term. It is useful for the reviewers of this PR, but not for maintaining the code. This comment would be better as a github comment rather than a code comment. Same for the subsequent comments.
| private static boolean allProjectsNullable(List<RexNode> projects) { | ||
| return projects.stream() | ||
| .allMatch(project -> project.getType().isNullable()); | ||
| /** Cast RexNode to the given type if only nullability differs. */ |
There was a problem hiding this comment.
I would also add "Otherwise throw". The current comment implies that in all other cases the type is left unchanged.
| return true; | ||
| } | ||
|
|
||
| /** Determines if all projects are strong and the condition is always true. */ |
There was a problem hiding this comment.
not clear what the "projects" are and what the condition is given single input parameter. Maybe you can add @param to describe where inputExplained comes from.
| if (rightProjects != null && joinType.generatesNullsOn(1) | ||
| && !allProjectsNullable(rightProjects)) { | ||
| return false; | ||
| private static List<RexNode> shiftAndAdjustProjectExpr(MutableJoin query, MutableJoin target, |
There was a problem hiding this comment.
I think JavaDoc at least for some parameters could also help.
In particular, what is the target?
| } | ||
|
|
||
| // In cases such as JoinOnLeftCalcToJoinUnifyRule and JoinOnCalcsToJoinUnifyRule rules, | ||
| // the left calc need to be pulled up the target, initialize the converter. |
There was a problem hiding this comment.
should this be "where the left calc needs to be pulled up"?
|
@mihaibudiu Thanks for the review, this function both shift and type adjustments, and I think it's really not easy to understand, so I've optimized those comments and added more of them. |
| * {@link JoinOnCalcsToJoinUnifyRule} <br/> | ||
| * | ||
| * @param query MutableRel of query | ||
| * @param target MutableRel of target |
There was a problem hiding this comment.
This information can be already obtained from the parameter declaration, the question I had is "what is the target?" Is the target the place where you are building the modified query?
There was a problem hiding this comment.
The naming here comes from UnifyRuleCall, UnifyRule is bottom-down matching, target means the RelNode of the mv that is trying to match.
If you're interested in this part, Refinement for Substitution-Based Materialized View Matching is a great resource
There was a problem hiding this comment.
Ideally that google doc would be posted somewhere publicly and linked from the code here.
There was a problem hiding this comment.
Sounds good, it can be done in other issue
mihaibudiu
added
the
LGTM-will-merge-soon
|
You can squash the commits for merging |
suibianwanwank
force-pushed
the
6528-null-if
branch
from
b6f813d to
d8ebdf8
Compare
Done✅ |
|
2 participants
This PR addresses both [CALCITE-6534] and [CALCITE-6528] and is based on [CALCITE-6501]. For the problem of nullablility due to Join null-side, if RexNode satisfies null-if-null, we will adjust the nullability, if not, we should reject this rewrite regardless of the nullablility.