Split out `arrow-data` into a separate crate #2746

tustvold · 2022-09-16T15:18:51Z

Which issue does this PR close?

Part of #2594

Rationale for this change

ArrayData represents the canonical representation of an arrow array, and is the language of the C Data interface. It therefore makes sense to split into its own crate. In theory this could form the basis for interoperability between different Rust array abstractions.

What changes are included in this PR?

Splits out ArrayData and related structures into a separate crate.

Are there any user-facing changes?

We can no longer implement From<ArrayRef> for ArrayData. In practice this only appears to be being used by pyarrow code, and can be replaced with calls to make_array or Array::data() as appropriate.

tustvold · 2022-09-16T15:20:09Z

arrow/src/array/array_decimal.rs

@@ -284,7 +284,11 @@ impl<T: DecimalType> DecimalArray<T> {

        // safety: self.data is valid DataType::Decimal as checked above
        let new_data_type = Self::TYPE_CONSTRUCTOR(precision, scale);
-        Ok(self.data().clone().with_data_type(new_data_type).into())
+        let data = self.data().clone().into_builder().data_type(new_data_type);


This method was previously pub(crate), I opted to just remove it in favour of the into_builder plumbing

cc @liukun4515 -- I think it is fine

tustvold · 2022-09-16T15:22:44Z

arrow-data/src/bitmap.rs

@@ -56,6 +56,10 @@ impl Bitmap {
        unsafe { bit_util::get_bit_raw(self.bits.as_ptr(), i) }
    }

+    pub fn buffer(&self) -> &Buffer {


I intend to deprecate buffer_ref in a follow up PR

tustvold · 2022-09-21T13:08:25Z

arrow-data/src/transform/mod.rs

@@ -0,0 +1,672 @@
+// Licensed to the Apache Software Foundation (ASF) under one


This is moved from array/transform/mod.rs but with the tests split out into arrow/tests/array_transform.rs

I think moving out the tests is a great idea -- I wonder if we need to do anything more to make them discoverable 🤔

tustvold · 2022-09-21T13:09:40Z

arrow/src/array/array.rs

@@ -358,6 +358,90 @@ pub trait ArrayAccessor: Array {
    unsafe fn value_unchecked(&self, index: usize) -> Self::Item;
 }

+impl PartialEq for dyn Array {


These are moved from arrow/equal/mod.rs

tustvold · 2022-09-21T13:10:19Z

arrow/src/array/equal/mod.rs

@@ -1,1464 +0,0 @@
-// Licensed to the Apache Software Foundation (ASF) under one


This is moved to arrow-data/equal/mod.rs with the array equality moved to arrow/array/array.rs and the tests moved to arrow/tests/array_equal.rs

tustvold · 2022-09-21T13:11:01Z

arrow-data/src/data.rs

@@ -1771,1124 +1746,30 @@ mod tests {
    }

    #[test]
-    #[should_panic(


These tests are moved to arrow/tests/array_validation.rs

tustvold · 2022-09-21T13:14:16Z

arrow-data/src/equal/boolean.rs

@@ -88,30 +88,3 @@ pub(super) fn boolean_equal(
        })
    }
 }
-
-#[cfg(test)]


These tests are moved to arrow/tests/array_equal.rs

tustvold · 2022-09-21T13:14:35Z

arrow-data/src/equal/list.rs

@@ -149,52 +148,3 @@ pub(super) fn list_equal<T: OffsetSizeTrait>(
        })
    }
 }
-
-#[cfg(test)]


These tests are moved to arrow/tests/array_equal.rs

tustvold · 2022-09-21T13:14:47Z

arrow-data/src/equal/mod.rs

@@ -0,0 +1,171 @@
+// Licensed to the Apache Software Foundation (ASF) under one


This is moved from arrow/array/equal/mod.rs

alamb

I can't say I reviewed every single line in this PR super carefully, but I did read them all and looked at the changes, and verified the tests are still present.

I also like the way that several of the larger tests are moved into arrow/tests/... integration style tests. This is similar to what arrow2 does 👍 Nice

Do we think it is appropriate to run any benchmarks?

LGTM I say we ship it

alamb · 2022-09-21T18:19:05Z

arrow-data/src/transform/mod.rs

@@ -0,0 +1,672 @@
+// Licensed to the Apache Software Foundation (ASF) under one


