LiaCastaneda commented on code in PR #24018:
URL: https://github.com/apache/datafusion/pull/24018#discussion_r3721412066
##########
datafusion/datasource/src/file.rs:
##########
@@ -351,6 +352,27 @@ pub trait FileSource: Any + Send + Sync {
fn schema_adapter_factory(&self) -> Option<Arc<dyn SchemaAdapterFactory>> {
None
}
+
+ /// Apply a function to all physical expressions used by this file source.
+ ///
+ /// This includes:
+ /// - Filter predicates (which may contain dynamic filters)
+ /// - Projection expressions
+ ///
+ /// The function `f` is called once for each expression. The function
should
+ /// return `TreeNodeRecursion::Continue` to continue visiting other
expressions,
+ /// or `TreeNodeRecursion::Stop` to stop visiting expressions early.
Review Comment:
this is outdated no? we can use `apply_expression_roots` now
##########
datafusion-examples/examples/relation_planner/table_sample.rs:
##########
@@ -749,6 +749,22 @@ impl ExecutionPlan for SampleExec {
Ok(Arc::new(stats))
}
+
+ fn apply_expressions(
+ &self,
+ f: &mut dyn FnMut(
+ &Arc<dyn datafusion::physical_plan::PhysicalExpr>,
+ ) -> Result<TreeNodeRecursion>,
+ ) -> Result<TreeNodeRecursion> {
+ datafusion::physical_plan::apply_expression_roots(
+ self.cache
+ .output_ordering()
+ .into_iter()
+ .flatten()
+ .map(|sort_expr| &sort_expr.expr),
+ f,
+ )
Review Comment:
yeah I think its fine to leave it as is, I think a helper for this would be
too much
##########
docs/source/library-user-guide/upgrading/54.0.0.md:
##########
@@ -165,6 +165,55 @@ where string types are preferred (`UNION`, `CASE
THEN/ELSE`, `NVL2`).
string-preferring behavior
- Crates that call `get_coerce_type_for_case_expression`
+### `ExecutionPlan::apply_expressions` is now a required method
+
+`apply_expressions` has been added as a **required** method on the
`ExecutionPlan` trait (no default implementation). The same applies to the
`FileSource` and `DataSource` traits. Any custom implementation of these traits
must now implement `apply_expressions`.
+
+**Who is affected:**
+
+- Users who implement custom `ExecutionPlan` nodes
+- Users who implement custom `FileSource` or `DataSource` sources
+
+**Migration guide:**
+
+Add `apply_expressions` to your implementation. Call `f` on each top-level
+`PhysicalExpr` your node owns. Do not recurse into child plans or expression
+children. Use `apply_expression_roots` for nodes with expressions so `Stop`
+short-circuits the iteration and `Jump` proceeds to the next root expression.
+
+**Node with no expressions:**
+
+```rust,ignore
+fn apply_expressions(
+ &self,
+ _f: &mut dyn FnMut(&Arc<dyn PhysicalExpr>) -> Result<TreeNodeRecursion>,
+) -> Result<TreeNodeRecursion> {
+ Ok(TreeNodeRecursion::Continue)
Review Comment:
Since we already have `apply_expression_roots` to abstract "walk these
expressions" case, would it be worth a small sibling helper for the "I have
none" case? Right now that's just `Ok(TreeNodeRecursion::Continue)` repeated at
every leaf/no-op node. Wondering if it would make sense to have something like
`apply_expression_continue()` living next to it, just a plain function.
I think something like `apply_expression_roots(std::iter::empty(), f)` does
the same thing, but since we're trying to abstract away `TreeNodeRecursion`,
wondering if it would still make sense to have the dedicated helper
##########
datafusion/physical-plan/src/async_func.rs:
##########
Review Comment:
+1 on this, looks like something I missed on the initial `apply_expressions`
PR
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
