[Variant] Implement new VariantValueArrayBuilder #8360

scovich · 2025-09-16T12:02:20Z

Which issue does this PR close?

Pre-work for [Variant] Implement a shred_variant function #8361

Rationale for this change

There is currently no good way to populate a new variant array with variant values that reference an existing metadata, but that functionality is needed when transforming existing variant data (e.g. for shredding and unshredding operations).

What changes are included in this PR?

Add a new VariantValueArrayBuilder that does not try to create new metadata; instead, it wraps a ReadOnlyMetadata around the VariantMetadata instance of the Variant value being inserted. This takes advantage of the new generic ParentState capability.

NOTE: The new array builder does not impl VariantBuilderExt because it does not have a MetadataBuilder instance -- the instance is created on demand as part of the insertion itself. Instead, callers can directly invoke VariantValueArrayBuilder::parent_state(). This approach avoids the considerable complexity of keeping an internal metadata column index in sync with whatever external indexing might produce the variant value to be appended. It also doesn't seem to matter -- I did some pathfinding of variant shredding (going from binary to shredded variant based on some target schema), and the VariantBuilderExt does not seem especially helpful for that code.

Are these changes tested?

New unit tests.

Are there any user-facing changes?

New class.

scovich · 2025-09-16T12:28:40Z

parquet-variant-compute/src/variant_array_builder.rs

+    }
+
+    /// Creates a builder-specific parent state
+    pub fn parent_state<'a>(


NOTE: This is pub to compensate for the fact that VariantValueArrayBuilder cannot impl VariantBuilderExt (because the metadata builder is created on-demand instead of once up front). See PR description for details.

(also see the updated unit test for a realistic example of how to use it)

scovich · 2025-09-16T12:38:35Z

@alamb -- this (along with the other small PR posted today) is the final groundwork for a new shred_variant function I've been prototyping. That function takes a binary variant as input, along with a target shredding schema (such as a primitive or struct). It converts the requested schema into an actual physical shredding schema with value and typed_value columns, and then attempts to shred the data into a new variant array with that schema.

It turns out that true shredding is just different enough from variant_get that it's worth implementing separately.

