Repository navigation
Make migration to Signature::nullary in 44.0.0 easier / less confusing #13763
Description
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
impl UnionExample {
pub fn new() -> Self {
Self {
signature: Signature::any(0, Volatility::Immutable),
}
}
}This compiled without error / incident
However, when I ran that on latest main I got a very strange (to me) error:
called
Result::unwrap()on anErrvalue: Plan("Error during planning: example_union does not support zero arguments. No function matches the given name and argument types 'example_union()'. You might need to add explicit type casts.\n\tCandidate functions:\n\texample_union()")
To fix it I found I had to:
- signature: Signature::any(0, Volatility::Immutable),
+ signature: Signature::nullary(Volatility::Volatile),I think this will make upgrading to 44 a pain
Describe the solution you'd like
I would like DataFusion to either:
- Support the old style signature for zero args
- Generate a clear error that tells me what I have to fix in my code (aka change to
nullary
Describe alternatives you've considered
No response
Additional context
Nullary appears to have been added in this PR by @jayzhan211
Reacted by Kyle Barron
Activity
[-]Make `Signature::nullary` easier to find[/-][+]Make migration to `Signature::nullary` easier / less confusing[/+][-]Make migration to `Signature::nullary` easier / less confusing[/-][+]Make migration to `Signature::nullary` in 44.0.0 easier / less confusing[/+]findepi commentedon Dec 13, 2024
Signature::anyis specified as "A specified number of arguments of any type"what's wrong with
Signature::any(0)?alamb commentedon Dec 13, 2024
I don't know. It would also be fine if that format kept working as before (maybe better).
jayzhan211 commentedon Dec 14, 2024
The reason why
Signature::nullaryis the preferred choice is that otherwise we can haveSignature::any(0),Signature::exact(0),Signature::coercible(0)that represent the same thing -- nullary. I hope we can have a single representation forNoArgcase. I think adding constraint toSignature::Any(n) that n > 0makes sense to me, we can have it as "Non-zero number of arguments of any type"alamb commentedon Dec 14, 2024
In my opinion the important thing is that someone who has an existing
Signature::any(0)etc is guided to how to fix the problem.Ideas (I can make PRs for this once we agree)
Option 1: Backwards compatibility:
Signature::any(0)andSignature::exact(0)andSignature::coercible(0)that does the same as nullary (I think this would be the most user friendly and would be backwards compatible)Option 2: Better errors
"Signature::any(0) is not supported, please useSignature::nullary`)Option 3: API w/ compile time checks
We 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):
jayzhan211 commentedon Dec 14, 2024
Option2 seems like a better choice
alamb commentedon Dec 17, 2024
I have begun working on Option 2. Here is a PR that adds better comments:
TypeSignature::NullAry-->TypeSignature::Nullaryand improve comments #13817alamb commentedon Dec 21, 2024
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
example_union()is very confusing to me as it implies that zero arguments are supported but in this case it is not.I think the issue is that users can construct a
Signature::any(0)but it is currently interpreted to be "invalid", but that can't be detected until execution time.I think this will be percived as a tricky/annoying regression for people who have existing udfs with
Signature::anyEdit: this isn't correct:
I also did some testing and it turns out that there are several other ways people could represent zero argument signatures (TypeSignature::Uniform(0, ..)for example which are still allowedalamb commentedon Dec 21, 2024
From my perspective the fact that
Signature::Any(0)used to work in prior versions and now does not is a regression that we should fixHere is a proposal to do so:
Signature::Nullary, improve error messages #13871