-
Notifications
You must be signed in to change notification settings - Fork 13.3k
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-29138][table-planner] fix project can not be pushed into lookup source #20717
Conversation
@Override | ||
public boolean matches(RelOptRuleCall call) { | ||
LogicalProject project = call.rel(0); | ||
return project.getProjects().stream().noneMatch(RexOver::containsOver); |
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.
Hi @lincoln-lil , just jumping into this PR, can you help me understand why here need to check for the RexOver
?
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.
@Aitozi some comment should be here, I'll add it.
The reason we don't push a project which contains over into a snapshot is snapshot on window aggregate is unsupported for now.
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.
thanks for your explanation. One more minor comment here, it seems can directly use !project.containsOver()
fd8bdcd
to
a9d360b
Compare
…sh down of test data
@flinkbot run azure |
1 similar comment
@flinkbot run azure |
an unstable connector case: https://dev.azure.com/apache-flink/apache-flink/_build/results?buildId=40566&view=ms.vss-test-web.build-test-results-tab successed in my own pipeline: https://dev.azure.com/lincoln86xy/lincoln86xy/_build/results?buildId=250&view=results |
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.
Thanks for the fixing! Looks good to me!
…p source This closes apache#20717
What is the purpose of the change
Projected pushdown is a basic optimization for table source , but actually not work for lookup source (which implments
SupportsProjectionPushDown
), this is unexpected, we should fix this.Brief change log
Add a new rule
ProjectSnapshotTransposeRule
for both batch and stream rule setsVerifying this change
ProjectSnapshotTransposeRuleTest
to cover both batch and streaming modeDoes this pull request potentially affect one of the following parts:
Documentation