fix: mark ScalarUDFImpl::invoke_batch as deprecated #15049
Conversation
should use invoke_with_args instead See apache#14123 (comment)
| number_rows: usize, | ||
| ) -> Result<ColumnarValue> { | ||
| #[allow(deprecated)] | ||
| self.inner.invoke_batch(args, number_rows) |
There was a problem hiding this comment.
Can we replace this invoke_batch with invoke_with_args? So that the allow(deprecated) can be removed.
There was a problem hiding this comment.
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.
| // All benches are single batch run with 8192 rows | ||
| let character_length = datafusion_functions::unicode::character_length(); | ||
|
|
||
| let return_type = DataType::Utf8; |
There was a problem hiding this comment.
I tried to look up what the correct return type should be, but there is nothing validating that I got it correct nor that it won't get broken in the future, e.g. if some of these functions start returning stringview instead. But given these are benches, I guess that's fine? If the type is used and wrong, presumably it'll complain somewhere, if it's not used and it's wrong it's not the end of the world.
There was a problem hiding this comment.
I agree -- this change is fine. Thank you
Thanks, clippy pointed out a whole bunch of invoke_batch usage, so I ended up cleaning them all 😅 |
There was a problem hiding this comment.
Thank you very much @Blizzara 🙏
We should also consider just removing the old functions (invoke_batch, invoke, invoke_no_args), to clear up the trait.
Yeah, I have been thinking that too -- maybe just remove them and even though it would be a breaking API change it would make things much less confusing 🤔
| // All benches are single batch run with 8192 rows | ||
| let character_length = datafusion_functions::unicode::character_length(); | ||
|
|
||
| let return_type = DataType::Utf8; |
There was a problem hiding this comment.
I agree -- this change is fine. Thank you
|
Sure, I think let’s merge this in first, then I can followup with the removals |
4 participants
Which issue does this PR close?
See #14123 (comment)
Also (main) part of #14652 and closes #13515
Rationale for this change
People should use
invoke_with_argsinstead. Callinginvoke_batchfor any DF-builtin function will fail at runtime with something likeFunction X does not implement invoke but calledWe should also consider just removing the old functions (invoke_batch, invoke, invoke_no_args), to clear up the trait.
What changes are included in this PR?
Mark
invoke_batchas deprecated. It was already called so in the docstring, but the compiler doesn't read those.Are these changes tested?
Existing tests
Are there any user-facing changes?
New deprecation