andygrove opened a new issue, #6252: URL: https://github.com/apache/datafusion-comet/issues/6252
### Describe the bug The grouped accumulators behind integer `SUM` report the size of their struct instead of the state they hold. `SumIntGroupsAccumulatorLegacy`, `SumIntGroupsAccumulatorAnsi` and `SumIntGroupsAccumulatorTry` all return `std::mem::size_of_val(self)` from `GroupsAccumulator::size()` ([sum_int.rs#L534](https://github.com/apache/datafusion-comet/blob/bc4be39964cbe9cdb5f2a949740a8164e6b5755b/native/spark-expr/src/agg_funcs/sum_int.rs#L534), [#L687](https://github.com/apache/datafusion-comet/blob/bc4be39964cbe9cdb5f2a949740a8164e6b5755b/native/spark-expr/src/agg_funcs/sum_int.rs#L687), [#L895](https://github.com/apache/datafusion-comet/blob/bc4be39964cbe9cdb5f2a949740a8164e6b5755b/native/spark-expr/src/agg_funcs/sum_int.rs#L895)). That is the `Vec` header, not the `sums: Vec<Option<i64>>` it points to, so each one leaves out 16 bytes per group. The `Try` variant also leaves out `has_all_nulls`. DataFusion sizes a hash aggregate's reservation from its accumulators' `size()` plus the group values (`AggregateHashTable::memory_size` in datafusion-physical-plan 55.1.0). A grouped aggregate with integer sums therefore reserves less than it holds, spills later than it should, and can take the executor past `spark.memory.offHeap.size` without the pool noticing. Every `SUM` over `Int8`, `Int16`, `Int32` or `Int64` goes through these accumulators ([planner.rs#L2869-L2873](https://github.com/apache/datafusion-comet/blob/bc4be39964cbe9cdb5f2a949740a8164e6b5755b/native/core/src/execution/planner.rs#L2869-L2873)). For example, four integer sums over 10M groups hold about 640 MB that the aggregate never reserves. The decimal accumulator already gets this right ([sum_decimal.rs#L618-L621](https://github.com/apache/datafusion-comet/blob/bc4be39964cbe9cdb5f2a949740a8164e6b5755b/native/spark-expr/src/agg_funcs/sum_decimal.rs#L618-L621)). ### Steps to reproduce Run `update_batch` on one of these accumulators over 1M distinct group indices, then call `size()`. It stays at the struct size instead of growing past 16 MB. I found this by reading the code and haven't measured the effect on a query. ### Expected behavior `size()` includes the capacity of `sums`, and of `has_all_nulls` for the `Try` variant, the way `SumDecimalGroupsAccumulator::size()` does. ### Additional context These accumulators date from #2600 and #3054, so 1.0.0 and 1.1.0 are affected. -- 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]
