sfluor commented on code in PR #16519:
URL: https://github.com/apache/datafusion/pull/16519#discussion_r2174894499
##########
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:
Maybe to clarify, I did try this approach and the benchmarks shows on-par
performance but I'm wondering if maintenability wise it will be alright given
the multitude of arrow types we would need to support (I only did it for
`int64array` since this is what the benchmarks are using):
```
Gnuplot not found, using plotters backend
array_agg i64 merge_batch no nulls
time: [59.457 ns 60.325 ns 61.260 ns]
change: [+7.3648% +8.6679% +10.200%] (p = 0.00 <
0.05)
Performance has regressed.
Benchmarking array_agg i64 merge_batch all nulls, 100% of nulls point to a
zero length array: Collecting 100 samples in estimated 5.0000 s (135
array_agg i64 merge_batch all nulls, 100% of nulls point to a zero length
array
time: [36.705 ns 37.023 ns 37.408 ns]
change: [+2.9532% +3.9617% +5.0337%] (p = 0.00 <
0.05)
Performance has regressed.
Found 2 outliers among 100 measurements (2.00%)
2 (2.00%) high severe
Benchmarking array_agg i64 merge_batch all nulls, 90% of nulls point to a
zero length array: Collecting 100 samples in estimated 5.0000 s (135M
array_agg i64 merge_batch all nulls, 90% of nulls point to a zero length
array
time: [36.260 ns 36.470 ns 36.725 ns]
change: [+1.7526% +2.6531% +3.4398%] (p = 0.00 <
0.05)
Performance has regressed.
Found 1 outliers among 100 measurements (1.00%)
1 (1.00%) high mild
Benchmarking array_agg i64 merge_batch 30% nulls, 100% of nulls point to a
zero length array: Collecting 100 samples in estimated 5.0134 s (1.4
array_agg i64 merge_batch 30% nulls, 100% of nulls point to a zero length
array
time: [3.6204 µs 3.6433 µs 3.6793 µs]
change: [+0.4207% +0.9048% +1.4450%] (p = 0.00 <
0.05)
Change within noise threshold.
Found 23 outliers among 100 measurements (23.00%)
2 (2.00%) low severe
4 (4.00%) low mild
8 (8.00%) high mild
9 (9.00%) high severe
Benchmarking array_agg i64 merge_batch 70% nulls, 100% of nulls point to a
zero length array: Collecting 100 samples in estimated 5.0176 s (1.4
array_agg i64 merge_batch 70% nulls, 100% of nulls point to a zero length
array
time: [3.5311 µs 3.5430 µs 3.5560 µs]
change: [-3.9977% -2.2613% -0.8169%] (p = 0.00 <
0.05)
Change within noise threshold.
Found 6 outliers among 100 measurements (6.00%)
6 (6.00%) high mild
Benchmarking array_agg i64 merge_batch 30% nulls, 99% of nulls point to a
zero length array: Collecting 100 samples in estimated 5.0411 s (1800
array_agg i64 merge_batch 30% nulls, 99% of nulls point to a zero length
array
time: [2.8977 ms 2.9160 ms 2.9351 ms]
change: [-4.2235% -1.5538% +0.6217%] (p = 0.25 >
0.05)
No change in performance detected.
Benchmarking array_agg i64 merge_batch 70% nulls, 99% of nulls point to a
zero length array: Warming up for 3.0000 s
Warning: Unable to complete 100 samples in 5.0s. You may wish to increase
target time to 6.0s, enable flat sampling, or reduce sample count to 60.
Benchmarking array_agg i64 merge_batch 70% nulls, 99% of nulls point to a
zero length array: Collecting 100 samples in estimated 6.0400 s (5050
array_agg i64 merge_batch 70% nulls, 99% of nulls point to a zero length
array
time: [1.2365 ms 1.2409 ms 1.2454 ms]
change: [-0.2990% +0.4915% +1.1750%] (p = 0.20 >
0.05)
No change in performance detected.
Found 2 outliers among 100 measurements (2.00%)
1 (1.00%) high mild
1 (1.00%) high severe
Benchmarking array_agg i64 merge_batch 30% nulls, 90% of nulls point to a
zero length array: Collecting 100 samples in estimated 5.0041 s (1800
array_agg i64 merge_batch 30% nulls, 90% of nulls point to a zero length
array
time: [2.8680 ms 2.8800 ms 2.8922 ms]
change: [-1.2448% -0.5287% +0.1313%] (p = 0.14 >
0.05)
No change in performance detected.
Benchmarking array_agg i64 merge_batch 70% nulls, 90% of nulls point to a
zero length array: Warming up for 3.0000 s
Warning: Unable to complete 100 samples in 5.0s. You may wish to increase
target time to 6.0s, enable flat sampling, or reduce sample count to 60.
Benchmarking array_agg i64 merge_batch 70% nulls, 90% of nulls point to a
zero length array: Collecting 100 samples in estimated 6.0455 s (5050
array_agg i64 merge_batch 70% nulls, 90% of nulls point to a zero length
array
time: [1.2301 ms 1.2346 ms 1.2399 ms]
change: [-15.122% -9.6579% -5.7337%] (p = 0.00 <
0.05)
Performance has improved.
Found 5 outliers among 100 measurements (5.00%)
3 (3.00%) high mild
2 (2.00%) high severe
Benchmarking array_agg i64 merge_batch 30% nulls, 50% of nulls point to a
zero length array: Collecting 100 samples in estimated 5.1633 s (1800
array_agg i64 merge_batch 30% nulls, 50% of nulls point to a zero length
array
time: [2.8908 ms 2.9089 ms 2.9333 ms]
change: [-15.091% -10.045% -5.6585%] (p = 0.00 <
0.05)
Performance has improved.
Found 2 outliers among 100 measurements (2.00%)
1 (1.00%) high mild
1 (1.00%) high severe
Benchmarking array_agg i64 merge_batch 70% nulls, 50% of nulls point to a
zero length array: Warming up for 3.0000 s
Warning: Unable to complete 100 samples in 5.0s. You may wish to increase
target time to 6.0s, enable flat sampling, or reduce sample count to 60.
Benchmarking array_agg i64 merge_batch 70% nulls, 50% of nulls point to a
zero length array: Collecting 100 samples in estimated 6.0271 s (5050
array_agg i64 merge_batch 70% nulls, 50% of nulls point to a zero length
array
time: [1.2194 ms 1.2227 ms 1.2266 ms]
change: [-10.366% -7.3667% -4.7084%] (p = 0.00 <
0.05)
Performance has improved.
Found 8 outliers among 100 measurements (8.00%)
7 (7.00%) high mild
1 (1.00%) high severe
Benchmarking array_agg i64 merge_batch 30% nulls, 0% of nulls point to a
zero length array: Collecting 100 samples in estimated 5.0895 s (1800
array_agg i64 merge_batch 30% nulls, 0% of nulls point to a zero length array
time: [2.9292 ms 2.9877 ms 3.0750 ms]
change: [+0.6420% +2.9278% +6.0458%] (p = 0.02 <
0.05)
Change within noise threshold.
Found 6 outliers among 100 measurements (6.00%)
3 (3.00%) high mild
3 (3.00%) high severe
Benchmarking array_agg i64 merge_batch 70% nulls, 0% of nulls point to a
zero length array: Warming up for 3.0000 s
Warning: Unable to complete 100 samples in 5.0s. You may wish to increase
target time to 6.3s, enable flat sampling, or reduce sample count to 60.
Benchmarking array_agg i64 merge_batch 70% nulls, 0% of nulls point to a
zero length array: Collecting 100 samples in estimated 6.3183 s (5050
array_agg i64 merge_batch 70% nulls, 0% of nulls point to a zero length array
time: [1.2424 ms 1.2479 ms 1.2540 ms]
change: [-8.0349% -5.5341% -3.2153%] (p = 0.00 <
0.05)
Performance has improved.
Found 7 outliers among 100 measurements (7.00%)
2 (2.00%) high mild
5 (5.00%) high severe
```
--
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
