chore: remove deprecated variants of UDF's invoke (invoke, invoke_no_args, invoke_batch) #15123

Blizzara · 2025-03-10T13:06:40Z

Leaving only invoke_with_args

Which issue does this PR close?

Followup form #15049

Closes Deprecate and eventually remove ScalarUDF::invoke_batch #14652

Rationale for this change

The UDF trait has 4 different ways of implementing invoke, with 3 of them deprecated. This cleans the trait up.

What changes are included in this PR?

invoke, invoke_no_args, and invoke_batch are removed
invoke_with_args has its default implementation removed, forcing users to implement it
ScalarUDFImpl::fun is also removed, since that's been deprecated for a while and uses invoke
max UDF is fixed to implement invoke_with_args instead
a test dummy UDF is fixed to implement invoke_with_args instead

Are these changes tested?

Existing tests

Are there any user-facing changes?

yes, BREAKING CHANGE: remove deprecated variants of UDF's invoke (invoke, invoke_no_args, invoke_batch)

…args, invoke_batch) Leaving only invoke_with_args

Weijun-H · 2025-03-10T13:18:32Z

datafusion/core/tests/physical_optimizer/projection_pushdown.rs

@@ -89,6 +92,10 @@ impl ScalarUDFImpl for DummyUDF {
    fn return_type(&self, _arg_types: &[DataType]) -> Result<DataType> {
        Ok(DataType::Int32)
    }
+
+    fn invoke_with_args(&self, _args: ScalarFunctionArgs) -> Result<ColumnarValue> {
+        panic!("dummy - not implemented")


what about using not_impl_err instead of painc?

I just copied https://github.com/apache/datafusion/pull/15123/files#diff-351499880963d6a383c92e156e75019cd9ce33107724a9635853d7d4cd1898d0R1602 - but happy to change, would you prefer not_impl_err?

I checked other traits, it would prefer unimplemented!().

FWIW this is a test

Weijun-H · 2025-03-10T13:18:48Z

datafusion/expr/src/udf.rs

-    #[deprecated(since = "42.1.0", note = "Use `invoke_with_args` instead")]
-    pub fn invoke(&self, args: &[ColumnarValue]) -> Result<ColumnarValue> {
-        #[allow(deprecated)]
-        self.inner.invoke(args)
-    }
-


alamb · 2025-03-10T17:13:30Z

Should we remove the deprecated UDF API now? According to our API health policy:
Deprecated methods will remain in the codebase for a period of 6 major versions or 6 months, whichever is longer, to provide users ample time to transition away from them.
They were annotated on 2024/11/22. I think we should keep them until at least 2025/05/22 or 48.0.0. I'm not sure if downstream projects have updated their usage.

In general this is true -- however, in this case since several of the example APIs can't actually be used (see this section of the upgrade guide) I think it is worth considering just removing the APIs and forcing downstream crates to update

Blizzara · 2025-03-11T09:29:58Z

Yeah, from my perspective this one isn't so bad - implementing any of the deprecated methods is kinda okay, since the defaults are wired to propagate the call correctly here. Calling them is not fine, but as they're marked as deprecated that should be enough to prevent the calls.

The other removal (return_type_from_exprs) is more important since there the call chain doesn't work.

But then if we do one removal, it might make sense to just do the other one too, to end up with a clean interface.. Up to you guys, I'm happy to have this merged or turn it back into draft and merge later :)

alamb · 2025-03-13T11:01:56Z

But then if we do one removal, it might make sense to just do the other one too, to end up with a clean interface.. Up to you guys, I'm happy to have this merged or turn it back into draft and merge later :)

I agree

I made a note on the release ticket that we should write a note in the upgrade guide

Release DataFusion 47.0.0 (April 2025) #15072

alamb

Thank you @Blizzara -- I think we should make this change even though it isn't following the normal deprecation cycle

I think an API that compiles but errors at runtime is even worse than having to fix a compiler error.

…-invoke-variants

alamb · 2025-03-16T11:00:51Z

I merged this PR up from main and plan to leave it open for a day or two more. Then I'll merge it in.

alamb · 2025-03-16T11:02:10Z

Leaving only invoke_with_args

alamb · 2025-03-17T13:42:39Z

🔨 let's go

alamb · 2025-03-17T13:42:42Z

Thanks again @Weijun-H @jayzhan211 and @Blizzara

chore: remove deprecated variants of UDF's invoke (invoke, invoke_no_…

2ef4058

…args, invoke_batch) Leaving only invoke_with_args

github-actions bot added logical-expr Logical plan and expressions optimizer Optimizer rules labels Mar 10, 2025

clippy

dfce924

github-actions bot added sql SQL Planner core Core DataFusion crate proto Related to proto crate labels Mar 10, 2025

Weijun-H reviewed Mar 10, 2025

View reviewed changes

unused import

a8565a8

This comment was marked as resolved.

Sign in to view

Blizzara mentioned this pull request Mar 10, 2025

chore: remove ScalarUDFImpl::return_type_from_exprs #15130

Merged

jayzhan211 approved these changes Mar 12, 2025

View reviewed changes

alamb mentioned this pull request Mar 12, 2025

Release DataFusion 47.0.0 (April 2025) #15072

Open

37 tasks

alamb approved these changes Mar 13, 2025

View reviewed changes

alamb added 2 commits March 16, 2025 06:55

Merge remote-tracking branch 'apache/main' into avo/remove-deprecated…

e528789

…-invoke-variants

Merge remote-tracking branch 'apache/main' into avo/remove-deprecated…

a57d932

…-invoke-variants

alamb mentioned this pull request Mar 17, 2025

March 17, 2025: This week(s) in DataFusion #15269

Open

alamb merged commit 6be228c into apache:main Mar 17, 2025
27 checks passed

Blizzara deleted the avo/remove-deprecated-invoke-variants branch March 20, 2025 07:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: remove deprecated variants of UDF's invoke (invoke, invoke_no_args, invoke_batch) #15123

chore: remove deprecated variants of UDF's invoke (invoke, invoke_no_args, invoke_batch) #15123

Blizzara commented Mar 10, 2025 •

edited

Loading

Weijun-H Mar 10, 2025 •

edited

Loading

Blizzara Mar 10, 2025

Weijun-H Mar 10, 2025

alamb Mar 13, 2025

Weijun-H Mar 10, 2025

This comment was marked as resolved.

alamb commented Mar 10, 2025

Blizzara commented Mar 11, 2025

alamb commented Mar 13, 2025

alamb left a comment

alamb commented Mar 16, 2025

alamb commented Mar 16, 2025

alamb commented Mar 17, 2025

alamb commented Mar 17, 2025 •

edited

Loading

chore: remove deprecated variants of UDF's invoke (invoke, invoke_no_args, invoke_batch) #15123

chore: remove deprecated variants of UDF's invoke (invoke, invoke_no_args, invoke_batch) #15123

Conversation

Blizzara commented Mar 10, 2025 • edited 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?

Weijun-H Mar 10, 2025 • edited Loading

Choose a reason for hiding this comment

Blizzara Mar 10, 2025

Choose a reason for hiding this comment

Weijun-H Mar 10, 2025

Choose a reason for hiding this comment

alamb Mar 13, 2025

Choose a reason for hiding this comment

Weijun-H Mar 10, 2025

Choose a reason for hiding this comment

This comment was marked as resolved.

alamb commented Mar 10, 2025

Blizzara commented Mar 11, 2025

alamb commented Mar 13, 2025

alamb left a comment

Choose a reason for hiding this comment

alamb commented Mar 16, 2025

alamb commented Mar 16, 2025

alamb commented Mar 17, 2025

alamb commented Mar 17, 2025 • edited Loading

Blizzara commented Mar 10, 2025 •

edited

Loading

Weijun-H Mar 10, 2025 •

edited

Loading

alamb commented Mar 17, 2025 •

edited

Loading