I think moving out the tests is a great idea -- I wonder if we need to do anything more to make them discoverable 🤔

alamb · 2022-09-21T18:21:58Z

arrow-data/src/transform/variable_size.rs

    buffer: &mut MutableBuffer,
    offsets: &[T],
    values: &[u8],
    start: usize,
    len: usize,
 ) {
-    let start_values = offsets[start].to_usize().unwrap();
-    let end_values = offsets[start + len].to_usize().unwrap();
+    let start_values = offsets[start].as_();


TIL: https://docs.rs/num/latest/num/cast/trait.AsPrimitive.html#tymethod.as_

as_() is something I have not seen before. Fascinating.

alamb · 2022-09-21T18:23:16Z

arrow/src/array/array_decimal.rs

@@ -284,7 +284,11 @@ impl<T: DecimalType> DecimalArray<T> {

        // safety: self.data is valid DataType::Decimal as checked above
        let new_data_type = Self::TYPE_CONSTRUCTOR(precision, scale);
-        Ok(self.data().clone().with_data_type(new_data_type).into())
+        let data = self.data().clone().into_builder().data_type(new_data_type);


cc @liukun4515 -- I think it is fine

tustvold · 2022-09-22T09:35:54Z

This does not appear to have a discernible impact on the benchmarks - some fluctuate as is to be expected when you're talking nanoseconds, but no major change in either direction

ursabot · 2022-09-22T09:41:48Z

Benchmark runs are scheduled for baseline = 80c0f1a and contender = a927d7a. a927d7a is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on ec2-t3-xlarge-us-east-2] ec2-t3-xlarge-us-east-2
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on test-mac-arm] test-mac-arm
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on ursa-i9-9960x] ursa-i9-9960x
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on ursa-thinkcentre-m75q] ursa-thinkcentre-m75q
Buildkite builds:
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

github-actions bot added the arrow Changes to the arrow crate label Sep 16, 2022

tustvold commented Sep 16, 2022

View reviewed changes

Split out arrow-data

2680c16

tustvold force-pushed the split-out-arrow-data branch from c5b6ef5 to 2680c16 Compare September 21, 2022 12:43

RAT

58bf3de

tustvold commented Sep 21, 2022

View reviewed changes

tustvold marked this pull request as ready for review September 21, 2022 13:15

Fix integration test

79076e7

alamb changed the title ~~Split out arrow data~~ Split out arrow-data into a new crate Sep 21, 2022

alamb added the api-change Changes to the arrow API label Sep 21, 2022

alamb changed the title ~~Split out arrow-data into a new crate~~ Split out arrow-data into a separate crate Sep 21, 2022

alamb approved these changes Sep 21, 2022

View reviewed changes

tustvold merged commit a927d7a into apache:master Sep 22, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split out `arrow-data` into a separate crate #2746

Split out `arrow-data` into a separate crate #2746

tustvold commented Sep 16, 2022 •

edited

tustvold Sep 16, 2022

alamb Sep 21, 2022

tustvold Sep 16, 2022

tustvold Sep 21, 2022

alamb Sep 21, 2022

tustvold Sep 21, 2022

tustvold Sep 21, 2022

tustvold Sep 21, 2022

tustvold Sep 21, 2022

tustvold Sep 21, 2022

tustvold Sep 21, 2022

alamb left a comment

alamb Sep 21, 2022

alamb Sep 21, 2022

alamb Sep 21, 2022

tustvold commented Sep 22, 2022

ursabot commented Sep 22, 2022

		@@ -0,0 +1,672 @@
		// Licensed to the Apache Software Foundation (ASF) under one

		@@ -1,1464 +0,0 @@
		// Licensed to the Apache Software Foundation (ASF) under one

		@@ -0,0 +1,171 @@
		// Licensed to the Apache Software Foundation (ASF) under one

Split out arrow-data into a separate crate #2746

Split out arrow-data into a separate crate #2746

Conversation

tustvold commented Sep 16, 2022 • edited

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

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

alamb 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

tustvold commented Sep 22, 2022

ursabot commented Sep 22, 2022

Split out `arrow-data` into a separate crate #2746

Split out `arrow-data` into a separate crate #2746

tustvold commented Sep 16, 2022 •

edited