jayshrivastava commented on code in PR #24018:
URL: https://github.com/apache/datafusion/pull/24018#discussion_r3729421662
##########
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
Review Comment:
Right, moved.
##########
datafusion/physical-plan/src/async_func.rs:
##########
Review Comment:
I missed this. Just added it
##########
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:
`apply_expression_roots(std::iter::empty(), f)` is nice, but you have to use
type annotations in practice:
```
apply_expression_roots(
std::iter::empty::<&Arc<dyn PhysicalExpr>>(),
f,
)
```
This is a bit less clean so
I added a helper `apply_no_expressions` instead.
##########
datafusion/physical-plan/src/streaming.rs:
##########
Review Comment:
I missed this. Just added it.
##########
datafusion/physical-plan/src/repartition/mod.rs:
##########
@@ -1337,6 +1338,20 @@ impl ExecutionPlan for RepartitionExec {
vec![&self.input]
}
+ fn apply_expressions(
+ &self,
+ f: &mut dyn FnMut(&Arc<dyn PhysicalExpr>) -> Result<TreeNodeRecursion>,
+ ) -> Result<TreeNodeRecursion> {
+ match self.partitioning() {
+ Partitioning::Hash(exprs, _) =>
crate::apply_expression_roots(exprs, f),
Review Comment:
I think I just missed the expressions in `StreamingTableExec`. Will update
it.
##########
datafusion/physical-plan/src/aggregates/mod.rs:
##########
@@ -763,6 +764,32 @@ struct AggrDynFilter {
supported_accumulators_info: Vec<PerAccumulatorDynFilter>,
}
+fn plan_contains_expression_id(
+ plan: &Arc<dyn ExecutionPlan>,
+ expression_id: u64,
+) -> Result<bool> {
+ let mut found = false;
+ plan.apply(|node| {
+ node.apply_expressions(&mut |root| {
+ root.apply(|expr| {
Review Comment:
I think this pattern is expected. I followed these docs for
`apply_expressions` on logical plans to implement this for physical plans:
https://github.com/apache/datafusion/blob/5514194fa7362e7fbc2e3f891779e5df90f57322/datafusion/expr/src/logical_plan/plan.rs?plain=1#L135-L144.
##########
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:
Fixed!
--
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]
