Skip to content

feat: add selective non-destructive grouped state reads - #74

Merged
rkrishn7 merged 9 commits into
branch-55from
preserving-grouped-state-api
Aug 27, 2026
Merged

feat: add selective non-destructive grouped state reads#74
rkrishn7 merged 9 commits into
branch-55from
preserving-grouped-state-api

Conversation

@rkrishn7

Copy link
Copy Markdown

Which issue does this PR close?

Rationale for this change

Long-lived grouped aggregation state currently must be drained and rebuilt to read a subset of groups. Preserving reads avoid that full-state round trip.

What changes are included in this PR?

  • Adds GroupSelection for all groups or ordered group indices.
  • Adds optional preserving read APIs and capability checks to GroupValues and GroupsAccumulator.
  • Implements selected reads for built-in group values and grouped accumulators.

Are these changes tested?

Yes. Added tests cover repeated and reordered reads, duplicates, nulls, variable-width values, and updates after reads.

Validated with:

  • cargo fmt --all
  • cargo clippy --all-targets --all-features -- -D warnings
  • Relevant datafusion-expr-common, datafusion-functions-aggregate-common, datafusion-functions-aggregate, and datafusion-physical-plan unit tests

Are there any user-facing changes?

Yes. This adds optional public trait methods and GroupSelection. Existing implementations remain compatible through default methods.

@rkrishn7
rkrishn7 changed the base branch from branch-54 to branch-55 August 25, 2026 02:03
@rkrishn7
rkrishn7 force-pushed the preserving-grouped-state-api branch from 9d880ee to 06d254a Compare August 25, 2026 02:04

@xudong963 xudong963 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the thorough implementation and coverage. I found two correctness issues in valid preserving-read paths: an empty-selection panic for struct MIN/MAX, and unchecked offset truncation for selected variable-width group values. Requesting changes for these two cases.

Comment thread datafusion/functions-aggregate/src/min_max/min_max_struct.rs
Comment thread datafusion/physical-plan/src/aggregates/group_values/multi_group_by/bytes.rs Outdated

@zhuqi-lucas zhuqi-lucas left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Read through the API and the impls. Mostly looks right - the evaluate/state split on the adapter lines up exactly with the two Accumulator contracts, and the byte-map keys path gets the offsets and the null slot right. Few things inline.

Comment thread datafusion/expr-common/src/groups_accumulator.rs Outdated
Comment thread datafusion/functions-aggregate/src/average.rs Outdated
Comment thread datafusion/physical-plan/src/aggregates/group_values/mod.rs
@rkrishn7
rkrishn7 merged commit ff0a7fa into branch-55 Aug 27, 2026
69 checks passed
@rkrishn7
rkrishn7 deleted the preserving-grouped-state-api branch August 27, 2026 00:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants