Conversation
|
This is now ready for review. cc @alamb, @berkaysynnada |
alamb
left a comment
There was a problem hiding this comment.
Thank you @peter-toth -- I think this looks quite nice and easier to undertand
I left some suggestions on comments / naming that might help, but I also think we could do them as a follow on PR
datafusion/common/src/tree_node.rs
Outdated
| } | ||
|
|
||
| /// Transformation helper to process tree nodes that are siblings. | ||
| /// Transformation helper to process sequence of iterable tree nodes that are siblings. |
There was a problem hiding this comment.
I found the reason / logic for continue / jump quite subtle when trying to make TreeNodeMutator. I have some suggested comments below to explain the rationale that might help
datafusion/common/src/tree_node.rs
Outdated
| /// Transformation helper to process sequence of tree node containing expressions. | ||
| /// This macro is very similar to [TransformedIterator::map_until_stop_and_collect] to | ||
| /// process nodes that are siblings, but it accepts an initial transformation and a | ||
| /// sequence of pairs of an expression and its transformation. |
There was a problem hiding this comment.
This is quite clever. I have some suggestions on naming that would have helped me:
Use F to mirror the nomenclature of TransformedIterator. I thought $TRANSFORMED_EXPR was an actually expr (but it is actually a closure that is invoked I think 🤔 )
$TRANSFORMED_EXPR_0 --> F0
$TRANSFORMED_EXPR --> F.
I think it would help to document each parameter and the return value (specifically it looks like it returns Transformed<(data, ..., data)>?
There was a problem hiding this comment.
Sure, let me fix these and come back yo you.
There was a problem hiding this comment.
Renamed in a36f1aa and added rustdoc to the macro. Let me know if it needs more details.
datafusion/common/src/tree_node.rs
Outdated
| macro_rules! map_until_stop_and_collect { | ||
| ($TRANSFORMED_EXPR_0:expr, $($EXPR:expr, $TRANSFORMED_EXPR:expr),*) => {{ | ||
| $TRANSFORMED_EXPR_0.and_then(|Transformed { data: data0, mut transformed, mut tnr }| { | ||
| let data = ( |
There was a problem hiding this comment.
Maybe we could name this something other than data so it was clearer what type it is (maybe all_datas?)
| null_treatment, | ||
| )) | ||
| }), | ||
| }) => map_until_stop_and_collect!( |
Co-authored-by: Andrew Lamb <[email protected]>
|
The test failure seems unrelated to this PR and looks like a non-deterministic test... |
I agree https://github.com/apache/arrow-datafusion/actions/runs/8543135777/job/23406267036?pr=9876 I restarted it |
|
The CI test passed on rerun. I filed #9930 to track the non deterministic test (and we are trying to get to issue 10,000!) |
|
Thanks @alamb for the review! |
Which issue does this PR close?
Closes #9457, part of #8913
Rationale for this change
The current implementation of
Expr::map_children()is very complex, this PR tries to simplify its code.What changes are included in this PR?
This PR adds a
map_until_stop_and_collect()macro, which is similar toTransformedIterator::map_until_stop_and_collect()to process sibling tree nodes, but it can process heterogeneous tree node containing expressions.Are these changes tested?
Yes, with existing UTs.
Are there any user-facing changes?
No.