You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "zjregee (via GitHub)" <gi...@apache.org> on 2025/02/17 08:29:07 UTC
[PR] migrate string functions to `inovke_with_args` [datafusion]
zjregee opened a new pull request, #14722:
URL: https://github.com/apache/datafusion/pull/14722
## Which issue does this PR close?
- Closes #14708.
## Rationale for this change
## What changes are included in this PR?
Deprecate and remove `invoke_batch` in string functions and migrate to providing `invoke_with_args`.
## Are these changes tested?
Yes.
## Are there any user-facing changes?
None.
--
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] migrate string functions to `inovke_with_args` [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #14722:
URL: https://github.com/apache/datafusion/pull/14722#issuecomment-2665592608
🚀
--
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] migrate string functions to `inovke_with_args` [datafusion]
Posted by "zjregee (via GitHub)" <gi...@apache.org>.
zjregee commented on PR #14722:
URL: https://github.com/apache/datafusion/pull/14722#issuecomment-2664558268
And I noticed that #14686 introduced a call `invoke_batch` that was going to be outdated, I modified it here.
--
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] migrate string functions to `inovke_with_args` [datafusion]
Posted by "zjregee (via GitHub)" <gi...@apache.org>.
zjregee commented on code in PR #14722:
URL: https://github.com/apache/datafusion/pull/14722#discussion_r1959024013
##########
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:
Thanks for your suggestion, I have optimized this in the submission.
--
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] migrate string functions to `inovke_with_args` [datafusion]
Posted by "zjregee (via GitHub)" <gi...@apache.org>.
zjregee commented on PR #14722:
URL: https://github.com/apache/datafusion/pull/14722#issuecomment-2662865272
cc: @goldmedal
--
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] migrate string functions to `inovke_with_args` [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #14722:
URL: https://github.com/apache/datafusion/pull/14722
--
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] migrate string functions to `inovke_with_args` [datafusion]
Posted by "goldmedal (via GitHub)" <gi...@apache.org>.
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-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