peter-toth commented on code in PR #8891:
URL: https://github.com/apache/arrow-datafusion/pull/8891#discussion_r1502934276
##########
datafusion/common/src/tree_node.rs:
##########
@@ -88,132 +177,138 @@ pub trait TreeNode: Sized {
///
/// If an Err result is returned, recursion is stopped immediately
///
- /// If [`VisitRecursion::Stop`] is returned on a call to pre_visit, no
+ /// If [`TreeNodeRecursion::Stop`] is returned on a call to pre_visit, no
/// children of that node will be visited, nor is post_visit
/// called on that node. Details see [`TreeNodeVisitor`]
///
- /// If using the default [`TreeNodeVisitor::post_visit`] that does
+ /// If using the default [`TreeNodeVisitor::f_up`] that does
/// nothing, [`Self::apply`] should be preferred.
- fn visit<V: TreeNodeVisitor<N = Self>>(
+ fn visit<V: TreeNodeVisitor<Node = Self>>(
&self,
visitor: &mut V,
- ) -> Result<VisitRecursion> {
- handle_tree_recursion!(visitor.pre_visit(self)?);
- handle_tree_recursion!(self.apply_children(&mut |node|
node.visit(visitor))?);
- visitor.post_visit(self)
+ ) -> Result<TreeNodeRecursion> {
+ handle_visit_recursion_down!(visitor.f_down(self)?);
+ handle_visit_recursion_up!(self.apply_children(&mut |n|
n.visit(visitor))?);
+ visitor.f_up(self)
Review Comment:
This is the
https://github.com/apache/arrow-datafusion/pull/8891#discussion_r1490932349
topic, that whether we should to call `f_up(n)` after `f_down(n)` if `f_down`
returns `Jump`.
If you changed only the `TreeNodeVisitor` logic in your experiment then it
`ExprIdentifierVisitor` must be the root cause of the failures as that's the
only one returning `TreeNodeRecursion::Jump` in its `f_down`.
I think we could fix those failures if we duplicated the
https://github.com/apache/arrow-datafusion/blob/ec86acbc1fbc0da1e0bec9ad066a5177ec586c96/datafusion/optimizer/src/common_subexpr_eliminate.rs#L669-L671
from its `f_down` to its `f_up` and return the original node unmodified if the
check is true.
But I feel this just confirms my ponint in
https://github.com/apache/arrow-datafusion/pull/8891#discussion_r1490932349
that we can do anything in `f_down` with a node and if we prune the node's
subtree (return `Jump`) then it makes no sense to call `f_up` on it.
--
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]
