-
Notifications
You must be signed in to change notification settings - Fork 13k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[FLINK-32831][table-planner] RuntimeFilterProgram should aware join type when looking for the build side #23216
Conversation
Hi @lsyldliu, could you help to review this PR when you are free? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wanglijie95 Thanks for your contribution, I left some comments.
...java/org/apache/flink/table/planner/plan/optimize/program/FlinkRuntimeFilterProgramTest.java
Show resolved
Hide resolved
...ain/java/org/apache/flink/table/planner/plan/optimize/program/FlinkRuntimeFilterProgram.java
Show resolved
Hide resolved
Thanks for review @lsyldliu , I 've addressed or replied your comments, PTAL. |
...java/org/apache/flink/table/planner/plan/optimize/program/FlinkRuntimeFilterProgramTest.java
Show resolved
Hide resolved
...urces/org/apache/flink/table/planner/plan/optimize/program/FlinkRuntimeFilterProgramTest.xml
Outdated
Show resolved
Hide resolved
45a88f8
to
9528691
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wanglijie95 Thanks for your contribution, I left some comments.
...ain/java/org/apache/flink/table/planner/plan/optimize/program/FlinkRuntimeFilterProgram.java
Show resolved
Hide resolved
...ain/java/org/apache/flink/table/planner/plan/optimize/program/FlinkRuntimeFilterProgram.java
Outdated
Show resolved
Hide resolved
...java/org/apache/flink/table/planner/plan/optimize/program/FlinkRuntimeFilterProgramTest.java
Show resolved
Hide resolved
...java/org/apache/flink/table/planner/plan/optimize/program/FlinkRuntimeFilterProgramTest.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wanglijie95 Thanks for contribution, LGTM
…filter related exec node
…ype when looking for the build side
f7a1fea
to
fb7068d
Compare
…ize of runtime filter to cover more cases
fb7068d
to
5ed6e94
Compare
…eFilterITcase have applied runtime filter.
...ain/java/org/apache/flink/table/planner/plan/optimize/program/FlinkRuntimeFilterProgram.java
Outdated
Show resolved
Hide resolved
… join type when looking for the build side
CI fails due to FLINK-32751. @flinkbot run azure |
…eFilterITcase have applied runtime filter. This closes #23216
What is the purpose of the change
Currently, runtime filter program will try to look for an Exchange as build side to avoid affecting MultiInput. It will try to push down the runtime filter builder if the original build side is not Exchange.
Currenlty, the builder-push-down does not aware the join type, which may lead to incorrect results(For example, push down the builder to the right input of left-join).
We should only support following cases:
Verifying this change
Add test
testBuildSideIsLeftJoinWithoutExchange
Does this pull request potentially affect one of the following parts:
@Public(Evolving)
: (no)Documentation