-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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 BuiltInWindow*
to StandardWindow*
#13536
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @irenjj -- this is looking close. I have a few suggestions. Let me know what you think
pub use sliding_aggregate::SlidingAggregateWindowExpr; | ||
pub use udf::UDFWindowExpr; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please add a "backwards compatible typedef" here to help people upgrading who might have code that refers to BuiltInWindowExpr
and BuiltInWindowFunctionExpr?
For example
datafusion/datafusion/physical-expr-common/src/sort_expr.rs
Lines 505 to 506 in 6a02384
#[deprecated(since = "43.0.0", note = "use &LexOrdering instead")] | |
pub type LexOrderingRef<'a> = &'a [PhysicalSortExpr]; |
Something like
#[deprecated(since = "44.0.0", note = "use UDFWindowExpr")]
pub type BuiltInWindowExpr = UDFWindowExpr;
#[deprecated(since = "44.0.0", note = "use UDFWindowFunctionExpr")]
pub type BuiltInWindowFunctionExpr = UDFWindowFunctionExpr;
mod sliding_aggregate; | ||
mod udf; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is somewhat strange that we now have "UD ..." for user defined window functions as everything is a user defined window function.
Maybe we could use the names StandardWindowExpr
and StandardWindowFunctionExpr
instead
I think WindowExpr
can be either an aggregate (like sum
) or one of these standard window exprs (first_value, etc)
BuiltInWindow*
to UDFWindow*
BuiltInWindow*
to StandardWindow*
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great to me -- thank you @irenjj
Which issue does this PR close?
Part of #13473
Rationale for this change
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?