Fix GroupValues retained memory accounting - #25188
Draft
kosiew wants to merge 19 commits into
Draft
Conversation
- GroupValues::size() charges owner + retained buffers. - Column: added group-index, emit, vectorized, and vec backing. - Added capacity/reuse tests. - Updated tight spill test pools.
…) calls - Updated `row.rs` to subtract inline `RowConverter`/`Rows` descriptors when calculating nested `.size()` calls. - The outer `GroupValuesRows` descriptor is now charged only once, preventing duplicate size accounting. - This resolves overcounting of size for nested rows, improving the accuracy of size calculations. - No functional changes to the row data itself; only the size accounting logic has been refined. - Enhances performance and reliability for operations that rely on precise size metrics.
- Added a dedicated integration test in `datafusion/physical-plan/src/aggregates/group_values/multi_group_by/mod.rs` - The test validates scratch‑capacity and reuse behavior across: - Five distinct vectorized buffers - The `emit_scratch` logic path - Verifies that buffer growth (delta) correctly allocates additional capacity - Confirms that clearing the buffers retains the allocated charge, ensuring proper reuse without unnecessary reallocations
…pe row‑backed inline nested descriptors, add primitive row‑backed multi‑column tests, and enable 1,024 spill limit - All missing GroupColumn owner descriptors charged. - Row‑backed inline nested descriptors deduped. - Added primitive, row‑backed, multi‑column tests. - Existing 1,024 spill limit now spills; ordered test passes.
- Added `size_retains_reusable_buffers_after_emit` test to verify reusable buffers are retained after emit. - Checks `rows_buffer` and hash scratch retention after `EmitTo::First`. - Re‑interns values and rechecks accounting to ensure correct behavior after re‑emission.
- Moved comment to correct emit test.
- Increase SLT peak from **9.2 KB** to **9.4 KB**. - Retain spill test plan while adding assertions for **spill count** and **spill bytes**. - Update pool sizes: **non‑distinct 1,000,000** entries and **DISTINCT 4,256,000** entries. - Revised overall plan to reflect the metric adjustments and new test assertions.
github-actions
Bot
added
core
…TINCT pool - Modified the partial‑aggregation logic to skip only when memory limits are exceeded, rather than under broader conditions. - Reduced the DISTINCT pool size from `4_256_000` to `1_000_000` to lower memory consumption and improve performance in constrained environments.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #25188 +/- ##
==========================================
- Coverage 81.91% 81.90% -0.02%
==========================================
Files 1132 1133 +1
Lines 421280 424954 +3674
Branches 421280 424954 +3674
==========================================
+ Hits 345111 348041 +2930
- Misses 55767 56274 +507
- Partials 20402 20639 +237 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…llability spill test - Updated `nested_nullability.rs` test budget from `1_000_000` to `4_256_000`. - Identified cause: DISTINCT struct state experiences a transient peak that exhausts the 1 MiB fair pool during spilling. - Adjusted the budget to accommodate the peak memory usage, ensuring the test passes reliably.
- Pins `target_partitions=1` for the DISTINCT spill scenario, ensuring a deterministic topology. - Retains the 1 MiB pool and associated spill assertions to validate correctness. - Removes scheduler‑dependent Partial/Final aggregate competition that was causing out‑of‑memory (OOM) failures.
…iation during spill recovery - After materializing spill state, rebuild accumulators from empty equivalents. - Releases retained accumulator capacity before reservation reconciliation. - Covers hash + ordered single/final spill paths.
- Updated `nested_nullability.rs` to wrap `FairSpillPool` in `TrackConsumersPool`. - Next OOM includes: - consumer names - spillability - live reservations - peak bytes - Fair allocation behavior remains unchanged.
…ll regression and adjust spill behavior - Disables `single_distinct_aggregation_to_group_by` only in the DISTINCT spill regression. - Asserts that the physical plan contains exactly one `AggregateExec`. - Retains the 1‑partition + 1 MiB fair spill pool configuration. - Leaves rewrite coverage elsewhere unchanged.
…st and rebuild empty partial table - Partial OOM drain now emits states via `EmitTo::First(batch_size)` instead of full materialization. - Eliminates full state‑batch materialization before slicing, reducing memory overhead. - Rebuilds an empty partial table after drain and resumes input processing to maintain continuity. - Addresses a 500 B grouping‑set regression and adds an `early_emit_count` assertion to catch premature emissions. - Updates partial hash tests to verify bounded incremental output behavior.
…r messages to match FinalHashAggregateStream
kosiew
force-pushed
the
memcalc-11-23393
branch
from
September 12, 2026 16:10
deb8299 to
dde38d4
Compare
…egate memory spill test
…dd regression test - Shrinks the group‑index and emits the scratch vector. - Releases and rebuilds the vector’s scratch capacity. - Resets the Boolean bitmap capacity safely. - Adds a zero‑reset regression test to verify the behavior.
…he ownership - `cached_values` changed from `Weak<dyn Array>` to `Option<Weak<dyn Array>>`. - Cache retains its identity hint; it no longer owns or pins the input allocation. - Added regression test to confirm a live identity cache while the input dictionary is released after drop. - Reused `clear()` in the shrink path, resolving warnings‑denied dead‑code lint.
…_by): shrink_to remaining_row_indices instead of clear Optimizes memory usage by calling `shrink_to(num_rows)` on `self.remaining_row_indices` rather than `clear()`. This avoids reallocating a new vector each time the buffer is reused, reducing overhead and improving performance in vectorized aggregations.
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
2 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.
Which issue does this PR close?
sizefunctions #23393Rationale for this change
Aggregate memory accounting can under-report retained memory in
GroupValues. In particular, owner descriptors and reusable allocations such as group-index lists, emit scratch storage, and vectorized-operation buffers can remain allocated without being reflected inGroupValues::size().Under memory pressure, partial hash aggregation can also materialize the complete aggregate state before slicing it into output batches. This can require the full state to fit in memory at the point where early emission is intended to relieve memory pressure.
This PR makes retained-state accounting include the memory owned by the
GroupValuesimplementations and changes partial aggregate early emission to drain state incrementally in bounded batches.What changes are included in this PR?
GroupValuesPrimitive,GroupValuesRows, andGroupValuesColumnmemory accounting to include their owner descriptors and retained owned allocations.GroupColumnowner state.clear_shrinkis requested.Are these changes tested?
Yes. The patch adds and updates focused regression coverage, including:
size_includes_boxed_owner_descriptorsize_includes_owner_and_retained_allocationssize_includes_boxed_primitive_and_row_backed_ownerssize_includes_collision_emit_and_vectorized_bufferssize_retains_vectorized_and_emit_scratch_capacityclear_shrink_releases_vectorized_and_emit_scratch_capacitysize_retains_reusable_buffers_after_emithash_cache_does_not_retain_dictionary_valuestest_partial_hash_stream_incrementally_emits_on_memory_pressuretest_partial_hash_stream_emits_bounded_batch_when_full_state_does_not_fittest_partial_hash_stream_releases_groups_after_incremental_emitExisting spilling and nested-nullability aggregate tests are also adjusted to exercise spilling under the corrected memory accounting, including native
DISTINCTaggregation.Are there any user-facing changes?
There are no public API changes.
Queries using hash aggregation under constrained memory should have more accurate memory accounting, and partial aggregation can release state incrementally during memory pressure instead of requiring the complete partial aggregate state to be materialized before emission.
LLM-generated code disclosure
This PR includes LLM-generated code and comments. All LLM-generated content has been manually reviewed.