wiedld commented on code in PR #10035:
URL:
https://github.com/apache/arrow-datafusion/pull/10035#discussion_r1561419459
##########
datafusion/common/src/tree_node.rs:
##########
@@ -43,17 +42,32 @@ macro_rules! handle_transform_recursion_up {
}};
}
-/// Defines a visitable and rewriteable tree node. This trait is implemented
-/// for plans ([`ExecutionPlan`] and [`LogicalPlan`]) as well as expression
-/// trees ([`PhysicalExpr`], [`Expr`]) in DataFusion.
+/// API for visiting (read only) and rewriting tree data structures .
+///
+/// Note: this trait is implemented for plans ([`ExecutionPlan`] and
+/// [`LogicalPlan`]) as well as expression trees ([`PhysicalExpr`], [`Expr`]).
+///
+/// # Common APIs:
+/// * [`Self::apply`] and [`Self::visit`] for inspecting (without
modification) borrowed nodes
+/// * [`Self::transform`] and [`Self::rewrite`] for rewriting owned nodes
+///
+/// # Terminology
+/// The following terms are used in this trait
+///
+/// * `f_down`: Invoked before any children of the current node are visited.
+/// * `f_up`: Invoked after all children of the current node are visited.
+/// * `f`: closure that is applied to the current node.
+/// * `map_*`: applies a transformation to rewrite owned nodes
+/// * `apply_*`: invokes a function on borrowed nodes
+/// * `transform_`: applies a transformation to rewrite owned nodes
Review Comment:
I took a shot at consolidating the terminology. It's probably wrong, but
hopefully it will help the conversation. 🙈 (edited)
| API | action | traversal | ref | actor |
|------ |--------|-----------|-----|-----|
| apply | traverse, no rewrite | top-down, pre-order | &self | FnMut |
| transform | traverse & rewrite | **suffix dependent** | self | **suffix
dependent** |
| - | - | *_up suffix = bottom-up, post-order | - | - |
| - | - | *_down suffix = top-down, pre-order | - | - |
| - | - | *_up_down suffix = does both, see docs | - | - |
| - | - | - | - | Fn |
| - | - | - | - | *_mut suffix = FnMut |
| visit | visit, no rewrite | depth-first †| &self | dyn TreeNodeVisitor |
| rewrite | visit & rewrite | depth-first †| &self | dyn TreeNodeRewrite |
| apply_children | inspect children, no rewrite | not traversal per se‡ |
&self | Fn
| map_children | inspect children & rewrite **in place** | not traversal per
se‡ | self | FnMut
†The specific implementations of the visitors (TreeNodeVisitor or
TreeNodeRewrite) can impact the traversal patterns.
‡ I think the implementations decide how to handle any travsersal (a.k.a.
does the mapping of children inspect it's own children). But I'm not really
clear on this one.
Caveat (edited): I didn't attempt to generalize the `map_*` terminology (vs
transform), and there is alot I haven't dug into yet.
--
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]
