Fix GroupValues retained memory accounting - #25188
Draft
kosiew wants to merge 15 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.94% +0.02%
==========================================
Files 1132 1133 +1
Lines 421280 423753 +2473
Branches 421280 423753 +2473
==========================================
+ Hits 345111 347248 +2137
- Misses 55767 55900 +133
- Partials 20402 20605 +203 ☔ 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
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
GroupValues::size()can underreport memory retained by group-value state. In particular, the owner descriptors are not consistently included, and the column-based implementation does not account for several retained allocations used for collision handling, emit scratch space, and vectorized operations.This can make aggregate memory accounting lower than the memory actually retained by
GroupValues, including after reusable buffers have been cleared or groups have been emitted.This PR updates the accounting so each
GroupValuesimplementation reports its owner descriptor and owned retained allocations exactly once, using allocation capacity where appropriate.What changes are included in this PR?
This PR:
GroupValuesPrimitive,GroupValuesRows, andGroupValuesColumnmemory accounting.GroupColumnowner descriptors in theirsize()implementations.RowsandRowConverterdescriptors.GroupValuesColumnallocation backing its group-column collection.Are these changes tested?
Yes. This PR adds focused regression tests covering:
GroupValuesPrimitivememory accounting.GroupValuesRowsmemory accounting.GroupColumnowner descriptors.GroupValuesColumn.The tests compute expected sizes from the relevant owner descriptors and retained allocations rather than relying on global allocator measurements.
Are there any user-facing changes?
There are no API changes.
Aggregate memory accounting is more complete, so workloads using these
GroupValuesimplementations may report or reserve more memory than before. This can affect when memory limits cause aggregation to spill, reflecting memory that was already retained but previously omitted fromGroupValues::size().LLM-generated code disclosure
This PR includes LLM-generated code and comments. All LLM-generated content has been manually reviewed.