[Variant] Implement read support for remaining primitive types #7644

superserious-dev · 2025-06-11T15:05:49Z

Which issue does this PR close?

Closes #7630.

What changes are included in this PR?

This PR implements support for the following primitive variant types:

Binary
Date
TimestampMicros
TimestampNtzMicros
Int16
Int32
Int64
Decimal4
Decimal8
Decimal16
Float
Double

The following types are not yet implemented(see here for details):

TimeNTZ
TimestampNanos
TimestampNtzNanos
UUID

Are there any user-facing changes?

Users who opt-in to the Variant feature can use these primitives.

superserious-dev · 2025-06-11T15:17:57Z

parquet-variant/tests/variant_interop.rs

        ("primitive_string", Variant::String("This string is longer than 64 bytes and therefore does not fit in a short_string and it also includes several non ascii characters such as 🐢, 💖, ♥\u{fe0f}, 🎣 and 🤦!!")),
-        //("primitive_timestamp", Variant::Null),
-        //("primitive_timestampntz", Variant::Null),
+        ("primitive_timestamp", Variant::TimestampMicros(NaiveDate::from_ymd_opt(2025, 4, 16).unwrap().and_hms_milli_opt(16, 34, 56, 780).unwrap().and_utc())),


regen.py has this as 2025-04-16T12:34:56.78 whereas it appears to be 2025-04-16T16:34:56.78 in the test case primitive_timestamp.value(which is the value that this test checks). I believe this is because the timezone is treated as local to the computer that ran the regen script.

I ran the regen script in the Americas/New_York timezone -- does that line up correctly?

If so, then I think we (I) can make a PR to parquet-testing to clarify that in the repo

Those seem to be the same two values? Did I misread something?

Yes, this lines up perfectly: the Eastern timezone is 4 hours behind UTC and the naive input timestamp is being shifted to UTC.
(The two timestamps are 4 hours apart.)

superserious-dev · 2025-06-11T15:22:50Z

