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

superserious-dev · 2025-08-20T16:06:19Z

Which issue does this PR close?

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

Are these changes tested?

Yes

Are there any user-facing changes?

Can use variant_get for shredded numeric types

superserious-dev · 2025-08-20T16:11:30Z

parquet-variant/src/variant.rs

Needed to support DataType::Float16.

Unrelated to this issue: Seems like it would make sense to also add Variant::as_f16 for symmetry.

Filed [Variant] Add Variant::as_f16 #8228

superserious-dev · 2025-08-20T16:12:32Z

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

I moved this variable into the macro itself, although passing this into the macro would also work.

alamb · 2025-08-20T18:52:48Z

FYI @carpecodeum and @scovich do you have some time to review this PR?

scovich

Similar to

#8105

... it will be interesting to see how this ends up meshing with

#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-20T21:43:43Z

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

if https://github.com/apache/arrow-rs/pull/8105/files#diff-ed469f33b865066aa56a96aa5279ce67d4bf85ea34c7a8c12ffb16afef107b6fR23-R31 merges, this simplifies to:

Suggested change

for i in 0..$variant_array.len() {

if primitive_array.is_null(i) {

array_builder.append_null();

} else {

let value = primitive_array.value(i);

array_builder.append_variant(Variant::from(value));

}

}

primitive_conversion_array!($arrow_type, $typed_value, array_builder);

TBD whether it's worth defining the macro just to turn 3 lines into one?

Yeah, re-using that macro after it merges would work.

[Variant] Implement VariantArray::value for shredded variants #8105 has merged

I switched over to using the merged macro.

scovich · 2025-08-20T21:47:12Z

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

aside: I'm a bit surprised that fmt didn't simplify these match arms:

Suggested change

DataType::Int8 => {

cast_partially_shredded_primitive!(

typed_value,

variant_array,

Int8Type,

DataType::Int8

)

}

DataType::Int8 => cast_partially_shredded_primitive!(

typed_value,

variant_array,

Int8Type,

DataType::Int8

),

I wonder if it's having trouble inferring things across the macro boundary, so it doesn't know that it can simplify the match arms. I've noticed that macros sometimes behave strangely when it comes to formatting(and LSP diagnostics).

I actually suspect fmt just plain isn't running, for some reason? I've seen other places in this file that aren't formatted as I would have expected.

I did an experiment with messing up the formatting of the file and running cargo fmt manually. It modifies the file back to it's current state. I don't have any rustfmt.toml customizations, so I think this is the formatted result.

ok... weird, but I guess fmt does what it does

superserious-dev · 2025-08-22T00:38:59Z

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

Not sure why cargo fmt aligns the comment like this.

wild guess: it would pass 100 char limit if indented like the others

superserious-dev · 2025-08-22T00:44:31Z

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

Yeah, re-using that macro after it merges would work.

alamb

Thank you @superserious-dev and @scovich -- this PR looks good to me. I think it needs to have some merge conflicts resolved and we'll be ready go merge it.

alamb · 2025-08-26T10:26:10Z

parquet-variant/src/variant.rs

Filed [Variant] Add Variant::as_f16 #8228

alamb · 2025-08-26T10:27:54Z

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

Now that #8208 is done, we could use the ReadOnlyMetadataBuilder (as a follow on PR)

superserious-dev · 2025-08-27T19:03:36Z

Incorporated @scovich's suggestion and rebased on the latest main.

One question: This PR introduces a few macros(eg. numeric_perfectly_shredded_test,numeric_partially_shredded_test, numeric_perfectly_shredded_variant_array_fn, numeric_partially_shredded_variant_array_fn) to reduce some boilerplate with the numeric tests; @klion26 Introduced an alternate approach for using functions that are generic over PrimitiveArray

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

