Improve documentation for ExprVisitor, port simple uses to new walking function #4916
Conversation
github-actions
bot
added
logical-expr
| @@ -83,20 +85,16 @@ pub fn grouping_set_to_exprlist(group_expr: &[Expr]) -> Result<Vec<Expr>> { | |||
| } | |||
| } | |||
|
|
|||
| /// Recursively walk an expression tree, collecting the unique set of column names | |||
| /// Recursively walk an expression tree, collecting the unique set of columns | |||
There was a problem hiding this comment.
here is a pretty good example of reducing the ceremony required do to a walk of an expr by using inspect_expr_pre rather than having to define a visitor explicitly
| pub fn expr_to_columns(expr: &Expr, accum: &mut HashSet<Column>) -> Result<()> { | ||
| inspect_expr_pre(expr, |expr| { |
Thanks @jackwener -- I totally understand! |
|
Benchmark runs are scheduled for baseline = ba9fc12 and contender = ef4ca7e. ef4ca7e 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?
N/A
Rationale for this change
Follow on to #4906
Some uses of
ExpressionVisitorneed its full power (visit both on pre/post traversal, see https://github.com/apache/arrow-datafusion/blob/d49c805c93498b542c130c77099b719f2a4ea8ba/datafusion/optimizer/src/common_subexpr_eliminate.rs#L421) but several uses just need a simple town down traversal.#4906 added a convenience function to do that traversal.
What changes are included in this PR?
walk_expr_downtoinspect_expr_preinspect_expr_preto simplify themI am hoping to do something similar to ExpressionMutator for rewriting exprs.
Are these changes tested?
Covered by existing tests