Simplify TreeNode recursions #9965
Conversation
github-actions
bot
added
logical-expr
| @@ -355,7 +296,7 @@ pub trait TreeNode: Sized { | |||
| /// Apply the closure `F` to the node's children. | |||
| fn apply_children<F: FnMut(&Self) -> Result<TreeNodeRecursion>>( | |||
| &self, | |||
| f: &mut F, | |||
| f: F, | |||
There was a problem hiding this comment.
This is not related to handling recursions just a small code cleanup.
|
cc @alamb, @berkaysynnada |
There was a problem hiding this comment.
Thank you @peter-toth -- I think this looks great to me 🙏
I have one small suggestion on naming but otherwise this looks good to me
| TreeNodeRecursion::Jump => visitor.f_up(self), | ||
| TreeNodeRecursion::Stop => Ok(TreeNodeRecursion::Stop), | ||
| } | ||
| visitor |
There was a problem hiding this comment.
❤️ -- this is really nice. I would have helped when i was fooling around with the TreeNodeMutator
| @@ -432,6 +373,45 @@ pub enum TreeNodeRecursion { | |||
| Stop, | |||
| } | |||
|
|
|||
| impl TreeNodeRecursion { | |||
There was a problem hiding this comment.
These are much clearer. 👌 very nice
|
|
||
| /// Continues visiting nodes with `f` depending on the current [`TreeNodeRecursion`] | ||
| /// value and the fact that `f` is visiting the current node's parent. | ||
| pub fn visit_parent<F: FnOnce() -> Result<TreeNodeRecursion>>( |
There was a problem hiding this comment.
I was confused at first about the parent term here -- I think it might be clearer to call this function visit_self or visit_node as it is used to visit the node (potentially) after visiting its children, rather than visiting the parent
There was a problem hiding this comment.
I'm ok with visit_self or visit_node. My rationale for the current visit_children / visit_sibling / visit_parent naming was to specify the relative position to the last tnr.
This is a bit more detailed example that I will implement in #9913 for LogicalPlan::visit_with_subqueries and after we fix #9913 (comment) (so the subquery plans are treated like additional children):
// visit the current node (self)
visitor.f_down(self)?
// `visit_children()` because last tnr is from the current node (self) and the continuation
// closure visits the current node (self)'s subqueries (that we treat as children)
.visit_children(|| self.apply_subqueries(|c| c.visit_with_subqueries(visitor)))?
// `visit_sibling()` because last tnr is from visiting the last subquery (that we treated
// as a child) and continuation closure visits the current node (self)'s children, that are
// siblings to the subqueries
.visit_sibling(|| self.apply_children(|s| s.visit_with_subqueries(visitor)))?
// `visit_parent()` because last tnr is from transforming the last child and continuation
// cloure visits the current node (self, that is the parent of the last child)
.visit_parent(|| visitor.f_up(self))
So I'm ok with renaming visit_parent to visit_self or visit_node, but then visit_sibling sounds weird.
Actually, how about visit_down / visit_next / visit_up?
There was a problem hiding this comment.
I think the current naming is more descriptive than visit_down / visit_next / visit_up -- let's go with what is in this PR for now and we can iterate in the future if needed
| /// Maps the [`Transformed`] object to the result of the given `f` depending on the | ||
| /// current [`TreeNodeRecursion`] value and the fact that `f` is changing the current | ||
| /// node's parent. | ||
| pub fn transform_parent<F: FnOnce(T) -> Result<Transformed<T>>>( |
There was a problem hiding this comment.
likewise here - maybe it would make more sense to call this transform_self or transform_node
|
Thanks again @peter-toth |
|
Thanks for the review! |
Which issue does this PR close?
Part of #8913.
Rationale for this change
This PR simplifies
TreeNoderecursion declarations by addingTreeNodeRecursion::visit_children(),TreeNodeRecursion::visit_sibling()andTreeNodeRecursion::visit_parent()helpers to simplify handling of the current
TreeNodeRecursionwith continuation closures by specifying the next node position relative to the current one.Similarly, the transforming recursions are simplified with
Transfomed::visit_children(),Transfomed::visit_sibling()andTransfomed::visit_parent()helpers.
After this PR recursion definition of
TreeNode::visit()is as simple as that:and
TreeNode::rewrite()became:What changes are included in this PR?
This PR:
TransformedIteratortoTreeNodeIteratorand adds a newTreeNodeIterator::apply_until_stop()helper to simplifyTreeNode::apply_children()implementations.Are these changes tested?
Yes, with existint UTs.
Are there any user-facing changes?
No.