Re: [PR] Stop copying LogicalPlan and Exprs in `PushDownFilter` (4%-6% faster planning) [datafusion]

[via GitHub]

jackwener commented on code in PR #10444:
URL: https://github.com/apache/datafusion/pull/10444#discussion_r1602521417


##########
datafusion/optimizer/src/push_down_filter.rs:
##########
@@ -150,44 +151,33 @@ pub struct PushDownFilter {}
 /// non-preserved side it can be more tricky.
 ///
 /// Returns a tuple of booleans - (left_preserved, right_preserved).
-fn lr_is_preserved(plan: &LogicalPlan) -> Result<(bool, bool)> {
-    match plan {
-        LogicalPlan::Join(Join { join_type, .. }) => match join_type {
-            JoinType::Inner => Ok((true, true)),
-            JoinType::Left => Ok((true, false)),
-            JoinType::Right => Ok((false, true)),
-            JoinType::Full => Ok((false, false)),
-            // No columns from the right side of the join can be referenced in 
output
-            // predicates for semi/anti joins, so whether we specify t/f 
doesn't matter.
-            JoinType::LeftSemi | JoinType::LeftAnti => Ok((true, false)),
-            // No columns from the left side of the join can be referenced in 
output
-            // predicates for semi/anti joins, so whether we specify t/f 
doesn't matter.
-            JoinType::RightSemi | JoinType::RightAnti => Ok((false, true)),
-        },
-        LogicalPlan::CrossJoin(_) => Ok((true, true)),
-        _ => internal_err!("lr_is_preserved only valid for JOIN nodes"),
+fn lr_is_preserved(join_type: JoinType) -> Result<(bool, bool)> {

Review Comment:
   👍



##########
datafusion/optimizer/src/push_down_filter.rs:
##########
@@ -941,22 +935,65 @@ impl OptimizerRule for PushDownFilter {
                     None => 
extension_plan.node.inputs().into_iter().cloned().collect(),
                 };
                 // extension with new inputs.
+                let child_plan = LogicalPlan::Extension(extension_plan);
                 let new_extension =
                     child_plan.with_new_exprs(child_plan.expressions(), 
new_children)?;
 
-                match conjunction(keep_predicates) {
+                let new_plan = match conjunction(keep_predicates) {
                     Some(predicate) => LogicalPlan::Filter(Filter::try_new(
                         predicate,
                         Arc::new(new_extension),
                     )?),
                     None => new_extension,
-                }
+                };
+                Ok(Transformed::yes(new_plan))
             }
-            _ => return Ok(Transformed::no(plan)),
-        };
+            child => {
+                filter.input = Arc::new(child);
+                Ok(Transformed::no(LogicalPlan::Filter(filter)))
+            }
+        }
+    }
+}
+
+/// Creates a new LogicalPlan::Filter node.
+pub fn make_filter(predicate: Expr, input: Arc<LogicalPlan>) -> 
Result<LogicalPlan> {
+    Filter::try_new(predicate, input).map(LogicalPlan::Filter)
+}
 
-        Ok(Transformed::yes(new_plan))
+/// Replace the existing child of the single input node with `new_child`.
+///
+/// Starting:
+/// ```text
+/// plan
+///   child
+/// ```
+///
+/// Ending:
+/// ```text
+/// plan
+///   new_child
+/// ```
+fn insert_below(
+    plan: LogicalPlan,
+    new_child: LogicalPlan,
+) -> Result<Transformed<LogicalPlan>> {

Review Comment:
   It's a great common method to me👍



-- 
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-unsubscr...@datafusion.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org