[GitHub] [arrow-datafusion] Jimexist commented on a change in pull request #515: refactor sort exec stream and combine batches

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

Jimexist commented on a change in pull request #515:
URL: https://github.com/apache/arrow-datafusion/pull/515#discussion_r646132592



##########
File path: datafusion/src/physical_plan/sort.rs
##########
@@ -277,9 +250,14 @@ impl SortStream {
                 .map_err(DataFusionError::into_arrow_external_error)
                 .and_then(move |batches| {
                     let now = Instant::now();
-                    let result = sort_batches(&batches, &schema, &expr);
+                    // combine all record batches into one for each column
+                    let combined = common::combine_batches(&batches, schema.clone())?;
+                    // sort combined record batch
+                    let result = combined
+                        .map(|batch| sort_batch(batch, schema, &expr))
+                        .transpose()?;
                     sort_time.add(now.elapsed().as_nanos() as usize);

Review comment:
       Note here the semantic is changed because on err it would no longer log a metric which shall be the correct behavior




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