ArrayData Enumeration for Primitive, Binary and UTF8 #3749

tustvold · 2023-02-22T17:58:50Z

Which issue does this PR close?

Part of #1799
Relates to #1176
Closes #1802

Rationale for this change

This starts the process of fleshing out what the elements of the ArrayData enumeration will consist of, with implementations for primitive and byte arrays. I wanted to get something up for review to get feedback on the general direction, and to also avoid a single massive PR.

What changes are included in this PR?

This PR adds enumerations ArrayDataPrimitive and ArrayDataBytes, that consist of the enumerable variants of the generic PrimitiveArrayData and BytesArrayData.

These provide for an idiomatic way to enumerate the possible array types, with the view that this would eventually replace the current trait object approach.

Additionally downcast / downcast_ref / From are provided to facilitate going from the enumeration to/from the generic form. This is critical to allowing types such as PrimitiveArray to be wrappers around PrimitiveArrayData whilst being constructable from an ArrayData enumeration containing ArrayDataPrimitive.

I anticipate functionality gradually being moved onto these ArrayData abstractions, to allow code-sharing with arrow2, and to reduce monomorphisation overheads for PrimitiveArray, but I've purposefully kept them simple for now

Are there any user-facing changes?

tustvold · 2023-02-22T18:00:34Z

arrow-buffer/src/buffer/boolean.rs

This closes #1802 as discussed in #3700 this needs to be a new type to avoid breaking existing workloads

tustvold · 2023-02-22T18:02:01Z

arrow-data/src/data/bytes.rs

This two-level formulation is somewhat arcane, but allows BytesArrayData to be typed solely on actual physical types, without needing a logical ByteArrayType.

tustvold · 2023-02-22T18:02:34Z

arrow-data/src/data/types.rs

This mirrors what the eventual ArrayData enumeration will look like, with each of these variants corresponding to an ArrayData variant

Is this only used for validating physical type when constructing typed ArrayData like PrimitiveArrayData.new does?

I anticipate it being used in other safe array data constructors, but yes this is the only current use-case

viirya · 2023-02-23T22:34:19Z

arrow-data/src/data/bytes.rs

+
+        /// Cast [`BytesArrayData`] to [`ArrayDataBytesOffset`]
+        fn upcast<B: Bytes + ?Sized>(
+            v: BytesArrayData<Self, B>,


What difference between ArrayDataBytes and BytesArrayData? Seems ArrayDataBytes is the data of bytes types (binary and str). But BytesArrayData?

I got it after reading the enums and struct BytesArrayData below. Maybe a descriptive comment at the top helpful.

arrow-data/src/data/primitive.rs

arrow-data/src/data/bytes.rs

viirya · 2023-02-27T07:47:08Z

arrow-data/src/data/primitive.rs

+pub trait Primitive: private::PrimitiveSealed + ArrowNativeType {
+    const VARIANT: PrimitiveType;
+}


Seems this trait and the const need document.

I will add some high-level documentation of the ArrayData "pattern" when I switch ArrayData over (and make these public)

viirya · 2023-02-27T07:49:11Z

arrow-data/src/data/primitive.rs

+        assert!(
+            matches!(physical, PhysicalType::Primitive(p) if p == T::VARIANT),
+            "Illegal physical type for PrimitiveArrayData, expected {:?} got {:?}",
+            T::VARIANT,
+            physical
+        );


Suggested change

assert!(

matches!(physical, PhysicalType::Primitive(p) if p == T::VARIANT),

"Illegal physical type for PrimitiveArrayData, expected {:?} got {:?}",

T::VARIANT,

physical

);

assert!(

matches!(physical, PhysicalType::Primitive(p) if p == T::VARIANT),

"Illegal physical type for PrimitiveArrayData of datatype {:?}, expected {:?} got {:?}",

data_type,

T::VARIANT,

physical

);

viirya · 2023-02-27T07:51:18Z

arrow-data/src/data/types.rs

+pub enum OffsetType {
+    Small,
+    Large,
+}


Hmm, as this is for physical type, should it be something like Int32, Int64?

viirya · 2023-02-27T07:53:00Z

arrow-data/src/data/types.rs

+            DataType::Binary => Self::Bytes(OffsetType::Small, BytesType::Binary),
+            DataType::FixedSizeBinary(_) => Self::FixedSizeBinary,
+            DataType::LargeBinary => Self::Bytes(OffsetType::Large, BytesType::Binary),


Just to keep variable length binary close.

Suggested change

DataType::Binary => Self::Bytes(OffsetType::Small, BytesType::Binary),

DataType::FixedSizeBinary(_) => Self::FixedSizeBinary,

DataType::LargeBinary => Self::Bytes(OffsetType::Large, BytesType::Binary),

DataType::Binary => Self::Bytes(OffsetType::Small, BytesType::Binary),

DataType::LargeBinary => Self::Bytes(OffsetType::Large, BytesType::Binary),

DataType::FixedSizeBinary(_) => Self::FixedSizeBinary,

viirya

This looks like a good start.

ursabot · 2023-02-27T14:11:51Z

Benchmark runs are scheduled for baseline = 96791ea and contender = dae7a71. dae7a71 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 Feb 22, 2023

tustvold changed the title ~~Array data struct~~ ArrayData Enumeration for Primitive, Binary and UTF8 Feb 22, 2023

tustvold commented Feb 22, 2023

View reviewed changes

tustvold added 11 commits February 23, 2023 09:58

Add BooleanBuffer

bab9d22

Add NullBuffer

332931e

Add PrimitiveArrayData

b103522

Add BytesArrayData

6ae6b84

Move module

bd93c15

Make private for now

76f55ab

Move NullBuffer to arrow-buffer

a7cb0b0

Format

e144dc6

More docs

7653fd6

Seal traits

68f27f5

Doc

c1527b5

tustvold force-pushed the array-data-struct branch from 07c79dc to c1527b5 Compare February 23, 2023 09:59

tustvold marked this pull request as ready for review February 23, 2023 09:59

tustvold mentioned this pull request Feb 23, 2023

Discussion: relationship / unification of arrow-rs and arrow2 going forward #1176

Closed

viirya reviewed Feb 23, 2023

View reviewed changes

viirya reviewed Feb 24, 2023

View reviewed changes

arrow-data/src/data/primitive.rs Show resolved Hide resolved

viirya mentioned this pull request Feb 27, 2023

Sliceable BitMap #3700

Closed

viirya reviewed Feb 27, 2023

View reviewed changes

arrow-data/src/data/bytes.rs Show resolved Hide resolved

viirya reviewed Feb 27, 2023

View reviewed changes

arrow-data/src/data/bytes.rs Show resolved Hide resolved

viirya reviewed Feb 27, 2023

View reviewed changes

viirya approved these changes Feb 27, 2023

View reviewed changes

Review feedback

85cc99e

tustvold merged commit dae7a71 into apache:master Feb 27, 2023

This was referenced Feb 27, 2023

ArrayData Enumeration for Remaining Layouts #3769

Merged

Use NullBuffer in ArrayData #3775

Closed

ArrayData Enumeration for Primitive, Binary and UTF8 #3749

ArrayData Enumeration for Primitive, Binary and UTF8 #3749

Uh oh!

Conversation

tustvold commented Feb 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

tustvold Feb 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Uh oh!

Uh oh!

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!

viirya left a comment

Choose a reason for hiding this comment

Uh oh!

ursabot commented Feb 27, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tustvold commented Feb 22, 2023 •

edited

Loading

tustvold Feb 22, 2023 •

edited

Loading