alamb commented on code in PR #14650:
URL: https://github.com/apache/datafusion/pull/14650#discussion_r1956785686
##########
datafusion/physical-optimizer/src/enforce_sorting/replace_with_order_preserving_variants.rs:
##########
@@ -45,7 +45,7 @@ use itertools::izip;
pub type OrderPreservationContext = PlanContext<bool>;
/// Updates order-preservation data for all children of the given node.
-pub fn update_children(opc: &mut OrderPreservationContext) {
+pub fn update_children_data(opc: &mut OrderPreservationContext) {
Review Comment:
I found the methods names hard to follow -- for example
`update_children_data` actually has a `OrderPreservationContext` 🤔
##########
datafusion/physical-plan/src/tree_node.rs:
##########
@@ -42,6 +42,8 @@ impl DynTreeNode for dyn ExecutionPlan {
/// A node object beneficial for writing optimizer rules, encapsulating an
[`ExecutionPlan`] node with a payload.
/// Since there are two ways to access child plans—directly from the plan and
through child nodes—it's recommended
/// to perform mutable operations via [`Self::update_plan_from_children`].
+/// After update `children`, please do the sync updating for `plan`'s children.
Review Comment:
I wonder if we could find some way to avoid having to remember to do this 🤔
If we have to remember to call a function it seems likely either our future
selves or others will forget to do so
##########
datafusion/physical-plan/src/tree_node.rs:
##########
@@ -42,6 +42,8 @@ impl DynTreeNode for dyn ExecutionPlan {
/// A node object beneficial for writing optimizer rules, encapsulating an
[`ExecutionPlan`] node with a payload.
/// Since there are two ways to access child plans—directly from the plan and
through child nodes—it's recommended
/// to perform mutable operations via [`Self::update_plan_from_children`].
+/// After update `children`, please do the sync updating for `plan`'s children.
Review Comment:
Maybe one way would be to make these fields not `pub` and then control access
so like add a method so the only way to update children woudl be
```rust
impl PlanContext {
fn update_children(&mut self, new_children: Vec<Self>) -> {
self.children = new_children;
self.update_child_data()
}
```
🤔
To be clear I am talking about some future 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: github-unsubscr...@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org
