You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "rluvaton (via GitHub)" <gi...@apache.org> on 2025/01/25 19:44:22 UTC
[PR] perf(array-agg): add fast path for array agg for `merge_batch` [datafusion]
rluvaton opened a new pull request, #14299:
URL: https://github.com/apache/datafusion/pull/14299
## Which issue does this PR close?
N/A
## Rationale for this change
We want faster DataFusion
## What changes are included in this PR?
Added fast path for `array_agg` accumulator for `merge_batch` that return the underlying values of the list in case there is no null that point to the non empty list starting from the first valid index and until last valid index
## Are these changes tested?
Existing tests
## Are there any user-facing changes?
Nope
-------
I see that there is no benchmark for `array_agg`, so I will try to add one in another PR so we can verify that this is indeed improving performance
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] perf(array-agg): add fast path for array agg for `merge_batch` [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #14299:
URL: https://github.com/apache/datafusion/pull/14299
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] perf(array-agg): add fast path for array agg for `merge_batch` [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #14299:
URL: https://github.com/apache/datafusion/pull/14299#issuecomment-2614600265
I ran the newly added `array_agg` benchmark and got some pretty serious performance improvements:
```
group improve-performance-for-array-agg-merge-batch main
----- --------------------------------------------- ----
array_agg i64 merge_batch all nulls, 100% of nulls point to a zero length array 1.00 87.3±0.09ns ? ?/sec 225.81 19.7±0.02µs ? ?/sec
array_agg i64 merge_batch all nulls, 90% of nulls point to a zero length array 1.00 86.3±0.07ns ? ?/sec 228.62 19.7±0.10µs ? ?/sec
```
I am not sure why all the runs aren't listed. I am looking into that
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] perf(array-agg): add fast path for array agg for `merge_batch` [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #14299:
URL: https://github.com/apache/datafusion/pull/14299#discussion_r1932828542
##########
datafusion/functions-aggregate/src/array_agg.rs:
##########
@@ -177,6 +177,67 @@ impl ArrayAggAccumulator {
datatype: datatype.clone(),
})
}
+
+ /// This function will return the underlying list array values if all valid values are consecutive without gaps (i.e. no null value point to a non empty list)
+ /// If there are gaps but only in the end of the list array, the function will return the values without the null values in the end
+ fn get_optional_values_to_merge_as_is(list_array: &ListArray) -> Option<ArrayRef> {
+ let offsets = list_array.value_offsets();
+ // Offsets always have at least 1 value
+ let initial_offset = offsets[0];
+ let null_count = list_array.null_count();
+
+ // If no nulls than just use the fast path
+ // This is ok as the state is a ListArray rather than a ListViewArray so all the values are consecutive
+ if null_count == 0 {
+ // According to Arrow specification, the first offset can be non-zero
Review Comment:
In fact we saw it here too:
- https://github.com/apache/arrow-rs/pull/7037
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] perf(array-agg): add fast path for array agg for `merge_batch` [datafusion]
Posted by "rluvaton (via GitHub)" <gi...@apache.org>.
rluvaton commented on PR #14299:
URL: https://github.com/apache/datafusion/pull/14299#issuecomment-2616079304
Already has a local implementation of array aggregation distinct with groups, that will try to upstream soon
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] perf(array-agg): add fast path for array agg for `merge_batch` [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #14299:
URL: https://github.com/apache/datafusion/pull/14299#issuecomment-2614619310
Here is my entire benchmark run
```
++ critcmp main improve-performance-for-array-agg-merge-batch
improve-performance-for-array-agg-merge-batch main
----- --------------------------------------------- ----
array_agg i64 merge_batch 30% nulls, 0% of nulls point to a zero length array 1.00 565.6±1.11µs ? ?/sec 1.00 565.4±1.29µs ? ?/sec
array_agg i64 merge_batch 30% nulls, 100% of nulls point to a zero length array 1.00 7.7±0.01µs ? ?/sec 73.20 562.7±1.07µs ? ?/sec
array_agg i64 merge_batch 30% nulls, 50% of nulls point to a zero length array 1.02 579.6±3.49µs ? ?/sec 1.00 568.4±28.98µs ? ?/sec
array_agg i64 merge_batch 30% nulls, 90% of nulls point to a zero length array 1.00 565.2±0.70µs ? ?/sec 1.00 563.3±0.61µs ? ?/sec
array_agg i64 merge_batch 30% nulls, 99% of nulls point to a zero length array 1.01 566.8±0.52µs ? ?/sec 1.00 562.4±1.00µs ? ?/sec
array_agg i64 merge_batch 70% nulls, 0% of nulls point to a zero length array 1.01 262.1±17.69µs ? ?/sec 1.00 259.3±15.89µs ? ?/sec
array_agg i64 merge_batch 70% nulls, 100% of nulls point to a zero length array 1.00 7.5±0.01µs ? ?/sec 34.02 256.2±1.94µs ? ?/sec
array_agg i64 merge_batch 70% nulls, 50% of nulls point to a zero length array 1.06 271.8±1.50µs ? ?/sec 1.00 256.5±0.53µs ? ?/sec
array_agg i64 merge_batch 70% nulls, 90% of nulls point to a zero length array 1.02 261.5±17.90µs ? ?/sec 1.00 256.0±0.54µs ? ?/sec
array_agg i64 merge_batch 70% nulls, 99% of nulls point to a zero length array 1.00 259.0±0.30µs ? ?/sec 1.00 259.6±16.06µs ? ?/sec
array_agg i64 merge_batch all nulls, 100% of nulls point to a zero length array 1.00 85.5±0.08ns ? ?/sec 230.91 19.7±0.01µs ? ?/sec
array_agg i64 merge_batch all nulls, 90% of nulls point to a zero length array 1.00 85.8±0.27ns ? ?/sec 229.79 19.7±0.02µs ? ?/sec
array_agg i64 merge_batch no nulls 1.00 103.4±0.21ns ? ?/sec 6215.33 642.8±1.08µs ? ?/sec
```
TLDR is that this PR appears to have a very significant performance improvement. 🚀
If no one beats me to it I will give it a good look in the upcoming week
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] perf(array-agg): add fast path for array agg for `merge_batch` [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #14299:
URL: https://github.com/apache/datafusion/pull/14299#issuecomment-2614619333
Nice work @rluvaton
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] perf(array-agg): add fast path for array agg for `merge_batch` [datafusion]
Posted by "rluvaton (via GitHub)" <gi...@apache.org>.
rluvaton commented on code in PR #14299:
URL: https://github.com/apache/datafusion/pull/14299#discussion_r1932880307
##########
datafusion/functions-aggregate/src/array_agg.rs:
##########
@@ -177,6 +177,67 @@ impl ArrayAggAccumulator {
datatype: datatype.clone(),
})
}
+
+ /// This function will return the underlying list array values if all valid values are consecutive without gaps (i.e. no null value point to a non empty list)
+ /// If there are gaps but only in the end of the list array, the function will return the values without the null values in the end
+ fn get_optional_values_to_merge_as_is(list_array: &ListArray) -> Option<ArrayRef> {
+ let offsets = list_array.value_offsets();
+ // Offsets always have at least 1 value
+ let initial_offset = offsets[0];
+ let null_count = list_array.null_count();
+
+ // If no nulls than just use the fast path
+ // This is ok as the state is a ListArray rather than a ListViewArray so all the values are consecutive
+ if null_count == 0 {
+ // According to Arrow specification, the first offset can be non-zero
Review Comment:
while writing this code I first found out that offsets does not have to start from 0
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] perf(array-agg): add fast path for array agg for `merge_batch` [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #14299:
URL: https://github.com/apache/datafusion/pull/14299#issuecomment-2621915540
Thanks again @rluvaton
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org