[GitHub] [arrow-datafusion] alamb commented on a diff in pull request #5092: refactor: Add `rewrite_expr` convenience method for rewriting `Expr`s

["alamb (via GitHub)" <gi...@apache.org>]

alamb commented on code in PR #5092:
URL: https://github.com/apache/arrow-datafusion/pull/5092#discussion_r1089123858


##########
datafusion/expr/src/expr_rewriter.rs:
##########
@@ -44,8 +44,44 @@ pub enum RewriteRecursion {
 
 /// Trait for potentially recursively rewriting an [`Expr`] expression
 /// tree. When passed to `Expr::rewrite`, `ExpressionVisitor::mutate` is
-/// invoked recursively on all nodes of an expression tree. See the
-/// 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

Review Comment:
   moved so it is more visible in rustdocs



##########
datafusion/expr/src/expr_rewriter.rs:
##########
@@ -429,27 +433,14 @@ pub fn normalize_col_with_schemas(
     schemas: &[&Arc<DFSchema>],
     using_columns: &[HashSet<Column>],
 ) -> Result<Expr> {
-    struct ColumnNormalizer<'a> {

Review 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



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org