-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Add PartitionEvaluatorArgs to WindowUDFImpl::partition_evaluator
#12804
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
Conversation
alamb
left a comment
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 -- this PR makes sense to me
I had a small API suggestions (but I also think we could do it as a follow on PR)
Also, I think this PR is an API change (for the better) so I added the api-change label
| /// Defines the state of the user-defined window function during | ||
| /// physical execution. |
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 description doesn't quite match what I think the structure does. Perhaps something like this would be more appropriate
| /// Defines the state of the user-defined window function during | |
| /// physical execution. | |
| /// Arguments passed to create user-defined window function state during | |
| /// physical execution. |
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.
Done.
| /// Returns `Some(expr)` argument at index if it exists, otherwise | ||
| /// returns `None`. | ||
| pub fn input_expr_at(&self, index: usize) -> Option<&Arc<dyn PhysicalExpr>> { | ||
| self.input_exprs.get(index) | ||
| } |
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 think an API that would be more consistent with others in this would be to expose the input_exprs directly.
Among other things that would allow users of this API to find out how many (len()) were passed as well as get an iterator, etc.
/// Returns the arguments passed to the function
pub fn input_exprs(&self) -> &'a [Arc<dyn PhysicalExpr>] {
self.input_exprs
}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.
Agreed.
I extended the change to also expose input_types directly for similar reasons.
/// Returns the expressions passed as arguments to the user-defined
/// window function.
pub fn input_exprs(&self) -> &'a [Arc<dyn PhysicalExpr>] {
self.input_exprs
}
/// Returns the [`DataType`]s corresponding to the input expressions
/// to the user-defined window function.
pub fn input_types(&self) -> &'a [DataType] {
self.input_types
}| fn create_evaluator(&self) -> Result<Box<dyn PartitionEvaluator>> { | ||
| self.fun.partition_evaluator_factory() | ||
| self.fun | ||
| .partition_evaluator_factory(PartitionEvaluatorArgs::new( |
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 think this is the key reason to add these args
|
Thanks again @jcsherin |
|
Thanks for the review @alamb |
Which issue does this PR close?
Closes #12803.
Rationale for this change
In
BuiltInWindowFunction::{Lead, Lag}to create an instance ofPartitionEvaluatordirectly/indirectly depends on the following being available:Currently none of this is available when implementing user-defined window functions.
What changes are included in this PR?
Are these changes tested?
Yes. Updated unit tests for
row_number()user-defined window function.Are there any user-facing changes?
Yes.