refactor(hash-aggr): Let partial-reduce mode aggregation error under memory limit - #24486
Open
2010YOUY01 wants to merge 1 commit into
Open
refactor(hash-aggr): Let partial-reduce mode aggregation error under memory limit#244862010YOUY01 wants to merge 1 commit into
2010YOUY01 wants to merge 1 commit into
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #24486 +/- ##
==========================================
+ Coverage 81.24% 81.99% +0.75%
==========================================
Files 1113 1117 +4
Lines 392744 409120 +16376
Branches 392744 409120 +16376
==========================================
+ Hits 319090 335467 +16377
- Misses 54900 55165 +265
+ Partials 18754 18488 -266 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
How about we come out the idea design for example early emission, instead of revert it back to error state 🤔 |
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 free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Which issue does this PR close?
Part of #22710
Rationale for this change
This PR is part of the aggregation refactor/migration; see the epic for details. Specifically, it implements larger-than-memory behavior for the PartialReduce aggregation mode. Note the OOM behavior differs from the legacy implementation , due to the following reasons:
Behavior difference
For
PartialReduceaggregation, this PR handles the memory limit by returning an error immediately. The existing behavior whenPartialReduceruns out of memory is to spill and then perform final streaming aggregation. This behavior is implemented in the legacy code:Why change the behavior
PartialReduceaggregation—reducing data locally before shuffle (Leverage PartialReduce AggregationExec mode to drastically reduce shuffle size datafusion-contrib/datafusion-distributed#360) think early emission may actually be the intended behavior.So I tend to believe the existing behavior was an auto-completion from AI, instead of an intentional design decision. Therefore, this PR takes the more conservative approach of returning an error on OOM. We can revisit and improve this behavior in follow-up PRs.
What changes are included in this PR?
partial_reduce_stream.rs: key changes, let aggregation update memory usage during execution, and return error when OOMAre these changes tested?
UTs
Are there any user-facing changes?