kosiew opened a new issue, #25736:
URL: https://github.com/apache/datafusion/issues/25736
# related PR
#25188
## Problem
`GroupValuesColumn` stores group hashes and `GroupIndexView` values in a
hashbrown `HashTable`, but its reported `map_size` is maintained using entry
capacity arithmetic. That approximation charges about the entry portion of
the allocation while omitting hashbrown control bytes and trailing layout.
The resulting `GroupValues::size()` is lower than the memory retained by the
map, particularly for small or recently grown tables.
The affected implementation is in
`datafusion/physical-plan/src/aggregates/group_values/multi_group_by/mod.rs`.
`GroupValuesColumn::size()` already accounts separately for group-column
builders, collision-chain vectors, emit scratch, vectorized-operation
buffers,
hash scratch, and its owner descriptor. Only the hash-table term is covered
by
this issue.
## Why it matters
`GroupValuesColumn` is the vectorized multi-column grouping path and can
retain
large maps alongside its per-column builders. Under-reporting the map weakens
memory-pool enforcement and spill decisions, while making this implementation
inconsistent with `ArrowBytesMap::size()`, which already uses
`HashTable::allocation_size()`.
This is an accounting-only correction. Collision-chain behavior, vectorized
buffers, emit/reuse behavior, and group-value materialization must remain
unchanged.
## Invariant / desired behavior
`GroupValuesColumn::size()` reports the complete retained allocation of its
`HashTable<(u64, GroupIndexView)>` exactly once:
- the map term uses `HashTable::allocation_size()`, including control bytes
and
trailing layout;
- retained capacity remains charged after partial emit, full emit, or reuse
until the table allocation is released;
- collision-chain list allocations and all other existing owner terms remain
separate and are not folded into or double counted by the map term;
- the reported result is independent of logical map length when capacity is
retained.
## Proposed direction
At `GroupValuesColumn::size()`, replace the approximate `map_size`
contribution
with `self.map.allocation_size()`, following the established
`ArrowBytesMap::size()` precedent.
Trace `map_size` and its `insert_accounted` updates afterward. If it exists
only
to report memory, remove the redundant field and use of
`HashTableAllocExt`; if another path still requires it, keep that path but do
not add both the incremental estimate and `allocation_size()` to the reported
total.
Retain the existing accounting for `group_index_lists`,
`emit_group_index_list_buffer`, `vectorized_operation_buffers`,
`hashes_buffer`, group-column builders, and `size_of::<Self>()`. Do not alter
their lifecycle merely to change the map formula.
## Scope
### In
-
`datafusion/physical-plan/src/aggregates/group_values/multi_group_by/mod.rs`.
- The hash-table term and any now-redundant incremental map-accounting state.
- Focused tests for map growth, forced collisions, emit/reuse, and vectorized
operation states.
### Out
- Collision-chain representation or group-index list policies.
- Vectorized-operation buffer sizing and emit scratch lifecycle.
- `GroupValuesPrimitive` or `GroupValuesRows`; they have separate issues.
- Changes to `HashTableAllocExt`, hashbrown internals, or global allocator
measurements.
## Acceptance criteria
- [ ] A grown `GroupValuesColumn` reports the map contribution from
`HashTable::allocation_size()`, including control-byte overhead.
- [ ] The map allocation is counted exactly once, with no parallel
`map_size` estimate in the total.
- [ ] Forced-collision tests continue to report collision-chain list capacity
separately from the hash-table allocation.
- [ ] After partial and full emit, retained map capacity remains charged
until
released, while all existing scratch-buffer accounting remains correct.
- [ ] Vectorized and scalarized grouping produce unchanged group values,
group IDs, and output arrays.
## Tests / verification
- Extend the existing `multi_group_by` capacity/reuse tests to compare the
map-related reported delta with `HashTable::allocation_size()` after
growth.
- Keep a forced-collision case and assert that map allocation and
`group_index_lists` are independently represented, avoiding a hard-coded
hashbrown layout size.
- Cover both a retained-capacity post-emit state and a vectorized intern
path;
verify subsequent reuse still produces correct results.
- 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]
