alamb commented on code in PR #23332:
URL: https://github.com/apache/datafusion/pull/23332#discussion_r3554897693
##########
datafusion/physical-plan/src/execution_plan.rs:
##########
@@ -235,6 +235,27 @@ pub trait ExecutionPlan: Any + Debug + DisplayAs + Send +
Sync {
children: Vec<Arc<dyn ExecutionPlan>>,
) -> Result<Arc<dyn ExecutionPlan>>;
+ /// Fast-path used by [`with_new_children_if_necessary`] when the new
Review Comment:
do we really have to add a new method to the trait? I am thinking of someone
who goes to implement an ExecutionPlan and is bewildered by all the possible
things that need to be implemented
For example, what if we deprecated `with_new_children` and renamed
`with_new_children_and_same_properties` to soemthing like `replace_children`
(e.g. that is clear it is different than with_new_children)
That way, in a few releases from now, ExecutionPlan can be trimmed down
(people will have to implement replace_children and remove their implementation
of with_new_children but at least ExecutionPlan is simpler in the long term
--
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]
