You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "berkaysynnada (via GitHub)" <gi...@apache.org> on 2024/03/05 07:33:32 UTC
[I] TreeNode Expr Implementation for LogicalPlan's [arrow-datafusion]
berkaysynnada opened a new issue, #9457:
URL: https://github.com/apache/arrow-datafusion/issues/9457
### Is your feature request related to a problem or challenge?
The recent update in #8891 has made significant improvements, making many `TreeNode` implementations simpler and more straightforward. However, the [map_children](https://github.com/apache/arrow-datafusion/blob/ac27428788cec96752a41eccb620956f7184047d/datafusion/expr/src/tree_node/expr.rs#L143) method in `Expr` enum appears to be quite complex. I think there's room for improvement in making this design more user-friendly, which would not only enhance user comprehension, but also help debugging.
Take, for instance, the `Expr::Case` pattern:
```
Expr::Case(Case {
expr,
when_then_expr,
else_expr,
}) => transform_option_box(expr, &mut f)?
.update_data(|new_expr| (new_expr, when_then_expr, else_expr))
.try_transform_node(|(new_expr, when_then_expr, else_expr)| {
Ok(when_then_expr
.into_iter()
.map_until_stop_and_collect(|(when, then)| {
transform_box(when, &mut f)?
.update_data(|new_when| (new_when, then))
.try_transform_node(|(new_when, then)| {
Ok(transform_box(then, &mut f)?
.update_data(|new_then| (new_when, new_then)))
})
})?
.update_data(|new_when_then_expr| {
(new_expr, new_when_then_expr, else_expr)
}))
})?
.try_transform_node(|(new_expr, new_when_then_expr, else_expr)| {
Ok(transform_option_box(else_expr, &mut f)?.update_data(
|new_else_expr| (new_expr, new_when_then_expr, new_else_expr),
))
})?
.update_data(|(new_expr, new_when_then_expr, new_else_expr)| {
Expr::Case(Case::new(new_expr, new_when_then_expr, new_else_expr))
}),
```
As shown, the function employs multiple nested and chained utility functions, which can be quite daunting. Simplifying these would significantly enhance readability and understanding.
### Describe the solution you'd like
New implementations of `TreeNode` for `Option` and `Box` may be implemented, or new simplifying utils may be written with a more understandable style.
### Describe alternatives you've considered
_No response_
### Additional context
_No response_
--
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: github-unsubscribe@arrow.apache.org.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] TreeNode Expr Implementation for LogicalPlan's [arrow-datafusion]
Posted by "berkaysynnada (via GitHub)" <gi...@apache.org>.
berkaysynnada commented on issue #9457:
URL: https://github.com/apache/arrow-datafusion/issues/9457#issuecomment-1978124348
@peter-toth I'm curious about your thoughts and solution suggestions on this issue.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] TreeNode Expr Implementation for LogicalPlan's [arrow-datafusion]
Posted by "peter-toth (via GitHub)" <gi...@apache.org>.
peter-toth commented on issue #9457:
URL: https://github.com/apache/arrow-datafusion/issues/9457#issuecomment-1978364210
I was thinking of a macro that accepts a sequence of expressions and their transformations:
```
let t = transform_siblings!(
expr, |e| transform_option_box(e, &mut f),
when_then_expr, |wte| wte.into_iter().map_until_stop_and_collect(|(when, then)| {
transform_siblings!(
when, |w| transform_box(w, &mut f),
then, |t| transform_box(t, &mut f),
),
}),
else_expr, |ee| transform_option_box(ee, &mut f)
)
t.update_data(|(new_expr, new_when_then_expr, new_else_expr)| {
Expr::Case(Case::new(new_expr, new_when_then_expr, new_else_expr))
})
```
Inside the macro we can build up the same `update_data` + `try_transform_node ` chain or maybe avoid that to make it easier to debug.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] TreeNode Expr Implementation for LogicalPlan's [arrow-datafusion]
Posted by "peter-toth (via GitHub)" <gi...@apache.org>.
peter-toth commented on issue #9457:
URL: https://github.com/apache/arrow-datafusion/issues/9457#issuecomment-1978161952
Thanks for pinging me @berkaysynnada.
I was thinking about using macros instead of chaining `Transfomed`s (`update_data()` + `try_transform_node()`). In a macro we still need to handle `tnr == Stop` from the result of the previous transformation, but we don't need to build those tuples (`(new_expr, when_then_expr, else_expr)`) at least.
But this week is a bit hectic for me so if you have any other idea or want to experiment with the above feel free to do it and ping me for review.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] TreeNode Expr Implementation for LogicalPlan's [arrow-datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb closed issue #9457: TreeNode Expr Implementation for LogicalPlan's
URL: https://github.com/apache/arrow-datafusion/issues/9457
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org