Add `GenericListViewArray` for `ListView` & `LargeListView` #5723

Kikkon · 2024-05-05T15:41:55Z

Which issue does this PR close?

A part of #5501

Rationale for this change

What changes are included in this PR?

Add GenericListViewArray implement

Are there any user-facing changes?

Kikkon · 2024-06-11T12:47:00Z

@tustvold If you have time, I would appreciate it if you could help me review this PR. Thanks!

arrow-array/src/array/list_view_array.rs

tustvold

Sorry for the delay, been swamped and then sick, but this is looking almost there

Kikkon · 2024-06-16T07:38:09Z

Thank you for taking the time to review my code despite being busy and unwell.

tustvold · 2024-06-26T10:15:00Z

For future reference, it is helpful for reviewers if you don't force push branches once reviews have been made, this makes it much easier to see what has changed. I'll try to find time to review this later today, but I now have to do a full review again which is quite time consuming on such a large PR

Kikkon · 2024-07-01T05:28:31Z

For future reference, it is helpful for reviewers if you don't force push branches once reviews have been made, this makes it much easier to see what has changed. I'll try to find time to review this later today, but I now have to do a full review again which is quite time consuming on such a large PR

@tustvold Thank you for your help. I will be more careful about the force push issue in my future commits. I am very sorry.

Kikkon · 2024-07-22T08:26:27Z

Hi @tustvold , perhaps when you're free this week, you could help me review this. This way, we can continue to move forward with this work. Thank you.

tustvold · 2024-07-22T08:38:57Z

I'm afraid I don't have capacity to assist with this initiative further, but perhaps @alamb may do. I am reducing my involvement in arrow-rs as I no longer use it in my day job

Kikkon · 2024-07-24T02:22:07Z

@alamb Perhaps you can help advance this PR when you have time. 🥸

westonpace

Some suggestions but overall this matches my understanding of list view arrays. Thanks for the work!

arrow-array/src/array/list_view_array.rs

westonpace · 2024-07-30T14:33:18Z

arrow-array/src/array/list_view_array.rs

+impl<OffsetSize: OffsetSizeTrait> Clone for GenericListViewArray<OffsetSize> {
+    fn clone(&self) -> Self {
+        Self {
+            data_type: self.data_type.clone(),
+            nulls: self.nulls.clone(),
+            values: self.values.clone(),
+            value_offsets: self.value_offsets.clone(),
+            value_sizes: self.value_sizes.clone(),
+        }
+    }
+}


Can you derive the Clone impl?

westonpace · 2024-07-30T14:36:24Z

arrow-array/src/array/list_view_array.rs

+        for i in 0..offsets.len() {
+            let length = values.len();
+            let offset = offsets[i].as_usize();
+            let size = sizes[i].as_usize();


Minor nit, is it faster to use offsets.iter().zip(sizes) (maybe the optimize down to the same thing?)

westonpace · 2024-07-30T14:37:00Z

arrow-array/src/array/list_view_array.rs

+    ///
+    /// * `offsets.len() != sizes.len()`
+    /// * `offsets.len() != nulls.len()`
+    /// * `offsets.last() > values.len()`


This implies you only check the last offset but you validate all offsets

I reviewed the code again and realized that I had already validated all the offsets, so I changed it to offsets[i] > values.len().

westonpace · 2024-07-30T14:37:36Z

arrow-array/src/array/list_view_array.rs

+                    length
+                )));
+            }
+            if offset.saturating_add(size) > values.len() {


Do you need the above length comparison if you also have this comparison?

westonpace · 2024-07-30T15:02:01Z

arrow-array/src/array/list_view_array.rs

+        let mut sizes = Vec::with_capacity(iter.size_hint().0);
+        for size in iter {
+            offsets.push(OffsetSize::usize_as(acc));
+            acc = acc.checked_add(size).expect("usize overflow");


Minor corner case nit: Imagine we are working with i32 offsets and we have 1Gi - 1 fixed size lists of size 2. Will this panic even thought it shouldn't.

westonpace · 2024-07-30T15:02:20Z

arrow-array/src/array/list_view_array.rs

+            acc = acc.checked_add(size).expect("usize overflow");
+            sizes.push(OffsetSize::usize_as(size));
+        }
+        OffsetSize::from_usize(acc).expect("offset overflow");


Why this final check? It seems redundant.

arrow-array/src/array/list_view_array.rs

westonpace · 2024-07-30T15:20:40Z

arrow-array/src/array/list_view_array.rs

+        // SAFETY:
+        // ArrayData is valid, and verified type above


Why SAFETY? We aren't doing anything unsafe here that I can see.

westonpace · 2024-07-30T15:23:49Z

arrow-array/src/array/list_view_array.rs

+        let sizes = list.value_sizes();
+        assert_eq!(sizes, &[3, 3, 3]);
+    }
+}


