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

Rename expr::window_function::WindowFunction to WindowFunctionDefintion for consistency #8347

Closed
alamb opened this issue Nov 28, 2023 · 6 comments · Fixed by #8382
Closed
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@alamb
Copy link
Contributor

alamb commented Nov 28, 2023

Is your feature request related to a problem or challenge?

Part of #8045 we are working
to consolidate the function implementation in DataFusion to make it easier to
manage DataFusion's built in list of functions as well as ensure user defined
and built in functions have the same feature sets.

I think it is also important to keep the Expr representation consistent for
Aggregate and Window functions to make DataFusion easier to work with, as well
as to permit eventually applying the same unification of built in and user
defined functions to aggregates and window functions.

Describe the solution you'd like

I would like to rename the WindowFunction in to https://github.com/apache/arrow-datafusion/blob/4c914ea1e5d3dc61f3552adcffb8356c90d73bac/datafusion/expr/src/window_function.rs#L35-L44 to WindowFunctionDefinition so we didn't have so many things named WindowFunction and to mirror the structure of ScalarFunction

Edited: I would like to unify Expr::WindowFunction and Expr::WindowUDF following the pattern in #8258 @2010YOUY01:

Describe alternatives you've considered

No response

Additional context

I think this is a pretty good first issue because we have an existing pattern in #8258

@alamb alamb added enhancement New feature or request good first issue Good for newcomers labels Nov 28, 2023
@alamb
Copy link
Contributor Author

alamb commented Nov 29, 2023

it seems like we already unified WindowFunction in

That is an excellent point. Thank you for the research @haohuaijin

Maybe we could rename https://github.com/apache/arrow-datafusion/blob/4c914ea1e5d3dc61f3552adcffb8356c90d73bac/datafusion/expr/src/window_function.rs#L35-L44 to WindowFunctionDefinition so we didn't have so many things named WindowFunction and to mirror the structure of ScalarFunctions

@edmondop
Copy link
Contributor

@haohuaijin are you already working on it or can I pick it up?

@haohuaijin
Copy link
Contributor

@edmondop I haven't started working yet, feel free to pick it up.

@edmondop
Copy link
Contributor

@alamb I am probably losing some pre-requisite, the Expr doesn't contain a WindowUDF variant but only a WindowFunction variant https://github.com/apache/arrow-datafusion/blob/main/datafusion/expr/src/expr.rs#L156 there is an AggregateUDF though...

@alamb alamb changed the title Unify Expr::WindowFunction and Expr::WindowUDF Rename expr::window_function::WindowFunction to WindowFunctionDefintion for consistency Nov 30, 2023
@alamb
Copy link
Contributor Author

alamb commented Nov 30, 2023

@alamb I am probably losing some pre-requisite, the Expr doesn't contain a WindowUDF variant but only a WindowFunction variant https://github.com/apache/arrow-datafusion/blob/main/datafusion/expr/src/expr.rs#L156 there is an AggregateUDF though...

You are right that the ticket description was incorrect, as @haohuaijin noted in #8347 (comment)

I have updated it to say "rename WindowFunctiontoWindowFunctionDefinition`" -- sorry for the confusion

I think AggregateUDF is covered by #8346

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
3 participants