Add Arrow Variant Extension Type, remove `Array` impl for `VariantArray` and `ShreddedVariantFieldArray` #8392

alamb · 2025-09-19T16:57:45Z

Which issue does this PR close?

closes [Variant] VariantArray::data_type returns StructType, causing Array::as_struct to panic #8319
closes [Variant] writing a VariantArray to parquet panics #8296

Rationale for this change

This is needed to read/write the Variant Parquet logical type and work with the rest of the Arrow Ecosystem

Note, this is broken out the larger PR here:

from Path Finding: Support reading/writing VariantArray to parquet with Variant LogicalType #8365

We need a way to write Variant encoded data to/from parquet, and the current way the VariantArray is implemented doesn't work (panics when writing to parquet). See tickets above

Instead of a impl Array it seems the better way to do this is using an Arrow Extension Type. See #8319 (comment) for more details

What changes are included in this PR?

remove the Array impl for VariantArray, which forces explict conversions back/forth when reading/writing
remove the Array impl for ShreddedVariantFieldArray, which forces explicit conversions back/forth when reading/writing
Add VariantType extension type

Are these changes tested?

Yes, with new code and tests

Are there any user-facing changes?

Yes, but this is not yet stable / released, so these changes have no impact on the releasability of this code

alamb · 2025-09-19T17:43:11Z

parquet-variant-compute/src/variant_array.rs

-use std::any::Any;
 use std::sync::Arc;

+/// Arrow Variant [`ExtensionType`].


Here is the new canonical variant type

alamb · 2025-09-19T17:43:58Z

parquet-variant-compute/src/variant_array.rs

+}
+
 /// An array of Parquet [`Variant`] values
 ///


I tried to update these docstrings to explain what is going on here and how to use the extension types

alamb · 2025-09-19T17:44:57Z

parquet-variant-compute/src/variant_array.rs

+    pub fn try_new(inner: &dyn Array) -> Result<Self, ArrowError> {
+        // Workaround lack of support for Binary
+        // https://github.com/apache/arrow-rs/issues/8387
+        let inner = cast_to_binary_view_arrays(inner)?;


This cast is unfortunate, but I think it is the best (simplest) solution while we work on the real solution

[Variant] Support BinaryArray in addition to BinaryViewArray for Variant #8387

alamb · 2025-09-19T17:45:44Z

parquet-variant-compute/src/variant_array.rs

+    }
+
+    /// Returns a new DataType representing this VariantArray's inner type
+    pub fn data_type(&self) -> &DataType {


these methods are just moved from the Array impl

alamb · 2025-09-19T17:45:58Z

parquet-variant-compute/src/variant_array.rs

        };

-        // Extract value and typed_value fields (metadata is not expected in ShreddedVariantFieldArray)
-        let value = inner_struct


this code is moved to ShreddingState

alamb · 2025-09-19T17:47:30Z

parquet-variant-compute/src/variant_array.rs

+    /// let struct_array: StructArray = get_struct_array();
+    /// let shredding_state = ShreddingState::try_from(&struct_array).unwrap();
+    /// ```
+    pub fn new(value: Option<BinaryViewArray>, typed_value: Option<ArrayRef>) -> Self {


This is now infallable per @scovich 's suggestion #8365 (comment)

alamb · 2025-09-19T17:47:48Z

parquet-variant-compute/src/variant_array.rs

    }
 }

