You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "peter-toth (via GitHub)" <gi...@apache.org> on 2024/04/18 09:57:40 UTC

[PR] Deprecate `TreeNode::transform_down_mut()` and `TreeNode::transform_up_mut()` methods [arrow-datafusion]

peter-toth opened a new pull request, #10126:
URL: https://github.com/apache/arrow-datafusion/pull/10126

   ## Which issue does this PR close?
   
   Closes https://github.com/apache/arrow-datafusion/issues/10097, part of: https://github.com/apache/arrow-datafusion/issues/10121.
   
   ## Rationale for this change
   
   Currently the only difference between `TreeNode::transform_down()`  and `TreeNode::transform_down_mut()` is that `transform_down()` accepts an `Fn`, while `transform_down_mut()` accepts an `FnMut` closure.
   As `Fn` is subtrait of `FnMut`, it is possible to use `transform_down_mut()` instead of `transform_down()` everywhere. But because `transform_down()` has a better name, let's fix `transform_down()` and get rid of `transform_down_mut()`. 
   The same applies to `TreeNode::transform_up()`  vs. `TreeNode::transform_up_mut()` as well.
   
   ## What changes are included in this PR?
   
   This PR:
   - Modifies `transform_down()` and `transform_up()` signatures to use `FnMut`.
   - Changes all `transform_down_mut()` calls to `transform_down()` and `transform_up_mut()` calls to `transform_up()`.
   - Deprecates `transform_down_mut()` and `transform_up_mut()`.
   - Removes `LogicalPlan::transform_down_mut_with_subqueries()` and `LogicalPlan::transform_up_mut_with_subqueries()` to keep `LogicalPlan::..._with_subqueries()` APIs in sync with `TreeNode` APIs. (Please note that  `transform_down_mut_with_subqueries()` and `transform_up_mut_with_subqueries()` haven't been released yet so we can freely remove them.)
   
   ## Are these changes tested?
   
   Yes, with existing UTs.
   
   ## Are there any user-facing changes?
   
   No.
   


-- 
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: [PR] Deprecate `TreeNode::transform_down_mut()` and `TreeNode::transform_up_mut()` methods [arrow-datafusion]

Posted by "peter-toth (via GitHub)" <gi...@apache.org>.
peter-toth commented on PR #10126:
URL: https://github.com/apache/arrow-datafusion/pull/10126#issuecomment-2066260765

   > Thanks, @peter-toth—this really simplifies things. I'm wondering if there's an option to avoid changing the transform's `Fn` to `FnMut` (thus not requiring `&mut` for closures) by using the rewrite API for passes that need mutability inevitably. AFAIK rewrite can handle that for the `transform_up` and `transform_down` cases.
   
   My concern is that it would require to refactor all current `transform_down_mut()` and `transform_up_mut()` usecases to use the `rewrite()` API, which is a bigger change required from the API users than just change to `transform_down()` (or `transform_up()`) and `&` to `&mut`.
   
   


-- 
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: [PR] Deprecate `TreeNode::transform_down_mut()` and `TreeNode::transform_up_mut()` methods [arrow-datafusion]

Posted by "peter-toth (via GitHub)" <gi...@apache.org>.
peter-toth commented on PR #10126:
URL: https://github.com/apache/arrow-datafusion/pull/10126#issuecomment-2063781528

   @alamb this PR might conflict with https://github.com/apache/arrow-datafusion/pull/10035, but I can update mine if yours gets merged first.


-- 
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: [PR] Deprecate `TreeNode::transform_down_mut()` and `TreeNode::transform_up_mut()` methods [arrow-datafusion]

Posted by "peter-toth (via GitHub)" <gi...@apache.org>.
peter-toth commented on code in PR #10126:
URL: https://github.com/apache/arrow-datafusion/pull/10126#discussion_r1572152024


##########
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:
   If we like this change I can update the PR description.



-- 
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: [PR] Improve `TreeNode` and `LogicalPlan` APIs to accept owned closures [arrow-datafusion]

Posted by "peter-toth (via GitHub)" <gi...@apache.org>.
peter-toth commented on PR #10126:
URL: https://github.com/apache/arrow-datafusion/pull/10126#issuecomment-2066477158

   I've updated the PR title and description to reflect the latest changes.


-- 
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: [PR] Deprecate `TreeNode::transform_down_mut()` and `TreeNode::transform_up_mut()` methods [arrow-datafusion]

Posted by "peter-toth (via GitHub)" <gi...@apache.org>.
peter-toth commented on code in PR #10126:
URL: https://github.com/apache/arrow-datafusion/pull/10126#discussion_r1572031151


##########
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:
   Yes, I did try this already and run into the same issue...
   But now I think it is doable with some smart refactor. Let me update this PR today or tomorrow and then we can decide if this change make sense or not



-- 
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: [PR] Improve `TreeNode` and `LogicalPlan` APIs to accept owned closures, deprecate `transform_down_mut()` and `transform_up_mut()` [datafusion]

Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #10126:
URL: https://github.com/apache/datafusion/pull/10126#issuecomment-2070731173

   Thanks again @peter-toth  -- epic work


-- 
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: commits-unsubscribe@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datafusion.apache.org
For additional commands, e-mail: commits-help@datafusion.apache.org

Re: [PR] Improve `TreeNode` and `LogicalPlan` APIs to accept owned closures, deprecate `transform_down_mut()` and `transform_up_mut()` [datafusion]

Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #10126:
URL: https://github.com/apache/datafusion/pull/10126


-- 
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: commits-unsubscribe@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datafusion.apache.org
For additional commands, e-mail: commits-help@datafusion.apache.org

Re: [PR] Deprecate `TreeNode::transform_down_mut()` and `TreeNode::transform_up_mut()` methods [arrow-datafusion]

Posted by "peter-toth (via GitHub)" <gi...@apache.org>.
peter-toth commented on code in PR #10126:
URL: https://github.com/apache/arrow-datafusion/pull/10126#discussion_r1572129964


##########
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:
   https://github.com/apache/arrow-datafusion/pull/10126/commits/d781ca714fe1ec6be25aa20f6bb15fdfc1419641 changes `TreeNode::apply()`, `TreeNode::transform()`, `TreeNode::transform_down()`, `TreeNode::transform_up()`, `TreeNode::transform_down_up()` and their `LogicalPlan::...with_subqueries()` variants to use owned closures.



-- 
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: [PR] Deprecate `TreeNode::transform_down_mut()` and `TreeNode::transform_up_mut()` methods [arrow-datafusion]

Posted by "berkaysynnada (via GitHub)" <gi...@apache.org>.
berkaysynnada commented on PR #10126:
URL: https://github.com/apache/arrow-datafusion/pull/10126#issuecomment-2065884864

   Thanks, @peter-toth—this really simplifies things. I'm wondering if there's an option to avoid changing the transform's `Fn` to `FnMut` (thus not requiring `&mut` for closures) by using the rewrite API for passes that need mutability inevitably. AFAIK rewrite can handle that for the `transform_up` and `transform_down` cases.


-- 
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: [PR] Deprecate `TreeNode::transform_down_mut()` and `TreeNode::transform_up_mut()` methods [arrow-datafusion]

Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #10126:
URL: https://github.com/apache/arrow-datafusion/pull/10126#discussion_r1572283379


##########
datafusion-examples/examples/function_factory.rs:
##########
@@ -164,7 +164,7 @@ impl ScalarUDFImpl for ScalarFunctionWrapper {
 impl ScalarFunctionWrapper {
     // replaces placeholders such as $1 with actual arguments (args[0]
     fn replacement(expr: &Expr, args: &[Expr]) -> Result<Expr> {
-        let result = expr.clone().transform(&|e| {
+        let result = expr.clone().transform(|e| {

Review Comment:
   I actually think this is a really nice improvement in usability -- to not have to put a `&` in front of the closure is 💯 



-- 
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: [PR] Improve `TreeNode` and `LogicalPlan` APIs to accept owned closures, deprecate `transform_down_mut()` and `transform_up_mut()` [datafusion]

Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #10126:
URL: https://github.com/apache/datafusion/pull/10126#issuecomment-2068006228

   I plan to merge this PR on Monday unless anyone else would like additional time to review or has additional comments


-- 
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: commits-unsubscribe@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datafusion.apache.org
For additional commands, e-mail: commits-help@datafusion.apache.org

Re: [PR] Deprecate `TreeNode::transform_down_mut()` and `TreeNode::transform_up_mut()` methods [arrow-datafusion]

Posted by "alamb (via GitHub)" <gi...@apache.org>.
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org