[Variant] Implement primitive type access for null/time/decimal* #8638

klion26 · 2025-10-17T07:11:37Z

Which issue does this PR close?

Closes [Variant] Support variant to arrow primitive support null/time/decimal_* #8637.
Support Variant to Arrow for Null/Time/Decimlal{4,8,16}

What changes are included in this PR?

Add logic in typed_value_to_variant/PrimitiveVariantToArrowRowBuilder for Null/Time/Decimal{4,8,16}
Implement PrimitiveFromVariant for Time64MicrosecondType
Add tests to cover the added logic

Are these changes tested?

Added some tests

Are there any user-facing changes?

No

klion26

@alamb @scovich Please help review this when you're free, thanks.

klion26 · 2025-10-17T07:16:54Z

parquet-variant-compute/src/variant_array.rs

            let value = boolean_array.value(index);
            Variant::from(value)
        }
-        DataType::Date32 => {


This has been adjusted to be grouped together with the Timestamp/Time.

klion26 · 2025-10-17T08:23:32Z

parquet-variant-compute/src/variant_to_arrow.rs

+    fn append_value(&mut self, v: ());
+}
+
+impl AppendValueTrait for NullBuilder {


The function is needed by the define_variant_to_primitive_builder macro.

The NullArray is kind of strange -- maybe rather than creating a full on builder for it, we could just keep count and then create the output array with https://docs.rs/arrow/latest/arrow/array/fn.new_null_array.html ?

NullArray (with DataType::Null) is not the same as an array (of some arbitrary other type) full of nulls tho? But if we know the intended type, then I agree that new_null_array is better (since NullArray::is_null always returns false = weird)

Ok, now that I'm actually looking at the code (and not just reacting to the comments) --

It seems like we don't actually need a NullBuilder at all? We know the capacity, so just use NullArray::new:

struct FakeNullBuilder(NullArray); impl FakeNullBuilder { fn new(capacity: usize) -> Self { Self(NullArray::new(capacity) } fn append_value(&mut self, _: ()) {} fn append_null(&mut self) {} fn finish(self) -> NullArray { self.0 } }

... and then the macro invocation us just:

|capacity| -> FakeNullBuilder { FakeNullBuilder::new(capacity) },

NullArray (with DataType::Null) is not the same as an array (of some arbitrary other type) full of nulls tho?
new_null_array with different types are different, the following unit test will fail

let first = new_null_array(&DataType::Int32, 3); let second = new_null_array(&DataType::Timestamp(TimeUnit::Nanosecond, None), 3); assert_eq!(*first, second.clone());

Use NullArray instead of new_null_array in the changed code because the compiler will panic with the trait bound Result<Arc<dyn Array>, ArrowError>: Array is not satisfied if I use new_null_array in FakeNullBuilder::finish

alamb

Thanks @klion26 -- this looks very neat. I left some comments, let me know what you think

cc @scovich

alamb · 2025-10-17T19:52:08Z

parquet-variant-compute/src/variant_array.rs

                index
            )
        }
+        DataType::Time64(TimeUnit::Microsecond) => {


Another possible type is DataType::Time64(TimeUnit::Nanosecond)

There is also a Time32 type in arrow: https://docs.rs/arrow/latest/arrow/datatypes/enum.DataType.html#variant.Time32

I am not sure if we want to handle that too

Only support the Time64(TimeUnit::Microsecond) here because that variant spec said that Time is TimeUnit::Microsecond.

supernit: perhaps we can move this branch to just below Date32 so that the order aligns the spec.

alamb · 2025-10-17T19:54:16Z

parquet-variant-compute/src/variant_to_arrow.rs

+    fn append_value(&mut self, v: ());
+}
+
+impl AppendValueTrait for NullBuilder {


The NullArray is kind of strange -- maybe rather than creating a full on builder for it, we could just keep count and then create the output array with https://docs.rs/arrow/latest/arrow/array/fn.new_null_array.html ?

scovich · 2025-10-18T17:21:05Z

parquet-variant-compute/src/type_conversion.rs

    datatypes::Date32Type::from_naive_date
 );
