feat: Store only the last row in the previous cursor for round robin tie-breaking purposes#23619
feat: Store only the last row in the previous cursor for round robin tie-breaking purposes#23619ariel-miculas wants to merge 7 commits into
Conversation
| /// any buffer the cursor's [`CursorValues`] holds. Used to remember a | ||
| /// partition's last row across a batch boundary without keeping the | ||
| /// whole exhausted batch's memory alive (see [`Self::is_eq_to_prev_one`]). | ||
| pub fn last_value(&self) -> T::SingleRowValue { |
There was a problem hiding this comment.
I feel like this should return an Option, in case values() is empty
There was a problem hiding this comment.
values() shouldn't be empty, there are some asserts in the code:
assert!(rows.num_rows() > 0);
assert!(array.len() > 0, "Empty array passed to FieldCursor");
I don't imagine a Cursor with empty values being useful.
impl RowValues {
/// Create a new [`RowValues`] from `rows` and a `reservation`
/// that tracks its memory. There must be at least one row
///
/// Panics if the reservation is not for exactly `rows.size()`
/// bytes or if `rows` is empty.
pub fn new(rows: Arc<Rows>, reservation: MemoryReservation) -> Self {
assert_eq!(
rows.size(),
reservation.size(),
"memory reservation mismatch"
);
assert!(rows.num_rows() > 0);
Self {
rows,
_reservation: reservation,
}
}
}
pub fn new<A: CursorArray<Values = T>>(
options: SortOptions,
array: &A,
reservation: MemoryReservation,
) -> Self {
assert!(array.len() > 0, "Empty array passed to FieldCursor");
let null_threshold = match options.nulls_first {
true => array.null_count(),
false => array.len() - array.null_count(),
};
There was a problem hiding this comment.
The previous code also assumed values is never empty:
fn is_eq_to_prev_row_in_prev_batch(&self, other: &Self) -> bool {
assert_eq!(self.offset, 0);
T::eq(
&self.values,
self.offset,
&other.values,
other.values.len() - 1,
)
}
|
@Dandandan wondering whether we can change ReusableRows to only hold one slot after this change, since the previous cursor will no longer need to be kept alive for the round robin tie-breaking feature. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #23619 +/- ##
==========================================
- Coverage 80.70% 80.70% -0.01%
==========================================
Files 1089 1089
Lines 368038 368089 +51
Branches 368038 368089 +51
==========================================
+ Hits 297031 297053 +22
- Misses 53308 53334 +26
- Partials 17699 17702 +3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Which issue does this PR close?
Rationale for this change
See the linked issue.
Note that this PR also contains the changes in #23606, so this will have to be rebased
What changes are included in this PR?
Store only the last row in prev_cursors instead of keeping the entire cursor
Are these changes tested?
Added a test to show the improvement
Are there any user-facing changes?
No