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


##########
datafusion/expr/src/logical_plan/plan.rs:
##########
@@ -537,55 +558,52 @@ impl LogicalPlan {
         Ok(true)
     }
 
-    /// Visit all inputs, including subqueries
-    pub fn visit_all_inputs<V>(&self, visitor: &mut V) -> Result<bool, 
V::Error>
+    /// applies visitor to any subqueries in the plan
+    fn visit_subqueries<V>(&self, v: &mut V) -> Result<bool, V::Error>
     where
         V: PlanVisitor,
     {
-        for input in self.all_inputs() {
-            if !input.accept(visitor)? {
-                return Ok(false);
+        // todo make observe_expressions fallable
+        let mut result = Ok(true);
+
+        self.observe_expressions(|expr| {
+            // terminate on first error
+            if result.is_err() {
+                return;
             }
-        }
+            // recursively look for subqueries
+            let walk_result = walk_expr_down(expr, |expr| {
+                match expr {
+                    Expr::Exists { subquery, .. }
+                    | Expr::InSubquery { subquery, .. }
+                    | Expr::ScalarSubquery(subquery) => {
+                        // use a synthetic plan so the visitor sees a
+                        // LogicalPlan::Subquery (even though it is
+                        // actually a Subquery alias)
+                        let synthetic_plan = 
LogicalPlan::Subquery(subquery.clone());
+                        if let Err(e) = synthetic_plan.accept(v) {
+                            //return Err(DataFusionError::Internal("TODO real 
error".into()))
+                            todo!();
+                        }
+                    },
+                    _ => {}
+                }
+                Ok(())
+            });
 
-        Ok(true)
-    }
+            if let Err(e) = walk_result {
+                todo!();
+            }
 
-    /// Get all plan inputs, including subqueries from expressions
-    fn all_inputs(&self) -> Vec<Arc<LogicalPlan>> {
-        let mut inputs = vec![];
-        for expr in self.expressions() {
-            Self::collect_subqueries(&expr, &mut inputs);
-        }
-        for input in self.inputs() {
-            inputs.push(Arc::new(input.clone()));
-        }
-        inputs
-    }
+        });
 
-    fn collect_subqueries(expr: &Expr, sub: &mut Vec<Arc<LogicalPlan>>) {
-        match expr {

Review Comment:
   This expr is missing all sorts of possible locations for subqueries (like IS 
NULL); @askoa  fixed one in 
https://github.com/apache/arrow-datafusion/pull/4900 but there are very likely 
more



-- 
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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Reply via email to