Add API for Creating Variant Values #7452

PinkCrow007 · 2025-04-29T00:10:14Z

Which issue does this PR close?

Variant: Rust API to Create Variant Values #7424

Rationale for this change

This PR implements a builder-style API for creating Variant values in Rust, following the Variant binary encoding specification. It supports reusing metadata within a single builder session, as discussed in the issue.

What changes are included in this PR?

Add Variants as a canonical Arrow Extension Type (built on top of Struct type).
Introduce VariantBuilder, ObjectBuilder, and ArrayBuilder for constructing Variant values.

Are there any user-facing changes?

This PR adds a new public API for programmatically creating Variant-encoded values.
No breaking changes.

CC: @alamb for visibility

alamb · 2025-04-30T01:44:28Z

Thank you so much for this @PinkCrow007 -- I have seen it and plan to review it, but may not have a chance for another day or two. So exciting!@

Weijun-H

Impressive work! Thanks @PinkCrow007 👍

arrow-variant/src/encoder/mod.rs

Weijun-H · 2025-04-30T05:28:32Z

arrow-variant/src/encoder/mod.rs

+/// Encodes a date value (days since epoch)
+pub fn encode_date(value: i32, output: &mut Vec<u8>) {
+    // Use primitive + date type
+    let header = primitive_header(VariantPrimitiveType::Date as u8);
+    output.push(header);
+    output.extend_from_slice(&value.to_le_bytes());
+}
+
+/// Encodes a timestamp value (milliseconds since epoch)
+pub fn encode_timestamp(value: i64, output: &mut Vec<u8>) {
+    // Use primitive + timestamp type
+    let header = primitive_header(VariantPrimitiveType::Timestamp as u8);
+    output.push(header);
+    output.extend_from_slice(&value.to_le_bytes());
+}
+
+/// Encodes a timestamp without timezone value (milliseconds since epoch)
+pub fn encode_timestamp_ntz(value: i64, output: &mut Vec<u8>) {
+    // Use primitive + timestamp_ntz type
+    let header = primitive_header(VariantPrimitiveType::TimestampNTZ as u8);
+    output.push(header);
+    output.extend_from_slice(&value.to_le_bytes());
+}
+
+/// Encodes a time without timezone value (milliseconds)
+pub fn encode_time_ntz(value: i64, output: &mut Vec<u8>) {
+    // Use primitive + time_ntz type
+    let header = primitive_header(VariantPrimitiveType::TimeNTZ as u8);
+    output.push(header);
+    output.extend_from_slice(&value.to_le_bytes());
+}


These functions are quite similar. To reduce duplication, could we create a more general encoder function, like encode_general(type_id: VariantPrimitiveType, value: i64, output: &mut Vec<u8>)? Or create a encoder trait for VariantPrimitiveType

That is a good point

I also think they don't need to be pub (maybe we could start with pub(crate)) as the main API people would use is the builder I think

Thanks @Weijun-H and @alamb ! I've refactored the primitive encoding using an Encoder trait for VariantPrimitiveType and set visibility to pub(crate). How does this overall design look to you now?

Thanks @PinkCrow007 -- will check it out tomorrow

arrow-variant/src/encoder/mod.rs

arrow-schema/src/extension/canonical/variant.rs

alamb

Thank you @PinkCrow007 -- this is very very nice work and I think it will form the basis of a great API to create variant values

I had some structural suggestions which I left, but the biggest suggestion is that I think the wonderful tests you have written would be improved significantly if we can change them to read back the Variant values that were written in the builders

Here is what I would like to propose to move forward:

I'll make a new PR with the scaffolding for a parquet-variant crate
I'll port some of the code for reading variant values there

Then I think we could add the builder code and tests you have in this PR and add them to the other crate.

But really this is great work

arrow-variant/src/builder/mod.rs

alamb · 2025-05-02T16:12:34Z

arrow-schema/src/error.rs

