-
Notifications
You must be signed in to change notification settings - Fork 2.4k
fix(physical-plan): honor distinct soft limits in SingleHashAggregateStream #25158
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -104,6 +104,10 @@ pub(crate) struct SingleHashAggregateStream { | |||||||||||||||||||
| /// Tracks the high-level stream lifecycle. The hash table owns the lower-level | ||||||||||||||||||||
| /// state for emitting output batches. | ||||||||||||||||||||
| state: Option<SingleHashAggregateState>, | ||||||||||||||||||||
|
|
||||||||||||||||||||
| /// When set, there are no aggregate expressions: AggregateExec routes | ||||||||||||||||||||
| /// limited non-DISTINCT aggregates to a different stream. | ||||||||||||||||||||
|
Comment on lines
+108
to
+109
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe we can also change it to 'see top comments for details'. |
||||||||||||||||||||
| group_values_soft_limit: Option<usize>, | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| /// Spill configuration and accumulated runs for single hash aggregation. | ||||||||||||||||||||
|
|
@@ -374,6 +378,7 @@ impl SingleHashAggregateStream { | |||||||||||||||||||
| hash_table, | ||||||||||||||||||||
| spill_context, | ||||||||||||||||||||
| }), | ||||||||||||||||||||
| group_values_soft_limit: agg.limit_options().map(|config| config.limit()), | ||||||||||||||||||||
| }) | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
|
|
@@ -449,6 +454,27 @@ impl SingleHashAggregateStream { | |||||||||||||||||||
| return Self::break_with_err(e); | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| // Soft limit optimization: | ||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we move this comment to Additionally we can follow the comment pattern in (first explain how the SQL get optimized to soft limit, and next the internal early termination mechanism)
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. and we can update control flow comment at |
||||||||||||||||||||
| // | ||||||||||||||||||||
| // Stop reading input once the in-memory table contains enough distinct | ||||||||||||||||||||
| // groups to satisfy the soft limit. | ||||||||||||||||||||
| // | ||||||||||||||||||||
| // When a limit is present, AggregateExec routes only unordered, | ||||||||||||||||||||
| // unfiltered DISTINCT aggregates to this stream. | ||||||||||||||||||||
| // | ||||||||||||||||||||
| // With no aggregate expressions, additional input can only match existing | ||||||||||||||||||||
| // groups or add new ones; it cannot change any existing group's output. | ||||||||||||||||||||
| // Since there is no ordering requirement and we already have enough | ||||||||||||||||||||
| // distinct groups, we can finish reading as if the input were exhausted. | ||||||||||||||||||||
| // | ||||||||||||||||||||
| // Reuse the input-exhausted transition to merge any existing spills | ||||||||||||||||||||
| // before producing output. The downstream limit operator enforces | ||||||||||||||||||||
| // the exact output row count. | ||||||||||||||||||||
| if self.hit_soft_group_limit(&hash_table) { | ||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I suggest to skip this optimization if we have spilled before Here is the pattern to follow, and also the explanaiton datafusion/datafusion/physical-plan/src/aggregates/hash_stream.rs Lines 857 to 865 in 7e5f40a
|
||||||||||||||||||||
| return self | ||||||||||||||||||||
| .close_input_and_prepare_output(hash_table, spill_context); | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| // Check memory reservation, and potentially spill. | ||||||||||||||||||||
| let timer = elapsed_compute.timer(); | ||||||||||||||||||||
| let resize_result = | ||||||||||||||||||||
|
|
@@ -490,30 +516,53 @@ impl SingleHashAggregateStream { | |||||||||||||||||||
| } | ||||||||||||||||||||
| Poll::Ready(Some(Err(e))) => Self::break_with_err(e), | ||||||||||||||||||||
| Poll::Ready(None) => { | ||||||||||||||||||||
| self.close_input(); | ||||||||||||||||||||
| match spill_context { | ||||||||||||||||||||
| Some(spill_context) if spill_context.has_spills() => { | ||||||||||||||||||||
| ControlFlow::Continue( | ||||||||||||||||||||
| SingleHashAggregateState::PreparingMergeInput { | ||||||||||||||||||||
| hash_table, | ||||||||||||||||||||
| spill_context, | ||||||||||||||||||||
| }, | ||||||||||||||||||||
| ) | ||||||||||||||||||||
| } | ||||||||||||||||||||
| _ => { | ||||||||||||||||||||
| let elapsed_compute = | ||||||||||||||||||||
| self.baseline_metrics.elapsed_compute().clone(); | ||||||||||||||||||||
| let timer = elapsed_compute.timer(); | ||||||||||||||||||||
| let result = hash_table.start_output(); | ||||||||||||||||||||
| timer.done(); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| match result { | ||||||||||||||||||||
| Ok(()) => ControlFlow::Continue( | ||||||||||||||||||||
| SingleHashAggregateState::ProducingOutput { hash_table }, | ||||||||||||||||||||
| ), | ||||||||||||||||||||
| Err(e) => Self::break_with_err(e), | ||||||||||||||||||||
| } | ||||||||||||||||||||
| self.close_input_and_prepare_output(hash_table, spill_context) | ||||||||||||||||||||
| } | ||||||||||||||||||||
| } | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| /// See comments in [`Self::group_values_soft_limit`] for details. | ||||||||||||||||||||
| fn hit_soft_group_limit( | ||||||||||||||||||||
| &self, | ||||||||||||||||||||
| hash_table: &AggregateHashTable<SingleMarker>, | ||||||||||||||||||||
| ) -> bool { | ||||||||||||||||||||
| self.group_values_soft_limit | ||||||||||||||||||||
| .is_some_and(|limit| limit <= hash_table.building_group_count()) | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| /// Stops consuming input and prepares the next execution phase. | ||||||||||||||||||||
| /// Called when the input is exhausted or the distinct soft limit is reached. | ||||||||||||||||||||
| /// | ||||||||||||||||||||
| /// If data has been spilled, transitions to `PreparingMergeInput` so the | ||||||||||||||||||||
| /// spilled and in-memory groups can be merged before output. Otherwise, | ||||||||||||||||||||
| /// starts output from the in-memory hash table and transitions to | ||||||||||||||||||||
| /// `ProducingOutput`. | ||||||||||||||||||||
| fn close_input_and_prepare_output( | ||||||||||||||||||||
| &mut self, | ||||||||||||||||||||
| mut hash_table: AggregateHashTable<SingleMarker>, | ||||||||||||||||||||
| spill_context: Option<Box<SingleSpillContext>>, | ||||||||||||||||||||
| ) -> SingleHashAggregateStateTransition { | ||||||||||||||||||||
| self.close_input(); | ||||||||||||||||||||
| match spill_context { | ||||||||||||||||||||
| Some(spill_context) if spill_context.has_spills() => { | ||||||||||||||||||||
| ControlFlow::Continue(SingleHashAggregateState::PreparingMergeInput { | ||||||||||||||||||||
| hash_table, | ||||||||||||||||||||
| spill_context, | ||||||||||||||||||||
| }) | ||||||||||||||||||||
| } | ||||||||||||||||||||
| _ => { | ||||||||||||||||||||
| let elapsed_compute = self.baseline_metrics.elapsed_compute().clone(); | ||||||||||||||||||||
| let timer = elapsed_compute.timer(); | ||||||||||||||||||||
| let result = hash_table.start_output(); | ||||||||||||||||||||
| timer.done(); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| match result { | ||||||||||||||||||||
| Ok(()) => { | ||||||||||||||||||||
| ControlFlow::Continue(SingleHashAggregateState::ProducingOutput { | ||||||||||||||||||||
| hash_table, | ||||||||||||||||||||
| }) | ||||||||||||||||||||
| } | ||||||||||||||||||||
| Err(e) => Self::break_with_err(e), | ||||||||||||||||||||
| } | ||||||||||||||||||||
| } | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
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 recommend to write this test differently (follow the pattern in 40a6454#diff-02af0439a3df656429990b220b80e50d8df259ce45c47e008460c1ca3781aca3)
The main difference is
select distinctquery, and get it optimized to aggregate with soft limitAggregateExec, otherwise we can't ensure if this soft limit optimization is applied -- limit can also be enforced by the downstreamLimitExecoperator.(I think only such e2e test is enough, we don't have to test it individually on
AggregateExec, since this optimization is only useful from such SQL patterns, and should not be directly used on theAggregateExec)