Lines 368 to 387 in 1dacecb

    
               fn perfectly_shredded_variant_array<T>(typed_value: PrimitiveArray<T>) -> ArrayRef 
        
               where 
        
                   T: arrow::datatypes::ArrowPrimitiveType, 
        
               { 
        
                   // At the time of writing, the `VariantArrayBuilder` does not support shredding. 
        
                   // so we must construct the array manually.  see https://github.com/apache/arrow-rs/issues/7895 
        
                   let (metadata, _value) = { parquet_variant::VariantBuilder::new().finish() }; 
        
                   let metadata = 
        
                       BinaryViewArray::from_iter_values(std::iter::repeat_n(&metadata, typed_value.len())); 
        
                   let struct_array = StructArrayBuilder::new() 
        
                       .with_field("metadata", Arc::new(metadata)) 
        
                       .with_field("typed_value", Arc::new(typed_value)) 
        
                       .build(); 
        
                   Arc::new( 
        
                       VariantArray::try_new(Arc::new(struct_array)).expect("should create variant array"), 
        
                   ) 
        
               }

. I can see pros and cons to each:

the macro approach reduces some boilerplate while making it harder to read the tests
the generic approach makes the tests easier to understand at the cost of some more boilerplate

Any suggestions for whether to keep the macros in this PR or switch over to the generic functions? There are also quite a few conversion left, so trying to consider what would work well for those.

StringTypes: DataType::Utf8, Utf8View, etc
BinaryTypes: Binary, BinaryView, etc
Boolean
Time Types
Date Types
Timestamp Types

scovich

Seems good enough to merge, possibly with a couple tweaks to tests (see below)

Open questions/follow-ups in my mind:

re

Can use variant_get for shredded numeric types

What happens if variant_get encounters a typed_value: LONG at the requested path, but the caller requested Some(Int32) type that doesn't match? Is there code somewhere else to handle the casting? (I don't see any unit tests that cover the case, either)
this PR modifies OutputBuilder and the types that implement it, while another open PR aims to delete the trait entirely:
- #8166
But that other PR seems to have stalled due to questions around null mask handling (**), and the tests in this PR will remain useful even if the implementation changes later.

Should we go ahead and merge this PR even knowing a lot of the code it touches will need to change Real Soon?

(**) Honestly, we should probably let the other PR merge without addressing the null mask question, because it's not making the problem worse.
re

the macro approach reduces some boilerplate while making it harder to read the tests

Any suggestions for whether to keep the macros in this PR or switch over to the generic functions?

I left a couple comments in the code with my suggestions. Nothing drastic.

scovich · 2025-08-27T19:14:45Z

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

