adriangb commented on code in PR #20009:
URL: https://github.com/apache/datafusion/pull/20009#discussion_r2769449720
##########
datafusion/physical-plan/src/execution_plan.rs:
##########
@@ -722,6 +722,51 @@ pub trait ExecutionPlan: Debug + DisplayAs + Send + Sync {
) -> Option<Arc<dyn ExecutionPlan>> {
None
}
+
+ /// Returns an iterator over a subset of [`PhysicalExpr`]s that are used
by this
+ /// [`ExecutionPlan`].
+ ///
+ /// This method provides a way to inspect the physical expressions within
a node without
+ /// knowing its specific type. These expressions could be predicates in a
filter, projection
+ /// expressions, or join conditions, etc.
+ ///
+ /// The default implementation returns `None`, indicating that the node
either has no physical
+ /// expressions or does not support exposing them.
+ fn physical_expressions<'a>(
+ &'a self,
+ ) -> Option<Box<dyn Iterator<Item = Arc<dyn PhysicalExpr>> + 'a>> {
+ None
+ }
+
+ /// Returns a new `ExecutionPlan` with its physical expressions replaced
by the provided
+ /// `exprs`.
+ ///
+ /// This method is the counterpart to
[`ExecutionPlan::physical_expressions`]. It allows
+ /// transforming the expressions within a node while preserving the node
itself and its
+ /// children.
+ ///
+ /// By default, the behavior of this method is similar to
[`ExecutionPlan::reset_state`] in the
+ /// sense that it also resets the internal state of [`ExecutionPlan`].
+ ///
+ /// # Constraints
+ ///
+ /// * The number of expressions in `params.exprs` must match the number of
expressions returned
+ /// by [`ExecutionPlan::physical_expressions`].
+ /// * The order of expressions in `params.exprs` must match the order they
were yielded by the
+ /// iterator from [`ExecutionPlan::physical_expressions`].
+ ///
+ /// # Returns
+ ///
+ /// * `Ok(Some(new_plan))` if the expressions were successfully replaced.
+ /// * `Ok(None)` if the node does not support replacing its expressions
(the default).
+ /// * `Err` if the number of expressions is incorrect or if another error
occurs during
+ /// replacement.
+ fn with_physical_expressions(
+ &self,
+ _params: ReplacePhysicalExpr,
+ ) -> Result<Option<Arc<dyn ExecutionPlan>>> {
+ Ok(None)
+ }
Review Comment:
I agree we need to be sure this is the right API before moving forward with
it
> how would you know to which of all those components above each expression
belongs?
What would the use cases be that need to differentiate this? Would they be
aggregate specific -> could they downcast the plan and call functions on
`AggregateExec` directly?
If something needs to (1) operate on all expressions and (2) have specific
logic depending on where that expression is that seems like a hard API to get
right.
--
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]
