jayzhan211 commented on code in PR #25497:
URL: https://github.com/apache/datafusion/pull/25497#discussion_r4053379692
##########
datafusion/functions-aggregate/src/array_agg.rs:
##########
@@ -1535,11 +1539,34 @@ impl OrderSensitiveArrayAggAccumulator {
}
let start = self.entries.len();
- let batch_idx = self.batches.len();
- self.batches.push(values);
- self.entries.extend(
- (0..row_count).map(|row_idx| OrderedArrayAggEntry { batch_idx,
row_idx }),
- );
+ let (batch_idx, row_offset) = match self.batches.last() {
+ Some(last_batch)
+ if last_batch.len() + row_count <=
ORDERED_ARRAY_AGG_COALESCE_ROWS =>
Review Comment:
Row-only threshold → every 1-row update re-copies the whole tail batch (≤63
rows) regardless of width. Measured, 2048 × 1-row `update_batch`, Utf8 payload,
update only, main → PR: 16 KB strings 1.25 → 17.0 ms (13.7×) for 1.2% less
`size()`; 256 KB strings 12.3 → 288 ms (23×) for 0.07%. Per-array overhead
saved is only ~170 B, so coalescing should stop once the tail is no longer tiny.
With the cap below: 16 KB 1.69 ms, 256 KB 18.9 ms, Int64 `size()` unchanged
(104,661 vs 445,141 on main).
```diff
+/// Skip coalescing once the tail batch is large enough that the fixed
+/// per-array overhead is negligible relative to the payload.
+const ORDERED_ARRAY_AGG_COALESCE_BYTES: usize = 4096;
+
let (batch_idx, row_offset) = match self.batches.last() {
Some(last_batch)
- if last_batch.len() + row_count <=
ORDERED_ARRAY_AGG_COALESCE_ROWS =>
+ if last_batch.len() + row_count <=
ORDERED_ARRAY_AGG_COALESCE_ROWS
+ && last_batch.get_array_memory_size()
+ + values.get_array_memory_size()
+ <= ORDERED_ARRAY_AGG_COALESCE_BYTES =>
{
```
Please add a wide-payload case (e.g. 4 KB Utf8, 1 row per `update_batch`) to
the new bench. Separately, the PR's own bench shows +29–42% at 1 row/update and
+18–35% at 8 rows/update for Int64 vs main (`concat` fixed cost, not memcpy) —
worth stating in the description as the CPU-for-memory trade
--
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]
