Skip to content

refactor: separate aggregate group completion from input ordering - #24697

Draft
xavlee wants to merge 2 commits into
apache:mainfrom
xavlee:refactor/aggregate-group-completion-mode
Draft

refactor: separate aggregate group completion from input ordering#24697
xavlee wants to merge 2 commits into
apache:mainfrom
xavlee:refactor/aggregate-group-completion-mode

Conversation

@xavlee

@xavlee xavlee commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Which issue does this PR relate to?

Rationale for this change

Aggregate execution currently derives both input-order metadata and the runtime mechanism for recognizing completed groups from InputOrderMode.

Completing a group incrementally is contingent on knowing that "after processing a group, we will never see this group again in this stream". As such, input ordering covers only a subset of valid group completion cases. A sorted input guarantees that after processing a group, we will never see it again; however, the general case proves that so long as all rows in a groups are contiguous, the group completion property is satisfied regardless of the input ordering.

Consider the following:

AAABBBCCCC -> sorted, can be emitted incrementally
CCCAAAABBB -> unsorted, can be emitted incrementally still 

This PR introduces a new GroupCompletionMode that decouples the input ordering from group completion semantics. Currently, we directly map existing InputOrderMode to this property. In a follow up PR, we will read group contiguity expressions to evaluate this property.

What changes are included in this PR?

  • Introduce private GroupCompletionMode::{None, Partial, Full} variants.
  • Derive GroupCompletionMode from InputOrderMode during AggregateExec construction.
  • Pass GroupCompletionMode through ordered aggregate tables, streams, and spill replay paths.
  • Use InputOrderMode for required ordering, output ordering, and maintains_input_order metadata.
  • Retain GroupOrdering::try_new(&InputOrderMode) as the public compatibility entry point.

The ordering-derived conversion is:

InputOrderMode::Linear                    -> GroupCompletionMode::None
InputOrderMode::PartiallySorted(indices) -> GroupCompletionMode::Partial(indices)
InputOrderMode::Sorted                    -> GroupCompletionMode::Full

Stack

  1. #24737 — test: cover unsorted contiguous groups in one partition
  2. #24697 — refactor: separate aggregate group completion from input orderingthis PR
  3. #24698 — feat: add narrow group-contiguous source property
  4. #24497 — feat: stream exact group-contiguous aggregates

Are these changes tested?

The characterization from #24737 remains Linear, None, Final, and single-hash after the refactor.

Are there any user-facing changes?

No. GroupCompletionMode is crate-private, and the public GroupOrdering constructor is unchanged.

Review this layer

View only this PR layer

@github-actions github-actions Bot added the physical-plan Changes to the physical-plan crate label Aug 26, 2026
@codecov-commenter

codecov-commenter commented Aug 26, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 85.41667% with 14 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.42%. Comparing base (ee59f62) to head (372429f).

Files with missing lines Patch % Lines
datafusion/physical-plan/src/aggregates/mod.rs 82.00% 2 Missing and 7 partials ⚠️
...tafusion/physical-plan/src/aggregates/order/mod.rs 83.33% 2 Missing ⚠️
.../aggregates/aggregate_hash_table/common_ordered.rs 50.00% 0 Missing and 1 partial ⚠️
...hysical-plan/src/aggregates/grouped_hash_stream.rs 0.00% 0 Missing and 1 partial ⚠️
...ysical-plan/src/aggregates/ordered_final_stream.rs 92.85% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #24697      +/-   ##
==========================================
- Coverage   81.42%   81.42%   -0.01%     
==========================================
  Files        1121     1121              
  Lines      402142   402197      +55     
  Branches   402142   402197      +55     
==========================================
+ Hits       327460   327501      +41     
- Misses      55484    55490       +6     
- Partials    19198    19206       +8     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@xavlee
xavlee force-pushed the refactor/aggregate-group-completion-mode branch 3 times, most recently from 72faa6e to f58e128 Compare August 27, 2026 19:17
@xavlee
xavlee force-pushed the refactor/aggregate-group-completion-mode branch from f58e128 to 372429f Compare August 27, 2026 20:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

physical-plan Changes to the physical-plan crate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants