Simplify TreeNode recursions

[peter-toth]

Which issue does this PR close?

Part of #8913.

Rationale for this change

This PR simplifies TreeNode recursion declarations by adding

• TreeNodeRecursion::visit_children(),
• TreeNodeRecursion::visit_sibling() and
• TreeNodeRecursion::visit_parent()

helpers to simplify handling of the current TreeNodeRecursion with 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() and
• Transfomed::visit_parent()

helpers.

After this PR recursion definition of TreeNode::visit() is as simple as that:

visitor.f_down(self)?
    .visit_children(|| self.apply_children(|c| c.visit(visitor)))?
    .visit_parent(|| visitor.f_up(self))

and TreeNode::rewrite() became:

rewriter.f_down(self)?
    .transform_children(|n| n.map_children(|c| c.rewrite(rewriter)))?
    .transform_parent(|n| rewriter.f_up(n))

What changes are included in this PR?

This PR:

• Adds the above helpers and contains some other minor code cleanup.
• Renames TransformedIterator to TreeNodeIterator and adds a new TreeNodeIterator::apply_until_stop() helper to simplify TreeNode::apply_children() implementations.

Are these changes tested?

Yes, with existint UTs.

Are there any user-facing changes?

No.

[peter-toth]

This is not related to handling recursions just a small code cleanup.

[peter-toth]

cc @alamb, @berkaysynnada

[alamb]

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

[alamb]

❤️ -- this is really nice. I would have helped when i was fooling around with the TreeNodeMutator

[alamb]

These are much clearer. 👌 very nice

[alamb]

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

[peter-toth]

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?

[alamb]

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

[alamb]

likewise here - maybe it would make more sense to call this transform_self or transform_node

[alamb]

Thanks again @peter-toth

[peter-toth]

Thanks for the review!