Copilot commented on code in PR #23399:
URL: https://github.com/apache/datafusion/pull/23399#discussion_r3598863734
##########
datafusion/functions-aggregate/src/count.rs:
##########
@@ -555,7 +555,17 @@ impl Accumulator for SlidingDistinctCountAccumulator {
}
fn size(&self) -> usize {
+ // Mirrors `DistinctCountAccumulator::full_size`: self + HashMap
+ // bucket array + per-key inner heap + DataType inner heap.
Review Comment:
The PR title mentions adding `mimalloc` tests, but the diff only changes
`SlidingDistinctCountAccumulator::size()` and does not add any allocator-based
size assertions. Either update the title/description to reflect the actual
change, or add the intended tests so this fix is protected against regressions.
##########
datafusion/functions-aggregate/src/count.rs:
##########
@@ -555,7 +555,17 @@ impl Accumulator for SlidingDistinctCountAccumulator {
}
fn size(&self) -> usize {
+ // Mirrors `DistinctCountAccumulator::full_size`: self + HashMap
+ // bucket array + per-key inner heap + DataType inner heap.
size_of_val(self)
+ + (size_of::<ScalarValue>() + size_of::<usize>()) *
self.counts.capacity()
+ + self
+ .counts
+ .keys()
+ .map(|k| k.size() - size_of_val(k))
+ .sum::<usize>()
+ + self.data_type.size()
+ - size_of_val(&self.data_type)
}
Review Comment:
`self.counts.capacity() * (size_of::<ScalarValue>() + size_of::<usize>())`
underestimates the actual heap reserved by hashbrown `HashMap` (bucket count is
> capacity and there is per-bucket control-byte overhead). This can still
under-report memory and weaken `MemoryPool` enforcement for large
COUNT(DISTINCT ...) windows. Consider using the existing `DFHeapSize`
implementation for `HashMap`/`DataType` to include hashbrown overhead and avoid
manual per-key iteration.
--
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]