Some other tests might be:

Test where the lists overlap (e.g. sizes = [5, 5] and offsets = [0, 3] and values has length 8.

Test where offsets does not cover the entire range of values (e.g. values has 50 things and offsets only go up to 10)

List of empty lists (with an empty values)

Co-authored-by: Weston Pace <[email protected]>

Kikkon · 2024-07-31T16:37:08Z

hi @westonpace please help me review again

westonpace

A few last small things.

westonpace · 2024-07-31T18:06:18Z

arrow-array/src/array/list_view_array.rs

+        for (offset, size) in offsets.iter().zip(sizes.iter()) {
+            let offset = offset.as_usize();
+            let size = size.as_usize();
+            if offset.saturating_add(size) > values.len() {


This should be a checked_add.

westonpace · 2024-07-31T18:10:25Z

arrow-array/src/array/list_view_array.rs

+        for size in iter {
+            offsets.push(OffsetSize::usize_as(acc));
+            acc = acc.saturating_add(size);
+            sizes.push(OffsetSize::usize_as(size));
+        }


I did like that you were checking for overflow here, my only concern was that it was in the wrong spot. What if, before the for loop, you checked to ensure that value.len() is less than or equal to the max offset? Then you can keep using saturating_add here. Or even better, use the basic add since we've verified it will never overflow.

I've reconsidered, and it seems that no additional validation is needed here. 🤔

alamb · 2024-09-18T20:11:20Z

I am depressed about the large review backlog in this crate. We are looking for more help from the community reviewing PRs -- see #6418 for more

Kikkon · 2024-09-21T12:50:25Z

@westonpace 😭 Apologies for the delay in processing this pull request due to work reasons. I hope you can help review it when you have time.

westonpace

One minor thing but I think this is good to go. Thanks for addressing the earlier reviews :)

westonpace · 2024-09-23T13:12:02Z

arrow-array/src/array/list_view_array.rs

+        //check if the size is usize
+


Not entirely sure I understand the meaning of this comment.

I think this comment is no longer needed, so I've removed it.

mbrobbel · 2024-09-24T13:00:41Z

arrow-array/src/array/list_view_array.rs

+        self.value_sizes[i]
+    }
+
+    /// Returns the offset for value as index `i`.


Suggested change

/// Returns the offset for value as index `i`.

/// Returns the offset for value at index `i`.

mbrobbel · 2024-09-24T13:01:05Z

arrow-array/src/array/list_view_array.rs

+        self.values.slice(offset, length)
+    }
+
+    /// Returns ith value of this list view array.


Maybe this should get a # Panics note?

mbrobbel · 2024-09-24T13:02:06Z

arrow-array/src/array/list_view_array.rs

+                    "Offset + size for {}ListViewArray must be within the bounds of the child array, got offset: {}, size: {}, child array length: {}",
+                    OffsetSize::PREFIX,
+                    offset,
+                    size,
+                    values.len()


Suggested change

"Offset + size for {}ListViewArray must be within the bounds of the child array, got offset: {}, size: {}, child array length: {}",

OffsetSize::PREFIX,

offset,

size,

values.len()

"Offset + size for {}ListViewArray must be within the bounds of the child array, got offset: {offset}, size: {size}, child array length: {}",

OffsetSize::PREFIX,

values.len()

mbrobbel · 2024-09-24T13:02:17Z

arrow-array/src/array/list_view_array.rs

+                "Length of offsets buffer and sizes buffer must be equal for {}ListViewArray, got {} and {}",
+                OffsetSize::PREFIX, len, sizes.len()


