Skip to content

Breaking change in "return_type_from_exprs" #14729

New issue

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

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

Already on GitHub? Sign in to your account

Closed
Blizzara opened this issue Feb 17, 2025 · 2 comments · Fixed by #15130
Closed

Breaking change in "return_type_from_exprs" #14729

Blizzara opened this issue Feb 17, 2025 · 2 comments · Fixed by #15130
Labels
bug Something isn't working

Comments

@Blizzara
Copy link
Contributor

Describe the bug

#14094 was a bit problematic for users with custom UDFs in that it may cause breaks at runtime.

The #[deprecated()] apparently doesn't affect for people implementing this trait, only those who call it.

As such, if someone (like us) had implemented return_type_from_exprs and had return_type just panic, that'd compile fine, but at runtime rather than calling the return_type_from_exprs DF would call this new return_type_from_args with its default impl delegating to return_type which would then panic.

Luckily, we had tests that caught it, but I think in this case it would have been better to fully remove the return_type_from_exprs so that users who override it would get compile failures, rather than runtime ones. Compile errors are annoying, sure, but still always better than compiling and doing wrong thing. And might still be good to remove it.

The "correct" way would have been to have a default return_type_from_args delegate to return_type_from_exprs, but I'm not sure if that's possible?

A worse alternative would have been to write the chain as return_type_from_exprs -> return_type_from_args -> return_type, and keep calling return_type_from_exprs. Not sure if that would have been possible either, or if the switch from return_type_from_exprs to return_type_from_args was necessary, and even still this wouldn't have helped if at some point in future someone would depend on return_type_from_args.

To Reproduce

No response

Expected behavior

No response

Additional context

As an aside, there are still couple places referring return_type_from_exprs as comments when they really mean the "from_args" variant: https://github.com/search?q=repo%3Aapache%2Fdatafusion+return_type_from_exprs+language%3ARust&type=code&l=Rust

@findepi
Copy link
Member

findepi commented Feb 18, 2025

The "correct" way would have been to have a default return_type_from_args delegate to return_type_from_exprs, but I'm not sure if that's possible?

That's what we should have done. Except that
return_type_from_exprs requires the expressions
return_type_from_args doesn't have the expressions

it would need to fabricate some fake expressions. This is doable, but still could break something/someone who depended on having access to factual non-literal input expressions in their return_type_from_exprs impl. (I don't see a sense in such implementation, just speculating.)

@jayzhan211
Copy link
Contributor

The "correct" way would have been to have a default return_type_from_args delegate to return_type_from_exprs, but I'm not sure if that's possible?

I think removing return_type_from_exprs entirely is a better option given that we don't have Expr and Schema anymore for return_type_from_args

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants