-
Notifications
You must be signed in to change notification settings - Fork 590
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
feat(frontend): add stateless two-phase ApproxPercentile
#17469
Conversation
6265b13
to
c9270bf
Compare
ApproxPercentile
ApproxPercentile
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.
Generally LGTM
2c0a899
to
2d76cae
Compare
…ibution of global agg
Co-authored-by: Eric Fu <[email protected]>
563269d
to
1dd7c2b
Compare
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
fn build_approx_percentile_agg( | ||
&self, | ||
input: PlanRef, | ||
approx_percentile_agg_call: &PlanAggCall, | ||
) -> PlanRef { | ||
let local_approx_percentile = | ||
StreamLocalApproxPercentile::new(input, approx_percentile_agg_call); | ||
let global_approx_percentile = StreamGlobalApproxPercentile::new( | ||
local_approx_percentile.into(), | ||
approx_percentile_agg_call, | ||
); | ||
global_approx_percentile.into() | ||
} |
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.
Is it possible to unify ApproxPercentile
executor with ApproxCountDistinct
to have something like BucketBasedAgg
? Just curious, not a suggestion.
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.
Hmm I'm not sure about the internals of approx count distinct. Does it involve 2 phase agg?
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.
No I mean is it possible to abstract a common executor like HashAgg that can do various kinds of aggregation that can fit into the framework. This seems not related to 2-phase agg, cuz HashAgg can be used in either local phase or global phase.
I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.
What's changed and what's your intention?
30% the changes in this PR are planner tests + binder tests
This is part of #17531.
We add stateless two-phase simple approx percentile, integrating it with
gen_stateless_two_phase_streaming_agg_plan
.StreamLocalApproxPercentile
is the local phase forapprox_percentile
.StreamGlobalApproxPercentile
is the global phase.We do not implement a
core
for either of these, since the fields they use are different (global step does not require any parameters, local step requires the parameters). Further, there's also nological
plan node which corresponds to these stream nodes, so there's no need to add acore
, it will just be overhead.We also add
KeyedMerge
which will combine the results from approx percentile and normal aggs, as well as pair-wise approx percentiles.Subsequent PRs
Checklist
./risedev check
(or alias,./risedev c
)Documentation
Release note
If this PR includes changes that directly affect users or other significant modifications relevant to the community, kindly draft a release note to provide a concise summary of these changes. Please prioritize highlighting the impact these changes will have on users.