alamb commented on a change in pull request #722:
URL: https://github.com/apache/arrow-datafusion/pull/722#discussion_r668985768
##########
File path: datafusion/src/physical_plan/sort_preserving_merge.rs
##########
@@ -255,15 +295,11 @@ impl SortKeyCursor {
}
(true, false) => return Ok(Ordering::Less),
(false, false) => {}
- (true, true) => {
- // TODO: Building the predicate each time is sub-optimal
Review comment:
🎉
##########
File path: datafusion/src/physical_plan/sort_preserving_merge.rs
##########
@@ -246,7 +274,19 @@ impl SortKeyCursor {
.zip(other.columns.iter())
.zip(options.iter());
- for ((l, r), sort_options) in zipped {
+ // Recall or initialise a collection of comparators for comparing
+ // columnar arrays of this cursor and "other".
+ let cmp = self
+ .batch_comparators
+ .entry(other.batch_idx)
+ .or_insert_with(|| Vec::with_capacity(other.columns.len()));
Review comment:
I was initially worried that mutating `cmp` below might mask errors
(e.g. that the number of columns had grown somehow), so I wonder if it is worth
putting the initialization of `cmp` into this callsite (rather than the loop
below)?
However, perhaps that avoids having to clone or collect `zipped`.
Please feel free to ignore this comment
##########
File path: datafusion/src/physical_plan/sort_preserving_merge.rs
##########
@@ -176,34 +178,60 @@ impl ExecutionPlan for SortPreservingMergeExec {
}
}
-/// A `SortKeyCursor` is created from a `RecordBatch`, and a set of
`PhysicalExpr` that when
-/// evaluated on the `RecordBatch` yield the sort keys.
+/// A `SortKeyCursor` is created from a `RecordBatch`, and a set of
+/// `PhysicalExpr` that when evaluated on the `RecordBatch` yield the sort
keys.
///
/// Additionally it maintains a row cursor that can be advanced through the
rows
/// of the provided `RecordBatch`
///
-/// `SortKeyCursor::compare` can then be used to compare the sort key pointed
to by this
-/// row cursor, with that of another `SortKeyCursor`
-#[derive(Debug, Clone)]
+/// `SortKeyCursor::compare` can then be used to compare the sort key pointed
to
+/// by this row cursor, with that of another `SortKeyCursor`. A cursor stores
+/// a row comparator for each other cursor that it is compared to.
struct SortKeyCursor {
columns: Vec<ArrayRef>,
- batch: RecordBatch,
cur_row: usize,
num_rows: usize,
+
+ // An index uniquely identifying the record batch scanned by this cursor.
+ batch_idx: usize,
+ batch: RecordBatch,
+
+ // A collection of comparators that compare rows in this cursor's batch to
+ // the cursors in other batches. Other batches are uniquely identified by
+ // their batch_idx.
Review comment:
is it worth mentioning that the index of the `Vec` are the sort column
positions (not other batch indexes)?
--
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]
