Nook001 opened a new issue, #26937:
URL: https://github.com/apache/datafusion/issues/26937
### Describe the bug
`GenericDistinctBuffer::size()` can significantly underestimate retained
memory because it calculates the HashSet allocation estimate using
`self.values.len()` rather than `self.values.capacity()`.
A HashSet may retain substantially more allocated capacity than its current
number of distinct elements, especially when processing inputs containing many
duplicates or merging overlapping distinct states.
The current implementation is:
```rust
pub fn size(&self) -> usize {
let num_elements = self.values.len();
let fixed_size = size_of_val(self) + size_of_val(&self.values);
estimate_memory_size::<T::Native>(
num_elements,
fixed_size,
).unwrap()
}
```
This implementation has two accounting issues:
1. **Retained capacity is undercounted.** The estimate is based on the
number of stored elements, even when the HashSet retains a much larger
allocation. 实际保留的容量被低估了。该估算值是根据存储的元素数量来确定的，而实际上，HashSet 所分配的内存空间要大得多。
2. **The HashSet structure is counted twice.** `size_of_val(self)` already
includes the embedded `self.values` field, so adding
`size_of_val(&self.values)` introduces duplicate fixed-size accounting.
As a result, the reported memory usage can differ substantially from the
allocation retained by the buffer.
### To Reproduce
We reproduced the issue using an isolated primitive `COUNT(DISTINCT)`
accumulator test with tracked heap allocations.
The representative case processes 8,192 input values containing only 128
distinct `Int64` values, resulting in a HashSet whose retained capacity is much
larger than its length.
| Measurement | Bytes |
| --------------------------------- | ------: |
| Original `size()` estimate | 2,344 |
| Measured retained heap allocation | 147,472 |
| Corrected `size()` estimate | 147,456 |
The original implementation reports approximately 1.6% of the measured
retained allocation.
The corrected estimate differs from the measurement by only 16 bytes in this
case.
These values were measured in the same environment. Exact allocations may
vary with the allocator and HashSet implementation.
We also verified the effect on memory-limit enforcement:
- With the original accounting, a query can retain substantially more memory
than the amount charged to its `MemoryReservation`.
- With corrected accounting, a sufficiently restrictive memory budget
produces `ResourcesExhausted`.
- Increasing the memory budget allows the query to complete with the
expected SQL results.
The issue is therefore observable both in isolated allocation measurements
and in memory-limit behavior.
### Expected behavior
`GenericDistinctBuffer::size()` should provide a reasonable estimate of its
retained memory allocation, including the capacity of its internal HashSet.
The estimate should:
- Account for retained HashSet capacity, including unused allocated buckets.
- Avoid double-counting the embedded HashSet structure.
- Correctly handle empty and non-empty collections.
- Remain consistent with the existing memory-accounting contract.
The estimate need not be byte-exact across different allocators, but it
should not substantially underestimate retained capacity.
### Additional context
## Impact
Inaccurate accumulator memory estimates can cause DataFusion's `MemoryPool`
to under-account retained memory.
This weakens memory-budget enforcement and may allow queries to consume
substantially more memory than their reservations indicate.
The proposed correction improves accounting accuracy. It does not directly
reduce actual allocations or change the SQL aggregation algorithm.
## Related work
- **Part of #23393** — The broader audit of inaccurate `size()`
implementations in DataFusion.
- **Related to #25650** — The broader investigation into differences between
`MemoryPool` reservations and actual allocations.
- **Related to #24099** — Previously merged capacity-accounting fixes for
other aggregate accumulators.
- **Related to #24857** — Previously merged changes addressing HashTable
allocation and accounting for grouped string `COUNT(DISTINCT)` aggregates.
This issue concerns the remaining capacity-accounting defect in
`GenericDistinctBuffer` and is independent of those earlier fixes.
The problem was discovered while investigating the multiple-distinct
aggregation performance issue #20942.
### Willingness to contribute
I plan to contribute a fix for this bug independently
--
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]