+impl_primitive_from_variant!(datatypes::Time64MicrosecondType, as_time_utc, |v| {
+    (v.num_seconds_from_midnight() * 1_000_000 + v.nanosecond() / 1_000) as i64


Is it normal to take the floor instead of rounding?

I think it is ok because nanosecond() will return u32, and the result will be truncated to zero.

scovich · 2025-10-18T17:25:09Z

parquet-variant-compute/src/variant_array.rs

+            generic_conversion_single_value!(
+                Decimal128Type,
+                as_primitive,
+                |v| VariantDecimal16::try_new(v, *s as u8).unwrap(),


Are we sure these try_new calls will never fail?

I mean, we are unshredding shredded data, but it's also... data... which is not trustworthy.

For safety, suggest to do something like:

Suggested change

|v| VariantDecimal16::try_new(v, *s as u8).unwrap(),

|v| VariantDecimal16::try_new(v, *s as u8).map_or(Variant::Null, Variant::from),

Thanks for this suggestion, fixed.

Ah, found that we can pass in different types here(VariantDecimal16 vs Variant::Decimal16), it is cool.

Yeah, it accepts any V: IntoVariant

scovich · 2025-10-18T17:26:08Z

parquet-variant-compute/src/variant_array.rs

+                    (v / 1_000_000) as u32,
+                    (v % 1_000_000) as u32 * 1000
+                )
+                .unwrap(),


As above -- are we absolutely sure the conversion will never fail? Or do we need a Variant::Null fallback?

scovich · 2025-10-18T17:32:53Z

parquet-variant-compute/src/variant_to_arrow.rs

+    fn append_value(&mut self, v: ());
+}
+
+impl AppendValueTrait for NullBuilder {


Ok, now that I'm actually looking at the code (and not just reacting to the comments) --

It seems like we don't actually need a NullBuilder at all? We know the capacity, so just use NullArray::new:

struct FakeNullBuilder(NullArray); impl FakeNullBuilder { fn new(capacity: usize) -> Self { Self(NullArray::new(capacity) } fn append_value(&mut self, _: ()) {} fn append_null(&mut self) {} fn finish(self) -> NullArray { self.0 } }

... and then the macro invocation us just:

|capacity| -> FakeNullBuilder { FakeNullBuilder::new(capacity) },

scovich · 2025-10-18T17:33:45Z

parquet-variant-compute/src/variant_to_arrow.rs

+define_variant_to_primitive_builder!(
+    struct VariantToNullArrowRowBuilder<'a>
+    |_capacity| -> NullBuilder { NullBuilder::new() },
+    | v | v.as_null(),


nit: odd spacing there?

Suggested change

| v | v.as_null(),

|v| v.as_null(),

klion26

@alamb @scovich Thanks for the review! I've updated the code. Please take another look.

CI failed because

Using default tag: latest
  Error response from daemon: Head "https://registry-1.docker.io/v2/amd64/rust/manifests/latest": received unexpected HTTP status: 503 Service Unavailable
  Warning: Docker pull failed with exit code 1, back off 1.328 seconds before retry.

klion26 · 2025-10-20T02:18:22Z

parquet-variant-compute/src/type_conversion.rs

    datatypes::Date32Type::from_naive_date
 );
+impl_primitive_from_variant!(datatypes::Time64MicrosecondType, as_time_utc, |v| {
+    (v.num_seconds_from_midnight() * 1_000_000 + v.nanosecond() / 1_000) as i64


I think it is ok because nanosecond() will return u32, and the result will be truncated to zero.

klion26 · 2025-10-20T02:21:00Z

parquet-variant-compute/src/variant_array.rs

+            generic_conversion_single_value!(
+                Decimal128Type,
+                as_primitive,
+                |v| VariantDecimal16::try_new(v, *s as u8).unwrap(),


Thanks for this suggestion, fixed.

Ah, found that we can pass in different types here(VariantDecimal16 vs Variant::Decimal16), it is cool.

klion26 · 2025-10-20T02:33:56Z

parquet-variant-compute/src/variant_array.rs

                index
            )
        }
+        DataType::Time64(TimeUnit::Microsecond) => {


Only support the Time64(TimeUnit::Microsecond) here because that variant spec said that Time is TimeUnit::Microsecond.

klion26 · 2025-10-20T08:31:07Z

parquet-variant-compute/src/variant_to_arrow.rs

+    fn append_value(&mut self, v: ());
+}
+
+impl AppendValueTrait for NullBuilder {


NullArray (with DataType::Null) is not the same as an array (of some arbitrary other type) full of nulls tho?
new_null_array with different types are different, the following unit test will fail

let first = new_null_array(&DataType::Int32, 3); let second = new_null_array(&DataType::Timestamp(TimeUnit::Nanosecond, None), 3); assert_eq!(*first, second.clone());

Use NullArray instead of new_null_array in the changed code because the compiler will panic with the trait bound Result<Arc<dyn Array>, ArrowError>: Array is not satisfied if I use new_null_array in FakeNullBuilder::finish

scovich

LGTM. One nit and one follow-up item to potentially track.

scovich · 2025-10-20T16:49:50Z

parquet-variant-compute/src/variant_array.rs

+            generic_conversion_single_value!(
+                Decimal128Type,
+                as_primitive,
+                |v| VariantDecimal16::try_new(v, *s as u8).map_or(Variant::Null, Variant::from),


We may need a follow-up item to track turning this into a proper Result instead of silently converting to NULL on failure? That would require VariantArray::value to return a Result<Variant>, which is a biggish change but honestly seems appropriate given that we can't guarantee the input shredding is even physically valid, let alone logically valid? It seems too sharp an edge to just panic.

@alamb -- thoughts?

Filed an issue #8672 to track this.

We may need a follow-up item to track turning this into a proper Result instead of silently converting to NULL on failure?

I think this makes a lot of sense.

We may need a follow-up item to track turning this into a proper Result instead of silently converting to NULL on failure? That would require VariantArray::value to return a Result, which is a biggish change but honestly seems appropriate given that we can't guarantee the input shredding is even physically valid, let alone logically valid? It seems too sharp an edge to just panic.

@alamb -- thoughts?

How about we add a try_value() method that returns a Result and just have value() unwrap the result?

scovich · 2025-10-20T16:52:33Z

parquet-variant-compute/src/variant_to_arrow.rs

+define_variant_to_primitive_builder!(
+    struct VariantToNullArrowRowBuilder<'a>
+    |capacity| -> FakeNullBuilder { FakeNullBuilder::new(capacity) },
+    |value|  value.as_null(),


Should we just hard-wire |_| Some(Variant::Null) here?
All input variant subtypes ultimately produce the same outcome because both append_null and append_value are no-ops.

This is more obvious: the result is always Variant::Null; updated the code.
As macro can't receive _ for ident (need change to pat and more), changed to _value

alamb

Thank you @klion26 @scovich and @friendlymatthew

alamb · 2025-10-21T20:03:12Z

parquet-variant-compute/src/variant_array.rs

+            generic_conversion_single_value!(
+                Decimal128Type,
+                as_primitive,
+                |v| VariantDecimal16::try_new(v, *s as u8).map_or(Variant::Null, Variant::from),


We may need a follow-up item to track turning this into a proper Result instead of silently converting to NULL on failure? That would require VariantArray::value to return a Result, which is a biggish change but honestly seems appropriate given that we can't guarantee the input shredding is even physically valid, let alone logically valid? It seems too sharp an edge to just panic.

@alamb -- thoughts?

How about we add a try_value() method that returns a Result and just have value() unwrap the result?

liamzwbao

LGTM!

liamzwbao · 2025-10-21T22:50:23Z

parquet-variant-compute/src/variant_array.rs

                index
            )
        }
+        DataType::Time64(TimeUnit::Microsecond) => {


supernit: perhaps we can move this branch to just below Date32 so that the order aligns the spec.

alamb · 2025-10-22T15:03:57Z

🚀 thanks @liamzwbao @friendlymatthew @klion26 and @scovich

github-actions bot added the parquet-variant parquet-variant* crates label Oct 17, 2025

klion26 force-pushed the primitive-null/time/decimal branch 3 times, most recently from 1d7e5e8 to b5bfaed Compare October 17, 2025 07:53

klion26 commented Oct 17, 2025

View reviewed changes

alamb reviewed Oct 17, 2025

View reviewed changes

scovich reviewed Oct 18, 2025

View reviewed changes

klion26 added 3 commits October 20, 2025 16:37

[Variant] Implement primitive type access for null/time/decimal*

3797829

remove debug log

e1300ce

address comments

78ebf7b

klion26 force-pushed the primitive-null/time/decimal branch from 7658b17 to 78ebf7b Compare October 20, 2025 08:38

klion26 commented Oct 20, 2025

View reviewed changes

scovich approved these changes Oct 20, 2025

View reviewed changes

address comment

8981443

klion26 mentioned this pull request Oct 21, 2025

[Varaint] Support VariantArray::value to return a Result<Variant> #8672

Open

klion26 requested a review from alamb October 21, 2025 15:50

alamb approved these changes Oct 21, 2025

View reviewed changes

liamzwbao approved these changes Oct 21, 2025

View reviewed changes

address comment to reorder match arm

e4afa24

alamb merged commit a7572eb into apache:main Oct 22, 2025
17 checks passed

klion26 deleted the primitive-null/time/decimal branch October 23, 2025 13:30

	\|v\| VariantDecimal16::try_new(v, *s as u8).unwrap(),
	\|v\| VariantDecimal16::try_new(v, *s as u8).map_or(Variant::Null, Variant::from),

[Variant] Implement primitive type access for null/time/decimal* #8638

[Variant] Implement primitive type access for null/time/decimal* #8638

Uh oh!

Conversation

klion26 commented Oct 17, 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 these changes tested?

Are there any user-facing changes?

Uh oh!

klion26 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!

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!

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!

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!

scovich Oct 20, 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!

klion26 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!

Choose a reason for hiding this comment

Uh oh!

scovich 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!

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

klion26 commented Oct 17, 2025 •

edited

Loading

scovich Oct 20, 2025 •

edited

Loading

alamb commented Oct 22, 2025 •

edited

Loading