You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "kosiew (via GitHub)" <gi...@apache.org> on 2026/09/25 06:55:54 UTC
[I] Use Exact Hash-Table Allocation Accounting in `GroupValuesPrimitive` [datafusion]
kosiew opened a new issue, #25734:
URL: https://github.com/apache/datafusion/issues/25734
# Related PR
#25188
## Problem
`GroupValuesPrimitive::size()` estimates its hash-table allocation as
`map.capacity() * size_of::<(usize, u64)>()`. This counts only the entry array
and omits hashbrown's control bytes and trailing group allocation. The reported
size is therefore smaller than the retained allocation, especially for small
tables where layout overhead is a larger fraction of the total.
The implementation is in
`datafusion/physical-plan/src/aggregates/group_values/single_group_by/primitive.rs`.
The same file already reports the owner descriptor and the value vector, so the
remaining issue is the map term itself.
## Why it matters
`GroupValues::size()` drives memory-pool reservations and spill decisions. An
under-reported map can allow a query to retain more memory than the pool
believes it owns, and makes primitive single-column grouping inconsistent with
`ArrowBytesMap::size()`, which uses `HashTable::allocation_size()`.
This is a retained-capacity accounting correction, not a change to hashing,
group identity, insertion behavior, or map reuse.
## Invariant / desired behavior
`GroupValuesPrimitive::size()` reports the complete retained allocation of its
`HashTable<(usize, u64)>` exactly once:
- the map term uses `HashTable::allocation_size()`, including control bytes and
trailing layout;
- the value vector and `size_of::<Self>()` remain charged once;
- clearing or emitting values does not stop retained map capacity from being
charged until the allocation is released;
- the result does not depend on logical map length when capacity is retained.
## Proposed direction
Replace the `capacity() * size_of::<(usize, u64)>()` term in
`GroupValuesPrimitive::size()` with `self.map.allocation_size()`, following the
existing `ArrowBytesMap::size()` implementation in
`datafusion/physical-expr-common/src/binary_map.rs`.
Use the existing hashbrown allocation API directly. Do not change the
`insert_accounted` path or introduce allocator-live-byte measurements; this
issue is about making the public memory estimate reflect the allocation layout.
## Scope
### In
- `datafusion/physical-plan/src/aggregates/group_values/single_group_by/primitive.rs`.
- The primitive map contribution to `GroupValuesPrimitive::size()`.
- Focused tests for empty, grown, and retained-after-emit map capacity.
### Out
- Hashing, collision handling, group-id assignment, and emit semantics.
- `GroupValuesRows` or `GroupValuesColumn` changes; they have separate issue
boundaries so each implementation can be reviewed independently.
- Changes to `HashTableAllocExt`, hashbrown, or the memory-pool contract.
- Global allocator-live-byte assertions.
## Acceptance criteria
- [ ] An empty primitive group-values instance reports the map allocation using
the exact hash-table layout rather than an entry-size approximation.
- [ ] After inserting enough distinct values to allocate the map, the map term
equals `HashTable::allocation_size()` and includes control-byte overhead.
- [ ] After `EmitTo::First` or `EmitTo::All`, any retained map capacity remains
included until the map allocation is actually released.
- [ ] Existing grouping and emit results remain unchanged.
- [ ] The implementation does not double count the map allocation through any
other field or accounting helper.
## Tests / verification
- Add or extend focused `primitive.rs` tests that compare the reported size
delta against the independently observed `map.allocation_size()` delta;
avoid asserting a platform-specific absolute byte count.
- Exercise a post-emit/reuse state so a lower logical length cannot hide
retained capacity.
- 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: github-unsubscribe@datafusion.apache.org.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] Use Exact Hash-Table Allocation Accounting in `GroupValuesPrimitive` [datafusion]
Posted by "mohitgurav20 (via GitHub)" <gi...@apache.org>.
mohitgurav20 commented on issue #25734:
URL: https://github.com/apache/datafusion/issues/25734#issuecomment-5845869656
take
Hi @kosiew! I'd like to work on this issue. I've already implemented exact `HashTable::allocation_size()` accounting for `GroupValuesRows` (in PR #25750) and am familiar with `primitive.rs`.
I'll update `GroupValuesPrimitive::size()` to use `self.map.allocation_size()`, add focused tests comparing reported size against `map.allocation_size()` across empty, grown, and post-emit/reuse states, and run all required lints/formatters.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] Use Exact Hash-Table Allocation Accounting in `GroupValuesPrimitive` [datafusion]
Posted by "mohitgurav20 (via GitHub)" <gi...@apache.org>.
mohitgurav20 commented on issue #25734:
URL: https://github.com/apache/datafusion/issues/25734#issuecomment-5866448779
thanks sir for assigning . would make a pr and feel free to review it
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] Use Exact Hash-Table Allocation Accounting in `GroupValuesPrimitive` [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb closed issue #25734: Use Exact Hash-Table Allocation Accounting in `GroupValuesPrimitive`
URL: https://github.com/apache/datafusion/issues/25734
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org