goldmedal commented on code in PR #14722:
URL: https://github.com/apache/datafusion/pull/14722#discussion_r1958413375
##########
datafusion/functions/benches/concat.rs:
##########
@@ -39,8 +40,15 @@ fn criterion_benchmark(c: &mut Criterion) {
let mut group = c.benchmark_group("concat function");
group.bench_function(BenchmarkId::new("concat", size), |b| {
b.iter(|| {
- // TODO use invoke_with_args
- criterion::black_box(concat().invoke_batch(&args,
size).unwrap())
+ criterion::black_box(
+ concat()
+ .invoke_with_args(ScalarFunctionArgs {
+ args: args.clone(),
Review Comment:
Maybe we can move `clone` out of `black_box`. It can decrease the impact of
`clone` for the benchmark result. (It's more close to the previous version.)
```suggestion
let args_cloned = args.clone();
criterion::black_box(
concat()
.invoke_with_args(ScalarFunctionArgs {
args: args_cloned,
```
In my laptop, if we place `clone` in the `black_box,` the result will slow
down by about 5%.
##########
datafusion/functions/benches/concat.rs:
##########
@@ -39,8 +40,15 @@ fn criterion_benchmark(c: &mut Criterion) {
let mut group = c.benchmark_group("concat function");
group.bench_function(BenchmarkId::new("concat", size), |b| {
b.iter(|| {
- // TODO use invoke_with_args
- criterion::black_box(concat().invoke_batch(&args,
size).unwrap())
+ criterion::black_box(
+ concat()
+ .invoke_with_args(ScalarFunctionArgs {
+ args: args.clone(),
Review Comment:
I think other benchmark cases can be improved by moving out the `clone()`.
--
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
