[Variant] Implement `VariantArray::value` for shredded variants by klion26 · Pull Request #8105 · apache/arrow-rs

klion26 · 2025-08-11T02:52:21Z

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.

Closes [Variant] Implement VariantArray::value for shredded variants #8091 .

Rationale for this change

Implement VariantArray::value for some more shredded variants(eg. primitive_conversion/generic_conversion/non_generic_conversion).

What changes are included in this PR?

Extract all macroRules to a separate module type_conversion.rs
Add a macro for variant value

Are these changes tested?

Covered by the existing test

Are there any user-facing changes?

No

klion26 · 2025-08-11T02:57:30Z

@alamb I've drafted a PR for #8091 to confirm if this is the right direction; if so, I'll continue the implementation; if not, I can revise it

klion26 · 2025-08-11T02:59:13Z

parquet-variant-compute/src/cast_to_variant.rs

Comment out this for the draft version; I will remove it in the future.

alamb

Thank you @klion26 -- this is very cool. I am sorry for the delay in reviewing. I left some comments -- let me know what you think

alamb · 2025-08-13T18:56:02Z

parquet-variant-compute/src/lib.rs

this is an interesting idea -- to have two variations of each macro. It is probably a reasonable place to start

However, I think it would be nice to avoid having macros in the root lib.rs file -- what do you think about making a new module like parquet-variant-compute/src/arrow_types.rs or parquet-variant-compute/src/type_conversion.rs to hold the code to convert back and forth between Arrow types and Variant types

@scovich added similar code in VariantToPrimitive here: #8122

So I am thinking having a module to start collecting such code would be useful

Place the macros in parquet-variant-compute/src/type_conversion.rs is better, will check #8122 and fix the remainings

alamb · 2025-08-13T18:56:50Z

parquet-variant-compute/src/variant_array.rs

looks good -- we can probably change the DataType::Int32 branch to use this macro too, right?

Also, I think we should add a test

Yes, only Int16 is implemented to confirm the direction

Indeed, we need to add tests to cover this logic.

klion26

@alamb Thanks for the review; I'll refine the PR and contact you after I'm done.

klion26 · 2025-08-14T10:59:43Z

parquet-variant-compute/src/lib.rs

Place the macros in parquet-variant-compute/src/type_conversion.rs is better, will check #8122 and fix the remainings

klion26 · 2025-08-14T11:02:18Z

parquet-variant-compute/src/variant_array.rs

Yes, only Int16 is implemented to confirm the direction

Indeed, we need to add tests to cover this logic.

alamb · 2025-08-14T16:59:01Z

marking as draft so it is clear this one isn't waiting on a review

klion26 · 2025-08-18T03:54:23Z

@alamb, please help review this when you have time. Thanks. This is ready to review now.

klion26 · 2025-08-18T03:56:29Z

parquet-variant-compute/src/type_conversion.rs

Changed to a separate macro other than a macro rules in the same macro as before, I think the current implementation is better for the user to choose which one they need.

👍

I was thinking about if there is some way to avoid defining 2 macros for each type of array, but given the branches are different (array.is_null --> null or Variant::Null ) I suspect the complexity of refactoring required would outweight any "don't repeat yourself" benefits

klion26 · 2025-08-18T04:47:43Z

parquet-variant-compute/src/variant_get/output/mod.rs

Will add the remaining types in a follow-up pr

alamb

Thank you @klion26 -- I think this makes sense to me and moves us towards a nice set of "convert arrow --> variant" and back again

cc @carpecodeum @mprammer and @scovich

alamb · 2025-08-18T20:01:08Z

parquet-variant-compute/src/type_conversion.rs

👍

I was thinking about if there is some way to avoid defining 2 macros for each type of array, but given the branches are different (array.is_null --> null or Variant::Null ) I suspect the complexity of refactoring required would outweight any "don't repeat yourself" benefits

alamb · 2025-08-18T20:03:42Z

parquet-variant-compute/src/type_conversion.rs

alamb · 2025-08-20T18:23:37Z

This PR has a conflict. Can you resolve it @klion26 ?

Thank you!

alamb · 2025-08-20T18:23:47Z

FYI @scovich

scovich

The approach generally looks good, but I'm not sure how it fits with e.g.

#8166 ?

