feat: support named arguments for aggregate and window udfs#18389
Conversation
github-actions
bot
added
documentation
bubulalabu
changed the title
Jefffrey
left a comment
Jefffrey
left a comment
There was a problem hiding this comment.
Looks good to me overall; some CI failures to address though
| ... | ||
| ``` | ||
|
|
||
| ### Named Arguments for Window UDFs |
There was a problem hiding this comment.
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.
I removed all the explicit cases that were redundant
There was a problem hiding this comment.
Sorry what do you mean by this? I was referring to the documentation
There was a problem hiding this comment.
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.
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.
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.
|
Thanks @bubulalabu |
…8389) ## Which issue does this PR close? Addresses portions of apache#17379. ## Rationale for this change Add support for aggregate and window UDFs in the same way as we did it for scalar UDFs here: apache#18019 ## Are these changes tested? Yes ## Are there any user-facing changes? Yes, the changes are user-facing, documented, purely additive and non-breaking.
…8389) ## Which issue does this PR close? Addresses portions of apache#17379. ## Rationale for this change Add support for aggregate and window UDFs in the same way as we did it for scalar UDFs here: apache#18019 ## Are these changes tested? Yes ## Are there any user-facing changes? Yes, the changes are user-facing, documented, purely additive and non-breaking.
2 participants
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.