peter-toth commented on code in PR #8672:
URL: https://github.com/apache/arrow-datafusion/pull/8672#discussion_r1438401600
##########
datafusion/expr/src/tree_node/expr.rs:
##########
@@ -24,16 +24,13 @@ use crate::expr::{
};
use crate::{Expr, GetFieldAccess};
-use datafusion_common::tree_node::{TreeNode, VisitRecursion};
+use datafusion_common::tree_node::TreeNode;
use datafusion_common::{internal_err, DataFusionError, Result};
impl TreeNode for Expr {
- fn apply_children<F>(&self, op: &mut F) -> Result<VisitRecursion>
- where
- F: FnMut(&Self) -> Result<VisitRecursion>,
- {
- let children = match self {
- Expr::Alias(Alias{expr,..})
+ fn children_nodes(&self) -> Vec<Self> {
Review Comment:
> Sometimes there may be cases where one needs to do multiple passes over
the same tree (e.g. doing one top down and one bottom up pass consecutively).
Or we may want to run different logic on the same tree.
This is also supported with `transform_with_payload()`
(https://github.com/apache/arrow-datafusion/pull/8664) that does a top-down and
a bottom-up transforms too and capable to propagate different payloads in the 2
directions.
> So it may be a little premature to conclude we can do away with "derived"
trees and just keep the base four.
I'm happy to try refactoring the remaining 4 (`OrderPreservationContext`,
`PipelineStatePropagator`, `PlanWithCorrespondingCoalescePartitions` and
`PlanWithCorrespondingSort`) in scope of
https://github.com/apache/arrow-datafusion/pull/8664 next week and check the
above statement.
(I thought a smaller PR that shows 1-2 examples for both up and down
directions would be easier to review, but probably we can do it in 1 big PR if
that's prefered.)
--
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]
