Remove ArrayData from Array (#3880) by tustvold · Pull Request #4061 · apache/arrow-rs

tustvold · 2023-04-12T11:08:57Z

Which issue does this PR close?

Closes #3880

Rationale for this change

See ticket

What changes are included in this PR?

Are there any user-facing changes?

Yes, this inherently changes the way that slicing behaves

It also adds a statically typed BooleanArray::slice that somehow was missed in #3930

tustvold · 2023-04-12T11:13:03Z

arrow-ipc/src/writer.rs

    }
 }

-/// Returns byte width for binary value_offset buffer spec.


This logic had to be reworked to not rely on a non-zero ArrayData::offset to trigger the re-encoding logic

tustvold · 2023-04-12T11:18:54Z

arrow-array/src/array/list_array.rs


        for i in 0..sliced_array.len() {
-            if bit_util::get_bit(&null_bits, sliced_array.offset() + i) {
+            if bit_util::get_bit(&null_bits, 1 + i) {


This change is necessary because the array no longer "keeps" the offset

As above I think maybe it would make sense to at least deprecate Array::offset as it is always 0 now (and as part of the deprecation message explain why)

tustvold · 2023-04-12T11:19:16Z

arrow-array/src/array/mod.rs

    ///
    /// ```
-    /// use arrow_array::{Array, Int32Array};
+    /// use arrow_array::{Array, BooleanArray};


Switch to BooleanArray which still "preserves" the offset

I think we should deprecate this API for the reasons explained abve

tustvold · 2023-04-12T11:21:42Z

arrow-array/src/array/fixed_size_list_array.rs

-    #[should_panic(
-        expected = "FixedSizeListArray child array length should be a multiple of 3"
-    )]
+    #[should_panic(expected = "assertion failed: (offset + length) <= self.len()")]


Given this is a sanity check, and implies an invalid ArrayData, I'm not sure the complexity of having a nicer error message is worth it

tustvold · 2023-04-12T11:21:59Z

arrow-array/src/array/fixed_size_list_array.rs

-                if *len > 0 {
-                    // check that child data is multiple of length
-                    assert_eq!(
-                        values.len() % *len as usize,


This test is redundant with the slice below

tustvold · 2023-04-12T11:22:18Z

arrow-array/src/array/fixed_size_list_array.rs

-        assert_eq!(
-            data.buffers().len(),
-            0,
-            "FixedSizeListArray data should not contain a buffer for value offsets"
-        );
-        assert_eq!(
-            data.child_data().len(),
-            1,
-            "FixedSizeListArray should contain a single child array (values array)"
-        );


These tests are redundant as they would imply an invalid ArrayData

tustvold · 2023-04-12T11:22:41Z

arrow-array/src/array/fixed_size_binary_array.rs

        );
        assert_eq!(2, fixed_size_binary_array.len());
-        assert_eq!(5, fixed_size_binary_array.value_offset(0));
+        assert_eq!(0, fixed_size_binary_array.value_offset(0));


The slice pushdown results in this changing

alamb

Very impressive @tustvold -- some said it couldn't be done but seeing your string of incremental PRs over the last few months to remove ArrayData with the strongly typed abstractions has been inspirational

I know it seemingly took longer than it might have without the constraints of existing users, I think this change will be that much more valuable over the long term.

So thank you again

alamb · 2023-04-12T17:19:06Z

arrow-array/src/array/boolean_array.rs

+        self.values.is_empty()
+    }
+
+    /// Returns a zero-copy slice of this array with the indicated offset and length.


alamb · 2023-04-12T17:22:12Z

arrow-array/src/array/dictionary_array.rs

+        Self {
+            data_type: self.data_type.clone(),
+            keys: self.keys.slice(offset, length),
+            values: self.values.clone(),


the values aren't sliced because the keys may still refer to any logical value in the dictionary

alamb · 2023-04-12T17:23:01Z

arrow-array/src/array/dictionary_array.rs

    }
+
+    fn get_buffer_memory_size(&self) -> usize {
+        self.dictionary.get_buffer_memory_size()


should this function and the one below it also account for the self.values?

self.values will get counted by self.dictionary, self.values is just a downcast borrow of the inner array

alamb · 2023-04-12T17:24:22Z

arrow-array/src/array/fixed_size_binary_array.rs

    /// All elements have the same length as the array is a fixed size.
    #[inline]
    pub fn value_length(&self) -> i32 {
-        self.length


this is a bug, right?

I renamed length to value_length to avoid confusion with len

alamb · 2023-04-12T17:27:22Z

arrow-array/src/array/fixed_size_list_array.rs

+        self.len == 0
+    }
+
+    fn offset(&self) -> usize {


what is the meaning of offset in this new world where the offset is stored on the underlying typed array? Maybe we should remove it from the Array trait 🤔

Yeah, it's certainly doesn't have clear semantics anymore. As this PR is already quite large I'll remove it in a follow on PR

alamb · 2023-04-12T17:29:19Z

arrow-array/src/array/list_array.rs


        for i in 0..sliced_array.len() {
-            if bit_util::get_bit(&null_bits, sliced_array.offset() + i) {
+            if bit_util::get_bit(&null_bits, 1 + i) {


As above I think maybe it would make sense to at least deprecate Array::offset as it is always 0 now (and as part of the deprecation message explain why)

alamb · 2023-04-12T17:30:31Z

arrow-array/src/array/map_array.rs

-    /// Returns the total number of bytes of memory occupied by the buffers owned by this [MapArray].
    fn get_buffer_memory_size(&self) -> usize {
-        self.data.get_buffer_memory_size()
+        let mut size = self.entries.get_buffer_memory_size();


What about the other fields like keys, values, etc?

keys and values are stored within entries, they're the children of the StructArray. Perhaps it would be worth making entries StructArray 🤔

alamb · 2023-04-12T17:31:08Z

arrow-array/src/array/mod.rs

    ///
    /// ```
-    /// use arrow_array::{Array, Int32Array};
+    /// use arrow_array::{Array, BooleanArray};


I think we should deprecate this API for the reasons explained abve

alamb · 2023-04-12T17:42:09Z

arrow-array/src/array/union_array.rs

-        // TODO: Slice buffers directly (#3880)
-        self.data.slice(offset, length).into()
+        let (offsets, fields) = match self.offsets.as_ref() {
+            Some(offsets) => (Some(offsets.slice(offset, length)), self.fields.clone()),


I was confused by this at first -- some comments here (and/or in the self.offsets I think would help

Specifically I think if self.offsets is SOme(..) that means DenseUnion and the offsets point to the correct children so slicing the offsets but not the actual children makes sense

if self.offsets is None, then it is a sparse union, so all the children line up and we need to slice them individually.

I had to re-read https://arrow.apache.org/docs/format/Columnar.html#union-layout to make sure

alamb · 2023-04-12T17:46:17Z

arrow-ipc/src/writer.rs

-        _ => unreachable!(),
-    }
-}
+    let offsets = match start_offset.as_usize() {


could you please add some comments about what this is doing (namely that it is "rebasing" the offsets to all be zero based)? That would be easy to overlook and was somewhat encoded in the method names previously

alamb · 2023-04-12T17:48:42Z

As part of the completion of this project, @tustvold could you write up some description of the relationship between Buffer / PrimitveBuffer / ArrayData, etc?

Perhaps we can take inspiration (or copy/modify) the guide from:
https://jorgecarleitao.github.io/arrow2/main/guide/

If you think it makes sense I will file a ticket

tustvold · 2023-04-12T18:00:57Z

If you think it makes sense I will file a ticket

I think this is definitely valuable, and will mesh nicely with #3879

alamb · 2023-04-12T19:21:34Z

If you think it makes sense I will file a ticket

Filed #4071

Remove ArrayData from Array (apache#3880)

c2c61cf

github-actions bot added the arrow Changes to the arrow crate label Apr 12, 2023

tustvold commented Apr 12, 2023

View reviewed changes

Fix doc

d7dd502

tustvold commented Apr 12, 2023

View reviewed changes

tustvold added the api-change Changes to the arrow API label Apr 12, 2023

Fix pyarrow-integration-testing

dd7dc10

This was referenced Apr 12, 2023

Add StructArray Constructors (#3879) #4064

Merged

Add ListArray Constructors (#3879) #4065

Merged

Add PrimitiveArray::try_new (#3879) #4067

Merged

Add DictionaryArray Constructors (#3879) #4068

Merged

alamb approved these changes Apr 12, 2023

View reviewed changes

Review feedback

d8b1c6b

alamb mentioned this pull request Apr 12, 2023

Document arrow-rs architecture and structure #4071

Open

tustvold merged commit efd8b92 into apache:master Apr 12, 2023

tustvold mentioned this pull request Apr 13, 2023

Store StructArray entries in MapArray #4085

Merged

This was referenced Apr 25, 2023

Update to arrow 38 apache/datafusion#6115

Merged

Inconsistent Slicing of ArrayData for StructArray #1750

Closed

This was referenced May 9, 2023

Add Sliced ListArray test (#3748) #4186

Merged

Support Sliced Complex Types In IPC Writer #2080

Closed

Update arrow rustdocs (#4071) #4197

Merged

tustvold mentioned this pull request Jun 1, 2023

Slicing list<struct<...>> doesn't pass the list offsets to struct children #594

Closed

tustvold mentioned this pull request Aug 1, 2024

Optimize take kernel for BinaryViewArray and StringViewArray #6168

Merged

Comments

Conversation

tustvold commented Apr 12, 2023

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb commented Apr 12, 2023

Uh oh!

tustvold commented Apr 12, 2023

Uh oh!

alamb commented Apr 12, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants