-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Make migration to Signature::nullary
in 44.0.0 easier / less confusing
#13763
Comments
Signature::nullary
easier to findSignature::nullary
easier / less confusing
Signature::nullary
easier / less confusingSignature::nullary
in 44.0.0 easier / less confusing
|
I don't know. It would also be fine if that format kept working as before (maybe better). |
The reason why |
In my opinion the important thing is that someone who has an existing Ideas (I can make PRs for this once we agree) Option 1: Backwards compatibility:
Option 2: Better errors
Option 3: API w/ compile time checksWe could also take a more drastic approach and verify this at compile time by making the compiler reject non zero numbers (I think this API churn is more expensive than the value saves than worth it at the moment): pub enum Signature {
Exact(NonZeroUsize)
...
} |
Option2 seems like a better choice |
I have begun working on Option 2. Here is a PR that adds better comments: |
I started writing some tests for this I could not find a way to make the error messages less confusing. Specifically the error looks like this:
The last suggestion of I think the issue is that users can construct a I think this will be percived as a tricky/annoying regression for people who have existing udfs with Edit: this isn't correct: |
From my perspective the fact that Here is a proposal to do so: |
Is your feature request related to a problem or challenge?
While working to reproduce the issue from @kylebarron here:
He had this signature for a UDF that has no arguments
This compiled without error / incident
However, when I ran that on latest main I got a very strange (to me) error:
To fix it I found I had to:
I think this will make upgrading to 44 a pain
Describe the solution you'd like
I would like DataFusion to either:
nullary
Describe alternatives you've considered
No response
Additional context
Nullary appears to have been added in this PR by @jayzhan211
The text was updated successfully, but these errors were encountered: