-
Notifications
You must be signed in to change notification settings - Fork 461
[flink]Support partition pushdown for more filters in Flink connector #420
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]Support partition pushdown for more filters in Flink connector #420
Conversation
9a09bfe to
8d7cc2e
Compare
|
@Alibaba-HZY Thanks for the contribution. We are reaching the code freeze deadline of v0.6. Therefore, I planned this issue into the next version. Besides, this pull request is quite huge. Splitting into multiple pull requests can speed up the review process. |
ok i will submit 2pr. |
| remainingFilters.add(filter); | ||
| } else { | ||
| Predicate p = predicateOptional.get(); | ||
| if (!p.visit(partitionPredicateVisitor)) { |
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.
The name PartitionPredicateVisitor is quite strange. Based on its implementation semantics and usage, perhaps it should be called PartitionPredicateMatcher? (Although I checked and found that Apache Paimon also uses this naming.)
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.
If the class renamed PartitionPredicateMatcher, then his ’visit‘ methods should be renamed to ‘match’?
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.
I propose a public method match() to wrap the internal visit() method as a better approach. match() serves as the semantic interface, and visit() is its implementation.
...-flink/fluss-flink-common/src/main/java/com/alibaba/fluss/flink/source/FlinkTableSource.java
Outdated
Show resolved
Hide resolved
...nk-common/src/main/java/com/alibaba/fluss/flink/source/enumerator/FlinkSourceEnumerator.java
Outdated
Show resolved
Hide resolved
|
@Alibaba-HZY It seems the pull request branch is currently far behind the main branch, and several conflicts exist. Could you kindly rebase it from the main branch when possible? This will help us proceed with more thorough testing for this PR. |
|
@Alibaba-HZY I have tested this PR in several complex scenarios. Overall, the changes look good, but there are still a few minor issues that need attention:
|
thank you for your test, i will fix thoes problems |
...-flink/fluss-flink-common/src/main/java/com/alibaba/fluss/flink/source/FlinkTableSource.java
Outdated
Show resolved
Hide resolved
...-flink/fluss-flink-common/src/main/java/com/alibaba/fluss/flink/source/FlinkTableSource.java
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| @Test | ||
| void testStreamingReadWithCombinedFilters2() throws Exception { |
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.
What is the difference between the test cases testStreamingReadWithCombinedFilters1 and testStreamingReadWithCombinedFilters2?
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.
i think i can combine thoese
|
This PR LGTM. I've tested it across several rounds in different production scenarios, and it works almost well. Once https://github.com/apache/fluss/pull/515/files is merged, this PR should be ready for final review by @wuchong after cleaning up the commits. |
| convertPartitionInfoToInternalRow( | ||
| partitionInfo))) | ||
| .collect(Collectors.toList()); | ||
| LOG.info( |
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.
The listPartitions() will be called every 10 seconds (default setting) in streaming task, so this log will also be printed every 10 seconds, even if no new partitions are discovered. It will drown out other useful logs.
...nk-common/src/main/java/com/alibaba/fluss/flink/source/enumerator/FlinkSourceEnumerator.java
Show resolved
Hide resolved
d88c76c to
434a4f4
Compare
d1c103f to
2a3892b
Compare
wuchong
left a comment
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 @Alibaba-HZY , I added a commit to improve the code a bit.
Besides, please add UTs for the PredicateConverter, and try to support more data types if it is not complex since #1264 is merged.
463e7d0 to
f0e7af6
Compare
|
@Alibaba-HZY, just checking in how’s the progress on this PR? Let me know if you need any help or if there’s anything I can do to move it forward! |
|
I'm almost done and will commit tonight or tomorrow.
…---- Replied Message ----
| From | Jark ***@***.***> |
| Date | 09/21/2025 15:50 |
| To | apache/fluss ***@***.***> |
| Cc | HZY ***@***.***>,
Mention ***@***.***> |
| Subject | Re: [apache/fluss] [flink]Support partition pushdown for more filters in Flink connector (PR #420) |
wuchong left a comment (apache/fluss#420)
@Alibaba-HZY, just checking in how’s the progress on this PR? Let me know if you need any help or if there’s anything I can do to move it forward!
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
left comments in your dingding |
f0e7af6 to
9489f6a
Compare
2b155cc to
774051b
Compare
|
@Alibaba-HZY I updated the implementation to fix bugs and add documentation. |
774051b to
c68b73d
Compare
…ushdown and fix bugs
c68b73d to
9fbfce4
Compare
|
Merging... |
[flink]
Purpose
Support partition pushdown in Flink connector
Linked issue: close #1711
Tests
com.alibaba.fluss.connector.flink.source.FlinkTableSourceITCase#testStreamingReadPartitionPushDown
com.alibaba.fluss.predicate.PredicateBuilderTest
com.alibaba.fluss.predicate.PredicateTest
API and Format
no
Documentation
no