[Variant] Implement row builders for cast_to_variant #8299

scovich · 2025-09-09T01:40:35Z

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 [Variant] Use row-oriented builders in cast_to_variant #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

scovich

A few hopefully useful notes for reviewers

scovich · 2025-09-09T01:46:30Z

parquet-variant-compute/src/type_conversion.rs

 /// Convert the input array to a `VariantArray` row by row, using `method`
 /// not requiring a generic type to downcast the generic array to a specific
 /// array type and `cast_fn` to transform each element to a type compatible with Variant
+#[allow(unused)]


These macros are fully unused now that #8280 has merged, but I wasn't sure if they are useful to keep around "just in case" ?

scovich · 2025-09-09T01:48:38Z

parquet-variant-compute/src/variant_array_builder.rs

+    /// Appending NULL to a variant array produces an actual NULL value
+    fn append_null(&mut self) {


This new append_null capability was critical to making all this work:

Appending NULL to a variant array builder produces an actual NULL

Appending NULL to an object field builder is a no-op (field is missing, as per variant spec)

Otherwise, appending NULL to a variant value or variant list produces Variant::Null

This seems like it will overwrite any value that is currently set -- so if someone did

builder.append_value(..); builder.append_null(); // will wipe out the value ??

I am not sure this is a problem or not, I just wanted to confirm my understanding

That is correct. The variant builder API is pretty broken in general if builder methods are invoked more than once, so IMO this isn't making it any worse.

scovich · 2025-09-09T01:49:15Z

parquet-variant-json/src/from_json.rs

    Ok(())
 }

-struct ObjectFieldBuilder<'o, 'v, 's> {


Moved to builders.rs

TBH, I never quite understood why it was defined here in the first place.

scovich · 2025-09-09T01:50:48Z

parquet-variant-compute/src/cast_to_variant.rs

This file was mostly gutted (but unit tests left in place), with the real work now being done by row builders defined in arrow_to_variant.rs via make_arrow_to_variant_row_builder.

scovich · 2025-09-09T01:51:35Z

parquet-variant-compute/src/cast_to_variant.rs

+// TODO do we need a cast_with_options to allow specifying conversion behavior,
+// e.g. how to handle overflows, whether to convert to Variant::Null or return
+// an error, etc. ?


Neither changed nor moved... it's just diff weirdness.

parquet-variant-compute/src/arrow_to_variant.rs

scovich · 2025-09-09T02:13:12Z

parquet-variant-compute/src/arrow_to_variant.rs

+        // WARNING: normalized_keys panics if values is empty
+        let normalized_keys = match values.len() {
+            0 => Vec::new(),
+            _ => dict_array.normalized_keys(),
+        };


NOTE: I'm pretty sure the original code did not handle this corner case

scovich · 2025-09-09T02:16:19Z

parquet-variant-compute/src/arrow_to_variant.rs

+        Ok(())
+    }
+
+    fn set_run_for_index(&mut self, index: usize) -> Result<(), ArrowError> {


This was easily the most annoying part of the whole columnar/row-builder transformation -- there's technically no guarantee that subsequent append_row calls use sequentially increasing index values, so we have to handle jumps both forward and backward while hopefully being efficient in the common case of sequential index values.

ANother reason that a batch API might be better

parquet-variant-compute/src/arrow_to_variant.rs

scovich · 2025-09-09T02:30:24Z

CC @alamb @carpecodeum

scovich

Attn @codephage2020 -- this PR conflicts with

#8233

Most of the code that other PR touched has been deleted by this PR, so I did my best to capture the intent in the new framework.

While resolving conflicts, I also had to expand the define_row_builder! macro to handle fallible conversions, which increased its complexity a bit. Hopefully still readable. The merge commit 26ae5c0 actually makes it pretty easy to see what changed.

scovich · 2025-09-09T20:17:59Z

parquet-variant-compute/src/cast_to_variant.rs

-/// Options for controlling the behavior of `cast_to_variant_with_options`.
-#[derive(Debug, Clone, PartialEq, Eq)]
-pub struct CastOptions {
-    /// If true, return error on conversion failure. If false, insert null for failed conversions.
-    pub strict: bool,
-}
-
-impl Default for CastOptions {
-    fn default() -> Self {
-        Self { strict: true }
-    }
-}


Moved to type_conversions.rs

scovich · 2025-09-09T20:18:42Z

parquet-variant-compute/src/type_conversion.rs

-    }};
+/// Options for controlling the behavior of `cast_to_variant_with_options`.
+#[derive(Debug, Clone, PartialEq, Eq)]
+pub struct CastOptions {


Moved from cast_to_variant.rs

scovich · 2025-09-09T20:19:11Z

parquet-variant/src/builder.rs

 }

+/// A [`VariantBuilderExt`] that inserts a new field into a variant object.
+pub struct ObjectFieldBuilder<'o, 'v, 's> {


Moved from from_json.rs

alamb

Thank you @scovich . I went through this fairly carefully and it is very impressive. I like the API that hides converting an ArrowArray and writes to a VariantBuilderExt 👍

My biggest comment / suggestion is to consider making the API vectorized (convert the entire Arrow Array) but I think we can do that as a follow on PR

cc @liamzwbao @codephage2020 @carpecodeum and @klion26 for your reviewing pleasure.

Given the amount of code in this PR and likelihood of conflicts, I would like to merge it sooner rather than later. Please everyone give a shout out if they want more time to review, or if they are happy to review post merge

alamb · 2025-09-09T20:44:04Z

parquet-variant-compute/src/variant_array_builder.rs

+    /// Appending NULL to a variant array produces an actual NULL value
+    fn append_null(&mut self) {


This seems like it will overwrite any value that is currently set -- so if someone did

builder.append_value(..); builder.append_null(); // will wipe out the value ??

I am not sure this is a problem or not, I just wanted to confirm my understanding

alamb · 2025-09-09T20:48:50Z

parquet-variant-compute/src/arrow_to_variant.rs

+}
+
+impl<'a> ArrowToVariantRowBuilder<'a> {
+    pub fn append_row(


is there any reason in the future this couldn't be vectorized?

Something like

pub fn append_all_rows( &mut self, builder: &mut impl VariantBuilderExt, ...

as written here it is going to have a per-row type dispatch

I think we could do it as a follow on PR potentially if needed

alamb · 2025-09-09T20:50:00Z

parquet-variant-compute/src/arrow_to_variant.rs

+}
+
+/// Factory function to create the appropriate row builder for a given DataType
+pub(crate) fn make_arrow_to_variant_row_builder<'a>(


alamb · 2025-09-09T20:51:31Z

parquet-variant-compute/src/arrow_to_variant.rs

+                }
+            }
+
+            fn append_row(&self, builder: &mut impl VariantBuilderExt, index: usize) -> Result<(), ArrowError> {


as mentioned above, it might eventually be a good idea to make a vectorized version of this method (that gets given something that can create VariantBuilderExt) but let's get this working first and then optimize later if/when needed

alamb · 2025-09-09T20:52:14Z

parquet-variant-compute/src/arrow_to_variant.rs

+
+define_row_builder!(
+    struct Decimal32ArrowToVariantBuilder<'a> {
+        scale: i8,


this is quite clever

alamb · 2025-09-09T20:54:11Z

parquet-variant-compute/src/arrow_to_variant.rs

+        Ok(())
+    }
+
+    fn set_run_for_index(&mut self, index: usize) -> Result<(), ArrowError> {


ANother reason that a batch API might be better

alamb · 2025-09-09T20:58:35Z

parquet-variant-compute/src/arrow_to_variant.rs

+        let mut variant_builder = array_builder.variant_builder();
+        row_builder.append_row(&mut variant_builder, 0).unwrap();
+        variant_builder.finish();
+
+        // Test null value
+        let mut variant_builder = array_builder.variant_builder();
+        row_builder.append_row(&mut variant_builder, 1).unwrap();
+        variant_builder.finish();
+
+        // Test second value
+        let mut variant_builder = array_builder.variant_builder();
+        row_builder.append_row(&mut variant_builder, 2).unwrap();
+        variant_builder.finish();


This code segment is repeated many times. Maybe we could make a function like this

fn copy_all(input_array: &dyn Array) -> VariantArray { for i in 0..input_array.len() { let mut variant_builder = array_builder.variant_builder(); row_builder.append_row(&mut variant_builder, 1).unwrap(); variant_builder.finish(); } }

A more ideal version in my mind would convert the entire Array (not just a row at a time). Something like this (untested). Subsets of rows could be handled via slice()ing the Array

// make conevrter let mut row_builder = make_arrow_to_variant_row_builder(int_array.data_type(), &int_array, &options).unwrap(); // create output variant array let mut variant_builder = array_builder.variant_builder(); // convert all rows from the array row_builder.convert(&mut array_builder).unwrap();

Thanks for pointing out the boilerplate explosion. It should be fixed in the latest commit (~400 LoC eliminated)

alamb · 2025-09-09T21:00:11Z

parquet-variant-compute/src/cast_to_variant.rs

-            )));
-        }
+    // Process each row using the row builder
+    for i in 0..input.len() {


that is certainly nice

scovich · 2025-09-09T21:34:06Z

Re

My biggest comment / suggestion is to consider making the API vectorized (convert the entire Arrow Array) but I think we can do that as a follow on PR

And #8299 (comment) -- that run-end encoding could be handled more easily in a vectorized API.

And #8299 (comment) that suggests an append_all_rows() method.

And #8299 (comment) that also wonders about vectorization.

I'll try to give one response that covers them all:

I think it's reasonable to consider adding a bulk append type API, but we have to be cognizant of the limitations and challenges it will face:

We will need a new trait that knows how to create (and finish!) variant builder instances
Variant building is inherently row-based, so any builder that ultimately needs to produce a variant array or variant object as its output will have a trivial append_all_rows that just calls append_row in a loop (like today), in order to recursively build up the fields/elements of the variant it creates.
The API would be very nice for converting primitive arrays to variant, because they don't need to recurse on anything. Also nice because we could potentially define a specialized impl just for VariantArrayBuilder, so we don't have to deal with that new variant builder create+finish trait.
Casting a list of primitive values is an interesting intermediate case, where one should be able to append all the elements of a given list in one shot. But that might require the new create+finish trait? Or maybe it just needs a second specialization for ListBuilder?
Maybe instead of a no-arg append_all_rows(), we should consider a ranged append_many_rows(start..end)? One could always pass .. to request encoding of all rows.

klion26 · 2025-09-10T06:22:35Z

Given the amount of code in this PR and likelihood of conflicts, I would like to merge it sooner rather than later. Please everyone give a shout out if they want more time to review, or if they are happy to review post merge

I'm happy to review post-merge. I'm following/understanding the code now, and don't have any blocking issue for now. The implementation is cool; I've learned a lot from this PR. I will post my comments if there are any, after the PR has been merged.

codephage2020

While resolving conflicts, I also had to expand the define_row_builder! macro to handle fallible conversions, which increased its complexity a bit. Hopefully still readable.

Really impressive work—Learned a lot, and no issues found. Everything looks solid to me! Thanks!

scovich · 2025-09-10T12:03:45Z

@alamb -- I have a few cosmetic cleanups to push (in a few minutes) and then this can merge whenever you're ready

scovich · 2025-09-10T14:37:41Z

Once this merges, I did some pathfinding on the bulk append and will push a couple (stacked) PR to start the discussion.

alamb · 2025-09-10T19:00:12Z

Awesome -- thanks @scovich and @codephage2020 -- this is really cool

klion26 · 2025-09-15T03:01:37Z

parquet-variant-compute/src/arrow_to_variant.rs

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


These added tests are very nice tests!

scovich added 30 commits September 8, 2025 16:23

Define VariantBuilderExt::append_null

61c3efb

checkpoint - primitive builders

ea1147f

manual fixup

bd565cf

checkpoint - change to enum instead of trait

358e2f8

manual fixup

e6caaaf

manual fixup

470fffb

checkpoint - run end

f129824

manual cleanup

74d7da1

simplify run end management

9f361fd

checkpoint - dictionary array

471a8ab

manual cleanup

82ccc23

checkpoint - lists

e08e07c

checkpoint - lists w/o slicing

87a3678

manual cleanup

2ac5f6d

checkpoint - maps and null map fix

4d8da85

manual cleanup

70a0c75

checkpoint - unions

b9239bb

manual cleanup

5b5ede7

checkpoint - float16

18bfe04

checkpoint - decimal

b5eaaa4

manual fixup

24a1d44

checkpoint - binary and string

9db05f1

manual fixes

abd9378

checkpoint - use offset size trait for string and binary buildres

582dc7f

checkpoint - timstamps

1c3fcf3

manual fixes

dc2d2ae

checkpoint - avoid deprecated timestamp functions

493de5d

only need to track whether a timestamp has a time zone

9de68eb

checkpoint - date and time

805abad

checkpoint - template date and time builders

29d445a

scovich added 7 commits September 8, 2025 16:23

checkpoint - macro supports extra fields

ad9ccf8

manual fix - macro hygiene

f5675de

checkpoint - better hygiene solution

4e9c697

manual fix - remove turbofish

637efde

fmt

20300c8

diff minimization

a8c667d

split out a new module for the builders

583f7b8

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

scovich mentioned this pull request Sep 9, 2025

[Variant] Support Shredded Objects in variant_get #8166

Closed

scovich added 2 commits September 8, 2025 18:45

fmt

c3603d6

Merge remote-tracking branch 'oss/main' into cast-to-variant-row-builder

5607503

scovich commented Sep 9, 2025

View reviewed changes

self review

22a7745

klion26 mentioned this pull request Sep 9, 2025

[Variant] cast_to_variant conflates empty map with NULL #8289

Closed

scovich added 2 commits September 9, 2025 06:43

simpler macro usage

c9fdb8c

Merge remote-tracking branch 'oss/main' into cast-to-variant-row-builder

26ae5c0

scovich commented Sep 9, 2025

View reviewed changes

alamb approved these changes Sep 9, 2025

View reviewed changes

codephage2020 approved these changes Sep 10, 2025

View reviewed changes

scovich added 2 commits September 10, 2025 05:16

cosmetic cleanups

96256a3

helper functions to reduce boiler plate in unit tests

6622e86

alamb merged commit dc9c3cd into apache:main Sep 10, 2025
13 checks passed

scovich mentioned this pull request Sep 11, 2025

[Variant] Support bulk-appends in cast_to_variant #8323

Open

klion26 reviewed Sep 15, 2025

View reviewed changes

		/// Appending NULL to a variant array produces an actual NULL value
		fn append_null(&mut self) {

[Variant] Implement row builders for cast_to_variant #8299

[Variant] Implement row builders for cast_to_variant #8299

Uh oh!

Conversation

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

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

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

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!

Choose a reason for hiding this comment

Uh oh!

scovich commented Sep 9, 2025

Uh oh!

klion26 commented Sep 10, 2025

Uh oh!

codephage2020 left a comment

Choose a reason for hiding this comment

Uh oh!

scovich commented Sep 10, 2025

scovich commented Sep 9, 2025 •

edited

Loading

scovich Sep 9, 2025 •

edited

Loading

klion26 Sep 15, 2025 •

edited

Loading