-
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
Adds WindowUDFImpl::reverse_expr
trait method + Support for IGNORE NULLS
#12662
Conversation
WindowUDFImpl::reverse_expr
+ Support for IGNORE NULLS
WindowUDFImpl::reverse_expr
+ Support for IGNORE NULLS
WindowUDFImpl::reverse_expr
+ Support for IGNORE NULLS
WindowUDFImpl::reverse_expr
trait method + Support for IGNORE NULLS
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.
I love it -- thank you @jcsherin
One thing I noticed is that there are no tests for this functionality (as in if we removed / broke the code no tests would fail). Is the idea that it will be test coverage added when we port some of the built in window functions?
@@ -351,6 +359,24 @@ pub trait WindowUDFImpl: Debug + Send + Sync { | |||
fn coerce_types(&self, _arg_types: &[DataType]) -> Result<Vec<DataType>> { | |||
not_impl_err!("Function {} does not implement coerce_types", self.name()) | |||
} | |||
|
|||
/// Allows customizing the behavior of the user-defined window |
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.
👍 -- this is a very nice API
Yes, this situation is temporary. When one of the built-in window functions like |
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 @jcsherin -- the temporary nature of this code makes sense to me
/// Allows customizing the behavior of the user-defined window | ||
/// function when it is evaluated in reverse order. | ||
fn reverse_expr(&self) -> ReversedUDWF { | ||
ReversedUDWF::NotSupported |
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.
given the newly added trait method returns a default value, I don't think this is a breaking API change
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.
Got it 👍
Thank you @jcsherin |
Which issue does this PR close?
Part of #8709.
Closes #12661.
Rationale for this change
IGNORE NULLS
is set.What changes are included in this PR?
WindowUDFExpr
with two new state fields for tracking if a window expression is reversed, and ifIGNORE NULLS
is used in the query.WindowUDFImpl::reverse_expr
with a default implementation.ignore_nulls
when creating user-defined window expressions.Are these changes tested?
Yes, against existing tests in CI.No.
When built-in functions like
lead
,lag
,nth_value
etc. are converted, this API will get tested by existing sqllogictests.Are there any user-facing changes?
Yes, this is a breaking API change:WindowUDFImpl::reverse_expr
No (default implementation provided for new trait method
WindowUDFImpl::reverse_expr
).