@@ -60,6 +60,8 @@ pub enum ArrowError {
    DictionaryKeyOverflowError,
    /// Error when the run end index in a REE array is bigger than the array length
    RunEndIndexOverflowError,
+    /// Error during Variant operations in `arrow-variant`.


If we add a new variant to this enum, it will be a "breaking API change" as then downstream projects would potentially have to update their code to handle new variants

We make releases with API changes every three months,
https://github.com/apache/arrow-rs?tab=readme-ov-file#release-versioning-and-schedule

So in other words, it would be great to remove this change from the PR so we can merge it faster.

alamb · 2025-05-02T16:14:06Z

arrow-schema/src/extension/canonical/mod.rs

@@ -37,6 +37,8 @@ mod uuid;
 pub use uuid::Uuid;
 mod variable_shape_tensor;
 pub use variable_shape_tensor::{VariableShapeTensor, VariableShapeTensorMetadata};
+mod variant;


I recommend we postpone adding the canonical extension type classes until we get farther along in the process and are in a better position to write tests.

In other words I recommend removing the changes in arrow-schema/src/extension/ as well in this pR

arrow-variant/Cargo.toml

alamb · 2025-05-02T16:22:21Z

arrow-variant/Cargo.toml

+arrow-array = { workspace = true }
+arrow-buffer = { workspace = true }
+arrow-cast = { workspace = true, optional = true }
+arrow-data = { workspace = true }
+arrow-schema = { workspace = true, features = ["canonical_extension_types"] }
+serde = { version = "1.0", default-features = false }
+serde_json = { version = "1.0", default-features = false, features = ["std"] }
+indexmap = "2.0.0"


I don't think any of these dependencies are used so we can remove them

arrow-array = { workspace = true } arrow-buffer = { workspace = true } arrow-cast = { workspace = true, optional = true } arrow-data = { workspace = true } serde = { version = "1.0", default-features = false } serde_json = { version = "1.0", default-features = false, features = ["std"] }

alamb · 2025-05-02T16:34:09Z

arrow-variant/src/builder/mod.rs

+        // Verify metadata contains all keys
+        let keys = get_metadata_keys(&metadata_buffer);
+        assert_eq!(keys.len(), 11, "Should have 11 keys in metadata");
+        assert!(keys.contains(&"null".to_string()), "Missing 'null' key");
+        assert!(
+            keys.contains(&"bool_true".to_string()),
+            "Missing 'bool_true' key"
+        );
+        assert!(keys.contains(&"string".to_string()), "Missing 'string' key");
+
+        // Verify object has the correct number of entries
+        // First byte after header is the number of fields (if small object)
+        assert!(value_buffer.len() > 1, "Value buffer too small");
+        let num_fields = value_buffer[1];
+        assert_eq!(num_fields as usize, 11, "Object should have 11 fields");


Rather than testing these "internal" fields I think the tests would be better if they tested that the resulting value is a readable Variant value. See my next comment below

alamb · 2025-05-02T16:34:38Z

arrow-variant/src/builder/mod.rs

+        assert!(!variant.value().is_empty());
+    }
+
+    // =========================================================================


this is a very impressive set of test cases 👌

alamb · 2025-05-02T16:39:00Z

arrow-variant/src/encoder/mod.rs

+/// Encodes a date value (days since epoch)
+pub fn encode_date(value: i32, output: &mut Vec<u8>) {
+    // Use primitive + date type
+    let header = primitive_header(VariantPrimitiveType::Date as u8);
+    output.push(header);
+    output.extend_from_slice(&value.to_le_bytes());
+}
+
+/// Encodes a timestamp value (milliseconds since epoch)
+pub fn encode_timestamp(value: i64, output: &mut Vec<u8>) {
+    // Use primitive + timestamp type
+    let header = primitive_header(VariantPrimitiveType::Timestamp as u8);
+    output.push(header);
+    output.extend_from_slice(&value.to_le_bytes());
+}
+
+/// Encodes a timestamp without timezone value (milliseconds since epoch)
+pub fn encode_timestamp_ntz(value: i64, output: &mut Vec<u8>) {
+    // Use primitive + timestamp_ntz type
+    let header = primitive_header(VariantPrimitiveType::TimestampNTZ as u8);
+    output.push(header);
+    output.extend_from_slice(&value.to_le_bytes());
+}
+
+/// Encodes a time without timezone value (milliseconds)
+pub fn encode_time_ntz(value: i64, output: &mut Vec<u8>) {
+    // Use primitive + time_ntz type
+    let header = primitive_header(VariantPrimitiveType::TimeNTZ as u8);
+    output.push(header);
+    output.extend_from_slice(&value.to_le_bytes());
+}


That is a good point

I also think they don't need to be pub (maybe we could start with pub(crate)) as the main API people would use is the builder I think

alamb · 2025-05-02T16:39:35Z

arrow-variant/src/lib.rs

+// specific language governing permissions and limitations
+// under the License.
+
+//! [`arrow-variant`] contains utilities for working with the [Arrow Variant][format] binary format.


❤️ 📖

alamb · 2025-05-02T16:40:30Z

arrow-variant/src/lib.rs

+/// Builder API for creating variant values
+pub mod builder;
+/// Encoder module for converting values to Variant binary format
+pub mod encoder;


I think we should start with a minimal API surface area (only expose the Builder and Varaint types directly)

Suggested change

/// Builder API for creating variant values

pub mod builder;

/// Encoder module for converting values to Variant binary format

pub mod encoder;

/// Builder API for creating variant values

mod builder;

/// Encoder module for converting values to Variant binary format

mod encoder;

alamb · 2025-05-02T16:44:11Z

Thank yoU @Weijun-H for the review as well

Co-authored-by: Andrew Lamb <[email protected]>

alamb

Thank you @PinkCrow007 -- this is (again) quite amazing. The code you have sketched out in this PR I think will form the basis for all variant processing going forward. Very impressive

What I suggest we should do now is start merging this PR piece by piece into the repo. I have created a PR here to add a skeleton structure we can work on

[Variant] Add (empty) parquet-variant crate, update parquet-testing pin #7485

That PR also updates the datafusion-testing submodule so it contains binary examples so we can test interoperability with Spark

I think the next PR into the repo should add the Variant struct type along with some basic "read the existing binary values" type tests.

alamb · 2025-05-08T18:56:28Z

arrow-variant/src/variant.rs

+use arrow_schema::ArrowError;
+use std::fmt;
+
+/// A Variant value in the Arrow binary format


Technically Variant is part of the Parquet spec, not part of the Arrow spec 🤷

Suggested change

/// A Variant value in the Arrow binary format

/// A Variant value in the Parquet binary format

alamb · 2025-05-08T18:58:21Z

arrow-variant/src/variant.rs

+
+/// A Variant value in the Arrow binary format
+#[derive(Debug, Clone, PartialEq)]
+pub struct Variant<'a> {


This is a good start as it does not copy the values. However I think it may have a few issues:

