Uh oh!
There was an error while loading. Please reload this page.
fix: reduce memory allocation overhead during partial aggregation ear… - #22165
Merged
Dandandan merged 1 commit intoMay 19, 2026
Merged
Conversation
…ly emit When the partial hash aggregation (with GroupOrdering::None) cannot reserve additional memory, it calls self.emit(EmitTo::First(n), where n is the largest integer multiple of self.batch_size, meaning it's close to the length of group_values. Using drain + collect leads to an additional copy of all these n values, since collect allocates a new Vec and drain doesn't modify the original vector's capacity. To avoid copying the largest allocation, choose a strategy which always allocates the minimum between n and remaining. Fixesapache#22164
alamb
approved these changes
May 14, 2026
alamb
left a comment
Contributor
There was a problem hiding this comment.
Makes sense to me -- thanks @ariel-miculas
Dandandan
approved these changes
May 19, 2026
Uh oh!
There was an error while loading. Please reload this page.
RyanJamesStewart added a commit
to RyanJamesStewart/datafusion
that referenced
this pull request
May 21, 2026
Follow-up to apache#22165, which replaced `drain(0..n).collect()` with the `split_vec_min_alloc` helper in the `bytes.rs` and `primitive.rs` `take_n` paths but did not cover the byte-view builder. `ByteViewGroupValueBuilder::take_n_inner` had the same idiom on `self.views`: `drain(0..n).collect()` always allocates `n` elements and leaves the retained vec at its pre-emit capacity. Under an OOM-triggered `EmitTo::First(n)` emit, `n` is close to `len`, so this copies the largest allocation. Routing through `split_vec_min_alloc` allocates `min(n, len - n)` instead, matching the other builders. Behavior is unchanged; existing `test_byte_view_take_n` / `test_byte_view_take_n_partial_completed_nonzero_index` cover both the drain and split_off branches.
This was referenced May 21, 2026
Closed
RyanJamesStewart added a commit
to RyanJamesStewart/datafusion
that referenced
this pull request
May 21, 2026
Addresses the review on apache#22416: - split_vec_min_alloc: shrink_to_fit the emitted prefix in the split_off branch. datafusion accounts memory by capacity rather than length, so the prefix must not retain the pre-split (larger) allocation's capacity. - Remove the duplicate split_vec_min_alloc from physical-plan's multi_group_by/mod.rs (added in apache#22165); bytes.rs and primitive.rs now use the shared datafusion_common::utils version. - Fold in the ByteViewGroupValueBuilder::take_n change previously proposed as apache#22205, so this PR converts every split_vec_min_alloc call site in one place. All split_vec_min_alloc, take_n and min_max tests pass.
ariel-miculas pushed a commit
to ariel-miculas/datafusion
that referenced
this pull request
May 27, 2026
…mitted prefix (apache#22416) ## Which issue does this PR close? Related to apache#22164 and apache#22165. ## Rationale for this change apache#22165 added a `split_vec_min_alloc` helper so that `EmitTo::First(n)` allocates `min(n, len - n)` instead of always copying `n` elements. This PR finishes and corrects that work, following review: - The helper was `pub(super)` inside `datafusion-physical-plan`, so it could not be reused. It moves to `datafusion_common::utils` as the single shared copy. - The `split_off` branch returned the original allocation as the emitted prefix: short length, but original (larger) capacity. datafusion accounts memory by capacity rather than length, so that prefix did not actually release memory under pressure. The helper now calls `shrink_to_fit` on it. - Two further `EmitTo::First(n)` paths still used the `drain(..n).collect()` idiom: the min/max accumulators and `ByteViewGroupValueBuilder::take_n`. Both now route through the shared helper. ## What changes are included in this PR? - `datafusion_common::utils::split_vec_min_alloc`: the shared helper, with `shrink_to_fit` on the emitted prefix. - `datafusion-physical-plan`: the duplicate helper is removed; `bytes.rs`, `primitive.rs` and `bytes_view.rs` use the shared one. - `datafusion-functions-aggregate`: `MinMaxStructAccumulator` and `MinMaxBytesAccumulator` use it in `emit_to`. This supersedes apache#22205, whose `ByteViewGroupValueBuilder::take_n` change is included here. ## Are these changes tested? Yes. Unit tests for the helper (both branches and the boundaries) plus the existing `take_n` and `min_max` tests pass. ## Are there any user-facing changes? No. --------- Co-authored-by: RyanJamesStewart <RyanJamesStewart@users.noreply.github.com> Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
This was referenced Jun 2, 2026
naman-modi pushed a commit
to infino-ai/datafusion
that referenced
this pull request
Jul 22, 2026
…mitted prefix (apache#22416) ## Which issue does this PR close? Related to apache#22164 and apache#22165. ## Rationale for this change apache#22165 added a `split_vec_min_alloc` helper so that `EmitTo::First(n)` allocates `min(n, len - n)` instead of always copying `n` elements. This PR finishes and corrects that work, following review: - The helper was `pub(super)` inside `datafusion-physical-plan`, so it could not be reused. It moves to `datafusion_common::utils` as the single shared copy. - The `split_off` branch returned the original allocation as the emitted prefix: short length, but original (larger) capacity. datafusion accounts memory by capacity rather than length, so that prefix did not actually release memory under pressure. The helper now calls `shrink_to_fit` on it. - Two further `EmitTo::First(n)` paths still used the `drain(..n).collect()` idiom: the min/max accumulators and `ByteViewGroupValueBuilder::take_n`. Both now route through the shared helper. ## What changes are included in this PR? - `datafusion_common::utils::split_vec_min_alloc`: the shared helper, with `shrink_to_fit` on the emitted prefix. - `datafusion-physical-plan`: the duplicate helper is removed; `bytes.rs`, `primitive.rs` and `bytes_view.rs` use the shared one. - `datafusion-functions-aggregate`: `MinMaxStructAccumulator` and `MinMaxBytesAccumulator` use it in `emit_to`. This supersedes apache#22205, whose `ByteViewGroupValueBuilder::take_n` change is included here. ## Are these changes tested? Yes. Unit tests for the helper (both branches and the boundaries) plus the existing `take_n` and `min_max` tests pass. ## Are there any user-facing changes? No. --------- Co-authored-by: RyanJamesStewart <RyanJamesStewart@users.noreply.github.com> Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com> (cherry picked from commit 0add046)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
…ly emit
Which issue does this PR close?
Rationale for this change
When the partial hash aggregation (with GroupOrdering::None) cannot reserve additional memory, it calls self.emit(EmitTo::First(n), where n is the largest integer multiple of self.batch_size, meaning it's close to the length of group_values. Using drain + collect leads to an additional copy of all these n values, since collect allocates a new Vec and drain doesn't modify the original vector's capacity. To avoid copying the largest allocation, choose a strategy which always allocates the minimum between n and remaining.
What changes are included in this PR?
Are these changes tested?
Yes, added unit tests
Are there any user-facing changes?
No