Skip to content

fix: mark ScalarUDFImpl::invoke_batch as deprecated #15049

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Mar 8, 2025
Merged

fix: mark ScalarUDFImpl::invoke_batch as deprecated #15049

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
6a954b2
fix: mark ScalarUDFImpl::invoke_batch as deprecated
Blizzara Mar 6, 2025
ce73e6c
fix deprecated usage that clippy warns about
Blizzara Mar 6, 2025
5eb52df
fix another deprecated usage that clippy warns about
Blizzara Mar 6, 2025
a981309
fix the rest of benches
Blizzara Mar 7, 2025
80e30b1
fix two more implementations - now all that's left is in udf.rs
Blizzara Mar 7, 2025
e1e53e7
fix clippy
Blizzara Mar 7, 2025
a0cacae
cleanup some leftover comments
Blizzara Mar 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions datafusion/expr/src/udf.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -225,11 +225,13 @@ impl ScalarUDF {
self.inner.is_nullable(args, schema)
}

#[deprecated(since = "46.0.0", note = "Use `invoke_with_args` instead")]
pub fn invoke_batch(
&self,
args: &[ColumnarValue],
number_rows: usize,
) -> Result<ColumnarValue> {
#[allow(deprecated)]
self.inner.invoke_batch(args, number_rows)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we replace this invoke_batch with invoke_with_args? So that the allow(deprecated) can be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really, since for invoke_with_args we'd need the return type which isn't provided here. Also I don't see that as very important change anyways, this is also invoke_batch and deprecated so passing directly through makes sense, and both these invoke_batchs can be removed at the same time.

}

Expand All @@ -244,15 +246,15 @@ impl ScalarUDF {
///
/// Note: This method is deprecated and will be removed in future releases.
/// User defined functions should implement [`Self::invoke_with_args`] instead.
#[deprecated(since = "42.1.0", note = "Use `invoke_batch` instead")]
#[deprecated(since = "42.1.0", note = "Use `invoke_with_args` instead")]
pub fn invoke_no_args(&self, number_rows: usize) -> Result<ColumnarValue> {
#[allow(deprecated)]
self.inner.invoke_no_args(number_rows)
}

/// Returns a `ScalarFunctionImplementation` that can invoke the function
/// during execution
#[deprecated(since = "42.0.0", note = "Use `invoke_batch` instead")]
#[deprecated(since = "42.0.0", note = "Use `invoke_with_args` instead")]
pub fn fun(&self) -> ScalarFunctionImplementation {
let captured = Arc::clone(&self.inner);
#[allow(deprecated)]
Expand Down Expand Up @@ -613,6 +615,7 @@ pub trait ScalarUDFImpl: Debug + Send + Sync {
/// User defined functions should implement [`Self::invoke_with_args`] instead.
///
/// See <https://github.com/apache/datafusion/issues/13515> for more details.
#[deprecated(since = "46.0.0", note = "Use `invoke_with_args` instead")]
fn invoke_batch(
&self,
args: &[ColumnarValue],
Expand Down Expand Up @@ -643,6 +646,7 @@ pub trait ScalarUDFImpl: Debug + Send + Sync {
/// [`ColumnarValue::values_to_arrays`] can be used to convert the arguments
/// to arrays, which will likely be simpler code, but be slower.
fn invoke_with_args(&self, args: ScalarFunctionArgs) -> Result<ColumnarValue> {
#[allow(deprecated)]
self.invoke_batch(&args.args, args.number_rows)
}

Expand Down
13 changes: 10 additions & 3 deletions datafusion/functions-nested/benches/map.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ use std::sync::Arc;

use datafusion_common::ScalarValue;
use datafusion_expr::planner::ExprPlanner;
use datafusion_expr::{ColumnarValue, Expr};
use datafusion_expr::{ColumnarValue, Expr, ScalarFunctionArgs};
use datafusion_functions_nested::map::map_udf;
use datafusion_functions_nested::planner::NestedFunctionPlanner;

Expand Down Expand Up @@ -94,11 +94,18 @@ fn criterion_benchmark(c: &mut Criterion) {
let keys = ColumnarValue::Scalar(ScalarValue::List(Arc::new(key_list)));
let values = ColumnarValue::Scalar(ScalarValue::List(Arc::new(value_list)));

let return_type = &map_udf()
.return_type(&[DataType::Utf8, DataType::Int32])
.expect("should get return type");

b.iter(|| {
black_box(
// TODO use invoke_with_args
map_udf()
.invoke_batch(&[keys.clone(), values.clone()], 1)
.invoke_with_args(ScalarFunctionArgs {
args: vec![keys.clone(), values.clone()],
number_rows: 1,
return_type,
})
.expect("map should work on valid values"),
);
});
Expand Down
31 changes: 22 additions & 9 deletions datafusion/functions/benches/strpos.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,9 @@
extern crate criterion;

use arrow::array::{StringArray, StringViewArray};
use arrow::datatypes::DataType;
use criterion::{black_box, criterion_group, criterion_main, Criterion};
use datafusion_expr::ColumnarValue;
use datafusion_expr::{ColumnarValue, ScalarFunctionArgs};
use rand::distributions::Alphanumeric;
use rand::prelude::StdRng;
use rand::{Rng, SeedableRng};
Expand Down Expand Up @@ -114,8 +115,11 @@ fn criterion_benchmark(c: &mut Criterion) {
&format!("strpos_StringArray_ascii_str_len_{}", str_len),
|b| {
b.iter(|| {
// TODO use invoke_with_args
black_box(strpos.invoke_batch(&args_string_ascii, n_rows))
black_box(strpos.invoke_with_args(ScalarFunctionArgs {
args: args_string_ascii.clone(),
number_rows: n_rows,
return_type: &DataType::Int32,
}))
})
},
);
Expand All @@ -126,8 +130,11 @@ fn criterion_benchmark(c: &mut Criterion) {
&format!("strpos_StringArray_utf8_str_len_{}", str_len),
|b| {
b.iter(|| {
// TODO use invoke_with_args
black_box(strpos.invoke_batch(&args_string_utf8, n_rows))
black_box(strpos.invoke_with_args(ScalarFunctionArgs {
args: args_string_utf8.clone(),
number_rows: n_rows,
return_type: &DataType::Int32,
}))
})
},
);
Expand All @@ -138,8 +145,11 @@ fn criterion_benchmark(c: &mut Criterion) {
&format!("strpos_StringViewArray_ascii_str_len_{}", str_len),
|b| {
b.iter(|| {
// TODO use invoke_with_args
black_box(strpos.invoke_batch(&args_string_view_ascii, n_rows))
black_box(strpos.invoke_with_args(ScalarFunctionArgs {
args: args_string_view_ascii.clone(),
number_rows: n_rows,
return_type: &DataType::Int32,
}))
})
},
);
Expand All @@ -150,8 +160,11 @@ fn criterion_benchmark(c: &mut Criterion) {
&format!("strpos_StringViewArray_utf8_str_len_{}", str_len),
|b| {
b.iter(|| {
// TODO use invoke_with_args
black_box(strpos.invoke_batch(&args_string_view_utf8, n_rows))
black_box(strpos.invoke_with_args(ScalarFunctionArgs {
args: args_string_view_utf8.clone(),
number_rows: n_rows,
return_type: &DataType::Int32,
}))
})
},
);
Expand Down
Loading