alamb commented on code in PR #24099:
URL: https://github.com/apache/datafusion/pull/24099#discussion_r3740551516


##########
datafusion/functions-aggregate/src/count.rs:
##########
@@ -774,7 +779,7 @@ impl GroupsAccumulator for CountGroupsAccumulator {
         Ok(vec![state_array])
     }
     fn size(&self) -> usize {
-        self.counts.capacity() * size_of::<usize>()
+        vec_capacity_bytes(&self.counts)

Review Comment:
   I could have s
   
   I agree this is more accurate, but i am not sure it will make any different 
for 64 bit machines (where size of i64 and usize are the same)
   
   ```rust
   struct CountGroupsAccumulator {
       /// Count per group.
       ///
       /// Note this is an i64 and not a u64 (or usize) because the
       /// output type of count is `DataType::Int64`. Thus by using `i64`
       /// for the counts, the output [`Int64Array`] can be created
       /// without copy.
       counts: Vec<i64>,
   }
   ```



##########
datafusion/functions-aggregate/src/sum.rs:
##########
@@ -617,7 +617,8 @@ impl Accumulator for SlidingDistinctSumAccumulator {
     }
 
     fn size(&self) -> usize {
-        size_of_val(self)
+        // Estimate the owned map buckets; implementation-specific control 
bytes are excluded.
+        size_of_val(self) + self.counts.capacity() * size_of::<(i64, usize)>()

Review Comment:
   similarly here I feel like the memory accounting for Vec and Hash map 
already exist somewhere -- it would be great to find that and reuse them



##########
datafusion/functions-aggregate/src/count.rs:
##########
@@ -620,6 +620,11 @@ impl Accumulator for CountAccumulator {
     }
 }
 
+/// Returns bytes reserved by a vector's backing allocation, excluding the 
`Vec` itself.
+fn vec_capacity_bytes<T>(values: &Vec<T>) -> usize {

Review Comment:
   I thought we already had this function (or something like it) somewhere. 



-- 
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]

Reply via email to