refactor: Add rewrite_expr convenience method for rewriting Exprs#5092
refactor: Add rewrite_expr convenience method for rewriting Exprs#5092alamb merged 6 commits intoapache:masterfrom
rewrite_expr convenience method for rewriting Exprs#5092Conversation
| /// comments on `Expr::rewrite` for details on its use | ||
| /// invoked recursively on all nodes of an expression tree. | ||
| /// | ||
| /// Performs a depth first walk of an expression and its children |
There was a problem hiding this comment.
moved so it is more visible in rustdocs
| schemas: &[&Arc<DFSchema>], | ||
| using_columns: &[HashSet<Column>], | ||
| ) -> Result<Expr> { | ||
| struct ColumnNormalizer<'a> { |
There was a problem hiding this comment.
Here is a pretty good example the boilerplate that can be avoided by using rewrite_expr rather than ExprRewriter (rust generates a closure that captures schemas and using_columns) rather than having to do it explicitly
|
I wonder if @jackwener / @HaoYang670 have any thoughts on this approach? |
There was a problem hiding this comment.
I really very like this PR. After merge it, we can more convenient to use rewrite_expr.
Thanks @alamb .
cc @mingmwang , how do you think it?
| /// | ||
| /// assert_eq!(rewritten, col("a") + lit(42)); | ||
| /// ``` | ||
| pub fn rewrite_expr<F>(expr: Expr, f: F) -> Result<Expr> |
There was a problem hiding this comment.
The real change in this PR is to add this function.
|
Really sorry for the late. I just came back from the Spring Festival. I will review this tomorrow, thanks. |
HaoYang670
left a comment
HaoYang670
left a comment
There was a problem hiding this comment.
Thank you, @alamb. I learned much from your PR.
| where | ||
| F: FnMut(Expr) -> Result<Expr>, | ||
| { | ||
| fn mutate(&mut self, expr: Expr) -> Result<Expr> { |
There was a problem hiding this comment.
Can we let ExprRewriter::mutate return Result<Option<E>> in the future, just like what we've done for LogicalPlan::try_optimize?
There was a problem hiding this comment.
That is an interesting idea. What usecase do you have in mind?
Since mutate takes the expr by ownership (rather than by reference), I think returning expr would be the same as not changing the original expr (perhaps the same as returning None?)
However, I do wonder what actually happens when rewriting Boxed exprs 🤔 maybe they are still copied
| exprs.into_iter().map(unnormalize_col).collect() | ||
| } | ||
|
|
||
| struct RewriterAdapter<F> { |
There was a problem hiding this comment.
It is better to add some comments for RewriterAdapter, but this is not a blocker.
…sion into alamb/rewrite_adapter
Co-authored-by: Remzi Yang <59198230+HaoYang670@users.noreply.github.com>
|
Thank you for the reviews @HaoYang670 and @jackwener |
|
Benchmark runs are scheduled for baseline = 67b1da8 and contender = 51d5840. 51d5840 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
4 participants
Note: this may look like a large change, but it is mostly comments.
Which issue does this PR close?
related to #4854
Rationale for this change
There are some rewrites (like constant propagation) that require the full generality of
ExprRewriterthat saves state on a previst on down and then rewrite while walking back up. However, most rewrites simply callmutate.Now you have to define a Visitor struct to pass arguments down the tree. Letting rust do the work for you via a closure makes for less code
What changes are included in this PR?
Changes:
rewrite_exprconvenience function (likeinspect_expr_pre) for the common case-fold-searchExprRewriterto demonstrate how to use itIf this PR is approved, I'll spend some time porting over other uses of
ExprRewritersAre these changes tested?
Yes, both a new doc tests and existing coverage
Are there any user-facing changes?
Developers can hopefully write lest code