fix: account for memory that we still hold on when splitting batch - #24852
fix: account for memory that we still hold on when splitting batch#24852rluvaton wants to merge 2 commits into
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #24852 +/- ##
==========================================
+ Coverage 81.59% 81.60% +0.01%
==========================================
Files 1123 1123
Lines 408413 408974 +561
Branches 408413 408974 +561
==========================================
+ Hits 333230 333740 +510
- Misses 55615 55621 +6
- Partials 19568 19613 +45 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| self.reservation.try_resize(hash_table.memory_size()); | ||
| let resize_result = self | ||
| .reservation | ||
| .try_resize(hash_table.memory_size() + state_batch_size); |
There was a problem hiding this comment.
This follows a failed resize, so charging the retained batch can make early emission fail with OOM after the batch is already allocated. If it cannot be retained, preserve progress (for example, yield it whole).
| PartialHashAggregateState::EmittingOnMemoryPressure { | ||
| hash_table, | ||
| remaining_groups: materialized_group_states, | ||
| batch_memory_size: state_batch_size, |
There was a problem hiding this comment.
MaterializedAggregateOutput already implements this materialize-once/slice-many lifecycle. Reuse it with a split child MemoryReservation, and test that the reservation drops after the last slice before polling to Done.
3 participants
Which issue does this PR close?
N/A
Rationale for this change
hold on batches between emit should be reserved
What changes are included in this PR?
reserve memory + test
What is the testing strategy for this PR?
unit test
Are there any user-facing changes?
no