[GitHub] [arrow-datafusion] mingmwang commented on a diff in pull request #2168: Implement fast path of with_new_children() in ExecutionPlan

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

mingmwang commented on code in PR #2168:
URL: https://github.com/apache/arrow-datafusion/pull/2168#discussion_r844527086


##########
ballista/rust/core/src/serde/mod.rs:
##########
@@ -508,15 +508,10 @@ mod tests {
             &self,
             children: Vec<Arc<dyn ExecutionPlan>>,
         ) -> datafusion::error::Result<Arc<dyn ExecutionPlan>> {
-            match children.len() {
-                1 => Ok(Arc::new(TopKExec {
-                    input: children[0].clone(),
-                    k: self.k,
-                })),
-                _ => Err(DataFusionError::Internal(
-                    "TopKExec wrong number of children".to_string(),
-                )),
-            }
+            Ok(Arc::new(TopKExec {

Review Comment:
   I removed the length check in the  `with_new_children()` method and move the check to `with_new_children_if_necessary()` so that we can avoid the duplicate logic in all the trait implementations. In future if someone misuse this and still call the `with_new_children()` and pass the wrong children param, it will lose the check, but if this is the case,  the real problem is he should not call `with_new_children()` and everyone should call `with_new_children_if_necessary()`.



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