peter-toth opened a new pull request, #8891:
URL: https://github.com/apache/arrow-datafusion/pull/8891
## Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and
enhancements and this helps us generate change logs for our releases. You can
link an issue to this PR using the GitHub syntax. For example `Closes #123`
indicates that this PR will close issue #123.
-->
Closes #.
## Rationale for this change
The `TreeNode::rewrite()` / `TreeNodeRewriter` is very inconsistent with
other `TreeNode` apply / visit functions:
- `TreeNodeRewriter.pre_visit()` currently returns `RewriteRecursion` so as
to control the recursion but that enum is very inconsistent with the
`VisitRecursion` enum that visit functions use.
- `RewriteRecursion::Stop` functionality is equal to
`VisitRecursion::Skip`, that is to skip (prune) the subtree (don't recurse into
children and don't run post-order functions).
- `RewriteRecursion::Skip` doesn't prevent recursing into children. This
is different to `VisitRecursion::Skip` where recursion into children is
executed.
`VisitRecursion::Stop` fully stops recursion which functionality isn't
available with `RewriteRecursion`.
- The transformation defined in `TreeNodeRewriter.mutate()` is sometimes
a top-down (pre-order) but other times a bottom-up (post-order) function
depending on `TreeNodeRewriter.pre_visit()` result.
This PR contains some refactors to `TreeNode::rewrite()` /
`TreeNodeRewriter` to make it more consistent with other `TreeNode` visit /
transform APIs.
1. I'm proposing to remove `RewriteRecursion` and rename `VisitRecursion` to
a common `TreeNodeRecursion` to control all (apply / visit / rewrite) APIs.
```rust
pub enum TreeNodeRecursion {
/// Continue recursion with the next node.
Continue,
/// Skip the current subtree.
Skip,
/// Stop recursion.
Stop,
}
```
and proposing to change the `TreeNodeRewriter` to incorporate an `f_down`
and a n`f_up` methods that both can change the node. Also the `f_down` returns
the common `TreeNodeRecursion` API to how to proceed with recursion:
```rust
pub trait TreeNodeRewriter: Sized {
/// The node type which is rewritable.
type Node: TreeNode;
/// Invoked while traversing down the tree before any children are
rewritten /
/// visited.
/// Default implementation returns the node unmodified and continues
recursion.
fn f_down(&mut self, node: Self::Node) -> Result<(Self::Node,
TreeNodeRecursion)> {
Ok((node, TreeNodeRecursion::Continue))
}
/// Invoked while traversing up the tree after all children have been
rewritten /
/// visited.
/// Default implementation returns the node unmodified.
fn f_up(&mut self, node: Self::Node) -> Result<Self::Node> {
Ok(node)
}
}
```
This solution is capable to replace all `RewriteRecursion`
functionalities and make the `TreeNodeRewriter`s not just cleaner but more
versatile.
2. This PR also proposes to change the `fn transform<F>(self, op: &F) ->
Result<Self>`, that is currently an alias to `transform_up()`, to a more
versatile `fn transform<FD, FU>(self, f_down: &mut FD, f_up: &mut FU) ->
Result<Self>`.
- Sometimes we don't need a full blown `TreeNodeRewriter` but defining 2
closures for top-down and bottom-up transformations is just enough.
- I think that that the missing direction definition of current
`transform()` makes is a bit confusing and explicitely using `transform_down()`
/ `transform_up()` is more robust. So we could use `transform()` for the 2
closure transformation.
## What changes are included in this PR?
This PR:
- Removes `RewriteRecursion`,
- Renames `VisitRecursion` to `TreeNodeRecursion`,
- Modifies `TreeNodeRewriter` to incorporate an `f_down` and `f_up` methods,
- Modifies `TreeNode::transform` to accept an `f_down` and `f_up` closures.
## Are these changes tested?
Existng tests.
## Are there any user-facing changes?
Yes.
--
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]
