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

Reply via email to