(I'm guessing the infrastructure for casting arrays remains useful, and the call/use site would just change if/when the other PR merges?)

scovich · 2025-08-20T19:52:22Z

parquet-variant-compute/src/type_conversion.rs

nit: why not just:

Suggested change

continue;

}

$builder.append_variant(Variant::from(array.value(i)));

} else {

$builder.append_variant(Variant::from(array.value(i)));

}

I'm fine with the ruturn/break/continue as soon as possible way, but if we have a strong preference here, I can change it.

Definitely not a strong preference, no need to change if you like it as-is.

(I personally prefer early return/continue/etc any time the else block would be more than one line because it starts to save a lot of indentation. For one line, it's about the same either way)

scovich · 2025-08-20T20:07:42Z

parquet-variant-compute/src/type_conversion.rs

It seems like this is the only "real" macro, and the other two are just special cases?
But for that to work, this macros shouldn't take $input and $method as separate arguments, so caller is e.g.

- non_generic_conversion_array!(as_boolean, |v| v, input, builder); + non_generic_conversion_array!(input.as_boolean(), |v| v, builder);

If that's acceptable then we can do:

macro_rules! non_generic_conversion_array { ($array:expr, $cast_fn:expr, $builder:expr) => {{ let array = $array; ... }}; }

and then

macro_rules! generic_conversion_array { ($t:ty, $method:ident, $cast_fn:expr, $input:expr, $builder:expr) => { non_generic_conversion_array!($input.$method::<$t>, $cast_fn, $builder) }; }

and then

macro_rules! primitive_conversion { ($t:ty, $input:expr, $builder:expr) => { generic_conversion_array!($t, as_primitive, |v| v, $input, $builder) }; }

A similar factoring should work for the xxx_conversion_scalar macros.

Added in a separate commit. The code looks cleaner than before. In the new version, there's something changed for the user. If we want to use the primitive_conversion macro, we may need to import the three macros(encounter this in parquet-variant-compute/src/variant_get/output.variant.rs).

Oh! I forgot about that. You just have to use fully qualified paths, starting with $crate:

macro_rules! primitive_conversion { ($t:ty, $input:expr, $builder:expr) => { $crate::type_conversion::generic_conversion_array!($t, as_primitive, |v| v, $input, $builder) }; }

etc

And if you want the macro to be usable outside the current crate, then instead:

use self as parquet_variant_compute; macro_rules! primitive_conversion { ($t:ty, $input:expr, $builder:expr) => { parquet_variant_compute::type_conversion::generic_conversion_array!($t, as_primitive, |v| v, $input, $builder) }; }

... which allows the same path to work both inside and outside the crate.

Tho I suspect it would still break if somebody brought in the crate with a different name?

I'm not sure those definitions should even be public tho? Why not ditch the #[macro_export] and:

macro_rules! primitive_conversion { ($t:ty, $input:expr, $builder:expr) => { $crate::type_conversion::generic_conversion_array!($t, as_primitive, |v| v, $input, $builder) }; } pub(crate) use primitive_conversion;

scovich · 2025-08-20T20:32:33Z

parquet-variant-compute/src/type_conversion.rs

variant doesn't support negative scale?

Oh, I see -- you're manually applying the scale and producing a scale=0 decimal result.

scovich · 2025-08-20T20:33:12Z

parquet-variant-compute/src/type_conversion.rs

Suggested change

// For example: 123 with scale -2 becomes 12300

// For example: 123 with scale -2 becomes 12300 with scale 0

scovich · 2025-08-20T20:33:58Z

parquet-variant-compute/src/type_conversion.rs

Suggested change

let multiplier = (10 as $value_type).pow((-*$scale) as u32);

let multiplier = <$value_type>::pow(10, (-*$scale) as u32);

scovich · 2025-08-20T20:38:08Z

parquet-variant-compute/src/type_conversion.rs

Are these fully accurate boundary checks? I worry that the truncating integer division could have corner cases.

I think we can do:

v.checked_mul(multiplier) .and_then(|v| <$variant_type>::try_new(v, 0).ok()) .map_or(Variant::Null, Into::into)

scovich · 2025-08-20T20:41:55Z

parquet-variant-compute/src/type_conversion.rs

Suggested change

if *$scale < 0 {

// For negative scale, we need to multiply the value by 10^|scale|

// For example: 123 with scale -2 becomes 12300

let multiplier = (10 as $value_type).pow((-*$scale) as u32);

// Check for overflow

if $v > 0 && $v > <$value_type>::MAX / multiplier {

return Variant::Null;

}

if $v < 0 && $v < <$value_type>::MIN / multiplier {

return Variant::Null;

}

<$variant_type>::try_new($v * multiplier, 0)

.map(|v| v.into())

.unwrap_or(Variant::Null)

} else {

<$variant_type>::try_new($v, *$scale as u8)

.map(|v| v.into())

.unwrap_or(Variant::Null)

}

let (v, scale) = if *$scale < 0 {

// For negative scale, we need to multiply the value by 10^-scale

// For example: 123 with scale -2 becomes 12300 with scale 0

let multiplier = <$value_type>::pow(10, (-*$scale) as u32);

($v.checked_mul($v), 0u8)

} else {

(Some($v), *$scale as u8)

};

$v.and_then(|v| <$variant_type>::try_new(v, scale).ok())

.map_or(Variant::Null, Into::into)

scovich · 2025-08-20T20:45:15Z

parquet-variant-compute/src/type_conversion.rs

Isn't this just non_generic_conversion_array?

Yes, I've removed the redundant macros. My bad for not figuring it out when copying it.

scovich · 2025-08-20T20:47:17Z

parquet-variant-compute/src/type_conversion.rs

Taking the advice above for non_generic_conversion_array:

Suggested change

let array = $input.$method::<$offset_type>();

for i in 0..array.len() {

if array.is_null(i) {

$builder.append_null();

continue;

}

let cast_value = $cast_fn(array.value(i));

$builder.append_variant(Variant::from(cast_value));

}

non_generic_conversion_array!($input.$method::<$offset_type>(), $cast_fn, $builder)

scovich · 2025-08-20T21:42:23Z

parquet-variant-compute/src/type_conversion.rs

Suggested change

macro_rules! primitive_conversion {

macro_rules! primitive_conversion_array {

(to match the others?)

klion26

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

klion26 · 2025-08-21T02:40:20Z

parquet-variant-compute/src/type_conversion.rs

klion26 · 2025-08-21T05:10:42Z

parquet-variant-compute/src/type_conversion.rs

I'm fine with the ruturn/break/continue as soon as possible way, but if we have a strong preference here, I can change it.

klion26 · 2025-08-21T05:13:04Z

parquet-variant-compute/src/type_conversion.rs

Yes, I've removed the redundant macros. My bad for not figuring it out when copying it.

klion26 · 2025-08-21T05:37:30Z

parquet-variant-compute/src/type_conversion.rs

klion26 · 2025-08-21T05:42:26Z

parquet-variant-compute/src/type_conversion.rs

Added in a separate commit. The code looks cleaner than before. In the new version, there's something changed for the user. If we want to use the primitive_conversion macro, we may need to import the three macros(encounter this in parquet-variant-compute/src/variant_get/output.variant.rs).

scovich

Looks good, but please double check that return in primitive_conversion_single_value -- I'm pretty sure it's incorrect and only works by accident today.

scovich · 2025-08-21T12:43:14Z

parquet-variant-compute/src/cast_to_variant.rs

+            generic_conversion_array!(
                Float16Type,
                as_primitive,
                |v: f16| -> f32 { v.into() },


aside:

Suggested change

|v: f16| -> f32 { v.into() },

f32::from,

scovich · 2025-08-21T12:45:31Z

parquet-variant-compute/src/cast_to_variant.rs

+            generic_conversion_array!(
                Decimal256Type,
                as_primitive,
                |v: i256| {


aside: L237-241 below could simplify to just:

v.to_i128().map_or( Variant::Null, decimal_to_variant_decimal!(v, scale, i128, VariantDecimal16, )

We can't change this, because compile thinks that v passed into the macro is i256, and seems we can't cast it when calling the macro.

Oh, I had a silly typo, sorry --

v.to_i128().map_or( Variant::Null, |v| decimal_to_variant_decimal!(v, scale, i128, VariantDecimal16), )

(missing |v| in the closure)

scovich · 2025-08-21T12:48:26Z

parquet-variant-compute/src/cast_to_variant.rs

+                    generic_conversion_array!(
                        Time64NanosecondType,
                        as_primitive,
                        |v| NaiveTime::from_num_seconds_from_midnight_opt(


aside: @alamb -- I'm not sure CI is running fmt against this file? At least, I've never seen it willing to omit trailing commas for non-macro invocations (L412), and it always formats multi-line lambdas with curly braces even tho I'd personally prefer it didn't:

|v| { NaiveTime::foo( a, b, ) }

scovich · 2025-08-21T12:50:03Z

parquet-variant-compute/src/type_conversion.rs

+    ($t:ty, $input:expr, $index:expr) => {{
+        let array = $input.as_primitive::<$t>();
+        if array.is_null($index) {
+            return Variant::Null;


return from a macro seems dangerous/wrong? It would return from whatever function invoked the macro which is probably not what the caller expected? Is it even what the macro writer intended? To return Variant::Null from the function on NULL, but the macro invocation produces a normal Variant otherwise?

Yes, return from a macro seems wrong, changed the implementation.

klion26 · 2025-08-21T13:37:09Z

@scovich Thanks for the quick review. I've addressed the comments. Please take another look.

klion26 · 2025-08-21T13:51:21Z

Fixed in the latest commit
The last commit added #[allow(unused_imports)] before the use half::f16;, because if we don't add this, CI will complain that unused import: half::f16`, but if we remove this import, then the ci complains as this (sample as below)

~~And also not sure why the CI didn't fail before, the import half::f16 has been added some time ago in #8073.~~

...
error[E0658]: the type `f16` is unstable
   --> parquet-variant-compute/src/cast_to_variant.rs:827:22
    |
827 |                 Some(f16::MIN),
    |                      ^^^
...

scovich · 2025-08-21T14:49:55Z

parquet-variant-compute/src/type_conversion.rs

@@ -54,7 +55,7 @@ macro_rules! non_generic_conversion_single_value {
 #[macro_export]


nit: There's no point (publicly) exporting a macro that references $crate -- compilation will fail for any use sites outside this crate. Can do pub(crate) use generic_conversion_array to make it visible everywhere inside the crate.

scovich · 2025-08-21T14:58:34Z

The last commit added #[allow(unused_imports)] before the use half::f16;, because if we don't add this, CI will complain that unused import: half::f16`, but if we remove this import, then the ci complains as this (sample as below)

And also not sure why the CI didn't fail before, the import half::f16 has been added some time ago in #8073.
...
error[E0658]: the type `f16` is unstable
   --> parquet-variant-compute/src/cast_to_variant.rs:827:22
    |
827 |                 Some(f16::MIN),
    |                      ^^^
...

Ah! The latter is in the #[cfg(test)] mod test which has a use super::*. When compiling the normal code, the test flag is not set and so the import is indeed unused. If you just move the use half::f16 to mod test the problem should go away.

scovich · 2025-08-21T15:04:02Z

parquet-variant-compute/src/type_conversion.rs

+macro_rules! generic_conversion_single_value {
+    ($t:ty, $method:ident, $cast_fn:expr, $input:expr, $index:expr) => {{
+        let array = $input.$method::<$t>();


Can this not be expressed in terms of non_generic_conversion_single_value?

scovich · 2025-08-21T15:04:31Z

parquet-variant-compute/src/type_conversion.rs

+macro_rules! primitive_conversion_single_value {
+    ($t:ty, $input:expr, $index:expr) => {{
+        let array = $input.as_primitive::<$t>();


Can this not be expressed in terms of generic_conversion_single_value?

scovich · 2025-08-21T15:05:56Z

Nothing but nits left at this point, can fix them before or after merge.

klion26 · 2025-08-21T15:43:01Z

@scovich Thank you very much for the detailed and patient review. Addressed the comments in the last commit. One more thing: I changed all the macros to pub(crate) as currently, they are being used in the current compute crate; I think aligning them to the same visibility is better.

alamb · 2025-08-23T10:53:17Z

Thanks again @klion26 and @scovich

klion26 · 2025-08-26T05:28:51Z

Thanks again very much for the careful review! @alamb @scovich

klion26 commented Aug 11, 2025

View reviewed changes

alamb reviewed Aug 13, 2025

View reviewed changes

klion26 commented Aug 14, 2025

View reviewed changes

alamb marked this pull request as draft August 14, 2025 16:58

klion26 force-pushed the variant_value_get branch from caa9420 to d682bfd Compare August 18, 2025 01:03

github-actions bot added the parquet-variant parquet-variant* crates label Aug 18, 2025

klion26 force-pushed the variant_value_get branch 2 times, most recently from 19e0b2b to 363f9ad Compare August 18, 2025 01:15

klion26 marked this pull request as ready for review August 18, 2025 01:24

klion26 commented Aug 18, 2025

View reviewed changes

klion26 requested a review from alamb August 18, 2025 04:47

alamb approved these changes Aug 18, 2025

View reviewed changes

scovich reviewed Aug 20, 2025

View reviewed changes

scovich mentioned this pull request Aug 20, 2025

[Variant] Support typed access for numeric types in variant_get #8179

Merged

klion26 added 2 commits August 21, 2025 13:51

Extract type_conversion and support typed value get for DataType::Int16

b124132

address comment

f813b08

klion26 force-pushed the variant_value_get branch from 783cb22 to f813b08 Compare August 21, 2025 05:52

refine macros

07c45ce

klion26 commented Aug 21, 2025

View reviewed changes

scovich approved these changes Aug 21, 2025

View reviewed changes

klion26 added 3 commits August 21, 2025 21:12

refine macro

74e0f38

address comments

e1752b7

fix style

efa327a

fix clippy check

6d1c929

scovich reviewed Aug 21, 2025

View reviewed changes

address comments

59239b3

alamb merged commit 81867eb into apache:main Aug 23, 2025
12 checks passed

klion26 deleted the variant_value_get branch August 26, 2025 05:28

-                continue;
-            }
-            $builder.append_variant(Variant::from(array.value(i)));
+            } else {
+                $builder.append_variant(Variant::from(array.value(i)));
+            }

	// For example: 123 with scale -2 becomes 12300
	// For example: 123 with scale -2 becomes 12300 with scale 0

	let multiplier = (10 as $value_type).pow((-*$scale) as u32);
	let multiplier = <$value_type>::pow(10, (-*$scale) as u32);

-        if *$scale < 0 {
-            // For negative scale, we need to multiply the value by 10^|scale|
-            // For example: 123 with scale -2 becomes 12300
-            let multiplier = (10 as $value_type).pow((-*$scale) as u32);
-            // Check for overflow
-            if $v > 0 && $v > <$value_type>::MAX / multiplier {
-                return Variant::Null;
-            }
-            if $v < 0 && $v < <$value_type>::MIN / multiplier {
-                return Variant::Null;
-            }
-            <$variant_type>::try_new($v * multiplier, 0)
-                .map(|v| v.into())
-                .unwrap_or(Variant::Null)
-        } else {
-            <$variant_type>::try_new($v, *$scale as u8)
-                .map(|v| v.into())
-                .unwrap_or(Variant::Null)
-        }
+        let (v, scale) = if *$scale < 0 {
+            // For negative scale, we need to multiply the value by 10^-scale
+            // For example: 123 with scale -2 becomes 12300 with scale 0
+            let multiplier = <$value_type>::pow(10, (-*$scale) as u32);
+            ($v.checked_mul($v), 0u8)
+        } else {
+            (Some($v), *$scale as u8)
+        };
+        $v.and_then(|v| <$variant_type>::try_new(v, scale).ok())
+            .map_or(Variant::Null, Into::into)

-        let array = $input.$method::<$offset_type>();
-        for i in 0..array.len() {
-            if array.is_null(i) {
-                $builder.append_null();
-                continue;
-            }
-            let cast_value = $cast_fn(array.value(i));
-            $builder.append_variant(Variant::from(cast_value));
-        }
+        non_generic_conversion_array!($input.$method::<$offset_type>(), $cast_fn, $builder)

	macro_rules! primitive_conversion {
	macro_rules! primitive_conversion_array {

		@@ -54,7 +55,7 @@ macro_rules! non_generic_conversion_single_value {
		#[macro_export]

Conversation

klion26 commented Aug 11, 2025 • 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 these changes tested?

Are there any user-facing changes?

Uh oh!

klion26 commented Aug 11, 2025

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!

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!

alamb commented Aug 14, 2025

Uh oh!

klion26 commented Aug 18, 2025

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!

alamb commented Aug 20, 2025

Uh oh!

alamb commented Aug 20, 2025

Uh oh!

scovich left a comment • 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!

klion26 Aug 21, 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!

klion26 commented Aug 11, 2025 •

edited

Loading

scovich left a comment •

edited

Loading

klion26 Aug 21, 2025 •

edited

Loading