Move many udf implementations from `invoke` to `invoke_batch` #13491

joseph-isaacs · 2024-11-19T15:51:41Z

Which issue does this PR close?

As part of #13238 invoke was replaced with invoke_batch.

Part of Perf: Allow User defined functions to potentially reuse their argument arrays (to avoid new allocations) #13516
Closes ScalarUDFImpl invoke improvements #13507

Rationale for this change

This will allow the removal of invoke. And maybe if we are quick we can rename invoke_with_args back to invoke.

What changes are included in this PR?

This PR moves function definitions over to use invoke_batch, so invoke can be removed.

Are these changes tested?

Are there any user-facing changes?

…invoke is passed the return type created for the udf instance

Do not yet deprecate `invoke_batch`, add docs to invoke_with_args

# Conflicts: # datafusion/expr/src/udf.rs

# Conflicts: # datafusion/expr/src/udf.rs # datafusion/functions/src/datetime/to_local_time.rs # datafusion/functions/src/utils.rs # datafusion/physical-expr/src/scalar_function.rs

joseph-isaacs · 2024-11-22T11:24:33Z

docs/source/user-guide/sql/scalar_functions.md

@@ -1954,32 +1954,6 @@ The following intervals are supported:
 - years
 - century

-#### Example


./dev/update_function_docs.sh did this?

I think it is because you removed the docs (perhaps accidentally) above -- i left a comment

alamb

Thanks @joseph-isaacs -- this looks like a great step forward

I had some suggestions / additional cleanups

THe only thing I think that needs to be fixed before merging this is reverting the loss of the documentation for date_bin

alamb · 2024-11-22T11:30:31Z

datafusion/expr/src/udf.rs

