peter-toth commented on PR #8891: URL: https://github.com/apache/arrow-datafusion/pull/8891#issuecomment-1895902893
Thanks for the feedback @berkaysynnada. > `transform()` is a simple method for traversing the graph with given self-concerned rules, being not interested in the type of traversal. This new version forces awareness of the traversal order, and there is also an implicit convention of traversing first bottom-up, then top-down. I feel that a simple (simpler than than `TreeNodeRewriter`) API that is also capable to do both direction transformation in 1 traversal would be useful but I agree, it doesn't necessarily need to replace the existing `transform()`. Also, just to add a bit more details to this part of the PR: While I was local testing this PR I accidentally changed all `transform()` to `transform_down()` (so not to its current aliased `transform_up()` implementation) and run into stack overflow errors in some tests. This suggested me that despite the `transform()` should be used only with closures that don't require any specific direction, some invocation do require `transform` to do its currently defined bottom-up traversal. I'm sure that most of the `transform()` usescases are direction agnostic, but I feel it would be less error prone if we required explicit direction from API users. > and there is also an implicit convention of traversing first bottom-up, then top-down. I'm not sure I get this part. -- 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]
