Add support for AggregateExpr, WindowExpr rewrite.#10742
Merged
alamb merged 4 commits intoapache:mainfrom May 31, 2024
Merged
Conversation
waynexia
approved these changes
May 31, 2024
Member
waynexia
left a comment
waynexia
left a comment
There was a problem hiding this comment.
Those new APIs look good to me, thanks @mustafasrepo
alamb
reviewed
May 31, 2024
| /// Rewrites [`AggregateExpr`], with new expressions given. The argument should be consistent | ||
| /// with the return value of the [`AggregateExpr::all_expressions`] method. | ||
| /// Returns `Some(Arc<dyn AggregateExpr>)` if re-write is supported, otherwise returns `None`. | ||
| fn with_new_expressions( |
Contributor
There was a problem hiding this comment.
The only thing that might be worth considering is that this API forces cloning. However, since everything is Arcd that seems relatively minor. Maybe something to keep in mind for the future.
Contributor
|
Thanks @mustafasrepo and @waynexia |
findepi
pushed a commit
to findepi/datafusion
that referenced
this pull request
Jul 16, 2024
* Initial commit * Minor changes * Minor changes * Update comments
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Closes #.
Rationale for this change
While working on another PR, in which I implement a new custom rule. I hit a usecase where I want to re-write
AggregateExpr, andWindowExpraccording to some kind of projection. (For instance , in terms of the schema of the child). In my use case, I find it useful to define an APIfn all_expressions(&self) -> (function args, order bys, ..)to return all expressions referred by the aggregate or window function (By the way currentfn expressions(&self)API only returns function arguments).With this API in place, we can update
Arc<dyn PhysicalExpr>s, at the outside (such as inside the rule). Then we can use updated expressions to re-writeArc<dyn AggregateExpr>by using following API:This behaviour, is very similar to the
fn expressions(),,fn with_expressions()methods forLogicalPlans.Also as far as I am aware @berkaysynnada's PR for projection optimization will also require such an API, to be able to check pushdown an aggregate expression or window expression successfully.
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?