Add ArrayDataLayout, port validation (#1799) #3818

tustvold · 2023-03-08T14:50:26Z

Which issue does this PR close?

Closes #.

Rationale for this change

Switching to the ArrayData enumeration requires a substantial amount of glue logic to convert between the struct and enum layouts. This PR therefore adds

An ArrayDataLayout that can be used to get a struct view of the enum variants, for the purposes of validation, etc..
Methods to create the enum variants from ArrayDataBuilder
Methods to slice array data variants

What changes are included in this PR?

Are there any user-facing changes?

tustvold · 2023-03-08T20:07:46Z

arrow-buffer/src/buffer/offset.rs

+        &self.0
+    }
+
+    /// Returns the inner [`Buffer`]


Suggested change

/// Returns the inner [`Buffer`]

/// Returns the inner [`ScalarBuffer`]

arrow-buffer/src/buffer/offset.rs

viirya · 2023-03-08T21:53:46Z

arrow-data/src/data/boolean.rs

+    ///
+    /// Panics if
+    /// - `nulls` and `values` are different lengths
+    /// - `data_type` is not compatible with `T`


Not sure what is T here? Maybe?

Suggested change

/// - `data_type` is not compatible with `T`

/// - `data_type` is not compatible with `PhysicalType::Boolean`

viirya · 2023-03-08T22:38:56Z

arrow-data/src/data/boolean.rs

+use arrow_buffer::buffer::{BooleanBuffer, NullBuffer};
+use arrow_schema::DataType;
+
+#[derive(Debug, Clone)]


I think this is also good to add layout link? https://arrow.apache.org/docs/format/Columnar.html#fixed-size-primitive-layout?

askoa · 2023-03-08T23:33:16Z

arrow-data/src/data/run.rs

@@ -116,22 +170,67 @@ impl<E: RunEnd> RunArrayData<E> {
    /// # Safety
    ///
    /// - `data_type` must be valid for this layout
-    /// - `run_ends` must contain monotonically increasing, positive values `<= child.len()`
+    /// - `run_ends` must contain monotonically increasing, positive values `<= len`
+    /// - `run_ends.len() == child.len()`


Not clear what child refers to. The variable child in below function is built using scalar buffer and its length will not be equal to run_ends.len().
Also, there are two children. So it'll be clear to mention which child is being referred in the comment.

alamb

Thank you @tustvold -- I went through this PR carefully.

I think this PR is related to #1799. I think it is important to understand the context for this PR. Specifically, as I understand it the rationale for this work is to provide an easy and zero cost conversion between arrow-rs and arrow2 arrays (see jorgecarleitao/arrow2#1429 for more details) by unifying the lowest levels using the ArrayData enumeration

Am I right in concluding that extracting ArrayDataLayout allow the data validation logic to be used in the new structure?

While this does create non trivial tech-debt (as now there are multiple ways to get at the lowest level arrow data), I think with some more documentation and providing some good examples we'll be in good shape to migrate them slowly over.

I am not sure how important it is to leave this PR open for longer to ensure anyone who might have an opinion has a chance to comment -- When you are ready to switch to the new ArrayDataEnumeration that certainly should have some comment time.

cc @ritchie46 and @sundy-li

alamb · 2023-03-09T15:47:23Z

arrow-buffer/src/buffer/boolean.rs

@@ -139,4 +139,9 @@ impl BooleanBuffer {
    pub fn inner(&self) -> &Buffer {
        &self.buffer
    }
+
+    /// Returns the inner [`Buffer`]


Suggested change

/// Returns the inner [`Buffer`]

/// Returns the inner [`Buffer`], consuming self

alamb · 2023-03-09T15:47:35Z

arrow-buffer/src/buffer/offset.rs

+        &self.0
+    }
+
+    /// Returns the inner [`ScalarBuffer`]


Suggested change

/// Returns the inner [`ScalarBuffer`]

/// Returns the inner [`ScalarBuffer`], consuming self

arrow-buffer/src/buffer/scalar.rs

alamb · 2023-03-09T15:51:05Z

arrow-buffer/src/buffer/scalar.rs

+        &self.buffer
+    }
+
+    /// Returns the inner [`Buffer`]


Suggested change

/// Returns the inner [`Buffer`]

/// Returns the inner [`Buffer`], consuming self

