Support list_view_array & large_list_view layout and basic construction #5576

Kikkon · 2024-03-31T13:44:24Z

Which issue does this PR close?

Closes #5501 .

Rationale for this change

What changes are included in this PR?

Add the basic implementation of ListView and LargeListView, as well as unit testing.

Are there any user-facing changes?

tustvold · 2024-04-03T08:38:35Z

arrow-buffer/src/buffer/size.rs

+use std::ops::Deref;
+
+#[derive(Debug, Clone)]
+pub struct SizeBuffer<O: ArrowNativeType>(ScalarBuffer<O>);


I'm not sure this really adds anything over ScalarBuffer?

arrow-array/src/array/list_view_array.rs

tustvold · 2024-04-03T08:42:35Z

arrow-array/src/array/list_view_array.rs

+        let value_offsets = unsafe { get_offsets(&data) };
+        let value_sizes = unsafe { get_sizes(&data) };


This is unsound as the OffsetBuffer constraints are not applicable to view types

tustvold · 2024-04-03T08:43:50Z

arrow-array/src/array/list_view_array.rs

+        assert_eq!(list_array.len(), 0)
+    }
+
+    #[test]


Could we possibly use the constructors in these tests instead of ArrayData, this should be much easier to follow and maintain

tustvold · 2024-04-03T08:48:40Z

arrow-array/src/array/list_view_array.rs

+        DataType::ListView
+    };
+
+    /// Returns the data type of the list view array


This comment appears to be incorrect

tustvold

Thank you for this, I only took a quick look as I am very far behind on reviews from being away and then sick, but this mostly looks good. I think the major changes needed are to remove the uses of OffsetBuffer and probably remove SizeBuffer as well, and just use ScalarBuffer.

arrow-array/src/array/list_view_array.rs

Kikkon · 2024-04-04T11:44:25Z

@tustvold 🚀 Thank you very much for your review, I will make the modifications based on the comments later this week.

Kikkon · 2024-04-10T12:38:58Z

Thank you for this, I only took a quick look as I am very far behind on reviews from being away and then sick, but this mostly looks good. I think the major changes needed are to remove the uses of OffsetBuffer and probably remove SizeBuffer as well, and just use ScalarBuffer.

@tustvold If you have some free time, perhaps you could help review it again. 😄

tustvold · 2024-04-10T12:48:03Z

It is on my list but I probably won't get to it until Friday

tustvold

Sorry for the slow review, I am a little concerned that this PR is very large and appears to copy-pasta a lot of the ListArray code incorrectly.

Perhaps we might split this up into smaller pieces, something like:

Add basic ArrayData construction and validation
Add ArrayData equality
Add GenericListViewArray
Add GenericListViewBuilder

As it stands it is quite hard to guage what is tested and what isn't, and there are definitely a number of code paths that are simply incorrect

arrow-array/src/array/list_view_array.rs

tustvold · 2024-04-15T10:11:04Z

arrow-array/src/array/list_view_array.rs

