apache / datafusion

Apache DataFusion SQL Query Engine
https://datafusion.apache.org/
Apache License 2.0
6.33k stars 1.2k forks source link

Rename / simplify `BuiltInWindowExpr` / `BuiltInWindowFunctionExpr` #13473

Open alamb opened 3 days ago

alamb commented 3 days ago

Is your feature request related to a problem or challenge?

However, there are still a few structures named BuiltInWindow* that now no longer reflect the reality that there are no more "built in" functions:

A BuiltInWindowExpr here:

https://github.com/apache/datafusion/blob/22c1f54411a02009629b3a76a43bd4343add045d/datafusion/physical-expr/src/window/built_in.rs#L38-L45

A BuiltInWindowFunctionExpr:

https://github.com/apache/datafusion/blob/22c1f54411a02009629b3a76a43bd4343add045d/datafusion/physical-expr/src/window/built_in_window_function_expr.rs#L30-L39

Which confusingly is implemented for WindowUDFExpr: https://github.com/apache/datafusion/blob/d840e987cd855fbbdc5d3e5d69683a5b4f279bb6/datafusion/physical-plan/src/windows/mod.rs#L218-L221

Describe the solution you'd like

  1. I would like to rename the confusing BuiltIn prefix for these structures
  2. Leave backwards compatibility typedefs type BuiltInWindowExpr = ... to ease migration

Describe alternatives you've considered

I would recommend first simply renaming the structs / traits to remove the BuiltIn prefix / module names

Then I also think it is worth seeing if the BuiltInWindowFunctionExpr trait is still needed, or if the code in physical plan could simply call WindowUDF directly

Additional context

No response

irenjj commented 2 days ago

take