alamb · 2023-03-09T15:56:53Z

arrow-data/src/data/bytes.rs

+            buffers: Buffers::two(self.offsets.inner().inner(), &self.values),
+            child_data: &[],
+        }
+    }
 }

 /// ArrayData for [fixed-size arrays](https://arrow.apache.org/docs/format/Columnar.html#fixed-size-primitive-layout) of bytes


is this the right link?

alamb · 2023-03-09T16:12:54Z

arrow-data/src/data/run.rs

+    /// The children of this RunArrayData
+    /// 1: the run ends
+    /// 2: the values
+    children: Box<[ArrayData; 2]>,


What is there a reason to store the run ends in children[0] as well as in run_ends? Aka why the redundancy.

So that we can return a slice of children in ArrayDataLayout, this type is a little funky because there is an impedence mismatch between the arrow representation and the arrow-rs representation

arrow-data/src/data/struct.rs

alamb · 2023-03-09T16:18:14Z

arrow-data/src/data/mod.rs

@@ -33,13 +33,17 @@ use crate::equal;
 mod buffers;
 pub use buffers::*;

+#[allow(unused)] // Private until ready (#1176)


I wonder if #1799 would be a better link?

#1176 was an old discussion ticket

arrow-data/src/data/mod.rs

tustvold · 2023-03-09T17:35:49Z

Am I right in concluding that extracting ArrayDataLayout allow the data validation logic to be used in the new structure?

Yup, it is the glue logic to avoid needing to change everything at once. Eventually it will be removed. It will never be made public.

I am not sure how important it is to leave this PR open for longer to ensure anyone who might have an opinion has a chance to comment

As this PR is almost entirely crate-private I intend to get this in, and we can continue to iterate on it

ursabot · 2023-03-09T18:17:16Z

Benchmark runs are scheduled for baseline = 2a3fd96 and contender = 495682a. 495682a 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 Mar 8, 2023

Add ArrayDataLayout (apache#1799)

578d8a4

tustvold force-pushed the array-data-layout branch from f4baea8 to 578d8a4 Compare March 8, 2023 20:06

tustvold marked this pull request as ready for review March 8, 2023 20:06

tustvold commented Mar 8, 2023

View reviewed changes

tustvold added 3 commits March 8, 2023 20:39

Fix ArrayData::buffer

8ff33f1

Don't export macros, yet

5ac61c3

Fix doc

2161c88

viirya reviewed Mar 8, 2023

View reviewed changes

askoa reviewed Mar 8, 2023

View reviewed changes

alamb changed the title ~~Add ArrayDataLayout (#1799)~~ Add ArrayDataLayout, port validation (#1799) Mar 9, 2023

alamb approved these changes Mar 9, 2023

View reviewed changes

tustvold added 2 commits March 9, 2023 17:18

Review feedback

620e1b8

Further review feedback

8c9cbf1

tustvold merged commit 495682a into apache:master Mar 9, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ArrayDataLayout, port validation (#1799) #3818

Add ArrayDataLayout, port validation (#1799) #3818

tustvold commented Mar 8, 2023 •

edited

Loading

tustvold Mar 8, 2023

viirya Mar 8, 2023

viirya Mar 8, 2023

askoa Mar 8, 2023

alamb left a comment

alamb Mar 9, 2023

alamb Mar 9, 2023

alamb Mar 9, 2023

alamb Mar 9, 2023

alamb Mar 9, 2023

tustvold Mar 9, 2023

alamb Mar 9, 2023

tustvold commented Mar 9, 2023

ursabot commented Mar 9, 2023

	/// Returns the inner [`Buffer`]
	/// Returns the inner [`ScalarBuffer`]

	/// - `data_type` is not compatible with `T`
	/// - `data_type` is not compatible with `PhysicalType::Boolean`

	/// Returns the inner [`Buffer`]
	/// Returns the inner [`Buffer`], consuming self

	/// Returns the inner [`ScalarBuffer`]
	/// Returns the inner [`ScalarBuffer`], consuming self

Add ArrayDataLayout, port validation (#1799) #3818

Add ArrayDataLayout, port validation (#1799) #3818

Conversation

tustvold commented Mar 8, 2023 • edited Loading

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

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

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 Mar 9, 2023

ursabot commented Mar 9, 2023

tustvold commented Mar 8, 2023 •

edited

Loading