-
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(streaming): remove dist key for global simple agg & rename to GlobalSimpleAgg
#3548
Conversation
Signed-off-by: Bugen Zhao <i@bugenzhao.com>
Signed-off-by: Bugen Zhao <i@bugenzhao.com>
Signed-off-by: Bugen Zhao <i@bugenzhao.com>
@@ -56,6 +56,7 @@ message MaterializeNode { | |||
// Local and global aggregator distinguish with each other in PlanNode definition. | |||
message SimpleAggNode { | |||
repeated expr.AggCall agg_calls = 1; | |||
// Not used for global simple agg. | |||
repeated uint32 distribution_keys = 2; |
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.
why not deleted?
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.
SimpleAggNode
is also used for local agg. 🤣
Signed-off-by: Bugen Zhao <i@bugenzhao.com>
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, thanks for doing this refactor :) Missed this in earlier PR.
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.
LSTM
BTW can we also change the name from "StreamSimpleAgg" to "StreamGlobalSimpleAgg" in this PR? 😢 https://github.com/singularity-data/risingwave/pull/3320/files
Codecov Report
@@ Coverage Diff @@
## main #3548 +/- ##
=======================================
Coverage 74.44% 74.44%
=======================================
Files 770 770
Lines 108424 108409 -15
=======================================
- Hits 80715 80707 -8
+ Misses 27709 27702 -7
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 |
Cool. |
GlobalSimpleAggExecutor
GlobalSimpleAgg
Signed-off-by: Bugen Zhao <i@bugenzhao.com>
d4dce3f
to
1432209
Compare
…o `GlobalSimpleAgg` (#3548) * remove dist key for global simple agg Signed-off-by: Bugen Zhao <i@bugenzhao.com> * rename to global Signed-off-by: Bugen Zhao <i@bugenzhao.com> * refine proto doc Signed-off-by: Bugen Zhao <i@bugenzhao.com> * refine comments Signed-off-by: Bugen Zhao <i@bugenzhao.com> * rename frontend plannode Signed-off-by: Bugen Zhao <i@bugenzhao.com> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
I hereby agree to the terms of the Singularity Data, Inc. Contributor License Agreement.
What's changed and what's your intention?
As title. We reuse
SimpleAggNode
for both global and local ones, wheredist_key
should be unused for global simple agg.Checklist
./risedev check
(or alias,./risedev c
)Refer to a related PR or issue link (optional)