-impl Array for VariantArray {


this was moved up in the file

alamb · 2025-09-19T17:49:06Z

parquet-variant-compute/src/variant_get.rs

+pub(crate) enum ShreddedPathStep {
    /// Path step succeeded, return the new shredding state
-    Success(&'a ShreddingState),
+    Success(ShreddingState),


this is the only part I am a little sad about -- that it now requires owing the ShreddingState which requires clone the state. While this is typically just a few Arcs it is still unfortunate

However, unless we change VariantArray to be reference based (e.g. VariantArray<'a>) I can't think of any way around it

I mean, if somebody is concerned about cloning ArrayRef like it's free... arrow-rs might be the wrong project for them to look very closely at?

Some potential ideas:

Have the caller of variant_get pass an owned VariantArray, and then deconstruct it using some kind of into_parts method? Caller anyway had to clone the various innards of the &dyn Array it would have started out with? But that has the distinct disadvantage of producing a logically read-only operation that consumes its input.

If we're ok with VariantArray being an exclusively local thing (i.e. never returned from functions), we could make both it and ShreddingState take references to their various arrays. That would produce VariantArray<'a>, ShreddingState<'a>, etc.

Make ShreddingState track Option<Cow<...>> internally, which might solve most of the ergonomics issues of 2/ without overly polluting the public API. The only downside is a generic lifetime parameter: VariantArray<'a>, ShreddingState<'a>, etc.

I think BinaryViewArrays also have a Vec that will get cloned. However, I think it is ok to clone for now, as the path / cloning happens once per array and so is likely amortized over a batch.

I tried option 1 briefly but I couldn't get it to work.

I'll let benchmarking be our guide and if we see problems with this clone, then perhaps we can pursue options 2 or 3.

alamb · 2025-09-19T17:49:52Z

parquet-variant-compute/src/variant_get.rs

-            "expected a VariantArray as the input for variant_get".to_owned(),
-        )
-    })?;
+    let variant_array = VariantArray::try_new(input)?;


I will admit the new try_new is quite a bit more ergonomic

alamb · 2025-09-19T17:50:21Z

parquet/tests/variant_integration.rs

            .column_by_name("var")
            .unwrap_or_else(|| panic!("No 'var' column found in parquet file {path:?}"));

-        // the values are read as


this code is moved to VariantArray::try_new

alamb · 2025-09-19T17:51:06Z

@scovich @klion26 and @codephage2020 this PR is ready for review

Copilot

Pull Request Overview

This PR removes the Array implementation from VariantArray and ShreddedVariantFieldArray and introduces a new VariantType extension type to properly represent Variant data in the Arrow ecosystem. This change enables proper integration with Parquet I/O operations which previously failed due to implementation issues.

Removes Array trait implementations that were causing Parquet integration problems
Introduces VariantType extension type following Arrow conventions for canonical representation
Updates all code to use explicit conversions between VariantArray and ArrayRef

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
parquet-variant-compute/src/variant_array.rs	Implements `VariantType` extension type, removes `Array` trait impls, adds conversion methods
parquet-variant-compute/src/variant_get.rs	Updates to use explicit `VariantArray` conversions instead of downcasting
parquet-variant-compute/src/variant_to_arrow.rs	Updates return type construction for `VariantArray`
parquet-variant-compute/src/variant_array_builder.rs	Updates `VariantArray` construction call
parquet-variant-compute/src/lib.rs	Exports new `VariantType`
parquet/tests/variant_integration.rs	Removes casting workaround code that's now handled internally
parquet-variant-compute/benches/variant_kernels.rs	Updates array conversion

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Copilot · 2025-09-19T17:52:12Z

parquet-variant-compute/src/variant_array.rs

            inner: inner.clone(),
            metadata: metadata.clone(),
-            shredding_state: ShreddingState::try_new(value, typed_value)?,
+            shredding_state: ShreddingState::new(value, typed_value),


The ShreddingState::new method is called here but the old try_new method was removed. Consider adding error handling if ShreddingState::new can potentially fail, or document why this is safe by construction.

parquet-variant-compute/src/variant_get.rs

alamb · 2025-09-19T21:03:37Z

After merging up from #8366, a new shred_object test is now failing:

called `Result::unwrap()` on an `Err` value: InvalidArgumentError("Found unmasked nulls for non-nullable StructArray field \"score\"")

I don't fully understand what this means and haven't had a chance to debug it. @scovich any ideas?

scovich · 2025-09-19T22:22:09Z

After merging up from #8366, a new shred_object test is now failing:
called `Result::unwrap()` on an `Err` value: InvalidArgumentError("Found unmasked nulls for non-nullable StructArray field \"score\"")
I don't fully understand what this means and haven't had a chance to debug it. @scovich any ideas?

The error means that some non-nullable column has null mask bits that are not covered by any ancestor column.

The fields of a shredded variant object are not nullable -- NULL is encoded by the child value and typed_value both being NULL. So it's possible that something is wrongly marking a row of some shredded object field as NULL, even tho the path to that value is non-NULL?

It's also possible that some struct (or variant array) is being wrongly constructed with some non-nullable fields that should actually be nullable?

scovich · 2025-09-19T22:53:11Z

The offending array is below.

I notice a couple weird things:

Looking at the test itself, the top-level null mask should only have one NULL, at index 6. I don't know how index 5, 8, and 9 are also NULL??
The null masks for score are also weird.

validity: [
  valid,
  valid,
  valid,
  valid,
  null,
  null,
  valid,
  null,
  null,
]

score: StructArray
-- validity:
[
  valid,
  valid,
  null,
  valid,
  null,
  null,
  null,
  null,
  valid,
]
[
-- child 0: "value" (BinaryView)
BinaryViewArray
[
  null,
  null,
  null,
  [45, 110, 105, 110, 101, 116, 121, 45, 102, 105, 118, 101],
  null,
  null,
  null,
  null,
  null,
]
-- child 1: "typed_value" (Float64)
PrimitiveArray<Float64>
[
  95.5,
  87.2,
  null,
  null,
  null,
  null,
  null,
  null,
  66.67,
]
]

age: StructArray
-- validity:
[
  valid,
  valid,
  valid,
  valid,
  null,
  null,
  null,
  null,
  null,
]
[
-- child 0: "value" (BinaryView)
BinaryViewArray
[
  null,
  null,
  null,
  [25, 116, 104, 105, 114, 116, 121],
  null,
  null,
  null,
  null,
  null,
]
-- child 1: "typed_value" (Int64)
PrimitiveArray<Int64>
[
  30,
  25,
  35,
  null,
  null,
  null,
  null,
  null,
  null,
]
]

scovich · 2025-09-20T00:12:59Z

@alamb -- Somehow you've managed to trigger a latent bug in my variant shredding code. I have no idea how the test ever passed before, nor why your PR should have anything to do with anything. But here's the fix that I confirmed unbreaks your PR when I stacked them both locally:

[Variant] Fix NULL handling for shredded object fields #8395

# Which issue does this PR close? - Fast-follow for #8366 - Related to #8392 # Rationale for this change Somehow, #8392 exposes a latent bug in #8366, which has improper NULL handling for shredded object fields. The shredding PR originally attempted to handle this case, but somehow the test did not trigger the bug and so the (admittedly incomplete) code was removed. See #8366 (comment). To be honest, I have no idea how the original ever worked correctly, nor why the new PR is able to expose the problem. # What changes are included in this PR? When used as a top-level builder, `VariantToShreddedVariantRowBuilder::append_null` must append NULL to its own `NullBufferBuilder`; but when used as a shredded object field builder, it must append non-NULL. Plumb a new `top_level` parameter through the various functions and into the two sub-builders it relies on, so they can implement the correct semantics. # Are these changes tested? In theory, yes (I don't know how the object shredding test ever passed). And it fixes the breakage in #8392. # Are there any user-facing changes? No

alamb · 2025-09-20T00:19:04Z

@alamb -- Somehow you've managed to trigger a latent bug in my variant shredding code. I have no idea how the test ever passed before, nor why your PR should have anything to do with anything. But here's the fix that I confirmed unbreaks your PR when I stacked them both locally:

[Variant] Fix NULL handling for shredded object fields #8395

Thanks! I merged that in and merged up from main

scovich

LGTM. The only open question was about shredding state cloning, but that doesn't affect correctness AFAIK

scovich · 2025-09-20T00:27:28Z

parquet-variant-compute/src/variant_array.rs

+        // Note don't check for metadata/value fields here because they may be
+        // absent in shredded variants


This doesn't sound quite right -- AFAIK the extension type should only annotate the top-level variant. Any shredded object fields or array elements nested inside should be identified according to spec, because at that point we know we're traversing a shredded variant (by virtue of having stepped into the typed_value column of a VariantArray. If anything looks out of place when trying to step into a shredded struct field or array element, that's just plain an error. No annotations needed to figure that out.

Even if, for some reason, we decide it's important not to require metadata, we should still enforce that the only fields present are some subset of metadata, value and typed_value, where the first two are binary. TBD whether we recurse into typed_value to verify the rest of the structure -- we probably should, but that could become expensive if people are converting from ArrayRef to VariantArray to ArrayRef a lot.

sounds good -- removed the comments

parquet-variant-compute/src/variant_array.rs

scovich · 2025-09-20T00:32:25Z

parquet-variant-compute/src/variant_array.rs

    }
+    /// Is the element at index null?
+    pub fn is_null(&self, index: usize) -> bool {
+        self.nulls().map(|n| n.is_null(index)).unwrap_or_default()


Isn't this just

Suggested change

self.nulls().map(|n| n.is_null(index)).unwrap_or_default()

self.nulls().map_or(false, |n| n.is_null(index))

Yes, and in fact when I did that clippy told me it could be simplified further to

self.nulls().is_some_and( |n| n.is_null(index))

parquet-variant-compute/src/variant_array.rs

parquet-variant-compute/src/variant_get.rs

Co-authored-by: Ryan Johnson <scovich@users.noreply.github.com>

github-actions bot added parquet Changes to the parquet crate parquet-variant parquet-variant* crates labels Sep 19, 2025

Remove Array impl for VariantArray and ShreddedVariantFieldArray`

bb11814

alamb force-pushed the alamb/variant_array branch from 750ac8a to bb11814 Compare September 19, 2025 17:05

alamb mentioned this pull request Sep 19, 2025

Path Finding: Support reading/writing VariantArray to parquet with Variant LogicalType #8365

Draft

alamb added 4 commits September 19, 2025 13:19

ArrayRef::from consistently

5cc767e

Avoid unecessary &

3e6b0cb

Introduce ShreddingState::try_from

9b0e89f

ShreddingState is infallable

4b1daab

alamb commented Sep 19, 2025

View reviewed changes

alamb marked this pull request as ready for review September 19, 2025 17:50

alamb requested a review from Copilot September 19, 2025 17:50

Copilot AI reviewed Sep 19, 2025

View reviewed changes

alamb mentioned this pull request Sep 19, 2025

[Variant] Define new shred_variant function #8366

Merged

alamb added 2 commits September 19, 2025 16:58

Merge remote-tracking branch 'apache/main' into alamb/variant_array

3306992

fix test

61812ba

scovich mentioned this pull request Sep 20, 2025

[Variant] Fix NULL handling for shredded object fields #8395

Merged

Merge remote-tracking branch 'apache/main' into alamb/variant_array

d1830b1

scovich approved these changes Sep 20, 2025

View reviewed changes

alamb added 4 commits September 20, 2025 05:45

remove confusing comments

b0d9059

Simplify VariantType::try_new

179e5f3

Simplify is_null

32fbb35

map_type --> rewrite_to_view

ba82096

alamb and others added 2 commits September 20, 2025 05:53

Improve formatting of rewrite_to_view_types

ddca836

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

b871df4

Co-authored-by: Ryan Johnson <scovich@users.noreply.github.com>

		// Note don't check for metadata/value fields here because they may be
		// absent in shredded variants

	self.nulls().map(\|n\| n.is_null(index)).unwrap_or_default()
	self.nulls().map_or(false, \|n\| n.is_null(index))

Add Arrow Variant Extension Type, remove Array impl for VariantArray and ShreddedVariantFieldArray #8392

Are you sure you want to change the base?

Add Arrow Variant Extension Type, remove Array impl for VariantArray and ShreddedVariantFieldArray #8392

Uh oh!

Conversation

alamb commented Sep 19, 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!

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 commented Sep 19, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Sep 19, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

alamb commented Sep 19, 2025

Uh oh!

scovich commented Sep 19, 2025

Uh oh!

scovich commented Sep 19, 2025

Uh oh!

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

Uh oh!

Uh oh!

alamb commented Sep 20, 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!

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!

Uh oh!

Uh oh!

Add Arrow Variant Extension Type, remove `Array` impl for `VariantArray` and `ShreddedVariantFieldArray` #8392

Add Arrow Variant Extension Type, remove `Array` impl for `VariantArray` and `ShreddedVariantFieldArray` #8392

alamb commented Sep 19, 2025 •

edited

Loading

scovich commented Sep 20, 2025 •

edited

Loading