[GitHub] [arrow-ballista] tustvold commented on a diff in pull request #687: Add ShuffleWriter trait

["tustvold (via GitHub)" <gi...@apache.org>]

tustvold commented on code in PR #687:
URL: https://github.com/apache/arrow-ballista/pull/687#discussion_r1118605224


##########
ballista/core/src/execution_plans/shuffle_writer.rs:
##########
@@ -139,7 +154,7 @@ impl ShuffleWriterExec {
         self.shuffle_output_partitioning.as_ref()
     }
 
-    pub fn execute_shuffle_write(
+    fn execute_shuffle_write_internal(

Review Comment:
   So there are two changes here
   
   * [`async_trait`] will box the future, so polling the future now goes through a dyn dispatch. This probably doesn't matter given this method is likely performing non-trivial IO
   * The returned future borrows self, i.e. can't outlive the borrow of self. This could matter, but given the pervasive use of `Arc` and `async move` it is probably easy enough to work around



-- 
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