-
Notifications
You must be signed in to change notification settings - Fork 525
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(simple-agg): use fe generated table catalog for simple agg + some code-reuse refactor #3514
Conversation
1ed4393
to
31e7673
Compare
Codecov Report
@@ Coverage Diff @@
## main #3514 +/- ##
=======================================
Coverage 74.48% 74.49%
=======================================
Files 771 771
Lines 108317 108342 +25
=======================================
+ Hits 80684 80709 +25
Misses 27633 27633
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 |
// Create state tables for each agg call. | ||
let mut state_tables = Vec::with_capacity(agg_calls.len()); | ||
for (agg_call, ks) in agg_calls.iter().zip_eq(&keyspace) { | ||
state_tables.push(generate_state_table( |
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.
Can we remove generate_state_table
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.
It's now only used in unit test. For test, the CN still infer it by itself. I should add a test only tag.
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.
Emmm I do not find one. Moved them all to test_utils.rs
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.
LGTM
31e7673
to
d64b51a
Compare
… some code-reuse refactor (#3514) * refactor(simple-agg): use fe generated table catalog for simple agg and some simple refactor * move generate_state_table to test mod (test only) Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
…nd some simple refactor
I hereby agree to the terms of the Singularity Data, Inc. Contributor License Agreement.
What's changed and what's your intention?
generate_state_table_from_proto
. Currently reused by global agg and simple agg.new_boxed_simple_agg_executor
. Currently used by tests in simple agg.Checklist
./risedev check
(or alias,./risedev c
)Refer to a related PR or issue link (optional)