[GitHub] [arrow] alamb commented on a change in pull request #8746: ARROW-10688: [Rust] [DataFusion] Implement CASE WHEN logical plan

[GitBox <gi...@apache.org>]

alamb commented on a change in pull request #8746:
URL: https://github.com/apache/arrow/pull/8746#discussion_r529051136



##########
File path: rust/datafusion/src/optimizer/utils.rs
##########
@@ -216,26 +238,51 @@ pub fn from_plan(
 
 /// Returns all direct children `Expression`s of `expr`.
 /// E.g. if the expression is "(a + 1) + 1", it returns ["a + 1", "1"] (as Expr objects)
-pub fn expr_sub_expressions(expr: &Expr) -> Result<Vec<&Expr>> {
+pub fn expr_sub_expressions(expr: &Expr) -> Result<Vec<Expr>> {
     match expr {
-        Expr::BinaryExpr { left, right, .. } => Ok(vec![left, right]),
-        Expr::IsNull(e) => Ok(vec![e]),
-        Expr::IsNotNull(e) => Ok(vec![e]),
-        Expr::ScalarFunction { args, .. } => Ok(args.iter().collect()),
-        Expr::ScalarUDF { args, .. } => Ok(args.iter().collect()),
-        Expr::AggregateFunction { args, .. } => Ok(args.iter().collect()),
-        Expr::AggregateUDF { args, .. } => Ok(args.iter().collect()),
-        Expr::Cast { expr, .. } => Ok(vec![expr]),
+        Expr::BinaryExpr { left, right, .. } => {
+            Ok(vec![left.as_ref().to_owned(), right.as_ref().to_owned()])
+        }
+        Expr::IsNull(e) => Ok(vec![e.as_ref().to_owned()]),
+        Expr::IsNotNull(e) => Ok(vec![e.as_ref().to_owned()]),
+        Expr::ScalarFunction { args, .. } => Ok(args.iter().map(|e| e.clone()).collect()),
+        Expr::ScalarUDF { args, .. } => Ok(args.iter().map(|e| e.clone()).collect()),
+        Expr::AggregateFunction { args, .. } => {
+            Ok(args.iter().map(|e| e.clone()).collect())
+        }
+        Expr::AggregateUDF { args, .. } => Ok(args.iter().map(|e| e.clone()).collect()),
+        Expr::Case {
+            expr,
+            when_then_expr,
+            else_expr,
+            ..
+        } => {
+            let mut expr_list: Vec<Expr> = vec![];

Review comment:
       The pattern I have seen elsewhere (aka that Postgres has) is two types of expression visitor -- one that just visits each node in an expression tree, and one that walks over the tree and returns a (potentially) rewritten node
   
   I have an expr visitor here in IOx: https://github.com/influxdata/influxdb_iox/blob/659da9264ab39e9d0b2ea3a15c082f8c25fe0a68/storage/src/util.rs#L8
   
   If you think it is a useful thing @andygrove -- I can take a shot at porting to Arrow




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

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