Skip to content

Deprecate and eventually remove ScalarUDF::invoke_batch #14652

New issue

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

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

Already on GitHub? # to your account

Closed
findepi opened this issue Feb 13, 2025 · 3 comments · Fixed by #15123
Closed

Deprecate and eventually remove ScalarUDF::invoke_batch #14652

findepi opened this issue Feb 13, 2025 · 3 comments · Fixed by #15123
Labels
api change Changes the API exposed to users of the crate functions Changes to functions implementation help wanted Extra attention is needed

Comments

@findepi
Copy link
Member

findepi commented Feb 13, 2025

ScalarUDF::invoke_batch is unused in production code.
The ScalarUDF::invoke_with_args is used instead

let output = self.fun.invoke_with_args(ScalarFunctionArgs {

ScalarUDF::invoke_batch should be deprecated and subsequently removed.

appropriate cleanup of ScalarUDFImpl interface is covered by #13515

@findepi findepi added api change Changes the API exposed to users of the crate functions Changes to functions implementation help wanted Extra attention is needed labels Feb 13, 2025
@goldmedal
Copy link
Contributor

goldmedal commented Feb 16, 2025

I think this issue would be a nice good first-issue collection. The remaining functions are as below:

  • array functions in datafusion/functions-nested/src
  • core functions in datafusion/functions/src/core
  • crypto function in datafusion/functions/src/crypto
  • datetime function in datafusion/functions/src/datetime
  • encoding function in datafusion/functions/src/encoding
  • regex function in datafusion/functions/src/regex
  • string function in datafusion/functions/src/string
  • unicode function in datafusion/functions/src/unicode

After they are done, we can mark invoke_batch as deprecated formally.

@Blizzara
Copy link
Contributor

Just FYI: As I note in #14729, it seems rust (neither compile nor clippy) doesn't warn about implementing a deprecated method, only calling it. As such, the eventual removal of invoke_batch will be a sudden compile-time break for anyone who's not manually checking changelogs.

I'm not saying it shouldn't be removed, I do think code needs to be able to evolve - just flagging this as it came as surprise to me :)

@goldmedal
Copy link
Contributor

Just FYI: As I note in #14729, it seems rust (neither compile nor clippy) doesn't warn about implementing a deprecated method, only calling it. As such, the eventual removal of invoke_batch will be a sudden compile-time break for anyone who's not manually checking changelogs.

The deprecated flag of invoke_batch was removed by #13491 (comment). I'm not sure why we removed it 🤔. But I think we should add it back after all the sub-issues are done.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
api change Changes the API exposed to users of the crate functions Changes to functions implementation help wanted Extra attention is needed
Projects
None yet
3 participants