+        ($test:ident, $data_fn:ident, $primitive_type:ty) => {
+            #[test]
+            fn $test() {


Since you're worried about readability -- I'm not sure we gain a lot by pulling the test function definition inside the macro? What if we left that to the caller, and swapped the remaining two args, so that:

numeric_partially_shredded_test!( get_variant_partially_shredded_int8_as_variant, partially_shredded_int8_variant_array, i8 );

becomes

#[test] fn get_variant_partially_shredded_int8_as_variant() { numeric_partially_shredded_test!(i8, partially_shredded_int8_variant_array); }

Updated. I agree that it looks more readable this way.

scovich · 2025-08-27T19:22:11Z

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

wild guess: it would pass 100 char limit if indented like the others

scovich · 2025-08-27T19:37:22Z

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

 use std::sync::Arc;

+macro_rules! cast_partially_shredded_primitive {
+    ($typed_value:expr, $variant_array:expr, $arrow_type:ty, $data_type:expr) => {{


$data_type arg is unused?

Good catch; removed.

scovich · 2025-08-27T19:46:45Z

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


+macro_rules! cast_partially_shredded_primitive {
+    ($typed_value:expr, $variant_array:expr, $arrow_type:ty, $data_type:expr) => {{
+        let mut array_builder = VariantArrayBuilder::new($variant_array.len());


This doesn't look quite right? Seems like we should either

Propagate the existing metadata column unchanged, or

Create a new column of empty metadata, since these are all primitives that don't need field names in the first place?

Actually... the current code is taking approach 2/, and is correct.

It could be optimized to just stamp out the empty metadata directly (or even use a dictionary array to reference the same one over and over).

If we pursued approach 1/, we wouldn't have to do any work at all -- just arc clone the existing column -- but we'd need to create a new kind of variant array builder that doesn't try to create a metadata column.

Either way, it seems like a potential TODO to track, rather than something to pursue in this PR.

scovich · 2025-08-27T19:50:12Z

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

        match typed_value.data_type() {
-            DataType::Int32 => {
-                let primitive_array = typed_value.as_primitive::<Int32Type>();
-                for i in 0..variant_array.len() {
-                    if variant_array.is_null(i) {
-                        array_builder.append_null();
-                        continue;
-                    }
-
-                    if typed_value.is_null(i) {
-                        // fall back to the value (variant) field
-                        // (TODO could copy the variant bytes directly)
-                        let value = variant_array.value(i);
-                        array_builder.append_variant(value);
-                        continue;
-                    }
-
-                    // otherwise we have a typed value, so we can use it directly
-                    let int_value = primitive_array.value(i);
-                    array_builder.append_variant(Variant::from(int_value));
-                }
-            }
+            DataType::Int8 => cast_partially_shredded_primitive!(


This feels like something that should be handled by downcast_primitive_array, except we don't support all primitive types yet?

This match is mostly dealing with arrow data types and primitive downcasting, which that macro should handle automagically?

downcast_primitive_array!( typed_value => { let mut array_builder = VariantArrayBuilder::new(variant_array.len()); for i in 0..variant_array.len() { if variant_array.is_null(i) { array_builder.append_null(); continue; } // We must use the typed_value, if present; otherwise fall back to the value let value = if typed_value.is_null(i) { variant_array.value(i) // TODO: copy the variant bytes directly? } else { typed_value.value(i).into() }; array_builder.append_variant(value); } Ok(Arc::new(array_builder.build())) } dt => { ... same error as today ... } }

superserious-dev · 2025-08-28T01:43:18Z

re casting, I left a comment here: #8087 (comment). Since casting is an extra step after typed access, I suggested implementing the conversion logic in #8086.

scovich · 2025-08-28T15:58:15Z

Can use variant_get for shredded numeric types

What happens if variant_get encounters a typed_value: LONG at the requested path, but the caller requested Some(Int32) type that doesn't match? Is there code somewhere else to handle the casting? (I don't see any unit tests that cover the case, either)

Any thoughts on this, @superserious-dev ?

superserious-dev · 2025-08-28T17:18:07Z

Can use variant_get for shredded numeric types

What happens if variant_get encounters a typed_value: LONG at the requested path, but the caller requested Some(Int32) type that doesn't match? Is there code somewhere else to handle the casting? (I don't see any unit tests that cover the case, either)

Any thoughts on this, @superserious-dev ?

Currently this PR only covers the case where the user has not requested a specific output type. I noticed there was a separate issue/discussion #8086 for casting, so I didn't implement it here. However I'd be fine with amending this PR to include the casting behavior for numeric types, if that is preferred.

For reference, the code for determining typed output(as_type is not None) vs. variant output(as_type is None) is here:

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

Lines 63 to 87 in 21a9a2a

    
           pub(crate) fn instantiate_output_builder<'a>( 
        
               options: GetOptions<'a>, 
        
           ) -> Result<Box<dyn OutputBuilder + 'a>> { 
        
               let GetOptions { 
        
                   as_type, 
        
                   path, 
        
                   cast_options, 
        
               } = options; 
        
               let Some(as_type) = as_type else { 
        
                   return Ok(Box::new(VariantOutputBuilder::new(path))); 
        
               }; 
        
               // handle typed output 
        
               match as_type.data_type() { 
        
                   DataType::Int32 => Ok(Box::new(PrimitiveOutputBuilder::<Int32Type>::new( 
        
                       path, 
        
                       as_type, 
        
                       cast_options, 
        
                   ))), 
        
                   dt => Err(ArrowError::NotYetImplemented(format!( 
        
                       "variant_get with as_type={dt} is not implemented yet", 
        
                   ))), 
        
               } 
        
           }

and it's called in variant_get here:

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

Line 45 in 1dacecb

let output_builder = instantiate_output_builder(options.clone())?;

There's a set of pre-existing tests that cover the specific case of int32 -> int32. This could be macro-ified for coverage of all the different combinations of numeric types, eg. int32->int64.

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

Lines 223 to 259 in 1dacecb

    
               /// Shredding: extract a value as an Int32Array 
        
               #[test] 
        
               fn get_variant_shredded_int32_as_int32_safe_cast() { 
        
                   // Extract the typed value as Int32Array 
        
                   let array = shredded_int32_variant_array(); 
        
                   // specify we want the typed value as Int32 
        
                   let field = Field::new("typed_value", DataType::Int32, true); 
        
                   let options = GetOptions::new().with_as_type(Some(FieldRef::from(field))); 
        
                   let result = variant_get(&array, options).unwrap(); 
        
                   let expected: ArrayRef = Arc::new(Int32Array::from(vec![ 
        
                       Some(34), 
        
                       None, 
        
                       None, // "n/a" is not an Int32 so converted to null 
        
                       Some(100), 
        
                   ])); 
        
                   assert_eq!(&result, &expected) 
        
               } 
        
               /// Shredding: extract a value as an Int32Array, unsafe cast (should error on "n/a") 
        
               #[test] 
        
               fn get_variant_shredded_int32_as_int32_unsafe_cast() { 
        
                   // Extract the typed value as Int32Array 
        
                   let array = shredded_int32_variant_array(); 
        
                   let field = Field::new("typed_value", DataType::Int32, true); 
        
                   let cast_options = CastOptions { 
        
                       safe: false, // unsafe cast 
        
                       ..Default::default() 
        
                   }; 
        
                   let options = GetOptions::new() 
        
                       .with_as_type(Some(FieldRef::from(field))) 
        
                       .with_cast_options(cast_options); 
        
                   let err = variant_get(&array, options).unwrap_err(); 
        
                   // TODO make this error message nicer (not Debug format) 
        
                   assert_eq!(err.to_string(), "Cast error: Failed to extract primitive of type Int32 from variant ShortString(ShortString(\"n/a\")) at path VariantPath([])"); 
        
               }

scovich · 2025-08-28T18:49:23Z

Can use variant_get for shredded numeric types

What happens if variant_get encounters a typed_value: LONG at the requested path, but the caller requested Some(Int32) type that doesn't match? Is there code somewhere else to handle the casting? (I don't see any unit tests that cover the case, either)

Any thoughts on this, @superserious-dev ?

Currently this PR only covers the case where the user has not requested a specific output type.

Ah, that makes it easy then!

Thanks for the clear explanation.

alamb · 2025-09-04T17:39:31Z

Sorry for the delay here. I was out for the last week but I am back now and catching up on PRs. I plan to merge this once CI passes

alamb · 2025-09-04T17:52:17Z

Thanks again @superserious-dev and @scovich

# Which issue does this PR close? - Closes #8150 - closes #8166 # Rationale for this change Add support for extracting fields from both shredded and non-shredded variant arrays at any depth (like "x", "a.x", "a.b.x") and casting them to Int32 with proper NULL handling for type mismatches. NOTE: This is a second attempt at * #8166 which suffered strong logical conflicts with * #8179 and which itself grew out of * #8122 See the other two PR for the vast majority of review commentary relating to this change. I started from the original PR commits (first three), performed the merge, and fixed up a bunch of issues. Manually diffing the before ([76b75ee..882aa4d](https://github.com/apache/arrow-rs/compare/76b75eebc..882aa4d69)) and after ([0ba91ae..f6fd915](https://github.com/apache/arrow-rs/compare/0ba91aed9..f6fd91583)) diffs gives the following non-trivial differences vs. the original PR * Ran `cargo fmt` * `typed_value_to_variant` now supports all primitive numeric types (previously only int16) * cast options plumbed through and respected * Fix a null buffer bug in `shredded_get_path` -- the original code was wrongly unioning in the null buffer from `typed_value` column: ```patch // Path exhausted! Create a new `VariantArray` for the location we landed on. - // Also union nulls from the final typed_value field we landed on - if let Some(typed_value) = shredding_state.typed_value_field() { - accumulated_nulls = arrow::buffer::NullBuffer::union( - accumulated_nulls.as_ref(), - typed_value.nulls(), - ); - } let target = make_target_variant( shredding_state.value_field().cloned(), shredding_state.typed_value_field().cloned(), accumulated_nulls, ); ``` * Remove the `get_variant_perfectly_shredded_int32_as_variant` test case, because #8179 introduced a battery of unit tests that cover the same functionality. * Remove now-unnecessary `.unwrap()` calls from object builder `finish` calls in unit tests * Fixed broken test code in `create_depth_1_shredded_test_data_working`, which captured the return value of a nested builder's `finish` (`()`) instead of the return value of the top-level builder. I'm not quite sure what this code was trying to do, but I changed it to just create a nested builder instead of a second top-level builder: ```patch fn create_depth_1_shredded_test_data_working() -> ArrayRef { // Create metadata following the working pattern from shredded_object_with_x_field_variant_array let (metadata, _) = { - let a_variant = { - let mut a_builder = parquet_variant::VariantBuilder::new(); - let mut a_obj = a_builder.new_object(); - a_obj.insert("x", Variant::Int32(55)); // "a.x" field (shredded when possible) - a_obj.finish().unwrap() - }; let mut builder = parquet_variant::VariantBuilder::new(); let mut obj = builder.new_object(); - obj.insert("a", a_variant); + + // Create the nested "a" object + let mut a_obj = obj.new_object("a"); + a_obj.insert("x", Variant::Int32(55)); + a_obj.finish(); + obj.finish().unwrap(); builder.finish() }; ``` * Similar fix (twice, `a_variant` and `b_variant`) for `create_depth_2_shredded_test_data_working` * `make_shredding_row_builder` now supports signed int and float types (unsigned int not supported yet) * A new `get_type_name` helper in row_builder.rs that gives human-readable data type names. I'm not convinced it's necessary (and the code is in the wrong spot, jammed in the middle of `VariantAsPrimitive` code. * `impl VariantAsPrimitive` for all signed int and float types * `PrimitiveVariantShreddingRowBuilder` now has a lifetime param because it takes a reference to cast options (it now respects unsafe vs. safe casting) # What changes are included in this PR? Everything in the original PR, plus merge in the main branch, fix logical conflicts and fix various broken tests. # Are these changes tested? All unit tests now pass. # Are there any user-facing changes? No (variant is not public yet) --------- Co-authored-by: carpecodeum <[email protected]> Co-authored-by: Andrew Lamb <[email protected]>

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

superserious-dev commented Aug 20, 2025

View reviewed changes

scovich reviewed Aug 20, 2025

View reviewed changes

superserious-dev force-pushed the typed-access-numeric-shredded branch from 95a4fab to db5183d Compare August 22, 2025 00:34

superserious-dev commented Aug 22, 2025

View reviewed changes

alamb mentioned this pull request Aug 26, 2025

[Variant] Add Variant::as_f16 #8228

Closed

alamb approved these changes Aug 26, 2025

View reviewed changes

superserious-dev and others added 3 commits August 27, 2025 11:10

[Variant] Support typed access for numeric types in variant_get

9bfc372

Merge branch 'main' into typed-access-numeric-shredded

0652b7c

Fix formatting

49390f4

superserious-dev force-pushed the typed-access-numeric-shredded branch from db5183d to 49390f4 Compare August 27, 2025 18:12

scovich approved these changes Aug 27, 2025

View reviewed changes

superserious-dev added 2 commits August 27, 2025 17:46

Remove unnecessary macro arg

df49760

Improve readability of tests

e3fefd1

Merge branch 'main' into typed-access-numeric-shredded

ae9948b

alamb merged commit 25cc570 into apache:main Sep 4, 2025
12 checks passed

alamb mentioned this pull request Sep 4, 2025

[Variant] Support Shredded Objects in variant_get #8166

Closed

This was referenced Sep 4, 2025

Merge arrow-rs/main into shredding-variant-part1 cmu-db/arrow-rs#5

Open

[Variant] Support Shredded Objects in variant_get (take 2) #8280

Merged

superserious-dev deleted the typed-access-numeric-shredded branch September 4, 2025 23:38

-        for i in 0..$variant_array.len() {
-            if primitive_array.is_null(i) {
-                array_builder.append_null();
-            } else {
-                let value = primitive_array.value(i);
-                array_builder.append_variant(Variant::from(value));
-            }
-        }
+        primitive_conversion_array!($arrow_type, $typed_value, array_builder);

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

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

Uh oh!

Conversation

superserious-dev commented Aug 20, 2025

Which issue does this PR close?

Are these changes tested?

Are there any user-facing changes?

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

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!

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!

superserious-dev commented Aug 27, 2025

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

superserious-dev commented Aug 28, 2025

Uh oh!

scovich commented Aug 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

superserious-dev commented Aug 28, 2025

Uh oh!

scovich left a comment •

edited

Loading

scovich commented Aug 28, 2025 •

edited

Loading