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

scovich · 2025-09-04T23:02:23Z

Which issue does this PR close?

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

[Variant] Support Shredded Objects in variant_get #8166
which suffered strong logical conflicts with
[Variant] Support typed access for numeric types in variant_get #8179
and which itself grew out of
[WIP][Variant] Very rough pathfinding for variant get/shredding #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 (76b75eebc..882aa4d69) and after (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:

  // 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 [Variant] Support typed access for numeric types in variant_get #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:

  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)

Resolves conflicts between PR 8166 (shredding support) and PR 8179 (multi-type support): - Preserves PR 8179's comprehensive multi-type support for all numeric primitives - Keeps PR 8166's superior row builder architecture and shredding support - Integrates both test suites for complete coverage - Maintains enhanced path parsing from PR 8166 The merge successfully combines: - Multi-type variant_get support (Int8, Int16, Int32, Int64, UInt8, UInt16, UInt32, UInt64, Float16, Float32, Float64) - Advanced shredding capabilities with row builder approach - Comprehensive test coverage from both PRs

scovich · 2025-09-04T23:36:30Z

attn @carpecodeum @alamb

carpecodeum · 2025-09-04T23:43:18Z

attn @carpecodeum @alamb

Thank you @scovich for this

alamb · 2025-09-05T16:13:47Z

attn @carpecodeum @alamb

Thank you @scovich for this

Perhaps you would like to merge this PR @carpecodeum ?

scovich · 2025-09-05T16:19:36Z

I'm also totally fine with people plundering this PR so the one with all the comment history can merge. And if somebody knows how to make it a useful/correct PR against the other PR (@alamb seems to be expert in this), that's another possibility.

Either way, I'd also hope we can catalog all the follow-up items before merging so they don't get lost. I think some (like null mask unioning) were already addressed, and I just didn't notice until yesterday.

alamb · 2025-09-05T16:31:39Z

I'm also totally fine with people plundering this PR so the one with all the comment history can merge. And if somebody knows how to make it a useful/correct PR against the other PR (@alamb seems to be expert in this), that's another possibility.

Either way, I'd also hope we can catalog all the follow-up items before merging so they don't get lost. I think some (like null mask unioning) were already addressed, and I just didn't notice until yesterday.

I will double check / catalog before I merge anything

carpecodeum · 2025-09-06T15:34:49Z

attn @carpecodeum @alamb

Thank you @scovich for this

Perhaps you would like to merge this PR @carpecodeum ?

Perhaps saw this a bit too late, I am fine with both the approaches honestly, I will review any other PR that stems from #8166

alamb · 2025-09-08T10:51:46Z

@scovich could you take this branch and create a new PR against arrow-rs (rather than the cmu fork) that we can merge?

scovich · 2025-09-08T11:42:25Z

@scovich could you take this branch and create a new PR against arrow-rs (rather than the cmu fork) that we can merge?

This PR is against apache:main, I think it can just merge as-is?

scovich · 2025-09-08T12:12:03Z

Oh, but the PR description still talks as if it were a merge to the other branch. Let me clean that up.

alamb · 2025-09-08T14:36:50Z

I will review this in a few hours. THank you so much @scovich and @carpecodeum

…t1-merge-main

alamb

Thank you @scovich @carpecodeum and @liamzwbao -- I think this is a big step forward. I realize it still needs a bunch of work, but I think it is good enough to merge at this point as it is significantly better than what is on main.

One thing I don't feel I have a good handle on yet is where we are in terms of what is supported and what is left to go. I think I will spend some more time working on tests to figure that out next.

Onwards!

alamb · 2025-09-08T17:03:57Z

parquet-variant-compute/src/variant_array.rs

-    /// Partially shredded:
-    /// * value is an object
-    /// * typed_value is a shredded object.
+    /// Imperfectly shredded: Shredded values reside in `typed_value` while those that failed to


thank you -- this made it much clearer to me

alamb · 2025-09-08T17:07:19Z

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

+    }
+}
+
+/// Helper trait for converting `Variant` values to arrow primitive values.


as a follow on, this trait might be more discoverable if we put it somewhere in parquet-variant-compute/src/type_conversion.rs

alamb · 2025-09-08T17:08:09Z

parquet-variant/src/path.rs

 }

-/// Create from &str
+/// Create from &str with support for dot notation


this probably eventually needs support for escaping, etc but is probably fine for now

alamb · 2025-09-08T17:15:34Z

I pushed a commit to remove an unnecessary #allow(unused) annotation and merge up from main.

Note to reviewers: This PR includes 1600+ LoC of new unit tests. The actual changes are half that big. # 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 #8310 # Rationale for this change The original `cast_to_variant` code did columnar conversions of types to variant. For primitive types this worked ok, but for deeply nested types it means repeatedly creating new variants (and variant metadata), only to re-code them by copying the variant values to new arrays (with new metadata and field ids). Very expensive. # What changes are included in this PR? Follow the example of #8280, and introduce a row builder concept that takes individual array values and writes them to an `impl VariantBuilderExt`. Row builders for complex types instantiate the appropriate list or object builder to pass to their children. # Are these changes tested? Existing unit tests continue to pass. Extensive new unit tests added as well. # Are there any user-facing changes? * `VariantBuilderExt` has a new `append_null` method. * `ObjectFieldBuilder` moved to `builder.rs` and made public

# Which issue does this PR close? * Quick-follow to #8280 # Rationale for this change The change from output builders to row builders left some no-longer-used files behind. # What changes are included in this PR? Delete the unused files. # Are these changes tested? N/A # Are there any user-facing changes? No

# Which issue does this PR close? * Follow-up to #8280 # Rationale for this change See #8280 (comment) # What changes are included in this PR? See description. # Are these changes tested? Code movement. Compilation suffices. # Are there any user-facing changes? No. --------- Co-authored-by: Andrew Lamb <[email protected]>

scovich and others added 11 commits August 22, 2025 14:24

[Variant] Very rough pathfinding for variant get/shredding

81270f1

[ADD] add shredding support for variant objects

2326b55

[FIX] remove unused annotation

882aa4d

checkpoint

a3e1934

checkpoint

04cac48

fmt

f365a23

checkpoint

59341f6

checkpoint

60179ce

checkpoint

89b3c75

manual cleanups

f6fd915

github-actions bot added the parquet-variant parquet-variant* crates label Sep 4, 2025

move definition to a better place

a32ccdb

scovich mentioned this pull request Sep 4, 2025

[Variant] Support Shredded Objects in variant_get #8166

Closed

scovich marked this pull request as ready for review September 8, 2025 11:42

scovich changed the title ~~Merge arrow-rs/main into cmu-db/shredding-variant-part1~~ [Variant] Support Shredded Objects in variant_get (take 2) Sep 8, 2025

alamb added 2 commits September 8, 2025 13:10

remove uneeded allow(unused)

7a66cfb

Merge remote-tracking branch 'apache/main' into shredding-variant-par…

3e4fafc

…t1-merge-main

alamb approved these changes Sep 8, 2025

View reviewed changes

alamb merged commit fb7d02e into apache:main Sep 8, 2025
12 checks passed

scovich mentioned this pull request Sep 9, 2025

[Variant] Implement row builders for cast_to_variant #8299

Merged

This was referenced Sep 11, 2025

[Variant] Remove unused output builder files #8320

Merged

[Variant] Move VariantAsPrimitive to type_conversions.rs #8321

Merged

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

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

Uh oh!

Conversation

scovich commented Sep 4, 2025 • edited by alamb 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!

scovich commented Sep 4, 2025

Uh oh!

carpecodeum commented Sep 4, 2025

Uh oh!

alamb commented Sep 5, 2025

Uh oh!

scovich commented Sep 5, 2025

Uh oh!

alamb commented Sep 5, 2025

Uh oh!

carpecodeum commented Sep 6, 2025

Uh oh!

alamb commented Sep 8, 2025

Uh oh!

scovich commented Sep 8, 2025

Uh oh!

scovich commented Sep 8, 2025

Uh oh!

alamb commented Sep 8, 2025

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

alamb Sep 8, 2025

Choose a reason for hiding this comment

Uh oh!

alamb Sep 8, 2025

Choose a reason for hiding this comment

Uh oh!

alamb Sep 8, 2025

Choose a reason for hiding this comment

Uh oh!

alamb commented Sep 8, 2025

Uh oh!

Uh oh!

Uh oh!

scovich commented Sep 4, 2025 •

edited by alamb

Loading