bench: remove stale array_expression benchmark#22143
Conversation
|
run benchmark array_expression |
|
🤖 Criterion benchmark running (GKE) | trigger CPU Details (lscpu)Comparing fix/array_replace_benchmark_kernel (b993851) to 937dfda (merge-base) diff File an issue against this benchmark runner |
|
🤖 Criterion benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagebase (merge-base)
branch
File an issue against this benchmark runner |
There was a problem hiding this comment.
@kumarUjjawal
Thanks for working on this. I do not have any blocking concerns, but I left one suggestion around benchmark organization and avoiding duplicate coverage.
| let four_arg_fields = vec![array_field, from_field, to_field, max_field]; | ||
|
|
||
| // Benchmark array functions | ||
| let mut group = c.benchmark_group("array_replace_int64"); |
There was a problem hiding this comment.
Nice addition. One thing I noticed is that datafusion/functions-nested/benches/array_replace.rs already contains a dedicated array_replace benchmark covering array_replace_udf, array_replace_n_udf, and array_replace_all_udf across the same int64 sizes, along with nested, string, bool, and fixed-size-binary cases.
It might be worth consolidating this into the dedicated array_replace.rs benchmark, or removing the now somewhat misleading array_expression benchmark target. That would help avoid duplicate benchmark maintenance and keep ownership clearer.
There was a problem hiding this comment.
Sounds good, I removed the duplicate array_expression
array_expression benchmark
kosiew
left a comment
There was a problem hiding this comment.
@kumarUjjawal
Thanks for the iteration.
Looks 👍 to me
Which issue does this PR close?
array_replacebenchmark does not exercise thearray_replacekernel #22121Rationale for this change
array_replace.rsalready coversarray_replace,array_replace_n, andarray_replace_allthrough the public UDF path, and includes the relevant int64 benchmark sizes plus broader nested, string, boolean, and fixed-size-binary cases.Keeping both benchmark targets would duplicate maintenance and make ownership unclear.
What changes are included in this PR?
This removes the stale
array_expressionbenchmark target.The dedicated
array_replacebenchmark remains the single benchmark owner for array replacement performance.Are these changes tested?
Yes, locally
Are there any user-facing changes?
No