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


##########
datafusion/optimizer/src/optimize_projections.rs:
##########
@@ -162,14 +162,36 @@ fn optimize_projections(
                 .map(|input| 
((0..input.schema().fields().len()).collect_vec(), false))
                 .collect::<Vec<_>>()
         }
+        LogicalPlan::Extension(extension) => {
+            let necessary_children_indices = if let 
Some(necessary_children_indices) =
+                extension.node.necessary_children_exprs(indices)
+            {
+                necessary_children_indices
+            } else {
+                // Requirements from parent cannot be routed down to user 
defined logical plan safely
+                return Ok(None);
+            };
+            let children = extension.node.inputs();
+            assert_eq!(children.len(), necessary_children_indices.len());

Review Comment:
   Can we make this an internal error instead of a panic, to try and provide a 
nicer failure mode?



##########
datafusion/optimizer/src/optimize_projections.rs:
##########
@@ -1192,4 +1354,126 @@ mod tests {
         \n    TableScan: test projection=[a]";
         assert_optimized_plan_equal(&plan, expected)
     }
+
+    // Optimize Projections Rule, pushes down projection through users defined 
logical plan node.
+    #[test]
+    fn test_user_defined_logical_plan_node() -> Result<()> {

Review Comment:
   As written it is hard to quickly understand what cases each test is covering.
   
   Would it be possible to add a comment to the tests explaining what they are 
testing (or maybe update the name)? 



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