-
Notifications
You must be signed in to change notification settings - Fork 526
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
refactor(fragmenter): make graph
its submodule & some refactors to improve style & readability
#2066
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2066 +/- ##
==========================================
- Coverage 70.83% 70.82% -0.01%
==========================================
Files 633 633
Lines 81240 81365 +125
==========================================
+ Hits 57544 57628 +84
- Misses 23696 23737 +41
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
graph
submodule of fragmenter
graph
submodule of fragmenter
graph
submodule of fragmenter
graph
submodule of fragmenter
& some style refactors
This reverts commit a576cc3.
graph
submodule of fragmenter
& some style refactorsgraph
its submodule & some refactors to improve style & readability
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.
Good work!
/// dependent table ids | ||
dependent_table_ids: HashSet<TableId>, | ||
/// upstream dispatch keys | ||
distribution_keys: Vec<i32>, |
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.
There should be multiple sets of distribution keys, one set per upstream?
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.
cc @zbzbw why there's only one set of distribution keys? 🤣
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.
cc @zbzbw why there's only one set of distribution keys? 🤣
My fault😢. The comment was misleading, this field contains current fragment's distribution key instead of upstream.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
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.
Rest LGTM!
Please fix the comment I marked, thanks!
Co-authored-by: Zhou, Bowen <44491322+zbzbw@users.noreply.github.com>
What's changed and what's your intention?
graph
is only used therepub
, to make the dependency more clearremove(Next PR)fragment_graph
&stream_graph_builder
from fields (make them as variables/parameters), so that the relationship is more clear, and mutable variables are more distinguishable.Checklist
[ ] I have written necessary docs and comments[ ] I have added necessary unit tests and integration testsRefer to a related PR or issue link (optional)