Skip to content
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

Open
Tracked by #13334
alamb opened this issue Dec 13, 2024 · 8 comments · May be fixed by #13881 or #13871
Open
Tracked by #13334

Make migration to Signature::nullary in 44.0.0 easier / less confusing #13763

alamb opened this issue Dec 13, 2024 · 8 comments · May be fixed by #13881 or #13871
Labels
bug Something isn't working enhancement New feature or request regression Something that used to work no longer does

Comments

@alamb
Copy link
Contributor

alamb commented Dec 13, 2024

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 an Err value: 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:

  1. Support the old style signature for zero args
  2. 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

@alamb alamb added the enhancement New feature or request label Dec 13, 2024
@alamb alamb changed the title Make Signature::nullary easier to find Make migration to Signature::nullary easier / less confusing Dec 13, 2024
@alamb alamb changed the title Make migration to Signature::nullary easier / less confusing Make migration to Signature::nullary in 44.0.0 easier / less confusing Dec 13, 2024
@findepi
Copy link
Member

findepi commented Dec 13, 2024

Signature::any is specified as "A specified number of arguments of any type"
what's wrong with Signature::any(0)?

@alamb
Copy link
Contributor Author

alamb commented Dec 13, 2024

Signature::any is specified as "A specified number of arguments of any type"
what's wrong with Signature::any(0)?

I don't know. It would also be fine if that format kept working as before (maybe better).

@jayzhan211
Copy link
Contributor

jayzhan211 commented Dec 14, 2024

The reason why Signature::nullary is the preferred choice is that otherwise we can have Signature::any(0), Signature::exact(0), Signature::coercible(0) that represent the same thing -- nullary. I hope we can have a single representation for NoArg case. I think adding constraint to Signature::Any(n) that n > 0 makes sense to me, we can have it as "Non-zero number of arguments of any type"

@alamb
Copy link
Contributor Author

alamb commented 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:

  1. Add special cases for Signature::any(0) and Signature::exact(0) and Signature::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

  1. make the error message more specific (something like "Signature::any(0) is not supported, please use Signature::nullary`)
  2. Add documentation to the enums so that if someone is lookiing for "what signature do I use for 0 argument function" they can find nullary quickly

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):

pub enum Signature {
  Exact(NonZeroUsize)
...
}

@jayzhan211
Copy link
Contributor

Option2 seems like a better choice

@alamb
Copy link
Contributor Author

alamb commented Dec 17, 2024

I have begun working on Option 2. Here is a PR that adds better comments:

@alamb
Copy link
Contributor Author

alamb commented 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:

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.

Candidate functions:
    example_union()

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::any

Edit: 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 allowed

@alamb alamb added regression Something that used to work no longer does bug Something isn't working labels Dec 21, 2024
@alamb
Copy link
Contributor Author

alamb commented 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 fix

Here is a proposal to do so:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request regression Something that used to work no longer does
Projects
None yet
3 participants