Suggested change

"Length of offsets buffer and sizes buffer must be equal for {}ListViewArray, got {} and {}",

OffsetSize::PREFIX, len, sizes.len()

"Length of offsets buffer and sizes buffer must be equal for {}ListViewArray, got {len} and {}",

OffsetSize::PREFIX, sizes.len()

mbrobbel · 2024-09-24T13:03:10Z

arrow-array/src/array/list_view_array.rs

+        self.value_offsets[i]
+    }
+
+    /// constructs a new iterator


Suggested change

/// constructs a new iterator

/// Constructs a new iterator

mbrobbel · 2024-09-24T13:03:41Z

arrow-array/src/array/list_view_array.rs

+    pub fn iter<'a>(&'a self) -> GenericListViewArrayIter<'a, OffsetSize> {
+        GenericListViewArrayIter::<'a, OffsetSize>::new(self)
+    }


Can't we use lifetime elision?

Suggested change

pub fn iter<'a>(&'a self) -> GenericListViewArrayIter<'a, OffsetSize> {

GenericListViewArrayIter::<'a, OffsetSize>::new(self)

}

pub fn iter(&self) -> GenericListViewArrayIter<'_, OffsetSize> {

GenericListViewArrayIter::<'_, OffsetSize>::new(self)

}

mbrobbel · 2024-09-24T13:06:26Z

arrow-array/src/array/list_view_array.rs

+                    "[Large]ListViewArray's child datatype {:?} does not \
+                             correspond to the List's datatype {:?}",
+                    values.data_type(),
+                    child_data_type


Suggested change

"[Large]ListViewArray's child datatype {:?} does not \

correspond to the List's datatype {:?}",

values.data_type(),

child_data_type

"{}ListViewArray's child datatype {:?} does not \

correspond to the List's datatype {:?}",

OffsetSize::PREFIX,

values.data_type(),

child_data_type

mbrobbel · 2024-09-24T13:06:59Z

arrow-array/src/array/list_view_array.rs

+                "[Large]ListViewArray's datatype must be [Large]ListViewArray(). It is {:?}",
+                data.data_type()


Suggested change

"[Large]ListViewArray's datatype must be [Large]ListViewArray(). It is {:?}",

data.data_type()

"{}ListViewArray's datatype must be {}ListViewArray(). It is {:?}",

OffsetSize::PREFIX,

OffsetSize::PREFIX,

data.data_type()

mbrobbel · 2024-09-24T13:07:59Z

arrow-array/src/array/list_view_array.rs

+
+/// A [`GenericListViewArray`] of variable size lists, storing offsets as `i32`.
+///
+// See [`ListViewBuilder`](crate::builder::ListViewBuilder) for how to construct a [`ListViewArray`]


Suggested change

// See [`ListViewBuilder`](crate::builder::ListViewBuilder) for how to construct a [`ListViewArray`]

/// See [`ListViewBuilder`](crate::builder::ListViewBuilder) for how to construct a [`ListViewArray`]

mbrobbel · 2024-09-24T13:08:04Z

arrow-array/src/array/list_view_array.rs

+
+/// A [`GenericListViewArray`] of variable size lists, storing offsets as `i64`.
+///
+// See [`LargeListViewBuilder`](crate::builder::LargeListViewBuilder) for how to construct a [`LargeListViewArray`]


Suggested change

// See [`LargeListViewBuilder`](crate::builder::LargeListViewBuilder) for how to construct a [`LargeListViewArray`]

/// See [`LargeListViewBuilder`](crate::builder::LargeListViewBuilder) for how to construct a [`LargeListViewArray`]

github-actions bot added the arrow Changes to the arrow crate label May 5, 2024

Kikkon force-pushed the list_view2 branch from 39fabd6 to 9d05600 Compare May 5, 2024 15:49

Kikkon mentioned this pull request May 7, 2024

Add ListViewArray and LargeListViewArray implementation and layout and basic construction #5501

Open

Kikkon force-pushed the list_view2 branch from 9d05600 to 5e0da46 Compare May 18, 2024 15:52

Kikkon marked this pull request as ready for review May 18, 2024 16:20

