wiedld commented on code in PR #7842:
URL: https://github.com/apache/arrow-datafusion/pull/7842#discussion_r1368880722
##########
datafusion/physical-plan/src/sorts/builder.rs:
##########
@@ -42,15 +37,15 @@ pub struct BatchBuilder {
reservation: MemoryReservation,
/// The current [`BatchCursor`] for each stream
- cursors: Vec<BatchCursor>,
+ cursors: Vec<Option<BatchCursor<C>>>,
Review Comment:
All of the cursor management/ownership is moved into the SortOrderBuilder,
and removed from the SortPreservingMergeStream. No second copy. Instead the
division of concerns is:
* SortPreservingMergeStream == only care about merging (via loser tree)
* SortOrderBuilder == holds the cursor (being advanced), and the sort_order
(being built).
This clearly demonstrates why I need to add more docs to the BatchCursor. 😅
Doing so.
##########
datafusion/physical-plan/src/sorts/builder.rs:
##########
@@ -42,15 +37,15 @@ pub struct BatchBuilder {
reservation: MemoryReservation,
/// The current [`BatchCursor`] for each stream
- cursors: Vec<BatchCursor>,
+ cursors: Vec<Option<BatchCursor<C>>>,
Review Comment:
All of the cursor management/ownership is moved into the SortOrderBuilder,
and removed from the SortPreservingMergeStream. No second copy. Instead the
division of concerns is:
* SortPreservingMergeStream == only care about merging (via loser tree)
* SortOrderBuilder == holds the cursor (being advanced), and the sort_order
(being built).
This clearly demonstrates why I need to add more docs to the BatchCursor. 😅
Doing so.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
