Skip to content

Commit 1e09a2a

Browse files
authored
perf: Avoid copying when materializing output in OrderedPartialAggregateStream (#25312)
## Which issue does this PR close? <!-- We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123. --> part of #25157 ## Rationale for this change <!-- Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed. Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes. Please explain the problem you are trying to solve in terms of the user-visible behavior, rather than the implementation. For example, "The code in `foo.rs` doesn't handle nulls" is a symptom of the implementation. "COUNT(DISTINCT) returns wrong results when the column contains nulls" is the user-visible problem. --> ### Cause See issue for the target query. The query plan looks like <details> <summary>Query plan, Click to expand</summary> ``` > explain SELECT count(*) FROM ( SELECT DISTINCT d_year, brand, class, cat, manu, cnt, amt FROM src ); +---------------+-------------------------------+ | plan_type | plan | +---------------+-------------------------------+ | physical_plan | ┌───────────────────────────┐ | | | │ ProjectionExec │ | | | │ -------------------- │ | | | │ count(*): │ | | | │ count(Int64(1)) │ | | | └─────────────┬─────────────┘ | | | ┌─────────────┴─────────────┐ | | | │ AggregateExec │ | | | │ -------------------- │ | | | │ aggr: count(1) │ | | | │ mode: Final │ | | | └─────────────┬─────────────┘ | | | ┌─────────────┴─────────────┐ | | | │ CoalescePartitionsExec │ | | | └─────────────┬─────────────┘ | | | ┌─────────────┴─────────────┐ | | | │ AggregateExec │ | | | │ -------------------- │ | | | │ aggr: count(1) │ | | | │ mode: Partial │ | | | └─────────────┬─────────────┘ | | | ┌─────────────┴─────────────┐ | | | │ ProjectionExec │ | | | └─────────────┬─────────────┘ | | | ┌─────────────┴─────────────┐ | | | │ AggregateExec │ | | | │ -------------------- │ | | | │ group_by: │ | | | │ d_year, brand, class, cat,│ | | | │ manu, cnt, amt │ | | | │ │ | | | │ mode: │ | | | │ FinalPartitioned │ | | | └─────────────┬─────────────┘ | | | ┌─────────────┴─────────────┐ | | | │ RepartitionExec │ | | | │ -------------------- │ | | | │ partition_count(in->out): │ | | | │ 14 -> 14 │ | | | │ │ | | | │ partitioning_scheme: │ | | | │ Hash([d_year@0, brand@1, │ | | | │ class@2, cat@3, manu@4 │ | | | │ , cnt@5, amt@6], 14) │ | | | │ │ | | | │ preserve_order: true │ | | | └─────────────┬─────────────┘ | | | ┌─────────────┴─────────────┐ | | | │ AggregateExec │ | | | │ -------------------- │ | | | │ group_by: │ | | | │ d_year, brand, class, cat,│ | | | │ manu, cnt, amt │ | | | │ │ | | | │ mode: Partial │ | | | └─────────────┬─────────────┘ | | | ┌─────────────┴─────────────┐ | | | │ DataSourceExec │ | | | │ -------------------- │ | | | │ files: 14 │ | | | │ format: parquet │ | | | └───────────────────────────┘ | | | | +---------------+-------------------------------+ ``` </details> It's slow due to inefficient output materializing in partial and final aggregation For internal mechanism, this comment explains 'why not X, and do Y instead' -- X is the existing impl, Y is what this PR does. - https://github.com/2010YOUY01/arrow-datafusion/blob/0f165d0129820a3bd66641609d9efb954c0cc018/datafusion/physical-plan/src/aggregates/ordered_partial_stream.rs#L219-L248 ### Fix To fully restore the performance, we have to fix: 1. Ordered partial aggregation (this PR) 2. Ordered final aggregation (maybe a follow-up PR) 2 uses almost the same mechanism as 1, so once this PR is reviewed, we can apply the pattern mechanically. After this PR, the query runs in: (on an M4 Pro MacBook Pro) ``` -- Still some gap due to final aggregation is not fixed yet Current main: 3.5s PR: 0.45s DataFusion 54.0: 0.37s ``` ## What changes are included in this PR? <!-- There is no need to duplicate the description in the issue here, but it is sometimes worth providing a summary of the individual changes in this PR. --> 1. Refactor the ordered-partial aggregation, so it's easier to implement incremental outputting with slicing 2. Implement the output materializing strategy mentioned above Note to read this PR, I suggest directly reading the new impl start from the entry point of state machine (`into_stream()`), instead of the diff, due to a large refactor. This refactor is necessary because its easier to implement this feature with a different state machine pattern. ## What is the testing strategy for this PR? <!-- We typically require tests for all PRs in order to: 1. Prevent the code from being accidentally broken by subsequent changes 4. Serve as another way to document the expected behavior of the code Briefly describe how this PR is tested, and point to the specific tests you added. For example: 'This new feature is covered by the `sqllogictest` cases added in `foo.slt`'. If this PR does not add tests, explain why. For example, if the change is already covered by existing tests, please mention it. You should also check the `codecov` bot reply on this PR to confirm the changed code is exercised. --> For correctness, existing tests have covered it. To prevent similar perf regression, we can do - #25310 ## Are there any user-facing changes? <!-- If there are user-facing changes then we may require documentation to be updated before approving the PR. If there are any breaking changes to public APIs, please add the `api change` label. -->
1 parent 993b3f3 commit 1e09a2a

3 files changed

Lines changed: 271 additions & 207 deletions

File tree

datafusion/physical-plan/src/aggregates/aggregate_hash_table/common_ordered.rs

Lines changed: 25 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -362,24 +362,6 @@ impl<AggrMode> OrderedAggregateTable<AggrMode> {
362362
Ok(Some(batch))
363363
}
364364

365-
/// Returns the [`EmitTo`], clamped to the specified batch size
366-
///
367-
/// Returns `(emit_to, should_remove_groups)`, where `emit_to` is the number
368-
/// of groups to emit from `GroupValues` / accumulators, and
369-
/// `should_remove_groups` indicates whether `GroupOrdering` must also shift
370-
/// its tracked indexes.
371-
pub(super) fn clamp_emit_to(
372-
&self,
373-
group_count: usize,
374-
emit_to: EmitTo,
375-
) -> (EmitTo, bool) {
376-
match emit_to {
377-
EmitTo::First(n) => (EmitTo::First(n.min(self.batch_size)), true),
378-
EmitTo::All if group_count <= self.batch_size => (EmitTo::All, false),
379-
EmitTo::All => (EmitTo::First(self.batch_size), false),
380-
}
381-
}
382-
383365
/// Aggregates one evaluated input batch after selecting the mode-specific
384366
/// accumulator operation.
385367
///
@@ -455,20 +437,34 @@ impl<AggrMode> OrderedAggregateTable<AggrMode> {
455437
let Some(emit_to) = self.buffer.group_ordering.emit_to() else {
456438
return Ok(None);
457439
};
458-
let (emit_to, should_remove_groups) =
459-
self.clamp_emit_to(self.buffer.group_values.len(), emit_to);
440+
let emit_to = match emit_to {
441+
EmitTo::First(n) => EmitTo::First(n.min(self.batch_size)),
442+
EmitTo::All if self.num_groups() > self.batch_size => {
443+
EmitTo::First(self.batch_size)
444+
}
445+
EmitTo::All => EmitTo::All,
446+
};
447+
self.materialize_groups(emit_to, materialize_accumulator_fn, accumulator_phase)
448+
.map(Some)
449+
}
460450

451+
/// Removes the selected groups once and materializes their output columns.
452+
/// The caller chooses the completed prefix and any output-size limit.
453+
pub(super) fn materialize_groups(
454+
&mut self,
455+
emit_to: EmitTo,
456+
materialize_accumulator_fn: MaterializeAccumulatorFn,
457+
accumulator_phase: AccumulatorPhase,
458+
) -> Result<RecordBatch> {
461459
let accumulator_metrics = Arc::clone(&self.aggregate_accumulator_metrics);
462460
let output = self.group_by_metrics.time_emitting(|| {
463461
let mut output = self.buffer.group_values.emit(emit_to)?;
464-
if should_remove_groups {
465-
match emit_to {
466-
EmitTo::First(n) => self.buffer.group_ordering.remove_groups(n),
467-
// `EmitTo::All` is only used after `input_done`, when all
468-
// buffered groups are known complete and the ordering state is
469-
// no longer needed.
470-
EmitTo::All => {}
471-
}
462+
// EOF can also emit a prefix when a caller limits its batch size,
463+
// but the completed ordering state no longer tracks group indexes.
464+
if let EmitTo::First(n) = emit_to
465+
&& matches!(self.buffer.group_ordering.emit_to(), Some(EmitTo::First(_)))
466+
{
467+
self.buffer.group_ordering.remove_groups(n);
472468
}
473469

474470
for (idx, acc) in self.buffer.accumulators.iter_mut().enumerate() {
@@ -484,6 +480,6 @@ impl<AggrMode> OrderedAggregateTable<AggrMode> {
484480
let batch = RecordBatch::try_new(Arc::clone(&self.output_schema), output)?;
485481
debug_assert!(batch.num_rows() > 0);
486482

487-
Ok(Some(batch))
483+
Ok(batch)
488484
}
489485
}

datafusion/physical-plan/src/aggregates/aggregate_hash_table/ordered_partial_table.rs

Lines changed: 12 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -89,26 +89,22 @@ impl OrderedAggregateTable<PartialMarker> {
8989
)
9090
}
9191

92-
/// Emits the next batch of partial state rows for groups proven complete by
93-
/// the input ordering.
94-
///
95-
/// For example, when the query is `GROUP BY a` and the input is ordered by
96-
/// `a`, seeing a latest input row with `a = 3` means all groups with `a < 3`
97-
/// are complete and safe to emit.
98-
///
99-
/// Key steps:
100-
/// 1. Ask `group_ordering` to decide how many groups can be emitted eagerly.
101-
/// 2. Remove the emitted groups from `group_ordering`, `GroupValues`, and
102-
/// all `GroupsAccumulator`s.
103-
///
104-
/// This may output small batches. Avoiding tiny batches is left to future
105-
/// ordered-aggregation optimizations.
106-
pub(in crate::aggregates) fn next_output_batch(
92+
/// Materializes all groups proven complete by the input ordering, leaving
93+
/// the active ordered-key range in the table.
94+
pub(in crate::aggregates) fn take_completed_state_batch(
10795
&mut self,
10896
) -> Result<Option<RecordBatch>> {
109-
self.next_output_batch_inner(
97+
if self.is_empty() {
98+
return Ok(None);
99+
}
100+
let Some(emit_to) = self.group_ordering().emit_to() else {
101+
return Ok(None);
102+
};
103+
self.materialize_groups(
104+
emit_to,
110105
HashAggregateAccumulator::state,
111106
AccumulatorPhase::State,
112107
)
108+
.map(Some)
113109
}
114110
}

0 commit comments

Comments
 (0)