[GitHub] [arrow] jorgecarleitao commented on a change in pull request #8034: ARROW-9464: [Rust] [DataFusion] Physical plan optimization rule to insert MergeExec when needed

[GitBox <gi...@apache.org>]

jorgecarleitao commented on a change in pull request #8034:
URL: https://github.com/apache/arrow/pull/8034#discussion_r475253287



##########
File path: rust/datafusion/src/execution/physical_plan/mod.rs
##########
@@ -74,6 +85,15 @@ impl Partitioning {
     }
 }
 
+/// Distribution schemes
+#[derive(Debug, Clone)]
+pub enum Distribution {
+    /// Unspecified distribution

Review comment:
       Maybe `/// no distribution is required`?

##########
File path: rust/datafusion/src/execution/physical_plan/mod.rs
##########
@@ -50,6 +50,17 @@ pub trait ExecutionPlan: Debug + Send + Sync {
     fn schema(&self) -> SchemaRef;
     /// Specifies the output partitioning scheme of this plan
     fn output_partitioning(&self) -> Partitioning;
+    /// Specifies the data distribution requirements of all the children for this operator
+    fn required_child_distribution(&self) -> Distribution {
+        Distribution::UnspecifiedDistribution
+    }
+    /// Get the children of this plan

Review comment:
       Suggestion for docs:
   
   ```
   /// `children` of this plan. This corresponds to all plans that this plan depends on. 
   /// This function should return an empty vector for `scans`.
   ```

##########
File path: rust/datafusion/src/execution/physical_plan/mod.rs
##########
@@ -50,6 +50,17 @@ pub trait ExecutionPlan: Debug + Send + Sync {
     fn schema(&self) -> SchemaRef;
     /// Specifies the output partitioning scheme of this plan
     fn output_partitioning(&self) -> Partitioning;
+    /// Specifies the data distribution requirements of all the children for this operator
+    fn required_child_distribution(&self) -> Distribution {
+        Distribution::UnspecifiedDistribution
+    }
+    /// Get the children of this plan
+    fn children(&self) -> Vec<Arc<dyn ExecutionPlan>>;
+    /// Replace the children of this execution plan
+    fn with_new_children(

Review comment:
       ```
   /// Returns a new plan where all children were replaced by new plans. 
   /// The size of `children` must be equal to the size of `ExecutionPlan::children()`.
   ```




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

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