alamb commented on code in PR #16519: URL: https://github.com/apache/datafusion/pull/16519#discussion_r2167112157
########## datafusion/functions-aggregate/src/array_agg.rs: ########## @@ -341,12 +341,20 @@ impl Accumulator for ArrayAggAccumulator { Some(values) => { // Make sure we don't insert empty lists if !values.is_empty() { - self.values.push(values); + // The ArrayRef might be holding a reference to its original input buffer, so + // storing it here directly copied/compacted avoids over accounting memory + // not used here. + self.values + .push(make_array(copy_array_data(&values.to_data()))); } Review Comment: I wonder if we should add a special case to copy_array_data to avoid copying the data when it already is only a single row / has no offset 🤔 Right now it seems to copy the data unconditionally which is a non trivial overhead on each row 🤔 https://github.com/apache/datafusion/blob/a87d6f21c96ea592c6e9f2a3f566d0c9862031e5/datafusion/common/src/scalar/mod.rs#L3564-L3567 Perhaps we can do that as a follow of PR -- 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-unsubscr...@datafusion.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org For additional commands, e-mail: github-h...@datafusion.apache.org