viirya commented on code in PR #8672:
URL: https://github.com/apache/arrow-datafusion/pull/8672#discussion_r1438372207
##########
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:
As I mentioned in above comment, the first try I did is to return
`Vec<&Self>` for `children_nodes`. I think it makes more sense to return
reference of children nodes instead of owned types (although for other
implementations it is smart pointers, but I don't want to assume that the
`TreeNode` impls must link with children nodes with `Arc` etc.).
But there is one trouble when implement `TreeNode ` for `Arc<T>`:
https://github.com/apache/arrow-datafusion/pull/8672#discussion_r1437957200.
I've tried some ideas locally to get rid of it, but in the end they don't work
so I changed `children_nodes` to return `Vec<Self>`. (still thinking if I can
remove it but it may be more change. 🤔 )
This can be seen as an incremental cleanup patch as it doesn't change
current API and keep current behavior but clean up the code as its purpose. In
the long term I'd like to see more fundamental change to `TreeNode` and this
can be seen as a short term effort to make the code easier to look/work on.
--
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]
