Conversation
| let values = Arc::new(create_list_array::<Int64Type>(8192, 1.0, 0.1)) as ArrayRef; | ||
| merge_batch_bench( | ||
| c, | ||
| "array_agg i64 merge_batch all nulls, 90% of nulls point to a zero length array", |
There was a problem hiding this comment.
this comment says 90 but the function call is create_list_array::<Int64Type>(8192, 1.0, 0.1) (which is 100 right?)
There was a problem hiding this comment.
Or is the idea that 10% nulls point at a non zero length array 🤔
There was a problem hiding this comment.
all null because null density (2nd arg) is 1.0, and 90% of nulls point to a zero length array, because the 3rd argument is the probability for non-zero length so it's 10% and 100 - 10 is 90% hence the 90% of nulls
I'll rename the argument in the function to be probability for zero length array
There was a problem hiding this comment.
Thanks -- that makes a lot more sense to me and I found it easier to understand. 🙏
| b.iter(|| { | ||
| #[allow(clippy::unit_arg)] | ||
| black_box( | ||
| ArrayAggAccumulator::try_new(&list_item_data_type) |
There was a problem hiding this comment.
As written this will create a new ArrayAggAccumulator on each iteration -- it might be more representative if the creation wasn't part of the measurement (aka create the accumulator outside b.iter() 🤔 )
There was a problem hiding this comment.
I know, but the otherwise this will add to the same local state and is not what is being tested, also creating is almost 0 cost
| let values = Arc::new(create_list_array::<Int64Type>(8192, 1.0, 0.1)) as ArrayRef; | ||
| merge_batch_bench( | ||
| c, | ||
| "array_agg i64 merge_batch all nulls, 90% of nulls point to a zero length array", |
There was a problem hiding this comment.
Thanks -- that makes a lot more sense to me and I found it easier to understand. 🙏
|
@alamb can you please merge this and run the benchmark on my performance improvement PR? |
|
Done @rluvaton -- thanks! BTW this is the script I use for benchmarking: https://github.com/alamb/datafusion-benchmarking/blob/main/compare_branch.sh I'll run it on your branch now |
|
Thank you, don't you have dedicated machines in the CI for running benchmarks? how would you otherwise 1. verify it's not only locally, 2. can reproduce 3. have somehow consistent results |
I run it on a gcp VM I would love to get the process more automated. See |
|
I'll see if I can help later this week. if you ran the benchmark I would appreciate positing it on the improvement PR and not here of course |
2 participants
Which issue does this PR close?
N/A
Rationale for this change
So we can see the improvement in
merge_batch#14299What changes are included in this PR?
added benchmark for array_agg
merge_batchfunctionAre these changes tested?
N/A
Are there any user-facing changes?
Nope