The lifetimes ('a) are the same for the Value and the Metadata which I think will make sharing metadata across multiple variants potentially tricky

There is no way to use match effectively to switch on variant type. Instead, I think it needs methods like is_object or is_array

As written I think these structures follow a pattern that is more common on Java or C++ (which is fine, but if we are going to make a native Rust library I think it is worth following standard rust Idioms)

I wonder if you considered the structure contemplated here: #7423

Specifically this structure:

/// Variant value. May contain references to metadata and value /// 'a is lifetime for metadata /// 'b is lifetime for value pub enum Variant<'a, 'b> { Variant::Null, Variant::Int8 ... // strings are stored in the value and thus have references to that value Variant::String(&'b str), Variant::ShortString(&'b str), // Objects and Arrays need the metadata and values, so store both. Variant::Object(VariantObject<'a, 'b>), VariantArray(VariantArray<'a, 'b>) } /// Wrapper over Variant Metadata pub struct VariantMetadata<'a> { metadata: &'a[u8], // perhaps access to header fields like dict length and is_sorted } /// Represents a Variant Object with references to the underlying metadata /// and value fields pub enum VariantObject<'a, 'b> { // pointer to metadata metadata: VariantMetadata<'a>, // pointer to value value: &'a [u8], }

alamb · 2025-05-08T19:00:58Z

arrow-variant/src/variant.rs

+    /// Converts the variant value to a serde_json::Value
+    pub fn as_value(&self) -> Result<serde_json::Value, ArrowError> {
+        let keys = crate::decoder::parse_metadata_keys(self.metadata)?;
+        crate::decoder::decode_value(self.value, &keys)


I think we should try and avoid using the JSON representation when decoding Variant values for such a low level API because:

They are likely inefficient (e.g. to access a Variant value it needs to be converted from bytes --> Json --> Variant)

They can't represent the types fully (e.g. there is no way to represent the different between Int and Float in JSON, all numbers are floats)

PinkCrow007 added 6 commits April 23, 2025 15:07

create variant.rs

bebb0da

create variant api draft

2fb0ab5

reuse encoder in builder

ecd618d

correct encoding

163a439

make encoder the core binary serialization logic library

1261027

clean code

13f4c10

github-actions bot added the arrow Changes to the arrow crate label Apr 29, 2025

alamb mentioned this pull request Apr 29, 2025

Weekly Plan: Andrew Lamb 2025-04-28 apache/datafusion#15880

Closed

26 tasks

fix format

c6c570c

PinkCrow007 force-pushed the variant-create-api branch from 44ddac2 to c6c570c Compare April 29, 2025 14:58

Weijun-H reviewed Apr 30, 2025

View reviewed changes

arrow-schema/src/extension/canonical/variant.rs Outdated Show resolved Hide resolved

alamb reviewed May 2, 2025

View reviewed changes

PinkCrow007 and others added 6 commits May 5, 2025 15:17

Remove magic numbers (per review)

acbb73c

Update link in comment

b04d9b5

Refactor encoder using trait to reduce redundancy

a97b31d

remove VariantError from ArrowError

aeefd1e

add unit tests in encoder

1971bf3

Update arrow-variant/Cargo.toml

df17d27

Co-authored-by: Andrew Lamb <[email protected]>

alamb mentioned this pull request May 7, 2025

Weekly Plan: Andrew Lamb 2025-05-05 apache/datafusion#15943

Open

23 tasks

PinkCrow007 added 2 commits May 8, 2025 01:07

Add get() and decoder for readable Variant inspection (initial)

a053cf5

fix format

0eb8db5

github-actions bot removed the arrow Changes to the arrow crate label May 8, 2025

alamb reviewed May 8, 2025

View reviewed changes

alamb mentioned this pull request May 8, 2025

[EPIC] [Parquet] Implement Variant type support in Parquet #6736

Open

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add API for Creating Variant Values #7452

Add API for Creating Variant Values #7452

PinkCrow007 commented Apr 29, 2025

alamb commented Apr 30, 2025

Weijun-H left a comment

Weijun-H Apr 30, 2025

alamb May 2, 2025

PinkCrow007 May 5, 2025

alamb May 7, 2025

alamb left a comment

alamb May 2, 2025

alamb May 2, 2025

alamb May 2, 2025

alamb May 2, 2025

alamb May 2, 2025

alamb May 2, 2025

alamb May 2, 2025

alamb May 2, 2025

alamb commented May 2, 2025

alamb left a comment

alamb May 8, 2025

alamb May 8, 2025

alamb May 8, 2025

	/// A Variant value in the Arrow binary format
	/// A Variant value in the Parquet binary format

Add API for Creating Variant Values #7452

Are you sure you want to change the base?

Add API for Creating Variant Values #7452

Conversation

PinkCrow007 commented Apr 29, 2025

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

alamb commented Apr 30, 2025

Weijun-H 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

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

Choose a reason for hiding this comment

alamb commented May 2, 2025

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