alamb commented on code in PR #10126:
URL:
https://github.com/apache/arrow-datafusion/pull/10126#discussion_r1571443453
##########
datafusion/expr/src/logical_plan/tree_node.rs:
##########
@@ -797,53 +797,31 @@ impl LogicalPlan {
/// Similarly to [`Self::transform`], rewrites this node and its inputs
using `f`,
/// including subqueries that may appear in expressions such as `IN (SELECT
/// ...)`.
- pub fn transform_with_subqueries<F: Fn(Self) -> Result<Transformed<Self>>>(
+ pub fn transform_with_subqueries<F: FnMut(Self) ->
Result<Transformed<Self>>>(
self,
- f: &F,
+ f: &mut F,
) -> Result<Transformed<Self>> {
self.transform_up_with_subqueries(f)
}
/// Similarly to [`Self::transform_down`], rewrites this node and its
inputs using `f`,
/// including subqueries that may appear in expressions such as `IN (SELECT
/// ...)`.
- pub fn transform_down_with_subqueries<F: Fn(Self) ->
Result<Transformed<Self>>>(
- self,
- f: &F,
- ) -> Result<Transformed<Self>> {
- handle_transform_recursion_down!(f(self), |c|
c.transform_down_with_subqueries(f))
- }
-
- /// Similarly to [`Self::transform_down_mut`], rewrites this node and its
inputs using `f`,
- /// including subqueries that may appear in expressions such as `IN (SELECT
- /// ...)`.
- pub fn transform_down_mut_with_subqueries<
- F: FnMut(Self) -> Result<Transformed<Self>>,
- >(
+ pub fn transform_down_with_subqueries<F: FnMut(Self) ->
Result<Transformed<Self>>>(
self,
f: &mut F,
) -> Result<Transformed<Self>> {
- handle_transform_recursion_down!(f(self), |c| c
- .transform_down_mut_with_subqueries(f))
+ handle_transform_recursion_down!(f(self), |c|
c.transform_down_with_subqueries(f))
}
/// Similarly to [`Self::transform_up`], rewrites this node and its inputs
using `f`,
/// including subqueries that may appear in expressions such as `IN (SELECT
/// ...)`.
- pub fn transform_up_with_subqueries<F: Fn(Self) ->
Result<Transformed<Self>>>(
Review Comment:
❤️
##########
datafusion/core/src/physical_optimizer/convert_first_last.rs:
##########
@@ -60,7 +60,7 @@ impl PhysicalOptimizerRule for OptimizeAggregateOrder {
plan: Arc<dyn ExecutionPlan>,
_config: &ConfigOptions,
) -> Result<Arc<dyn ExecutionPlan>> {
- plan.transform_up(&get_common_requirement_of_aggregate_input)
+ plan.transform_up(&mut get_common_requirement_of_aggregate_input)
Review Comment:
This is just so weird -- I think most users would expect that this looks
like this (though as I mentioned above, I couldn't make it work in a few
minutes)
```diff
- plan.transform_up(&mut get_common_requirement_of_aggregate_input)
+ plan.transform_up(get_common_requirement_of_aggregate_input)
```
##########
datafusion/common/src/tree_node.rs:
##########
@@ -145,40 +145,41 @@ pub trait TreeNode: Sized {
/// Convenience utility for writing optimizer rules: Recursively apply the
/// given function `f` to the tree in a bottom-up (post-order) fashion.
When
/// `f` does not apply to a given node, it is left unchanged.
- fn transform<F: Fn(Self) -> Result<Transformed<Self>>>(
+ fn transform<F: FnMut(Self) -> Result<Transformed<Self>>>(
self,
- f: &F,
+ f: &mut F,
Review Comment:
I tried a bit locally to change this to be `f: F` (aka take an "owned" F) --
which would make it easier to call this API also works -- so the callsites
don't need to be changed to be `&mut` all over the place
However, I tried it and I got some strange errors when instantiating macros
🤔
```
error: reached the recursion limit while instantiating `<expr::Expr as
TreeNode>::transform_up::<&mut &mut &mut &mut ...>`
-->
/Users/andrewlamb/Software/arrow-datafusion/datafusion/common/src/tree_node.rs:184:50
|
184 | handle_transform_recursion_up!(self, |c| c.transform_up(&mut
f), f)
| ^^^^^^^^^^^^^^^^^^^^^^
|
note: `transform_up` defined here
-->
/Users/andrewlamb/Software/arrow-datafusion/datafusion/common/src/tree_node.rs:180:5
|
180 | / fn transform_up<F: FnMut(Self) -> Result<Transformed<Self>>>(
181 | | self,
182 | | mut f: F,
183 | | ) -> Result<Transformed<Self>> {
| |__________________________________^
= note: the full type name has been written to
'/Users/andrewlamb/Software/arrow-datafusion/target/debug/deps/datafusion_expr-598aae957f8f2540.long-type.txt'
```
--
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]