tustvold reviewed Jun 11, 2024

View reviewed changes

arrow-array/src/array/list_view_array.rs Outdated Show resolved Hide resolved

tustvold reviewed Jun 11, 2024

View reviewed changes

arrow-array/src/array/list_view_array.rs Outdated Show resolved Hide resolved

tustvold reviewed Jun 11, 2024

View reviewed changes

arrow-array/src/array/list_view_array.rs Outdated Show resolved Hide resolved

tustvold reviewed Jun 11, 2024

View reviewed changes

arrow-array/src/array/list_view_array.rs Outdated Show resolved Hide resolved

tustvold reviewed Jun 11, 2024

View reviewed changes

arrow-array/src/array/list_view_array.rs Outdated Show resolved Hide resolved

tustvold reviewed Jun 11, 2024

View reviewed changes

Kikkon added 5 commits June 16, 2024 15:15

Add GenericListViewArray for ListView & LargeListView

4ab530e

rebase master

fbfa059

fix lint

862106e

remove rom_iter_primitive

0913b30

fix fixed array

562dcfb

Kikkon force-pushed the list_view2 branch from eecd8c1 to 562dcfb Compare June 16, 2024 07:18

lint

0a895b7

Kikkon requested a review from tustvold June 16, 2024 14:20

westonpace reviewed Jul 30, 2024

View reviewed changes

Kikkon and others added 3 commits July 31, 2024 21:10

Update arrow-array/src/array/list_view_array.rs

708ce90

Co-authored-by: Weston Pace <[email protected]>

Update arrow-array/src/array/list_view_array.rs

5dc9b58

Co-authored-by: Weston Pace <[email protected]>

fix comment

097c0b4

cargo fmt

33a896f

Kikkon requested a review from westonpace July 31, 2024 16:37

westonpace reviewed Jul 31, 2024

View reviewed changes

fix: check size

98fd023

Kikkon requested a review from westonpace September 21, 2024 12:50

westonpace approved these changes Sep 23, 2024

View reviewed changes

chore:remove unuse comment

65a646e

mbrobbel reviewed Sep 24, 2024

View reviewed changes

	/// Returns the offset for value as index `i`.
	/// Returns the offset for value at index `i`.

		"Length of offsets buffer and sizes buffer must be equal for {}ListViewArray, got {} and {}",
		OffsetSize::PREFIX, len, sizes.len()

		"[Large]ListViewArray's datatype must be [Large]ListViewArray(). It is {:?}",
		data.data_type()

	// See [`ListViewBuilder`](crate::builder::ListViewBuilder) for how to construct a [`ListViewArray`]
	/// See [`ListViewBuilder`](crate::builder::ListViewBuilder) for how to construct a [`ListViewArray`]

	// See [`LargeListViewBuilder`](crate::builder::LargeListViewBuilder) for how to construct a [`LargeListViewArray`]
	/// See [`LargeListViewBuilder`](crate::builder::LargeListViewBuilder) for how to construct a [`LargeListViewArray`]

Add GenericListViewArray for ListView & LargeListView #5723

Are you sure you want to change the base?

Add GenericListViewArray for ListView & LargeListView #5723

Conversation

Kikkon commented May 5, 2024

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

Kikkon commented Jun 11, 2024

tustvold left a comment

Choose a reason for hiding this comment

Kikkon commented Jun 16, 2024

tustvold commented Jun 26, 2024 • edited Loading

Kikkon commented Jul 1, 2024

Kikkon commented Jul 22, 2024

tustvold commented Jul 22, 2024 • edited Loading

Kikkon commented Jul 24, 2024 • edited Loading

westonpace left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kikkon commented Jul 31, 2024

westonpace left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

westonpace Jul 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb commented Sep 18, 2024

Kikkon commented Sep 21, 2024

westonpace left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Add `GenericListViewArray` for `ListView` & `LargeListView` #5723

Add `GenericListViewArray` for `ListView` & `LargeListView` #5723

tustvold commented Jun 26, 2024 •

edited

Loading

tustvold commented Jul 22, 2024 •

edited

Loading

Kikkon commented Jul 24, 2024 •

edited

Loading

westonpace Jul 31, 2024 •

edited

Loading