@@ -546,7 +546,7 @@ pub trait ScalarUDFImpl: Debug + Send + Sync {
    /// to arrays, which will likely be simpler code, but be slower.
    fn invoke_with_args(&self, args: ScalarFunctionArgs) -> Result<ColumnarValue> {
        #[allow(deprecated)]


we cam probably remove this #allow as well

alamb · 2024-11-22T11:31:15Z

datafusion/functions/benches/ltrim.rs

@@ -141,8 +141,8 @@ fn run_with_string_type<M: Measurement>(
        ),
        |b| {
            b.iter(|| {
-                #[allow(deprecated)] // TODO use invoke_batch
-                black_box(ltrim.invoke(&args))
+                #[allow(deprecated)] // TODO use invoke_with_args


likewise here I think we can remove the #allow

Comment below

datafusion/functions/benches/make_date.rs

datafusion/functions/src/datetime/date_bin.rs

alamb · 2024-11-22T11:33:25Z

datafusion/functions/src/utils.rs

@@ -149,7 +149,8 @@ pub mod test {
                    let return_type = return_type.unwrap();
                    assert_eq!(return_type, $EXPECTED_DATA_TYPE);

-                    let result = func.invoke_with_args(datafusion_expr::ScalarFunctionArgs{args: $ARGS, number_rows: cardinality, return_type: &return_type});
+                    #[allow(deprecated)]


It seems like it would be good to keep calling using invoke_with_args

alamb · 2024-11-22T11:33:34Z

datafusion/functions/src/utils.rs

@@ -170,7 +171,8 @@ pub mod test {
                    }
                    else {
                        // invoke is expected error - cannot use .expect_err() due to Debug not being implemented
-                        match func.invoke_with_args(datafusion_expr::ScalarFunctionArgs{args: $ARGS, number_rows: cardinality, return_type: &return_type.unwrap()}) {
+                        #[allow(deprecated)]


likewise here -- invoke_with_args

A lot of tests use this macro and pass in &[], so it quite a big change.

This makes sense in a follow up

alamb · 2024-11-22T11:34:05Z

docs/source/user-guide/sql/scalar_functions.md

@@ -1954,32 +1954,6 @@ The following intervals are supported:
 - years
 - century

-#### Example


I think it is because you removed the docs (perhaps accidentally) above -- i left a comment

datafusion/expr/src/udf.rs

alamb

Thanks @joseph-isaacs -- this is looking close to me, though it looks like a few bugs got introduced. I flagged some of them.

I can try to help tomorrow, but I am out of time today

alamb · 2024-11-23T11:42:02Z

datafusion/expr/src/udf.rs

@@ -330,7 +328,7 @@ where

 pub struct ScalarFunctionArgs<'a> {
    // The evaluated arguments to the function
-    pub args: &'a [ColumnarValue],
+    pub args: Vec<ColumnarValue>,


This is the change proposed in ✅

Perf: Allow User defined functions to potentially reuse their argument arrays (to avoid new allocations) #13516

alamb · 2024-11-23T11:43:57Z

datafusion/functions/src/datetime/date_bin.rs

@@ -544,7 +548,7 @@ mod tests {
                ColumnarValue::Array(timestamps),
                ColumnarValue::Scalar(ScalarValue::TimestampNanosecond(Some(1), None)),
            ],
-            batch_size,
+            1,


This seems incorrect -- the batch size was 6 values, not 1

I had a look for all of these, but its easy to miss one, all the others that pass an literal value take only scalars.

alamb · 2024-11-24T22:06:04Z

Here is what I suggest we do with this PR:

Remove the change for ScalarUDFImpl invoke improvements #13507 (let's do that in a separate PR)
Fix up the regressions introduced (where batch_size --> 1)

Then merge this one in.

If you have more time @joseph-isaacs it would be even better to change this PR so all the functions used ScalarFunctionArgs but I realize that is asking quite a bit

joseph-isaacs · 2024-11-25T10:25:07Z

I would have liked to move over to invoke_with_args, however its quite a bit more of an undertaking. Possibly in the future I can try

alamb · 2024-11-25T11:19:27Z

I would have liked to move over to invoke_with_args, however its quite a bit more of an undertaking. Possibly in the future I can try

Makes sense

joseph-isaacs · 2024-11-25T16:10:40Z

I think there was a transient failure

alamb · 2024-11-25T21:58:19Z

I think there was a transient failure

I retriggered the tests

alamb

Looks good to me -- thank you @joseph-isaacs

jayzhan211 · 2024-11-26T00:44:32Z

Thanks @joseph-isaacs @alamb

alamb · 2024-11-26T20:01:49Z

🙏 thank you -- we are making progresss

github-actions bot added documentation Improvements or additions to documentation sql SQL Planner logical-expr Logical plan and expressions physical-expr Physical Expressions optimizer Optimizer rules core Core DataFusion crate functions labels Nov 19, 2024

joseph-isaacs added 2 commits November 19, 2024 15:54

Added support for ScalarUDFImpl::invoke_with_return_type where the …

ae73371

…invoke is passed the return type created for the udf instance

Move from invoke to invoke batch

6b3db8c

joseph-isaacs force-pushed the ji/add-invoke-fix-tests branch from 19632b5 to 6b3db8c Compare November 19, 2024 15:56

joseph-isaacs added 2 commits November 19, 2024 16:39

ex

1e99688

of

8c98325

github-actions bot added the proto Related to proto crate label Nov 19, 2024

joseph-isaacs and others added 8 commits November 19, 2024 18:11

docs

9110ca9

fx

9877079

fx

a2695ff

fx

912f691

fix

423b260

fix

b04840a

Do not yet deprecate invoke_batch, add docs to invoke_with_args

88e5608

add ticket reference

9ec7e4a

joseph-isaacs marked this pull request as ready for review November 21, 2024 13:57

joseph-isaacs marked this pull request as draft November 21, 2024 13:57

joseph-isaacs added 7 commits November 21, 2024 14:01

Merge pull request #3 from alamb/alamb/docs_and_dep

99a65b5

Do not yet deprecate `invoke_batch`, add docs to invoke_with_args

Merge branch 'ji/add-invoke-with-return' into ji/add-invoke-fix-tests

47ee14f

# Conflicts: # datafusion/expr/src/udf.rs

fix

a1b266e

fix

9f40559

fix

e54519f

Merge branch 'main' into ji/add-invoke-fix-tests

1fbcf2e

# Conflicts: # datafusion/expr/src/udf.rs # datafusion/functions/src/datetime/to_local_time.rs # datafusion/functions/src/utils.rs # datafusion/physical-expr/src/scalar_function.rs

fix

dc3fee6

joseph-isaacs marked this pull request as ready for review November 22, 2024 10:58

joseph-isaacs mentioned this pull request Nov 22, 2024

Add ScalarUDFImpl::invoke_with_args to support passing the return type created for the udf instance #13290

Merged

joseph-isaacs added 3 commits November 22, 2024 11:00

fix agg

c159c26

unused

11d0ee9

update func docs

33923c7

joseph-isaacs commented Nov 22, 2024

View reviewed changes

alamb reviewed Nov 22, 2024

View reviewed changes

update tests and remove deprecation

51140b8

joseph-isaacs commented Nov 22, 2024

View reviewed changes

datafusion/expr/src/udf.rs Show resolved Hide resolved

remove dep

f997f1c

joseph-isaacs requested a review from alamb November 22, 2024 13:19

joseph-isaacs added 3 commits November 22, 2024 14:29

oops

5b04445

internal as vec

40b1860

dep

609cee2

alamb mentioned this pull request Nov 23, 2024

Perf: Allow User defined functions to potentially reuse their argument arrays (to avoid new allocations) #13516

Open

alamb reviewed Nov 23, 2024

View reviewed changes

joseph-isaacs added 2 commits November 25, 2024 10:47

fixup

b9835b9

fixup

55101eb

joseph-isaacs requested a review from alamb November 25, 2024 11:00

joseph-isaacs added 2 commits November 25, 2024 12:20

fix

153b186

fix

73629d6

alamb approved these changes Nov 25, 2024

View reviewed changes

jayzhan211 merged commit 25cb812 into apache:main Nov 26, 2024
27 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move many udf implementations from `invoke` to `invoke_batch` #13491

Move many udf implementations from `invoke` to `invoke_batch` #13491

joseph-isaacs commented Nov 19, 2024 •

edited by alamb

Loading

joseph-isaacs Nov 22, 2024

alamb Nov 22, 2024

alamb left a comment

alamb Nov 22, 2024

alamb Nov 22, 2024

joseph-isaacs Nov 22, 2024

alamb Nov 22, 2024

alamb Nov 22, 2024

joseph-isaacs Nov 22, 2024

joseph-isaacs Nov 22, 2024

alamb Nov 22, 2024

alamb left a comment

alamb Nov 23, 2024

joseph-isaacs Nov 25, 2024

alamb Nov 23, 2024

joseph-isaacs Nov 25, 2024

alamb commented Nov 24, 2024

joseph-isaacs commented Nov 25, 2024

alamb commented Nov 25, 2024

joseph-isaacs commented Nov 25, 2024

alamb commented Nov 25, 2024

alamb left a comment

jayzhan211 commented Nov 26, 2024

alamb commented Nov 26, 2024

Move many udf implementations from invoke to invoke_batch #13491

Move many udf implementations from invoke to invoke_batch #13491

Conversation

joseph-isaacs commented Nov 19, 2024 • edited by alamb Loading

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb commented Nov 24, 2024

joseph-isaacs commented Nov 25, 2024

alamb commented Nov 25, 2024

joseph-isaacs commented Nov 25, 2024

alamb commented Nov 25, 2024

alamb left a comment

Choose a reason for hiding this comment

jayzhan211 commented Nov 26, 2024

alamb commented Nov 26, 2024

Move many udf implementations from `invoke` to `invoke_batch` #13491

Move many udf implementations from `invoke` to `invoke_batch` #13491

joseph-isaacs commented Nov 19, 2024 •

edited by alamb

Loading