Minor: Move get_equal_orderings into BuiltInWindowFunctionExpr, remove BuiltInWindowFunctionExpr::as_any
#6619
Conversation
github-actions
bot
added
core
| .get_built_in_func_expr() | ||
| .as_any() | ||
| .is::<RowNumber>() |
There was a problem hiding this comment.
The point of the PR is to remove this special case for RowNumber (which I did by hoisting it into the trait)
datafusion/physical-expr/src/window/built_in_window_function_expr.rs
Outdated
Show resolved
Hide resolved
alamb
force-pushed
the
alamb/remove_as_any
branch
from
f878c99
to
9137a4a
Compare
| @@ -61,6 +58,24 @@ impl BuiltInWindowFunctionExpr for RowNumber { | |||
| &self.name | |||
| } | |||
|
|
|||
| fn add_equal_orderings(&self, builder: &mut OrderingEquivalenceBuilder) { | |||
| // Only the built-in `RowNumber` window function introduces a new | |||
There was a problem hiding this comment.
Maybe we can replace comment
// Only the built-in RowNumber window function introduces a new with
// The built-in RowNumber window function introduces a new. Since method implements it (and special handling is not done outside), we do not need stress this is the only window function with this property.
There was a problem hiding this comment.
👍 done in commit ad16502
There was a problem hiding this comment.
Thanks @alamb. Other than minor comments, this PR is LGTM!.
With these changes, if the user defined window function introduces ordering, user can add this information and utilize this information in subsequent stages.
By the way regarding the as_any() as fas as I can remember it is not introduces by us. I am not familiar with its purpose. However, even though we no longer have special handling outside and no use-case for it in datafusion code base. I can imagine it may be useful for others. I wonder what others think about it.
@jimexist (who I think added the first version of window functions and perhaps the trait), @andygrove, @viirya or @Dandandan do you have any thoughts about keeping / removing the Given the question, I think the safe thing will be to change this PR to put back the |
|
I have reverted the changes to |
Which issue does this PR close?
This is related to #5781
Rationale for this change
I would like to ensure that user defined window functions have the same expressive power as built in window functions.
In order to achieve this goal, all the important information about a window function is provided by a trait (rather than by other code special casing based on the type).
What changes are included in this PR?
get_equal_orderingsintoBuiltInWindowFunctionExprBuiltInWindowFunctionExpr::as_any(which is how I found the special case for RowNumber)Are these changes tested?
Covered by existing tests
Are there any user-facing changes?
No this only changes internal APIs
Note that my proposal in #6617 for user defined window functions is to make
BuiltInWindowFunctionExprpublic. However, even if we choose another option, I think this still PR makes the code more explicit and easier to find functionality related to window functions so I think it is still a good change