-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat: support named arguments for aggregate and window udfs #18389
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
base: main
Are you sure you want to change the base?
feat: support named arguments for aggregate and window udfs #18389
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me overall; some CI failures to address though
| ... | ||
| ``` | ||
|
|
||
| ### Named Arguments for Window UDFs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if this section on named parameters should be generic across scalar/window/aggregate functions, instead of repeating it for each?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed all the explicit cases that were redundant
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry what do you mean by this? I was referring to the documentation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
E.g. initially I explicitly tested if the order for unnamed and named arguments works for all kinds of UDFs. It's fine to test it only for one kind as all of them use the same underlying implementation.
If that wasn't what you were referring, it'd be great if you'd elaborate a bit on your initial comment. An example would be helpful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if there is some kind of misunderstanding, but my comment is referring to the documentation, specifically the adding-udfs.md page. I'm not talking about the tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My bad. Yes, there has been a misunderstanding on my end and your initial comment makes complete sense now. I'll sort it out soon.
Which issue does this PR close?
Addresses portions of #17379.
Rationale for this change
Add support for aggregate and window UDFs in the same way as we did it for scalar UDFs here: #18019
Are these changes tested?
Yes
Are there any user-facing changes?
Yes, the changes are user-facing, documented, purely additive and non-breaking.