+                )));
+            }
+        }
+        if len != sizes.len() {


I think this test should be moved ahead of the iteration above

tustvold · 2024-04-15T10:12:57Z

arrow-array/src/array/list_view_array.rs

+    /// Returns ith value of this list view array.
+    /// # Safety
+    /// Caller must ensure that the index is within the array bounds
+    pub unsafe fn value_unchecked(&self, i: usize) -> ArrayRef {


This method does not appear to be correct (and could probably do with a test)

arrow-array/src/array/list_view_array.rs

tustvold · 2024-04-15T10:14:24Z

arrow-array/src/array/list_view_array.rs

+    }
+
+    fn is_empty(&self) -> bool {
+        self.value_offsets.len() <= 1


This does not appear to be correct

tustvold · 2024-04-15T10:22:46Z

arrow-array/src/builder/generic_list_view_builder.rs

+    ///
+    /// Panics if the length of [`Self::values`] exceeds `OffsetSize::MAX`
+    #[inline]
+    pub fn append(&mut self, is_valid: bool, size: usize) {


The size should be inferred from the length of the values_builder, much like it is for GenericListBuilder

tustvold · 2024-04-15T10:25:13Z

arrow-buffer/src/buffer/scalar.rs

+    /// Create a new [`ScalarBuffer`] containing a single 0 value
+    pub fn new_empty() -> Self {
+        let buffer = MutableBuffer::from_len_zeroed(std::mem::size_of::<T>());
+        Self::from(buffer.into_buffer())
+    }


This method is not necessary, and is incorrect as it creates a ScalarBuffer that isn't empty

tustvold · 2024-04-15T10:26:11Z

arrow-data/src/data.rs

+    /// Returns a reference to the data in `buffer` as a typed slice
+    /// after validating. The returned slice is guaranteed to have at
+    /// least `len` entries.
+    fn typed_sizes<T: ArrowNativeType + num::Num>(&self) -> Result<&[T], ArrowError> {


This method isn't necessary

tustvold · 2024-04-15T10:26:39Z

arrow-data/src/data.rs

@@ -937,11 +972,23 @@ impl ArrayData {
                self.validate_offsets::<i32>(values_data.len)?;
                Ok(())
            }
+            DataType::ListView(field) => {
+                let values_data = self.get_single_valid_child_data(field.data_type())?;
+                self.validate_offsets::<i32>(values_data.len)?;


This validates the offsets w.r.t to the ListArray semantics, this is incorrect

tustvold · 2024-04-15T10:27:25Z

arrow-data/src/data.rs

@@ -929,6 +944,26 @@ impl ArrayData {
        Ok(())
    }

+    fn validate_sizes<T: ArrowNativeType + num::Num + std::fmt::Display>(


This appears to be incorrect

Kikkon · 2024-04-15T10:40:12Z

Sorry for the slow review, I am a little concerned that this PR is very large and appears to copy-pasta a lot of the ListArray code incorrectly.

Perhaps we might split this up into smaller pieces, something like:

Add basic ArrayData construction and validation

Add ArrayData equality

Add GenericListViewArray

Add GenericListViewBuilder

As it stands it is quite hard to guage what is tested and what isn't, and there are definitely a number of code paths that are simply incorrect

@tustvold Sounds good. I'll split it into several smaller pull requests separately. I'll ping you again once I'm ready.

feat: support live_view_array layout and basic construction

b9ded5b

github-actions bot added the arrow Changes to the arrow crate label Mar 31, 2024

Kikkon changed the title ~~Support live_view_array layout and basic construction~~ Support live_view_array & large_list_view layout and basic construction Mar 31, 2024

Kikkon force-pushed the feat/impl_list_view branch from 095a678 to 677bd2e Compare March 31, 2024 14:33

chore: cargo clippy & fmt

d8aba1b

Kikkon force-pushed the feat/impl_list_view branch from ad1c69b to d8aba1b Compare March 31, 2024 14:55

Kikkon mentioned this pull request Mar 31, 2024

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

Open

Kikkon marked this pull request as ready for review April 1, 2024 13:20

alamb mentioned this pull request Apr 1, 2024

DataFusion weekly project plan (Andrew Lamb) - April 1, 2024 apache/datafusion#9899

Closed

7 tasks

tustvold reviewed Apr 3, 2024

View reviewed changes

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

tustvold reviewed Apr 3, 2024

View reviewed changes

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

tustvold changed the title ~~Support live_view_array & large_list_view layout and basic construction~~ Support list_view_array & large_list_view layout and basic construction Apr 3, 2024

fix: remove offsetbuffer

955eb2b

Kikkon force-pushed the feat/impl_list_view branch from e26ac34 to 955eb2b Compare April 6, 2024 13:56

fix: doc

af38d14

Kikkon requested a review from tustvold April 6, 2024 15:05

tustvold reviewed Apr 15, 2024

View reviewed changes

Kikkon marked this pull request as draft April 15, 2024 11:03

chore: remove unused code

82b6095

Kikkon closed this Apr 18, 2024

Kikkon deleted the feat/impl_list_view branch April 18, 2024 12:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support list_view_array & large_list_view layout and basic construction #5576

Support list_view_array & large_list_view layout and basic construction #5576

Kikkon commented Mar 31, 2024

tustvold Apr 3, 2024

tustvold Apr 3, 2024

tustvold Apr 3, 2024

tustvold Apr 3, 2024

tustvold left a comment •

edited

Loading

Kikkon commented Apr 4, 2024

Kikkon commented Apr 10, 2024 •

edited

Loading

tustvold commented Apr 10, 2024

tustvold left a comment

tustvold Apr 15, 2024

tustvold Apr 15, 2024

tustvold Apr 15, 2024

tustvold Apr 15, 2024

tustvold Apr 15, 2024

tustvold Apr 15, 2024

tustvold Apr 15, 2024

tustvold Apr 15, 2024

Kikkon commented Apr 15, 2024

		let value_offsets = unsafe { get_offsets(&data) };
		let value_sizes = unsafe { get_sizes(&data) };

Support list_view_array & large_list_view layout and basic construction #5576

Support list_view_array & large_list_view layout and basic construction #5576

Conversation

Kikkon commented Mar 31, 2024

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

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

tustvold left a comment • edited Loading

Choose a reason for hiding this comment

Kikkon commented Apr 4, 2024

Kikkon commented Apr 10, 2024 • edited Loading

tustvold commented Apr 10, 2024

tustvold 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

Kikkon commented Apr 15, 2024

tustvold left a comment •

edited

Loading

Kikkon commented Apr 10, 2024 •

edited

Loading