parquet-variant/src/variant.rs

    pub fn try_new(metadata: &'m VariantMetadata, value: &'v [u8]) -> Result<Self, ArrowError> {
        let header = *first_byte_from_slice(value)?;
        let new_self = match get_basic_type(header)? {
            VariantBasicType::Primitive => match get_primitive_type(header)? {


All the decoders manually skip the first byte(header) in their implementation. It could makes sense to just pass in the value with this first byte removed.

I had wondered the same thing... but there's also an argument to be made that the header is indeed part of the variant value. A missing first byte would be a problem if we ever needed to retrieve the full value.

Maybe something to track as the implementation and usage of this API matures? I don't think we have enough information to make a confident decision quite yet?

I was able to make this change by splitting value: &[u8]into value_metadata: u8 and value_data: &[u8]. Keeping track separately enabled simplifying the match.

parquet-variant/src/decoder.rs

superserious-dev · 2025-06-11T15:38:25Z

parquet-variant/src/variant.rs

+        }
+    }
+
+    pub fn as_int16(&self) -> Option<i16> {


These as_* methods could benefit from code coverage. Happy to add unit tests for these either in this PR or a future one.

Yes, I think it would be great to add tests

What I recommend doing is adding doc examples that will:

Make it easier to explain what is happening

Run as part of CI

Something like

let variant_i32 = Variant::from(1337i32); // you can read an int32 value as int16 assert_eq!(variant_i32.as_i16(), Some(Variant::from(1337i16)); // but not as an int8 assert_eq(variant_i32.as_i8(), None)

If you have time to do it in this PR that would be great, otherwise a follow on would be wonderful

Doctests are a great idea. I added them for all the methods.

parquet-variant/src/variant.rs

alamb

Thanks @superserious-dev and @scovich -- I think this is a very nice step forward and contribution. It would be nice to clean up the left over TODOs and add the docs and tests for the as_* methods, but I also think this PR could be merged as is and that done as a follow on

Just let me know what you prefer.

cc @mkarbo

alamb · 2025-06-11T20:33:01Z

parquet-variant/src/decoder.rs

+    Decimal4 = 8,
+    Decimal8 = 9,
+    Decimal16 = 10,
    // TODO: Add types for the rest of primitives, once API is agreed upon


I think we can remove this TODO as well

It's still missing some primitive types (like Double, UUID) from the spec. I'll admit to not following the variant development closely, but it seems the TODO is still relevant.

I added Double. The other primitives don't appear to be ready to implement yet based on this comment.

parquet-variant/src/decoder.rs

alamb · 2025-06-11T20:39:21Z

parquet-variant/tests/variant_interop.rs

        ("primitive_string", Variant::String("This string is longer than 64 bytes and therefore does not fit in a short_string and it also includes several non ascii characters such as 🐢, 💖, ♥\u{fe0f}, 🎣 and 🤦!!")),
-        //("primitive_timestamp", Variant::Null),
-        //("primitive_timestampntz", Variant::Null),
+        ("primitive_timestamp", Variant::TimestampMicros(NaiveDate::from_ymd_opt(2025, 4, 16).unwrap().and_hms_milli_opt(16, 34, 56, 780).unwrap().and_utc())),


I ran the regen script in the Americas/New_York timezone -- does that line up correctly?

If so, then I think we (I) can make a PR to parquet-testing to clarify that in the repo

parquet-variant/src/variant.rs

alamb · 2025-06-11T20:44:17Z

parquet-variant/src/decoder.rs

+
+/// Decodes an Int32 from the value section of a variant.
+pub(crate) fn decode_int32(value: &[u8]) -> Result<i32, ArrowError> {
+    let value = i32::from_le_bytes(array_from_slice(value, 1)?);


I had to double check the reason for this constant 1 a few times. I found it pretty confusing that it is an offset.

We can always find a better way to structure this code in subsequent PRs, but it might make sense to slice the input value so this code always looks like the following (no offset) -- and eventually remove the offset parameter all together

let value = i32::from_le_bytes(array_from_slice(value, 0)?);

That being said since it is all crate private, I think this is totally good.

Made the change to shift the base offset to 0.

parquet-variant/src/decoder.rs

parquet-variant/src/variant.rs

alamb · 2025-06-11T20:53:10Z

parquet-variant/src/variant.rs

+        }
+    }
+
+    pub fn as_int16(&self) -> Option<i16> {


Yes, I think it would be great to add tests

What I recommend doing is adding doc examples that will:

Make it easier to explain what is happening

Run as part of CI

Something like

let variant_i32 = Variant::from(1337i32); // you can read an int32 value as int16 assert_eq!(variant_i32.as_i16(), Some(Variant::from(1337i16)); // but not as an int8 assert_eq(variant_i32.as_i8(), None)

If you have time to do it in this PR that would be great, otherwise a follow on would be wonderful

…from value-data

superserious-dev · 2025-06-12T14:40:34Z

Thanks @superserious-dev and @scovich -- I think this is a very nice step forward and contribution. It would be nice to clean up the left over TODOs and add the docs and tests for the as_* methods, but I also think this PR could be merged as is and that done as a follow on

Just let me know what you prefer.

cc @mkarbo

I'll push up another revision to address the comments in the near future.

scovich

Three main questions:

How permissive/aggressive should the Variant::as_XXX functions be in the casting they perform?
Should we define helper structs for decimal variant types, so we don't have to pass (value, scale) pairs manually everywhere we use them?
Does it actually make sense to provide From for the various primitive subtypes?

See comments for details.

parquet-variant/src/decoder.rs

scovich · 2025-06-12T14:46:10Z

parquet-variant/src/variant.rs

+    Decimal4 { integer: i32, scale: u8 },
+    Decimal8 { integer: i64, scale: u8 },
+    Decimal16 { integer: i128, scale: u8 },


It's hard to pass around the content of tuple/struct enum variants with multiple members -- you have to unpack them and pass scalar values around, which is bulky and error-prone (**). Should we consider creating a struct or family of structs for representing scalar decimal values?

(**)

It would be lovely if Variant::Decimal4 were an actual type in rust. It actually is a type under the hood, but the language doesn't expose it as such, because the enum discriminant has to be stored somewhere and rust usually embeds it in the variant itself to save space. See https://rust-lang.github.io/rfcs/2195-really-tagged-unions.html#guide-level-explanation for gory details.

Definitely something to consider. Perhaps the naming of the fields and potential new struct(s) could be refined in a follow-up PR?

I will file a ticket

Filed

[Variant] Introduce structs for Variant::Decimal types #7660

scovich · 2025-06-12T14:50:47Z

parquet-variant/src/variant.rs

+    }
+}
+
+impl<'m, 'v> From<i32> for Variant<'m, 'v> {


I'm not sure how useful these From would be in practice, especially once a variant builder/encoder API lands?

What use case(s) would they serve?
Are they designed to make unit tests easier to write?
Do we anticipate the variant builder/encoder would use them internally?

Also:

Variant::String and Variant::ShortString are problematic because there's no way to e.g. embed one in a Variant::Object later on.

We anyway couldn't create Variant::Object or Variant::Array (they would need a mutable VariantMetadataBuilder to store field names in, etc.)

I think they are useful for writing tests / examples, FWIW

Simple ones, at least. We'll need actual builders in order to construct the complex types.

indeed -- see the builder API from @PinkCrow007:

Initial Builder API for Creating Variant Values #7653

scovich · 2025-06-12T14:51:48Z

parquet-variant/src/variant.rs

+    }
+}
+
+impl<'m, 'v> From<i64> for Variant<'m, 'v> {


Newer versions of clippy suggest:

Suggested change

impl<'m, 'v> From<i64> for Variant<'m, 'v> {

impl From<i64> for Variant<'_, '_> {

(several of these cases)

Updated. Was able to see these lints by using cargo clippy -p parquet-variant -- -Wclippy::pedantic

scovich · 2025-06-12T15:10:53Z

parquet-variant/src/variant.rs

+    Date(NaiveDate),
+    TimestampMicros(DateTime<Utc>),
+    TimestampNTZMicros(NaiveDateTime),
+    Decimal4 { integer: i32, scale: u8 },


The spec calls the integer form of a decimal an "unscaled value."
Should we try to capture that in our naming here?

Seems like a good thing to do -- let's do it as a follow on ticket. I will file one

scovich · 2025-06-12T15:12:47Z

parquet-variant/tests/variant_interop.rs

        ("primitive_string", Variant::String("This string is longer than 64 bytes and therefore does not fit in a short_string and it also includes several non ascii characters such as 🐢, 💖, ♥\u{fe0f}, 🎣 and 🤦!!")),
-        //("primitive_timestamp", Variant::Null),
-        //("primitive_timestampntz", Variant::Null),
+        ("primitive_timestamp", Variant::TimestampMicros(NaiveDate::from_ymd_opt(2025, 4, 16).unwrap().and_hms_milli_opt(16, 34, 56, 780).unwrap().and_utc())),


Those seem to be the same two values? Did I misread something?

alamb

Thanks @superserious-dev and @scovich -- I think this PR is in good shape and I'll merge it and file some additional follow on tasks / PRs

alamb · 2025-06-13T14:26:02Z

parquet-variant/src/variant.rs

+    Date(NaiveDate),
+    TimestampMicros(DateTime<Utc>),
+    TimestampNTZMicros(NaiveDateTime),
+    Decimal4 { integer: i32, scale: u8 },


Seems like a good thing to do -- let's do it as a follow on ticket. I will file one

alamb · 2025-06-13T14:26:37Z

parquet-variant/src/variant.rs

+    Decimal4 { integer: i32, scale: u8 },
+    Decimal8 { integer: i64, scale: u8 },
+    Decimal16 { integer: i128, scale: u8 },


I will file a ticket

alamb · 2025-06-13T14:31:29Z

parquet-variant/src/variant.rs

+            Variant::Int16(i) => i.try_into().ok(),
+            Variant::Int32(i) => i.try_into().ok(),


In my opinion, these included APIs are just a convenience

Since the rust API directly exposes the Variant enum, I think it is flexible enough that people can implement whatever semantics they want on top of it using match

match variant { Variant::Int32(val) => { .. do system specific casting .. } }

I'll try and contribute an example shortly

alamb · 2025-06-13T14:32:11Z

parquet-variant/src/variant.rs

+        }
+    }
+
+    pub fn as_decimal_int32(&self) -> Option<(i32, u8)> {


Let's keep this one for now

alamb · 2025-06-13T15:42:33Z

I'll try and contribute an example shortly

Here is a PR: [Variant] Add variant docs and examples #7661

# Which issue does this PR close? We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. - Follow on to #7644 - Part of #6736 # Rationale for this change Using the parquet APIs came up in #7644 (comment) so I wanted to help contribute some additional documentation / tests # What changes are included in this PR? Add documentation and tests about `Variant`, specifically some examples of how to create `Variant` values # Are there any user-facing changes? More docs --------- Co-authored-by: Ryan Johnson <scovich@users.noreply.github.com>

superserious-dev added 6 commits June 11, 2025 07:43

Implement Binary variant

512b73a

Implement Date variant

d354c95

Implement TimestampMicros and TimestampNTZMicros variants

e0918d1

Implement Int16, Int32, and Int64 variants

2eb1d22

Implement Decimal4, Decimal8, and Decimal16 variants

f926138

Implement Float variant

aa64f33

github-actions bot added the parquet Changes to the parquet crate label Jun 11, 2025

superserious-dev commented Jun 11, 2025

View reviewed changes

parquet-variant/src/decoder.rs Show resolved Hide resolved

superserious-dev commented Jun 11, 2025

View reviewed changes

parquet-variant/src/variant.rs Outdated Show resolved Hide resolved

alamb approved these changes Jun 11, 2025

View reviewed changes

superserious-dev added 4 commits June 11, 2025 19:30

Merge branch 'apache:main' into primitive-type-read-support

a4c48c3

Remove unnecessary TODOs

8b40263

Simplify offset calculations for values by separating value-metadata …

85036ad

…from value-data

Add as_int* doc tests

ffa65fc

scovich reviewed Jun 12, 2025

View reviewed changes

superserious-dev added 10 commits June 12, 2025 09:48

Add as_naive_date doc tests

f6a6a7b

Add as_naive_datetime and as_datetime_utc doc tests

2f27c38

Add as_boolean doc tests

9a89cc7

Implement From<()> for Variant and add as_null doc tests

9fb65ac

Add as_u8_slice doc tests

1292140

Add as_string doc tests

81ee289

Add as_decimal_* doc tests

8e0985c

Implement Double variant

b62d776

Add as_f32 and as_f64 doc tests

932d925

cleanup

346c264

alamb approved these changes Jun 13, 2025

View reviewed changes

alamb merged commit 71ee9d9 into apache:main Jun 13, 2025
14 checks passed

This was referenced Jun 13, 2025

[Variant] Introduce structs for Variant::Decimal types #7660

Closed

[Variant] Add variant docs and examples #7661

Merged

superserious-dev deleted the primitive-type-read-support branch June 13, 2025 16:14

alamb mentioned this pull request Jun 20, 2025

[Variant] Implement read support for remaining primitive types #7630

Closed

alamb mentioned this pull request Jul 28, 2025

[Variant] Add negative tests for reading invalid primitive variant values #7645

Closed

	impl<'m, 'v> From<i64> for Variant<'m, 'v> {
	impl From<i64> for Variant<'_, '_> {

		Variant::Int16(i) => i.try_into().ok(),
		Variant::Int32(i) => i.try_into().ok(),

[Variant] Implement read support for remaining primitive types #7644

[Variant] Implement read support for remaining primitive types #7644

Uh oh!

Conversation

superserious-dev commented Jun 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

What changes are included in this PR?

Are there any user-facing changes?

Uh oh!

superserious-dev Jun 11, 2025 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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!

Uh oh!

alamb left a comment

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

superserious-dev commented Jun 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

scovich left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

superserious-dev commented Jun 11, 2025 •

edited

Loading

superserious-dev Jun 11, 2025 •

edited

Loading

superserious-dev commented Jun 12, 2025 •

edited

Loading