kosiew opened a new issue, #25735:
URL: https://github.com/apache/datafusion/issues/25735
# related PR
#25188
## Problem
`GroupValuesRows::size()` relies on the incrementally maintained `map_size`,
which is built from `HashTableAllocExt::insert_accounted` and adds
`size_of::<(u64, usize)>()` for each reserved growth. That accounting models
the entry array but does not represent hashbrown's complete allocation
layout,
including control bytes and the trailing group allocation. It can therefore
under-report the retained map allocation and can diverge from the table's
actual capacity after clear, retain, or reuse operations.
The affected implementation is in
`datafusion/physical-plan/src/aggregates/group_values/row.rs`. Its other
terms
cover row storage, converter state, scratch buffers, and the owner
descriptor;
this issue is limited to replacing the approximate map accounting.
## Why it matters
The row fallback is used for schemas that cannot use a specialized
group-value
builder, including nested and multi-column keys. Its map can be a significant
part of retained state. Since `GroupValues::size()` feeds memory-pool
accounting, omitting the table's control allocation weakens spill-pressure
estimates exactly on the general-purpose path.
The existing `ArrowBytesMap::size()` implementation demonstrates the intended
contract: use `HashTable::allocation_size()` for the whole table rather than
reconstructing the entry allocation from capacity and element size.
## Invariant / desired behavior
`GroupValuesRows::size()` charges the complete retained allocation of its
`HashTable<(u64, usize)>` exactly once:
- the map term is the current table allocation, including control bytes and
trailing layout;
- map capacity retained after `EmitTo::First`, `clear_shrink`, or reuse
remains
charged until the table releases it;
- row-converter, row-store, scratch-vector, and descriptor terms remain
unchanged and are not counted through the map term;
- grouping behavior and the existing `map_size` insertion semantics are not
changed unless the implementation proves that the incremental field is no
longer needed.
## Proposed direction
At the `GroupValuesRows::size()` owner boundary, replace the use of
`self.map_size` for the final reported map allocation with
`self.map.allocation_size()`, matching the `ArrowBytesMap` precedent.
Then trace all writes and reads of `map_size`. If it is only an accounting
side channel after this change, remove it and the associated
`insert_accounted` plumbing; otherwise retain it only for a separate runtime
purpose and ensure it is not added to `size()` alongside
`allocation_size()`.
The implementation must preserve `clear_shrink` and emit behavior. Do not
replace this with allocator instrumentation or a logical-length estimate.
## Scope
### In
- `datafusion/physical-plan/src/aggregates/group_values/row.rs`.
- The map-size formula and any now-dead incremental accounting field/helper
usage owned by `GroupValuesRows`.
- Deterministic tests covering growth, emit/reuse, and clear/shrink states.
### Out
- Row encoding, converter sizing, scratch-buffer policy, and spill
algorithms.
- `GroupValuesPrimitive` or `GroupValuesColumn`; they have separate issues.
- Changes to the shared `HashTableAllocExt` API or hashbrown internals.
- Global allocator-live-byte measurements.
## Acceptance criteria
- [ ] A grown `GroupValuesRows` reports a map contribution equal to
`self.map.allocation_size()`, including control-byte overhead.
- [ ] The map allocation is counted exactly once; `map_size` is not added in
parallel with `allocation_size()`.
- [ ] After partial emit, full emit, and `clear_shrink`, reported size
follows
retained table capacity rather than logical entry count.
- [ ] Subsequent interning after emit or shrink produces the same group
values
and group IDs as before the accounting change.
- [ ] Existing row-storage and scratch-buffer accounting remains unchanged.
## Tests / verification
- Add or extend `row.rs` tests that grow the table with distinct multi-column
or nested-compatible keys and compare the map-size delta with the table's
allocation-size delta, without hard-coding allocator layout bytes.
- Cover at least one retained-capacity state after emit and one after
`clear_shrink`, then reintern values to verify reuse.
- Run `cargo test -p datafusion-physical-plan`.
- Before merge, run `cargo fmt --all`,
`cargo clippy --all-targets --all-features -- -D warnings`, and
`./dev/rust_lint.sh`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
