[GitHub] [arrow-datafusion] waynexia commented on a diff in pull request #6129: fix: `common_subexpr_eliminate` and aggregates

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

waynexia commented on code in PR #6129:
URL: https://github.com/apache/arrow-datafusion/pull/6129#discussion_r1179987474


##########
datafusion/optimizer/src/common_subexpr_eliminate.rs:
##########
@@ -58,17 +60,14 @@ type Identifier = String;
 pub struct CommonSubexprEliminate {}
 
 impl CommonSubexprEliminate {
-    fn rewrite_expr(
+    fn rewrite_exprs_list(

Review Comment:
   👍 good refactor



##########
datafusion/optimizer/src/common_subexpr_eliminate.rs:
##########
@@ -352,6 +462,49 @@ fn build_recover_project_plan(schema: &DFSchema, input: LogicalPlan) -> LogicalP
     )
 }
 
+/// Which type of [expressions](Expr) should be considered for rewriting?

Review Comment:
   This enumeration is excellent! It provides a more organized approach to managing the "special cases".



##########
datafusion/optimizer/src/common_subexpr_eliminate.rs:
##########
@@ -197,16 +223,93 @@ impl OptimizerRule for CommonSubexprEliminate {
                 let new_aggr_expr = pop_expr(&mut new_expr)?;
                 let new_group_expr = pop_expr(&mut new_expr)?;
 
-                Some(LogicalPlan::Aggregate(Aggregate::try_new_with_schema(

Review Comment:
   I need file a PR to restructure this (very) huge map as it keeps expanding...



-- 
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