Implementing an actual shredding function also bypasses our current lack of an extension type (so there's actually no way to request shredding via variant_get).

scovich · 2025-09-16T12:59:34Z

this (along with the other small PR posted today) is the final groundwork for a new shred_variant function I've been prototyping. That function takes a binary variant as input, along with a target shredding schema (such as a primitive or struct). It converts the requested schema into an actual physical shredding schema with value and typed_value columns, and then attempts to shred the data into a new variant column with that schema.

Tracked as #8361

alamb

Thanks @scovich -- this looks great to me

I had some small test suggestions, but otherwise this looks great to me

alamb · 2025-09-16T18:46:59Z

parquet-variant-compute/src/variant_array_builder.rs

+/// the value column. It's useful for scenarios like variant unshredding, data
+/// transformation, or filtering where you want to reuse existing metadata.
+///
+/// The builder produces a [`BinaryViewArray`] that can be combined with existing


makes sense

alamb · 2025-09-16T18:47:42Z

parquet-variant-compute/src/variant_array_builder.rs

+/// let mut builder = VariantValueArrayBuilder::new(10);
+///
+/// // Append some values with their corresponding metadata
+/// // In practice, some of the variant values would be objects with internal metadata.


I think the metadata would be pre-existing (not "internal")?

Suggested change

/// // In practice, some of the variant values would be objects with internal metadata.

/// // In practice, some of the variant values would be objects with pre-existing metadata.

Eventually it would be nice to show an example of appending a pre-pre-existing Variant value, but I don't think it is necessary here

Sorry, I'm not sure I understand? Do you mean that I should call append_value on the result of VariantArray::value, instead of creating the variant values from thin air?

I think the metadata would be pre-existing (not "internal")?

I was trying to say that Variant::metadata() might return Some, if it's a complex type (vs. these primitive values that have no metadata). I'll try rewording?

it would be nice to show an example of appending a pre-existing Variant value

Check out the updated unit test?

parquet-variant-compute/src/variant_array_builder.rs

scovich · 2025-09-17T05:40:54Z

parquet-variant-compute/src/variant_to_arrow.rs

-        // TODO: We need a way to convert a Variant directly to bytes. In particular, we want to
-        // just copy across the underlying value byte slice of a `Variant::Object` or
-        // `Variant::List`, without any interaction with a `VariantMetadata` (because the shredding
-        // spec requires us to reuse the existing metadata when unshredding).
-        //
-        // One could _probably_ emulate this with parquet_variant::VariantBuilder, but it would do a
-        // lot of unnecessary work and would also create a new metadata column we don't need.


@alamb -- I just realized while putting up #8366 that the new VariantValueArrayBuilder is perfect for fixing this TODO (in addition to supporting shredding).

So now it has an immediate use, instead of only being a building block for the next PR.

scovich · 2025-09-17T05:42:30Z

parquet-variant/src/builder.rs

 impl WritableMetadataBuilder {
    /// Upsert field name to dictionary, return its ID
-    fn upsert_field_name(&mut self, field_name: &str) -> u32 {
+    pub fn upsert_field_name(&mut self, field_name: &str) -> u32 {


It probably should have been public all along, but now it's needed.

scovich · 2025-09-17T05:43:19Z

parquet-variant/src/variant.rs

    ///
    /// Returns `Some(&VariantMetadata)` for object and list variants,
-    pub fn metadata(&self) -> Option<&'m VariantMetadata<'_>> {
+    pub fn metadata(&self) -> Option<&VariantMetadata<'m>> {


This had the wrong lifetime specification, which led to weird borrow checker issues in this PR because the resulting VariantMetadata was tied to the lifetime of self instead of 'm -- even if cloned.

scovich · 2025-09-17T13:35:47Z

CC @codephage2020 @klion26 -- you might be interested in this change

alamb · 2025-09-17T19:29:37Z

I'll plan to merge this tomorrow unless @klion26 or @codephage2020 would like additional time to review

klion26

LGTM.

klion26 · 2025-09-17T11:30:55Z

parquet-variant-compute/src/variant_array_builder.rs

+    /// builder.append_value(Variant::from(42));
+    /// ```
+    pub fn append_value(&mut self, value: Variant<'_, '_>) {
+        let metadata = value.metadata().cloned().unwrap_or(EMPTY_VARIANT_METADATA);


Maybe not related to the current pr. Currently, we'll return None for Variant::metadata() if it's not object or list, do we need to return EMPTY_VARIANT_METDATA?

That's a great question!

Until now, that method was only used in tests, so it wasn't clear what the best semantics might be (plus, we didn't have the constant back then). I like the idea a lot tho -- it would eliminate the unwrap_or (which my next PR also has to use).

klion26 · 2025-09-18T03:32:51Z

parquet-variant-compute/src/variant_array_builder.rs

+    }
+
+    #[test]
+    fn test_variant_value_array_builder_with_objects() {


alamb · 2025-09-18T13:29:44Z

Thanks everyone -- I'll merge this PR to keep the code moving and we can keep iterating with future PRs.

scovich added 3 commits September 16, 2025 04:03

[Variant] Add constants for empty variant metadata

ab77733

make const instead of static

428aae1

[Variant] Implement new VariantValueArrayBuilder

97f99da

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

scovich commented Sep 16, 2025

View reviewed changes

scovich added 2 commits September 16, 2025 05:28

fix doctest

fb455c0

remove unneeded unused markers

2238b49

doc fixes

474fa31

alamb reviewed Sep 16, 2025

View reviewed changes

scovich added 2 commits September 16, 2025 20:02

Merge remote-tracking branch 'oss/main' into variant-value-builder

5959577

review feedback

f286c52

scovich force-pushed the variant-value-builder branch from 0a76ed6 to f286c52 Compare September 17, 2025 04:12

scovich requested a review from alamb September 17, 2025 04:13

binary variant row builder uses variant value builder

a476c72

scovich mentioned this pull request Sep 17, 2025

[Variant] Define new shred_variant function #8366

Merged

scovich commented Sep 17, 2025

View reviewed changes

alamb approved these changes Sep 17, 2025

View reviewed changes

klion26 reviewed Sep 18, 2025

View reviewed changes

klion26 approved these changes Sep 18, 2025

View reviewed changes

scovich added 2 commits September 17, 2025 22:21

review feedback

2126c7a

nit

fa2cded

scovich mentioned this pull request Sep 18, 2025

[WIP] Support Shredded Lists/Array in variant_get #8354

Draft

alamb merged commit f4840f6 into apache:main Sep 18, 2025
17 checks passed

	/// // In practice, some of the variant values would be objects with internal metadata.
	/// // In practice, some of the variant values would be objects with pre-existing metadata.

[Variant] Implement new VariantValueArrayBuilder #8360

[Variant] Implement new VariantValueArrayBuilder #8360

Uh oh!

Conversation

scovich commented Sep 16, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

scovich commented Sep 16, 2025

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!

scovich Sep 17, 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!

Uh oh!

Uh oh!

Uh oh!

scovich Sep 17, 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!

scovich Sep 17, 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!

scovich commented Sep 17, 2025

Uh oh!

alamb commented Sep 17, 2025

Uh oh!

klion26 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

scovich Sep 18, 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!

alamb commented Sep 18, 2025

Uh oh!

Uh oh!

Uh oh!

scovich commented Sep 16, 2025 •

edited

Loading

scovich commented Sep 16, 2025 •

edited

Loading

scovich Sep 17, 2025 •

edited

Loading

scovich Sep 17, 2025 •

edited

Loading

scovich Sep 17, 2025 •

edited

Loading

scovich Sep 18, 2025 •

edited

Loading