Refactor Builtin Window Function Implementation #4441
Conversation
mustafasrepo
changed the title
|
FYI, we did an internal review of this -- please let us know if there are any concerns and we will be happy to address them if necessary. |
There was a problem hiding this comment.
Looks like a good change to me -- thank you @mustafasrepo and @metesynnada
|
|
||
| /// Get values columns(argument of Window Function) | ||
| /// and order by columns (columns of the ORDER BY expression)used in evaluators | ||
| fn get_values_orderbys( |
There was a problem hiding this comment.
👍 this is a nice refactor of common functionality
| &self, | ||
| _batch: &RecordBatch, | ||
| ) -> Result<Box<dyn PartitionEvaluator>> { | ||
| fn create_evaluator(&self) -> Result<Box<dyn PartitionEvaluator>> { |
|
Benchmark runs are scheduled for baseline = a0485e7 and contender = 09aea09. 09aea09 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
Which issue does this PR close?
Closes #4440.
Rationale for this change
Handling of the Builtin Window Functions (such as
ROW_NUMBER,FIRST_VALUE, etc.) are quite different than accumulators (such asSUM,AVG, etc.) even though they have significant commonalities. We can increase readability if we make their implementation similar in spirit; and we can increase code reuse in this way too. This PR changes thePartitionEvaluatortrait such that it resembles theAccumulatortrait.What changes are included in this PR?
The
create_evaluatormethod ofBuiltInWindowFunctionExprno longer takes a record batch during creation. Instead; when we are doing evaluation, we pass the relevant batch section to theevaluatemethod of thePartitionEvaluatortrait.Are these changes tested?
N.A (Existing tests should work after refactor)
Are there any user-facing changes?