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)

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.

arrow-array/src/array/list_view_array.rs

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 Thank you for your review.

arrow-array/src/array/list_view_array.rs

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

Co-authored-by: Matthijs Brobbel <[email protected]>

alamb · 2024-10-01T20:05:24Z

I took a quick skim of this PR and the code, tests and documentation are looking quite nice I think -- thank you @Kikkon @westonpace and @mbrobbel

🚀

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 force-pushed the list_view2 branch from eecd8c1 to 562dcfb Compare June 16, 2024 07:18

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

westonpace reviewed Jul 30, 2024

View reviewed changes

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

westonpace reviewed Jul 31, 2024

View reviewed changes

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

westonpace approved these changes Sep 23, 2024

View reviewed changes

mbrobbel reviewed Sep 24, 2024

View reviewed changes

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

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

Kikkon and others added 21 commits September 29, 2024 21:07

Add GenericListViewArray for ListView & LargeListView

4591198

rebase master

59a49db

fix lint

9e5a82c

remove rom_iter_primitive

cf85d1c

fix fixed array

d7ffe39

lint

5f14766

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

8d117fe

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

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

b6707b3

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

fix comment

83113c3

cargo fmt

f9808ba

fix: check size

f3c47b4

chore:remove unuse comment

6c01723

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

8cc4b38

Co-authored-by: Matthijs Brobbel <[email protected]>

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

044f217

Co-authored-by: Matthijs Brobbel <[email protected]>

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

5a8d360

Co-authored-by: Matthijs Brobbel <[email protected]>

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

ccd0ca6

Co-authored-by: Matthijs Brobbel <[email protected]>

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

fd5b1c4

Co-authored-by: Matthijs Brobbel <[email protected]>

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

1a66311

Co-authored-by: Matthijs Brobbel <[email protected]>

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

27fb685

Co-authored-by: Matthijs Brobbel <[email protected]>

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

90c6ee7

Co-authored-by: Matthijs Brobbel <[email protected]>

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

fb4ac3a

Co-authored-by: Matthijs Brobbel <[email protected]>

Kikkon force-pushed the list_view2 branch from 69158be to fb4ac3a Compare September 29, 2024 13:08

Kikkon added 2 commits September 29, 2024 21:12

chore: add comment

f15f075

chore: add comment

6edadca

alamb merged commit fc05eca into apache:master Oct 1, 2024
26 checks passed

Kikkon deleted the list_view2 branch October 12, 2024 09:43

Kikkon restored the list_view2 branch October 12, 2024 09:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Kikkon commented May 5, 2024

Kikkon commented Jun 11, 2024

tustvold left a 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

westonpace Jul 30, 2024

westonpace Jul 30, 2024

westonpace Jul 30, 2024

Kikkon Jul 31, 2024

westonpace Jul 30, 2024

westonpace Jul 30, 2024

westonpace Jul 30, 2024

westonpace Jul 30, 2024

westonpace Jul 30, 2024

Kikkon commented Jul 31, 2024

westonpace left a comment

westonpace Jul 31, 2024

westonpace Jul 31, 2024 •

edited

Loading

Kikkon Sep 21, 2024

alamb commented Sep 18, 2024

Kikkon commented Sep 21, 2024

westonpace left a comment

westonpace Sep 23, 2024

Kikkon Sep 24, 2024

mbrobbel Sep 24, 2024

Kikkon Sep 29, 2024

alamb commented Oct 1, 2024

Add GenericListViewArray for ListView & LargeListView #5723

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

alamb commented Oct 1, 2024

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