[GitHub] [arrow-datafusion] alamb commented on a change in pull request #1624: Optimize `SortPreservingMergeStream` to avoid `SortKeyCursor` sharing

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

alamb commented on a change in pull request #1624:
URL: https://github.com/apache/arrow-datafusion/pull/1624#discussion_r789801367



##########
File path: datafusion/src/physical_plan/sorts/sort_preserving_merge.rs
##########
@@ -280,7 +287,11 @@ pub(crate) struct SortPreservingMergeStream {
     ///

Review comment:
       it looks like the comments here need to be updated (they still refer to `SortKeyCursor`)

##########
File path: datafusion/src/physical_plan/sorts/sort_preserving_merge.rs
##########
@@ -280,7 +287,11 @@ pub(crate) struct SortPreservingMergeStream {
     ///
     /// Exhausted cursors will be popped off the front once all
     /// their rows have been yielded to the output
-    cursors: Vec<VecDeque<Arc<SortKeyCursor>>>,
+    batches: Vec<VecDeque<Arc<RecordBatch>>>,

Review comment:
       Mechanically the changes in this PR make sense to me, but I don't quite follow how the elements in `batches` correspond to the cursors in `min_heap` (especially the code that drains all by the last `RecordBatch` [below](https://github.com/apache/arrow-datafusion/pull/1624/files#diff-0cd58001ad86fb06e8667fdac3d89409b4624fb2a21c882484beeaf00c9f5df7R528-R531). 
   
   




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