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 👍

Weijun-H · 2025-04-30T05:08:18Z

arrow-variant/src/encoder/mod.rs

+
+/// Encodes an integer value, choosing the smallest sufficient type
+pub fn encode_integer(value: i64, output: &mut Vec<u8>) {
+    if value >= -128 && value <= 127 {


Suggested change

if value >= -128 && value <= 127 {

if value >= i8::MIN.into() && value <= i8::MAX.into() {

Weijun-H · 2025-04-30T05:08:31Z

arrow-variant/src/encoder/mod.rs

+        // Int8
+        output.push(primitive_header(VariantPrimitiveType::Int8 as u8));
+        output.push(value as u8);
+    } else if value >= -32768 && value <= 32767 {


Suggested change

} else if value >= -32768 && value <= 32767 {

} else if value >= i16::MIN.into() && value <= i16::MAX.into() {

Weijun-H · 2025-04-30T05:09:02Z

arrow-variant/src/encoder/mod.rs

+        // Int16
+        output.push(primitive_header(VariantPrimitiveType::Int16 as u8));
+        output.extend_from_slice(&(value as i16).to_le_bytes());
+    } else if value >= -2147483648 && value <= 2147483647 {


Suggested change

} else if value >= -2147483648 && value <= 2147483647 {

} else if value >= i32::MIN.into() && value <= i32::MAX.into() {

Weijun-H · 2025-04-30T05:18:36Z

arrow-variant/src/encoder/mod.rs

+/// Timestamp	timestamp with time zone	18	TIMESTAMP(isAdjustedToUTC=true, NANOS)	8-byte little-endian
+/// TimestampNTZ	timestamp without time zone	19	TIMESTAMP(isAdjustedToUTC=false, NANOS)	8-byte little-endian
+/// UUID	uuid	20	UUID	16-byte big-endian
+pub enum VariantPrimitiveType {


It would be better to add a comment for the specification link here
https://github.com/apache/parquet-format/blob/master/VariantEncoding.md#encoding-types

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

Weijun-H · 2025-04-30T05:31:20Z

arrow-variant/src/encoder/mod.rs

+        ]);
+        assert_eq!(unscaled_value, large_value);
+    }
+}


Missing tests for timestamps, dates, and uuids

Weijun-H · 2025-04-30T05:38:41Z

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

+//! Variant
+//!
+//! <https://arrow.apache.org/docs/format/CanonicalExtensions.html#variant>


This link is unavailable because the content related to variant is missing

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

alamb · 2025-05-02T16:07:16Z

arrow-variant/src/builder/mod.rs

+//! # Example
+//!
+//! ```
+//! use std::io::Cursor;


I don't think the Cursor is used

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

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

arrow-variant/Cargo.toml

+[package]
+name = "arrow-variant"
+version = { workspace = true }
+description = "JSON to Arrow Variant conversion utilities"


Suggested change

description = "JSON to Arrow Variant conversion utilities"

description = "Rust API for reading/writing Apache Parquet Variant values"

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

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

Open

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

alamb reviewed May 2, 2025

View reviewed changes

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

Weijun-H Apr 30, 2025

Weijun-H Apr 30, 2025

Weijun-H Apr 30, 2025

Weijun-H Apr 30, 2025

alamb May 2, 2025

Weijun-H Apr 30, 2025 •

edited

Loading

Weijun-H Apr 30, 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 May 2, 2025

alamb May 2, 2025

alamb commented May 2, 2025

	if value >= -128 && value <= 127 {
	if value >= i8::MIN.into() && value <= i8::MAX.into() {

	} else if value >= -32768 && value <= 32767 {
	} else if value >= i16::MIN.into() && value <= i16::MAX.into() {

	} else if value >= -2147483648 && value <= 2147483647 {
	} else if value >= i32::MIN.into() && value <= i32::MAX.into() {

	description = "JSON to Arrow Variant conversion utilities"
	description = "Rust API for reading/writing Apache Parquet Variant values"

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Weijun-H Apr 30, 2025 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb commented May 2, 2025

Weijun-H Apr 30, 2025 •

edited

Loading