Repository navigation
Add benchmark for array_has/array_has_all/array_has_any - #18729
Conversation
2010YOUY01
left a comment
2010YOUY01
left a comment
There was a problem hiding this comment.
LGTM, thank you.
I noticed it only benchmarked the case that both needle and haystack are ScalarValue, perhaps we can also benchmark Array cases in the future.
Co-authored-by: Yongting You <[email protected]>
Co-authored-by: Yongting You <[email protected]>
Thank you @2010YOUY01 for review, i agree we can add |
| |b, &size| { | ||
| let array = (0..size).map(|i| lit(i as i64)).collect::<Vec<_>>(); | ||
| let list_array = make_array(array); | ||
| let needle_array = make_array(vec![lit(0_i64), lit(1_i64), lit(2_i64)]); |
There was a problem hiding this comment.
https://github.com/apache/datafusion/pull/18729/files#diff-ad38fd714a1e2020f92ec297d8d5fcb911fa5a7f3417ce965c44c06e462dbd74R35 calls this method with a size=1! The needle ([0, 1, 2]) won't be found in [0] !
| let list_array = make_array(array); | ||
| let needle = lit(0_i64); | ||
|
|
||
| b.iter(|| black_box(array_has(list_array.clone(), needle.clone()))) |
There was a problem hiding this comment.
Here you test the time for building the expression, not the time to execute it!
See how array_reverse bench uses array_reverse_inner() to execute it - https://github.com/zhuqi-lucas/arrow-datafusion/blob/e7a723b06fb8b164033514ac73cddaf1b512eac7/datafusion/functions-nested/benches/array_reverse.rs#L32-L34
or the map bench uses invoke_with_args() at https://github.com/zhuqi-lucas/arrow-datafusion/blob/e7a723b06fb8b164033514ac73cddaf1b512eac7/datafusion/functions-nested/benches/map.rs#L114
| BenchmarkId::new("found_at_start", array_size), | ||
| &array_size, | ||
| |b, &size| { | ||
| let array = (0..size).map(|i| lit(i as i64)).collect::<Vec<_>>(); |
There was a problem hiding this comment.
This generates sequential data which is not very realistic. Consider generating skewed and/or non-sequential uniform data.
| let list_array = make_array(array); | ||
| let needle = lit((size - 1) as i64); | ||
|
|
||
| b.iter(|| black_box(array_has(list_array.clone(), needle.clone()))) |
There was a problem hiding this comment.
For small arrays the cloning operation may dominate the time. Can you use a reference instead ?
Co-authored-by: Martin Grigorov <[email protected]>
|
Shall we merge this PR? |
@alamb I think we can merge this PR, at least we have a first version for the benchmark, we can improve it to add more cases in follow-up PRs. |
Which issue does this PR close?
Add benchmark for array_has/array_has_all/array_has_any before optimization.
Rationale for this change
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?