apache / datafusion

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

Remove `BuiltInWindowFunction` (LogicalPlans) #13393

Closed alamb closed 1 week ago

alamb commented 1 week ago

Which issue does this PR close?

Rationale for this change

@buraksen and @jcsherin ported the last window function over, so now we can remove the old BuiltInWindowFunction code

This was so tempting a cleanup that I had to give it a try (and I wanted to do some coding)

What changes are included in this PR?

Changes:

  1. Remove BuiltInWindowFunction
  2. Remove BuiltInWindowFunctionDefinition::BuiltInWindowFunction
  3. Remove Protobuf definitions
  4. Update code to compile

Are these changes tested?

Yes, by existing CI

alamb commented 1 week ago

There are actually several related "BuiltInWindowExpr" in physical-expr that need to be cleaned up as well (that are no longer related to BuiltInWindowExprs). We may also be able to remove a level of trait.

jonathanc-n commented 1 week ago

@alamb @buraksenn @Omega359 Pretty sure we can now clean up the old window docs, due to the nth_value migration being merged. Lemme see if I can do that really quick

alamb commented 1 week ago

@alamb @buraksenn @Omega359 Pretty sure we can now clean up the old window docs, due to the nth_value migration being merged. Lemme see if I can do that really quick

Yes, we can -- I think that is tracked by

jcsherin commented 1 week ago

LGTM 👍. Thanks @alamb

alamb commented 1 week ago

Thanks @crepererum and @jcsherin for the reviews