berkaysynnada commented on code in PR #8891:
URL: https://github.com/apache/arrow-datafusion/pull/8891#discussion_r1502507078
##########
datafusion/optimizer/src/simplify_expressions/or_in_list_simplifier.rs:
##########
@@ -0,0 +1,100 @@
+// Licensed to the Apache Software Foundation (ASF) under one
Review Comment:
Is this file leftover? There is no usage and duplicates
inlist_simplifier.rs. I think you have forgotten to remove it.
##########
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:
```suggestion
match visitor.f_down(self)? {
TreeNodeRecursion::Continue => {
handle_visit_recursion_up!(
self.apply_children(&mut |n| n.visit(visitor))?
);
visitor.f_up(self)
}
TreeNodeRecursion::Jump => visitor.f_up(self),
TreeNodeRecursion::Stop => Ok(TreeNodeRecursion::Stop),
}
```
I have tried to change here like this to add "f_up(a)" after "f_down(a)"
in the "f_down_jump_on_a_visits". "f_up(a)" is added but some other tests fail.
It happens because as you have also said that previous skip behavior was in
that way. What do you think that the failed test rules can be converted easily
to work in this way? I know there is no exact truth and I am not sure which is
more intuitive. However, I don't see any problem in keeping it like this.
--
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]
