gabotechs commented on code in PR #20009:
URL: https://github.com/apache/datafusion/pull/20009#discussion_r2769387143
##########
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'm not sure if this API is enough to be used in all the current
`ExecutionPlan` implementations.
Take `AggregateExec` for example:
https://github.com/apache/datafusion/blob/b80bf2ca8ef74900fee96a1cc169bdedf53b36fc/datafusion/physical-plan/src/aggregates/mod.rs#L625-L625
It has:
- a Vec of `AggregateFunctionExpr` where each entry contains `args:
Vec<Arc<dyn PhysicalExpr>>`
- the same Vec of `AggregateFunctionExpr` also contains
`Vec<PhysicalSortExpr>` (with one expression each)
- a `filter_expr: Vec<Option<Arc<dyn PhysicalExpr>>>`
- a `dynamic_filter`
If `AggregateExecb::physical_expressions` returned something like
`vec![expr1, expr2, expr3, expr4]`, how would you know to which of all those
components above each expression belongs? are the 4 expressions
`AggregateFunctionExpr`s? are the first two `AggregateFunctionExpr`s, the third
one a `filter_expr` and the last one a dynamic filter?
--
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]
