alamb commented on code in PR #7912:
URL: https://github.com/apache/arrow-datafusion/pull/7912#discussion_r1373417433
##########
datafusion/physical-plan/src/sorts/cursor.rs:
##########
@@ -122,42 +136,73 @@ impl<T: CursorValues> Ord for Cursor<T> {
/// Used for sorting when there are multiple columns in the sort key
#[derive(Debug)]
pub struct RowValues {
- rows: Rows,
+ rows: Arc<Rows>,
+
+ /// Lower bound of windowed RowValues.
Review Comment:
```suggestion
/// Lower bound within `rows`.
```
##########
datafusion/physical-plan/src/sorts/cursor.rs:
##########
@@ -122,42 +136,73 @@ impl<T: CursorValues> Ord for Cursor<T> {
/// Used for sorting when there are multiple columns in the sort key
#[derive(Debug)]
pub struct RowValues {
- rows: Rows,
+ rows: Arc<Rows>,
+
+ /// Lower bound of windowed RowValues.
+ offset: usize,
+ /// Upper bound of windowed RowValues (not inclusive).
Review Comment:
```suggestion
/// Upper bound within `rows` (not inclusive)
```
##########
datafusion/physical-plan/src/sorts/cursor.rs:
##########
@@ -122,42 +136,73 @@ impl<T: CursorValues> Ord for Cursor<T> {
/// Used for sorting when there are multiple columns in the sort key
#[derive(Debug)]
pub struct RowValues {
- rows: Rows,
+ rows: Arc<Rows>,
+
+ /// Lower bound of windowed RowValues.
+ offset: usize,
+ /// Upper bound of windowed RowValues (not inclusive).
+ limit: usize,
/// Tracks for the memory used by in the `Rows` of this
/// cursor. Freed on drop
#[allow(dead_code)]
- reservation: MemoryReservation,
+ reservation: Arc<MemoryReservation>,
}
impl RowValues {
- /// Create a new [`RowValues`] from `rows` and a `reservation`
- /// that tracks its memory. There must be at least one row
+ /// Create a new [`RowValues`] from `Arc<Rows>`.
Review Comment:
I think it takes a owned `Rows`, and the `Arc` is created internally
```suggestion
/// Create a new [`RowValues`] from `Rows`.
```
##########
datafusion/physical-plan/src/sorts/cursor.rs:
##########
@@ -40,6 +41,13 @@ pub trait CursorValues {
/// Returns comparison of `l[l_idx]` and `r[r_idx]`
fn compare(l: &Self, l_idx: usize, r: &Self, r_idx: usize) -> Ordering;
+
+ /// Slice at a given row index, returning a new Self
Review Comment:
```suggestion
/// Returns a zero-copy slice of this `CursorValues` with the indicated
offset and length.
```
##########
datafusion/physical-plan/src/sorts/cursor.rs:
##########
@@ -459,4 +560,296 @@ mod tests {
b.advance();
assert_eq!(a.cmp(&b), Ordering::Less);
}
+
+ #[test]
+ fn test_slice_primitive() {
+ let options = SortOptions {
+ descending: false,
+ nulls_first: true,
+ };
+
+ let buffer = ScalarBuffer::from(vec![0, 1, 2, 3]);
+ let mut cursor = new_primitive_cursor(options, buffer, 0);
+
+ // from start
+ let sliced = Cursor::new(cursor.cursor_values().slice(0, 1));
+ let expected = new_primitive_cursor(options,
ScalarBuffer::from(vec![0]), 0);
+ assert_eq!(
+ sliced.cmp(&expected),
+ Ordering::Equal,
+ "should slice from start"
+ );
+
+ // with offset
+ let sliced = Cursor::new(cursor.cursor_values().slice(1, 2));
+ let expected = new_primitive_cursor(options,
ScalarBuffer::from(vec![1]), 0);
+ assert_eq!(
+ sliced.cmp(&expected),
+ Ordering::Equal,
+ "should slice with offset"
+ );
+
+ // cursor current position != start
+ cursor.advance();
+ let sliced = Cursor::new(cursor.cursor_values().slice(0, 1));
+ let expected = new_primitive_cursor(options,
ScalarBuffer::from(vec![0]), 0);
+ assert_eq!(
+ sliced.cmp(&expected),
+ Ordering::Equal,
+ "should ignore current cursor position when sliced"
+ );
+
+ // slicing on a slice (a.k.a. combining offsets)
+ let sliced = Cursor::new(cursor.cursor_values().slice(1, 3));
+ let sliced = Cursor::new(sliced.cursor_values().slice(2, 1));
+ let expected = new_primitive_cursor(options,
ScalarBuffer::from(vec![3]), 0);
+ assert_eq!(
+ sliced.cmp(&expected),
+ Ordering::Equal,
+ "should respect previous slice/windowed boundaries, when
re-slicing"
+ );
+ }
+
+ #[test]
+ fn test_slice_primitive_nulls_first() {
+ let options = SortOptions {
+ descending: false,
+ nulls_first: true,
+ };
+
+ let is_min = new_primitive_cursor(options,
ScalarBuffer::from(vec![i32::MIN]), 0);
+ let is_null =
+ new_primitive_cursor(options, ScalarBuffer::from(vec![i32::MIN]),
1);
+
+ let buffer = ScalarBuffer::from(vec![i32::MIN, 79, 2, i32::MIN]);
+ let mut a = new_primitive_cursor(options, buffer, 2);
+ let buffer = ScalarBuffer::from(vec![i32::MIN, -284, 3, i32::MIN, 2]);
+ let mut b = new_primitive_cursor(options, buffer, 2);
+
+ // NULL == NULL
+ assert_eq!(a, is_null);
+ assert_eq!(a.cmp(&b), Ordering::Equal);
+
+ // i32::MIN > NULL
+ a = Cursor::new(a.cursor_values().slice(3, 1));
+ assert_eq!(a, is_min);
+ assert_eq!(a.cmp(&b), Ordering::Greater);
+
+ // i32::MIN == i32::MIN
+ b = Cursor::new(b.cursor_values().slice(3, 2));
+ assert_eq!(b, is_min);
+ assert_eq!(a.cmp(&b), Ordering::Equal);
+
+ // i32::MIN < 2
+ b = Cursor::new(b.cursor_values().slice(1, 1));
+ assert_eq!(a.cmp(&b), Ordering::Less);
+ }
+
+ #[test]
+ fn test_slice_primitive_nulls_last() {
+ let options = SortOptions {
+ descending: false,
+ nulls_first: false,
+ };
+
+ let is_min = new_primitive_cursor(options,
ScalarBuffer::from(vec![i32::MIN]), 0);
+ let is_null =
+ new_primitive_cursor(options, ScalarBuffer::from(vec![i32::MIN]),
1);
+
+ let buffer = ScalarBuffer::from(vec![i32::MIN, 79, 2, i32::MIN]);
+ let mut a = new_primitive_cursor(options, buffer, 2);
+ let buffer = ScalarBuffer::from(vec![i32::MIN, -284, 3, i32::MIN, 2]);
+ let mut b = new_primitive_cursor(options, buffer, 2);
+
+ // i32::MIN == i32::MIN
+ assert_eq!(a, is_min);
+ assert_eq!(a.cmp(&b), Ordering::Equal);
+
+ // i32::MIN < -284
+ b = Cursor::new(b.cursor_values().slice(1, 3)); // slice to full length
+ assert_eq!(a.cmp(&b), Ordering::Less);
+
+ // 79 > -284
+ a = Cursor::new(a.cursor_values().slice(1, 2)); // slice to shorter
than full length
+ assert_ne!(a, is_null);
+ assert_eq!(a.cmp(&b), Ordering::Greater);
+
+ // NULL == NULL
+ a = Cursor::new(a.cursor_values().slice(1, 1));
+ b = Cursor::new(b.cursor_values().slice(2, 1));
+ assert_eq!(a, is_null);
+ assert_eq!(b, is_null);
+ assert_eq!(a.cmp(&b), Ordering::Equal);
+ }
+
+ #[test]
+ #[should_panic(expected = "slice offset is out of bounds")]
+ fn test_slice_primitive_can_panic() {
+ let options = SortOptions {
+ descending: false,
+ nulls_first: true,
+ };
+
+ let buffer = ScalarBuffer::from(vec![0, 1, 2]);
+ let cursor = new_primitive_cursor(options, buffer, 0);
+
+ cursor
+ .cursor_values()
+ .slice(cursor.cursor_values().len(), 1);
+ }
+
+ fn new_row_cursor(cols: &[Arc<dyn Array>; 2]) -> Cursor<RowValues> {
+ let converter = RowConverter::new(vec![
+ SortField::new(DataType::Int16),
+ SortField::new(DataType::Float32),
+ ])
+ .unwrap();
+
+ let rows = converter.convert_columns(cols).unwrap();
+
+ let pool: Arc<dyn MemoryPool> =
Arc::new(GreedyMemoryPool::new(rows.size()));
+ let mut reservation = MemoryConsumer::new("test").register(&pool);
+ reservation.try_grow(rows.size()).unwrap();
+
+ Cursor::new(RowValues::new(rows, reservation))
+ }
+
+ #[test]
+ fn test_slice_rows() {
+ // rows
+ let cols = [
+ Arc::new(Int16Array::from_iter([Some(1), Some(2), Some(3),
Some(4)]))
+ as ArrayRef,
+ Arc::new(Float32Array::from_iter([
+ Some(1.3),
+ Some(2.5),
+ Some(4.),
+ Some(4.2),
+ ])) as ArrayRef,
+ ];
+
+ let mut a = new_row_cursor(&cols);
+ let mut b = new_row_cursor(&cols);
+ assert_eq!(a.cursor_values().len(), 4);
+
+ // 1,1.3 == 1,1.3
+ assert_eq!(a.cmp(&b), Ordering::Equal);
+
+ // advance A. slice B full length.
+ // 2,2.5 > 1,1.3
+ a.advance();
+ b = Cursor::new(b.cursor_values().slice(0, 3));
+ assert_eq!(a.cmp(&b), Ordering::Greater);
+
+ // slice B ahead by 2.
+ // 2,2.5 < 3,4.0
+ b = Cursor::new(b.cursor_values().slice(2, 1));
+ assert_eq!(a.cmp(&b), Ordering::Less);
+
+ // advanced cursor vs sliced cursor
+ assert_eq!(a.cursor_values().len(), 4);
+ assert_eq!(b.cursor_values().len(), 1);
+
+ // slicing on a slice (a.k.a. combining offsets)
+ let cursor = new_row_cursor(&cols);
+ let sliced = Cursor::new(cursor.cursor_values().slice(1, 3));
+ let sliced = Cursor::new(sliced.cursor_values().slice(2, 1));
+ let mut expected = new_row_cursor(&cols);
+ expected.advance();
+ expected.advance();
+ expected.advance();
+ assert_eq!(
+ sliced.cmp(&expected),
+ Ordering::Equal,
+ "should respect previous slice/windowed boundaries, when
re-slicing"
+ );
+ }
+
+ #[test]
+ #[should_panic(expected = "slice offset is out of bounds")]
+ fn test_slice_rows_can_panic() {
+ let cols = [
+ Arc::new(Int16Array::from_iter([Some(1)])) as ArrayRef,
+ Arc::new(Float32Array::from_iter([Some(1.3)])) as ArrayRef,
+ ];
+
+ let cursor = new_row_cursor(&cols);
+
+ cursor
+ .cursor_values()
+ .slice(cursor.cursor_values().len(), 1);
+ }
+
+ fn new_bytearray_cursor(
+ values_str: &str,
+ offsets: Vec<i32>,
+ ) -> Cursor<ArrayValues<ByteArrayValues<i32>>> {
+ let values = Buffer::from_slice_ref(values_str);
+ let offsets = OffsetBuffer::new(offsets.into());
+
+ let options = SortOptions {
+ descending: false,
+ nulls_first: true,
+ };
+
+ Cursor::new(ArrayValues {
+ values: ByteArrayValues {
+ offsets: offsets.clone(),
+ values: values.clone(),
+ },
+ null_threshold: 0,
+ options,
+ })
+ }
+
+ #[test]
+ fn test_slice_bytearray() {
+ let mut a = new_bytearray_cursor("hellorainbowworldzoo", vec![0, 5,
12, 17, 20]);
+ let mut b = new_bytearray_cursor("hellorainbowworldzoo", vec![0, 5,
12, 17, 20]);
+
+ let is_hello = new_bytearray_cursor("hello", vec![0, 5]);
+ let is_rainbow = new_bytearray_cursor("rainbow", vec![0, 7]);
+ let is_world = new_bytearray_cursor("world", vec![0, 5]);
+ let is_zoo = new_bytearray_cursor("zoo", vec![0, 3]);
+
+ // hello == hello
+ assert_eq!(a.cmp(&b), Ordering::Equal);
+
+ // advance A. slice B full length.
Review Comment:
thank you for these comments 👨🍳 👌
##########
datafusion/physical-plan/src/sorts/cursor.rs:
##########
@@ -459,4 +560,296 @@ mod tests {
b.advance();
assert_eq!(a.cmp(&b), Ordering::Less);
}
+
+ #[test]
+ fn test_slice_primitive() {
+ let options = SortOptions {
+ descending: false,
+ nulls_first: true,
+ };
+
+ let buffer = ScalarBuffer::from(vec![0, 1, 2, 3]);
+ let mut cursor = new_primitive_cursor(options, buffer, 0);
+
+ // from start
+ let sliced = Cursor::new(cursor.cursor_values().slice(0, 1));
+ let expected = new_primitive_cursor(options,
ScalarBuffer::from(vec![0]), 0);
+ assert_eq!(
+ sliced.cmp(&expected),
+ Ordering::Equal,
+ "should slice from start"
+ );
+
+ // with offset
+ let sliced = Cursor::new(cursor.cursor_values().slice(1, 2));
+ let expected = new_primitive_cursor(options,
ScalarBuffer::from(vec![1]), 0);
+ assert_eq!(
+ sliced.cmp(&expected),
+ Ordering::Equal,
+ "should slice with offset"
+ );
+
+ // cursor current position != start
+ cursor.advance();
+ let sliced = Cursor::new(cursor.cursor_values().slice(0, 1));
+ let expected = new_primitive_cursor(options,
ScalarBuffer::from(vec![0]), 0);
+ assert_eq!(
+ sliced.cmp(&expected),
+ Ordering::Equal,
+ "should ignore current cursor position when sliced"
+ );
+
+ // slicing on a slice (a.k.a. combining offsets)
+ let sliced = Cursor::new(cursor.cursor_values().slice(1, 3));
+ let sliced = Cursor::new(sliced.cursor_values().slice(2, 1));
+ let expected = new_primitive_cursor(options,
ScalarBuffer::from(vec![3]), 0);
+ assert_eq!(
+ sliced.cmp(&expected),
+ Ordering::Equal,
+ "should respect previous slice/windowed boundaries, when
re-slicing"
+ );
+ }
+
+ #[test]
+ fn test_slice_primitive_nulls_first() {
+ let options = SortOptions {
+ descending: false,
+ nulls_first: true,
+ };
+
+ let is_min = new_primitive_cursor(options,
ScalarBuffer::from(vec![i32::MIN]), 0);
+ let is_null =
+ new_primitive_cursor(options, ScalarBuffer::from(vec![i32::MIN]),
1);
+
+ let buffer = ScalarBuffer::from(vec![i32::MIN, 79, 2, i32::MIN]);
+ let mut a = new_primitive_cursor(options, buffer, 2);
+ let buffer = ScalarBuffer::from(vec![i32::MIN, -284, 3, i32::MIN, 2]);
+ let mut b = new_primitive_cursor(options, buffer, 2);
+
+ // NULL == NULL
+ assert_eq!(a, is_null);
+ assert_eq!(a.cmp(&b), Ordering::Equal);
+
+ // i32::MIN > NULL
+ a = Cursor::new(a.cursor_values().slice(3, 1));
+ assert_eq!(a, is_min);
+ assert_eq!(a.cmp(&b), Ordering::Greater);
+
+ // i32::MIN == i32::MIN
+ b = Cursor::new(b.cursor_values().slice(3, 2));
+ assert_eq!(b, is_min);
+ assert_eq!(a.cmp(&b), Ordering::Equal);
+
+ // i32::MIN < 2
+ b = Cursor::new(b.cursor_values().slice(1, 1));
+ assert_eq!(a.cmp(&b), Ordering::Less);
+ }
+
+ #[test]
+ fn test_slice_primitive_nulls_last() {
+ let options = SortOptions {
+ descending: false,
+ nulls_first: false,
+ };
+
+ let is_min = new_primitive_cursor(options,
ScalarBuffer::from(vec![i32::MIN]), 0);
+ let is_null =
+ new_primitive_cursor(options, ScalarBuffer::from(vec![i32::MIN]),
1);
+
+ let buffer = ScalarBuffer::from(vec![i32::MIN, 79, 2, i32::MIN]);
+ let mut a = new_primitive_cursor(options, buffer, 2);
+ let buffer = ScalarBuffer::from(vec![i32::MIN, -284, 3, i32::MIN, 2]);
+ let mut b = new_primitive_cursor(options, buffer, 2);
+
+ // i32::MIN == i32::MIN
+ assert_eq!(a, is_min);
+ assert_eq!(a.cmp(&b), Ordering::Equal);
+
+ // i32::MIN < -284
+ b = Cursor::new(b.cursor_values().slice(1, 3)); // slice to full length
+ assert_eq!(a.cmp(&b), Ordering::Less);
+
+ // 79 > -284
+ a = Cursor::new(a.cursor_values().slice(1, 2)); // slice to shorter
than full length
+ assert_ne!(a, is_null);
+ assert_eq!(a.cmp(&b), Ordering::Greater);
+
+ // NULL == NULL
+ a = Cursor::new(a.cursor_values().slice(1, 1));
+ b = Cursor::new(b.cursor_values().slice(2, 1));
+ assert_eq!(a, is_null);
+ assert_eq!(b, is_null);
+ assert_eq!(a.cmp(&b), Ordering::Equal);
+ }
+
+ #[test]
+ #[should_panic(expected = "slice offset is out of bounds")]
+ fn test_slice_primitive_can_panic() {
+ let options = SortOptions {
+ descending: false,
+ nulls_first: true,
+ };
+
+ let buffer = ScalarBuffer::from(vec![0, 1, 2]);
+ let cursor = new_primitive_cursor(options, buffer, 0);
+
+ cursor
+ .cursor_values()
+ .slice(cursor.cursor_values().len(), 1);
+ }
+
+ fn new_row_cursor(cols: &[Arc<dyn Array>; 2]) -> Cursor<RowValues> {
+ let converter = RowConverter::new(vec![
+ SortField::new(DataType::Int16),
+ SortField::new(DataType::Float32),
+ ])
+ .unwrap();
+
+ let rows = converter.convert_columns(cols).unwrap();
+
+ let pool: Arc<dyn MemoryPool> =
Arc::new(GreedyMemoryPool::new(rows.size()));
+ let mut reservation = MemoryConsumer::new("test").register(&pool);
+ reservation.try_grow(rows.size()).unwrap();
+
+ Cursor::new(RowValues::new(rows, reservation))
+ }
+
+ #[test]
+ fn test_slice_rows() {
+ // rows
+ let cols = [
+ Arc::new(Int16Array::from_iter([Some(1), Some(2), Some(3),
Some(4)]))
+ as ArrayRef,
+ Arc::new(Float32Array::from_iter([
+ Some(1.3),
+ Some(2.5),
+ Some(4.),
+ Some(4.2),
+ ])) as ArrayRef,
+ ];
+
+ let mut a = new_row_cursor(&cols);
+ let mut b = new_row_cursor(&cols);
+ assert_eq!(a.cursor_values().len(), 4);
+
+ // 1,1.3 == 1,1.3
+ assert_eq!(a.cmp(&b), Ordering::Equal);
+
+ // advance A. slice B full length.
+ // 2,2.5 > 1,1.3
+ a.advance();
+ b = Cursor::new(b.cursor_values().slice(0, 3));
+ assert_eq!(a.cmp(&b), Ordering::Greater);
+
+ // slice B ahead by 2.
+ // 2,2.5 < 3,4.0
+ b = Cursor::new(b.cursor_values().slice(2, 1));
+ assert_eq!(a.cmp(&b), Ordering::Less);
+
+ // advanced cursor vs sliced cursor
+ assert_eq!(a.cursor_values().len(), 4);
+ assert_eq!(b.cursor_values().len(), 1);
+
+ // slicing on a slice (a.k.a. combining offsets)
+ let cursor = new_row_cursor(&cols);
+ let sliced = Cursor::new(cursor.cursor_values().slice(1, 3));
+ let sliced = Cursor::new(sliced.cursor_values().slice(2, 1));
+ let mut expected = new_row_cursor(&cols);
+ expected.advance();
+ expected.advance();
+ expected.advance();
+ assert_eq!(
+ sliced.cmp(&expected),
+ Ordering::Equal,
+ "should respect previous slice/windowed boundaries, when
re-slicing"
+ );
+ }
+
+ #[test]
+ #[should_panic(expected = "slice offset is out of bounds")]
+ fn test_slice_rows_can_panic() {
+ let cols = [
+ Arc::new(Int16Array::from_iter([Some(1)])) as ArrayRef,
+ Arc::new(Float32Array::from_iter([Some(1.3)])) as ArrayRef,
+ ];
+
+ let cursor = new_row_cursor(&cols);
+
+ cursor
+ .cursor_values()
+ .slice(cursor.cursor_values().len(), 1);
+ }
+
+ fn new_bytearray_cursor(
+ values_str: &str,
+ offsets: Vec<i32>,
+ ) -> Cursor<ArrayValues<ByteArrayValues<i32>>> {
+ let values = Buffer::from_slice_ref(values_str);
+ let offsets = OffsetBuffer::new(offsets.into());
+
+ let options = SortOptions {
+ descending: false,
+ nulls_first: true,
+ };
+
+ Cursor::new(ArrayValues {
+ values: ByteArrayValues {
+ offsets: offsets.clone(),
+ values: values.clone(),
+ },
+ null_threshold: 0,
+ options,
+ })
+ }
+
+ #[test]
+ fn test_slice_bytearray() {
+ let mut a = new_bytearray_cursor("hellorainbowworldzoo", vec![0, 5,
12, 17, 20]);
+ let mut b = new_bytearray_cursor("hellorainbowworldzoo", vec![0, 5,
12, 17, 20]);
+
+ let is_hello = new_bytearray_cursor("hello", vec![0, 5]);
+ let is_rainbow = new_bytearray_cursor("rainbow", vec![0, 7]);
+ let is_world = new_bytearray_cursor("world", vec![0, 5]);
+ let is_zoo = new_bytearray_cursor("zoo", vec![0, 3]);
+
+ // hello == hello
+ assert_eq!(a.cmp(&b), Ordering::Equal);
+
+ // advance A. slice B full length.
Review Comment:
thank you for these comments 👨🍳 👌
##########
datafusion/physical-plan/src/sorts/cursor.rs:
##########
@@ -459,4 +560,296 @@ mod tests {
b.advance();
assert_eq!(a.cmp(&b), Ordering::Less);
}
+
+ #[test]
+ fn test_slice_primitive() {
+ let options = SortOptions {
+ descending: false,
+ nulls_first: true,
+ };
+
+ let buffer = ScalarBuffer::from(vec![0, 1, 2, 3]);
+ let mut cursor = new_primitive_cursor(options, buffer, 0);
+
+ // from start
+ let sliced = Cursor::new(cursor.cursor_values().slice(0, 1));
+ let expected = new_primitive_cursor(options,
ScalarBuffer::from(vec![0]), 0);
+ assert_eq!(
+ sliced.cmp(&expected),
+ Ordering::Equal,
+ "should slice from start"
+ );
+
+ // with offset
+ let sliced = Cursor::new(cursor.cursor_values().slice(1, 2));
+ let expected = new_primitive_cursor(options,
ScalarBuffer::from(vec![1]), 0);
+ assert_eq!(
+ sliced.cmp(&expected),
+ Ordering::Equal,
+ "should slice with offset"
+ );
+
+ // cursor current position != start
+ cursor.advance();
+ let sliced = Cursor::new(cursor.cursor_values().slice(0, 1));
+ let expected = new_primitive_cursor(options,
ScalarBuffer::from(vec![0]), 0);
+ assert_eq!(
+ sliced.cmp(&expected),
+ Ordering::Equal,
+ "should ignore current cursor position when sliced"
+ );
+
+ // slicing on a slice (a.k.a. combining offsets)
+ let sliced = Cursor::new(cursor.cursor_values().slice(1, 3));
+ let sliced = Cursor::new(sliced.cursor_values().slice(2, 1));
+ let expected = new_primitive_cursor(options,
ScalarBuffer::from(vec![3]), 0);
+ assert_eq!(
+ sliced.cmp(&expected),
+ Ordering::Equal,
+ "should respect previous slice/windowed boundaries, when
re-slicing"
+ );
+ }
+
+ #[test]
+ fn test_slice_primitive_nulls_first() {
Review Comment:
What is the reason for also testing with `nulls_first` / `nulls_last`? I
didn't see that the semantics of `slice` differ based on the that setting and
thus I don't think these tests add much/any new coverage
I suggest either making a loop in `test_slice_primitive` that uses the
different `SortOptions` ensuring they produce the same result or just removing
these two tests
##########
datafusion/physical-plan/src/sorts/cursor.rs:
##########
@@ -40,6 +41,13 @@ pub trait CursorValues {
/// Returns comparison of `l[l_idx]` and `r[r_idx]`
fn compare(l: &Self, l_idx: usize, r: &Self, r_idx: usize) -> Ordering;
+
+ /// Slice at a given row index, returning a new Self
+ ///
+ /// # Panics
+ ///
+ /// Panics if the slice is out of bounds, or memory is insufficient
Review Comment:
I didn't see any code that would panic about memory being insufficient --
since this is a zero copy slice, I am not sure the memory check is needed
--
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]
