Re: [PR] Rewrite physical expressions in execution plans [datafusion]

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

askalt commented on code in PR #20009:
URL: https://github.com/apache/datafusion/pull/20009#discussion_r2753622808


##########
datafusion/physical-plan/src/joins/hash_join/exec.rs:
##########
@@ -1384,6 +1386,55 @@ impl ExecutionPlan for HashJoinExec {
         }
         Ok(result)
     }
+
+    fn physical_expressions<'a>(
+        &'a self,
+    ) -> Option<Box<dyn Iterator<Item = Arc<dyn PhysicalExpr>> + 'a>> {
+        self.filter
+            .as_ref()
+            .map(|f| Box::new(std::iter::once(Arc::clone(&f.expression))) as Box<_>)
+    }
+
+    fn with_physical_expressions(
+        &self,
+        mut params: ReplacePhysicalExpr,
+    ) -> Result<Option<Arc<dyn ExecutionPlan>>> {
+        let expected_count = self.filter.iter().len();
+        let exprs_count = params.exprs.len();
+        assert_eq_or_internal_err!(
+            expected_count,
+            exprs_count,
+            "Inconsistent number of physical expressions for {}",
+            self.name()
+        );
+
+        let filter = self
+            .filter
+            .as_ref()
+            .zip(params.exprs.pop())
+            .map(|(f, expr)| {
+                JoinFilter::new(expr, f.column_indices.clone(), Arc::clone(&f.schema))
+            });
+
+        Ok(Some(Arc::new(Self {
+            left: Arc::clone(&self.left),
+            right: Arc::clone(&self.right),
+            on: self.on.clone(),
+            filter,
+            join_type: self.join_type,
+            join_schema: Arc::clone(&self.join_schema),
+            left_fut: Arc::clone(&self.left_fut),

Review Comment:
   1. Dynamic filter is the physical expression itself, so we should return it and provide an ability to replace it. 
   2. It seems for me that here is no advantage of the approach when `with_physical_expr` keeps the state. Typically, it will be called either on planning stage (when the state is not important) or between executions of the same plan (where the state must be reset). So let's switch the semantic. 
   



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

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


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