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

joseph-isaacs · 2024-11-07T11:38:41Z

Which issue does this PR close?

Rationale for this change

There is currently no way to determine the schema-defined nullability of the arrays from args: &[ColumnarValue],from the invoke_batch function. The array nullability Array::is_nullable() (which is available) only checks the array null count.

If the return type (from ScalarUDFImpl::return_type) is passed to the newly defined invoke method then this datatype can be inspected and any nullability information can be inferred.

What changes are included in this PR?

Define a new trait function invoke_batch_with_return_type which defaults to calling to invoke_batch.

Example usage can be seen here.

Are these changes tested?

Are there any user-facing changes?

There are no breaking changes, but a new optional method is added.

jayzhan211 · 2024-11-11T11:29:58Z

datafusion/expr/src/udf.rs

+        &self,
+        args: &[ColumnarValue],
+        number_rows: usize,
+        return_type: &DataType,


How about we add InvokeUDFArg struct that includes these to avoid breaking change in the future?

Do you think we should roll all the values into this struct or just number_rows and return_type?

including args

Any thoughts on the new function name, if we roll these in together. Since this subsumes invoke_batch, would we then want to deprecate that too?

I personally prefer single invoke function, but that will be a large change so not required to roll them together.

I think a better name than

pub fn invoke_batch_with_return_type( &self, args: InvokeArgs ) -> Result<ColumnarValue> {

is needed, maybe invoke_with_args(), I don't like it that much, but not sure on the name.

pub fn invoke_with_args( &self, args: InvokeArgs ) -> Result<ColumnarValue> {

findepi · 2024-11-11T18:58:47Z

In an ideal worlds, UDF would have single invoke method. This is why #13238 / #13064 / #13345 consolidation takes place.
Now, we add new invoke variant. Would we want all implementations to consolidate on this new variant, or would it feel awkward when doing so?
Passing back the return_type: &DataType might be sometimes useful, but generally feels redundant.

In #12819 (comment) i am attempting to clarify why simplify isn't useful for the use-case.

If not simplify, what if we had something like bind_return_type method that's guaranteed to be invoked before invoke / invoke_batch and which may return new UDF instance (if state changes) or nothing (per default impl). This way we would provide exactly and directly what's needed for the use-case, without changing invoke itself.

joseph-isaacs · 2024-11-12T11:47:37Z

I would also be happy to scope out another prepare/bind method on a UDF. But It seems like we need a consensus on the preferred solution @findepi @alamb.

alamb · 2024-11-13T13:44:08Z

I would also be happy to scope out another prepare/bind method on a UDF. But It seems like we need a consensus on the preferred solution @findepi @alamb.

What if we just changed the invoke_batch API (again?) like in #12819 (comment)

Maybe invoke_with_args(args: ScalarFunctionArgs) and mark invoke_batch deprecated 😬

I think that is about the best we can hope for -- it would be API churn on downstream users, but at least it will be mechanical and the deprecation warning will give them time to adjust

findepi · 2024-11-13T13:57:08Z

if constructing ScalarFunctionArgs is cheap (a wrapper around information already possessed), i'm fine with that.
putting the args in a struct makes future-proof
btw we might be able to use invoke name again on the hope it's the last time we change the API 🤞

joseph-isaacs · 2024-11-13T14:23:16Z

Cool, Are you sure we want to reuse invoke, this will stop anyone updating their DF version unless they remove invoke_no_args and then use ScalarFunctionArgs

joseph-isaacs · 2024-11-15T12:23:31Z

@findepi Would you prefer a breaking rename to invoke?

findepi · 2024-11-16T17:49:10Z

Given @alamb 's proposal invoke...(args: ScalarFunctionArgs), packing args into a struct could mean this is the last time we think about the name of the invoke function. In 5 years from now perspective, invoke sounds like a better name than inoke_with_args.

alamb · 2024-11-18T21:03:28Z

Cool, Are you sure we want to reuse invoke, this will stop anyone updating their DF version unless they remove invoke_no_args and then use ScalarFunctionArgs

It seems to me that invoke was deprecated in 42, and we have now released 43. I think it would be ok to reuse invoke now -- maybe with a not in the documentation that it used to have a different signature.

https://docs.rs/datafusion/latest/datafusion/logical_expr/trait.ScalarUDFImpl.html#method.invoke

So I propose we change the API "one last time". Note if we are going to do this I would like to also change the function to get owned arguments (so long term we can potentially do optimizations like reusing the underlying buffers with kernels like unary_mut

So something like

struct ScalarFunctionArgs<'a> {
  pub args: Vec<ColumnarValue>,
  pun num_rows: usize, 
  pub return_type: &'a DataType,
}

Thank you for bearing with us @joseph-isaacs

alamb · 2024-11-18T21:04:21Z

datafusion/physical-expr/src/scalar_function.rs

@@ -141,7 +141,11 @@ impl PhysicalExpr for ScalarFunctionExpr {
            .collect::<Result<Vec<_>>>()?;

        // evaluate the function
-        let output = self.fun.invoke_batch(&inputs, batch.num_rows())?;
+        let output = self.fun.invoke_with_args(ScalarFunctionArgs {


If we can pass in inputs directly here (not as a slice but as the Vec) that would be ideal

joseph-isaacs · 2024-11-19T13:03:35Z

This changes will require over 1000 test changes. Can we make this change incrementaily?

findepi · 2024-11-19T13:15:34Z

Sure, what increments would you propose? Perhaps we can start with the removal of current invoke.

jayzhan211 · 2024-11-19T13:27:22Z

This changes will require over 1000 test changes. Can we make this change incrementaily?

Definitely

joseph-isaacs · 2024-11-19T13:37:45Z

Merge a PR with invoke_with_args, and then work to rename to invoke. But this is quite an annoying reason to further delay the original intent of this PR.

But it does look like migrating to invoke_batch would help.

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

alamb · 2024-11-19T21:24:32Z

Merge a PR with invoke_with_args, and then work to rename to invoke. But this is quite an annoying reason to further delay the original intent of this PR.

Here is what I suggest to get this PR in:

Don't deprecate invoke_batch (at least for a while and not in this PR). People could use that just fine and then eventually if it gets annoying we can rename invoke_with_args

Then we can migrate the code in DataFusion over to use invoke_batch / invoke_with_args slowly without affecting other downstream users.

alamb · 2024-11-21T13:49:44Z

I feel bad about this PR stalling. I am working on a proposal to get it going and in. Sorry for the delay @joseph-isaacs

alamb · 2024-11-21T13:57:52Z

@joseph-isaacs here is a proposed patch that targets this PR that should get CI clean

Do not yet deprecate invoke_batch, add docs to invoke_with_args spiraldb/datafusion#3

With that change I think we can merge this PR in

Thank you for your patience

alamb · 2024-11-21T13:59:06Z

datafusion/expr/src/udf.rs

@@ -324,6 +327,16 @@ where
    }
 }

+pub struct ScalarFunctionArgs<'a> {


I wonder if this structure also provides a potential place for "preparing" a scalar function (e.g. to pre-compile a regex 🤔 ) - #8051

Would you suggest having a opaque block returned from a (new interface method) which is that that value to invoke?

Yes, I was thinking something like

trait ScalarUDFImpl { /// prepares to run the function, returning any state (such as /// a precompiled regex). Called once per instance of function in the query fn prepare(&self, args: &ScalarFunctionArgs) -> Option<Box<dyn Any>> { None } /// `prepared` field in ScalarFunctonArgs has the result of calling `prepare` fn invoke_with_args(&self, args: &ScalarFunctionArgs) -> ...

pub struct ScalarFunctionArgs<'a> { ... /// The result from a call to `prepare` for this function instance prepared: Option<Box<dyn Any>>, }

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

joseph-isaacs · 2024-11-21T14:02:37Z

Thannks @alamb

alamb · 2024-11-21T14:04:14Z

I have filed a follow on ticket to deprecate invoke_batch and invoke_with_args:

(Eventually) Deprecate ScalarUDFImpl::invoke_batch and move everything to ScalarUDFImpl::invoke_with_args #13515

I think once the CI has passed this PR will be good to go

Thank you for @joseph-isaacs as well as your help @jayzhan211 and @findepi

alamb · 2024-11-21T14:23:32Z

I also filed

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

Which I think would be quite cool

Omega359 · 2024-11-21T14:36:44Z

datafusion/expr/src/udf.rs

+    pub number_rows: usize,
+    // The return type of the scalar function returned (from `return_type` or `return_type_from_exprs`)
+    // when creating the physical expression from the logical expression
+    pub return_type: &'a DataType,


How did I not see this till now?

Please please please add SessionContext or at least SessionConfig to this. It would allow us to unblock so so many tickets

It should also be easy to add as a follow-up?

Sure, as long as it's done in this release cycle so that we don't churn the api even more between cycles

Yes -- I will file a ticket

Tracking with #13519

(BTW the change in the design to a struct I think allows for much easier non-breaking API additions in the future)

alamb · 2024-11-21T16:08:32Z

Let's get this in to unblock the next work!

joseph-isaacs · 2024-11-22T10:59:19Z

In #13491 I have migrated over to using invoke_batch in all test. I think if we are quick we can rename invoke_with_args back to invoke. @alamb @findepi

alamb · 2024-11-22T11:29:50Z

In #13491 I have migrated over to using invoke_batch in all test. I think if we are quick we can rename invoke_with_args back to invoke. @alamb @findepi

The more I think about it the less ideal I think renaming to invoke is -- I think it will just cause a bunch of confusion downstream.

Thank you for #13491 👀

github-actions bot added logical-expr Logical plan and expressions physical-expr Physical Expressions labels Nov 7, 2024

joseph-isaacs mentioned this pull request Nov 7, 2024

Access children DataType or return-type in ScalarUDFImpl::invoke #12819

Closed

joseph-isaacs marked this pull request as ready for review November 7, 2024 15:27

jayzhan211 reviewed Nov 11, 2024

View reviewed changes

joseph-isaacs requested a review from jayzhan211 November 11, 2024 13:59

jayzhan211 approved these changes Nov 12, 2024

View reviewed changes

github-actions bot added the functions label Nov 13, 2024

alamb mentioned this pull request Nov 18, 2024

Support custom field metadata in UDF #13458

Draft

alamb reviewed Nov 18, 2024

View reviewed changes

Added support for ScalarUDFImpl::invoke_with_return_type where the …

ae73371

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

joseph-isaacs force-pushed the ji/add-invoke-with-return branch from 642928e to ae73371 Compare November 19, 2024 15:54

alamb mentioned this pull request Nov 20, 2024

ScalarUDFImpl invoke improvements #13507

Closed

Do not yet deprecate invoke_batch, add docs to invoke_with_args

88e5608

alamb mentioned this pull request Nov 21, 2024

(Eventually) Deprecate ScalarUDFImpl::invoke_batch and move everything to ScalarUDFImpl::invoke_with_args #13515

Open

add ticket reference

9ec7e4a

alamb mentioned this pull request Nov 21, 2024

Do not yet deprecate invoke_batch, add docs to invoke_with_args spiraldb/datafusion#3

Merged

alamb changed the title ~~Added support for ScalarUDFImpl::invoke_with_return_type where the invoke is passed the return type created for the udf instance~~ Add ScalarUDFImpl::invoke_with_args to support passing the return type created for the udf instance Nov 21, 2024

alamb reviewed Nov 21, 2024

View reviewed changes

alamb mentioned this pull request Nov 21, 2024

Specialized / Pre-compiled / Prepared ScalarUDFs #8051

Open

Merge pull request #3 from alamb/alamb/docs_and_dep

99a65b5

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

joseph-isaacs requested review from alamb and jayzhan211 November 21, 2024 14:02

alamb approved these changes Nov 21, 2024

View reviewed changes

alamb mentioned this pull request Nov 21, 2024

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

Closed

Omega359 reviewed Nov 21, 2024

View reviewed changes

alamb mentioned this pull request Nov 21, 2024

Add SessionConfig reference to ScalarFunctionArgs #13519

Open

alamb merged commit edbd93a into apache:main Nov 21, 2024
25 checks passed

Omega359 mentioned this pull request Nov 22, 2024

feat: Add ConfigOptions to ScalarFunctionArgs #13527

Draft

This was referenced Dec 3, 2024

Dec 3. 2024: This week in DataFusion #13630

Open

Performance: enable array allocation reuse (ScalarFunctionArgs gets owned ColumnReference) #13637

Merged

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

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

Conversation

joseph-isaacs commented Nov 7, 2024 • 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?

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

jayzhan211 Nov 12, 2024 • edited Loading

Choose a reason for hiding this comment

joseph-isaacs Nov 12, 2024 • edited Loading

Choose a reason for hiding this comment

findepi commented Nov 11, 2024

joseph-isaacs commented Nov 12, 2024

alamb commented Nov 13, 2024

findepi commented Nov 13, 2024

joseph-isaacs commented Nov 13, 2024

joseph-isaacs commented Nov 15, 2024

findepi commented Nov 16, 2024

alamb commented Nov 18, 2024

Choose a reason for hiding this comment

joseph-isaacs commented Nov 19, 2024

findepi commented Nov 19, 2024

jayzhan211 commented Nov 19, 2024

joseph-isaacs commented Nov 19, 2024 • edited Loading

alamb commented Nov 19, 2024

alamb commented Nov 21, 2024

alamb commented Nov 21, 2024

alamb Nov 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joseph-isaacs commented Nov 21, 2024

alamb commented Nov 21, 2024

alamb commented Nov 21, 2024

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 commented Nov 21, 2024

joseph-isaacs commented Nov 22, 2024 • edited Loading

alamb commented Nov 22, 2024

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

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

joseph-isaacs commented Nov 7, 2024 •

edited

Loading

jayzhan211 Nov 12, 2024 •

edited

Loading

joseph-isaacs Nov 12, 2024 •

edited

Loading

joseph-isaacs commented Nov 19, 2024 •

edited

Loading

alamb Nov 21, 2024 •

edited

Loading

joseph-isaacs commented Nov 22, 2024 •

edited

Loading