Skip to content

Variant integration fixes #8438

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mbrobbel merged 13 commits into from
Sep 25, 2025
Merged

Variant integration fixes #8438

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
e08bcaa
[Variant] Fix several incorrect variant integration test cases
scovich Sep 24, 2025
4f45954
more fixes; remove illegal unit tests
scovich Sep 24, 2025
811f4a5
Merge remote-tracking branch 'oss/main' into variant-integration-fixes
scovich Sep 24, 2025
127e3ae
self review
scovich Sep 24, 2025
fe4d628
Merge branch 'main' into variant-integration-fixes
alamb Sep 24, 2025
c98edee
review feedback
scovich Sep 24, 2025
ab129d9
tighten up illegal shredding type checks
scovich Sep 24, 2025
54056fe
comment
scovich Sep 24, 2025
8a2e4af
forgot to delete Float16 tests
scovich Sep 24, 2025
edc7ecd
decimal comment
scovich Sep 24, 2025
6a6e067
Merge branch 'main' into variant-integration-fixes
scovich Sep 25, 2025
5de3489
fix comments
scovich Sep 25, 2025
8dd3ab7
decimal precision fix
scovich Sep 25, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
85 changes: 32 additions & 53 deletions parquet-variant-compute/src/variant_array.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ use arrow::buffer::NullBuffer;
use arrow::compute::cast;
use arrow::datatypes::{
Date32Type, Float16Type, Float32Type, Float64Type, Int16Type, Int32Type, Int64Type, Int8Type,
UInt16Type, UInt32Type, UInt64Type, UInt8Type,
};
use arrow_schema::extension::ExtensionType;
use arrow_schema::{ArrowError, DataType, Field, FieldRef, Fields};
Expand Down Expand Up @@ -353,37 +352,18 @@ impl VariantArray {
/// Note: Does not do deep validation of the [`Variant`], so it is up to the
/// caller to ensure that the metadata and value were constructed correctly.
pub fn value(&self, index: usize) -> Variant<'_, '_> {
match &self.shredding_state {

@scovich scovich Sep 24, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There was already substantial logic duplication among the different match arms, and it only got worse once typed_value_to_variant started requiring the value column (needed for both error checking now, and later when handling partially shredded objects). It turned out that directly referencing the two fields was a lot simpler.

@scovich scovich Sep 25, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Follow-up that continues this line of thought:

ShreddingState::Unshredded { value, .. } => {
// Unshredded case
Variant::new(self.metadata.value(index), value.value(index))
}
ShreddingState::Typed { typed_value, .. } => {
// Typed case (formerly PerfectlyShredded)
if typed_value.is_null(index) {
Variant::Null
} else {
typed_value_to_variant(typed_value, index)
}
}
ShreddingState::PartiallyShredded {
value, typed_value, ..
} => {
// PartiallyShredded case (formerly ImperfectlyShredded)
if typed_value.is_null(index) {
Variant::new(self.metadata.value(index), value.value(index))
} else {
typed_value_to_variant(typed_value, index)
}
match (self.typed_value_field(), self.value_field()) {
// Always prefer typed_value, if available
(Some(typed_value), value) if typed_value.is_valid(index) => {
typed_value_to_variant(typed_value, value, index)
}
ShreddingState::AllNull => {
// AllNull case: neither value nor typed_value fields exist
// NOTE: This handles the case where neither value nor typed_value fields exist.
// For top-level variants, this returns Variant::Null (JSON null).
// For shredded object fields, this technically should indicate SQL NULL,
// but the current API cannot distinguish these contexts.
Variant::Null
// Otherwise fall back to value, if available
(_, Some(value)) if value.is_valid(index) => {
Variant::new(self.metadata.value(index), value.value(index))
}
// It is technically invalid for neither value nor typed_value fields to be available,
// but the spec specifically requires readers to return Variant::Null in this case.
_ => Variant::Null,
}
}

Expand Down Expand Up @@ -796,8 +776,17 @@ impl StructArrayBuilder {
}

/// returns the non-null element at index as a Variant
fn typed_value_to_variant(typed_value: &ArrayRef, index: usize) -> Variant<'_, '_> {
match typed_value.data_type() {
fn typed_value_to_variant<'a>(
typed_value: &'a ArrayRef,
value: Option<&BinaryViewArray>,
index: usize,
) -> Variant<'a, 'a> {
let data_type = typed_value.data_type();
if value.is_some_and(|v| !matches!(data_type, DataType::Struct(_)) && v.is_valid(index)) {

@klion26 klion26 Sep 25, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll panic here if (data_type is not DataType::Struct(_)) and (v.is_valid(index)), do we need to panic if data_type is DataType::Struct and v.is_valid(index) here?

@scovich scovich Sep 25, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do not need to panic if we have a struct here -- that corresponds to a partially shredded variant object, where the value is a variant object and the typed_value is a struct. Eventually, the code that handles partial shredding will detect if the value is not a variant object or contains field names that conflict with those of the typed_value, but that will happen in a different location. I have it prototyped locally and can push a PR once this one merges.

// Only a partially shredded struct is allowed to have values for both columns
panic!("Invalid variant, conflicting value and typed_value");

@scovich scovich Sep 24, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This whole panic thing is becoming increasingly awkward as more and more valid error cases arise. Especially because:

  1. Variant data is untrusted (coming in from the user), so we have to expect malformed data
  2. All prod uses of VariantArray::value are in fallible code that could return an error, if given the opportunity.

Now that VariantArray no longer implements Array, we have the option to make value fallible (or add a fallible try_value if we really want to keep the panicky version).

@alamb alamb Sep 24, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think adding try_value sounds like a good idea to me

However, it seems to me that most of these checks can be done once per array (e.g. this check for value and compare to the datatype doesn't change row by row, so paying the cost to do the validation on each row feels wasteful to me)

Can we perhaps move this check into the constructor of VariantArray 🤔

@scovich scovich Sep 24, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this one is a row-oriented check, unlike the columnar type checks I added in rewrite_to_view_types:

For a specific row, both value and typed_value were non-NULL and typed_value is not a struct. I suppose we could try to memoize the "not a struct" part in order to avoid the overhead of that matches! invocation, but (a) checking for a specific enum variant is really cheap; and (b) where would we store the answer between invocations of value method, given that we don't build any kind of a tree?

}
match data_type {
DataType::Boolean => {
let boolean_array = typed_value.as_boolean();
let value = boolean_array.value(index);
Expand All @@ -809,17 +798,11 @@ fn typed_value_to_variant(typed_value: &ArrayRef, index: usize) -> Variant<'_, '
let date = Date32Type::to_naive_date(value);
Variant::from(date)
}
DataType::FixedSizeBinary(binary_len) => {
// 16-byte FixedSizeBinary is always UUID; all other sizes are illegal.
DataType::FixedSizeBinary(16) => {
let array = typed_value.as_fixed_size_binary();
// Try to treat 16 byte FixedSizeBinary as UUID
let value = array.value(index);
if *binary_len == 16 {
if let Ok(uuid) = Uuid::from_slice(value) {
return Variant::from(uuid);
}
}
let value = array.value(index);
Variant::from(value)
Uuid::from_slice(value).map_or(Variant::Null, Variant::from)
Comment thread
scovich marked this conversation as resolved.
Outdated
}
DataType::BinaryView => {
let array = typed_value.as_binary_view();
Expand All @@ -843,18 +826,6 @@ fn typed_value_to_variant(typed_value: &ArrayRef, index: usize) -> Variant<'_, '
DataType::Int64 => {
primitive_conversion_single_value!(Int64Type, typed_value, index)
}
DataType::UInt8 => {

@alamb alamb Sep 24, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand this correctly, the point is that since the Variant spec has no unsigned types, it wouldn't be permissible to shred out such arrow types

https://github.com/apache/parquet-format/blob/master/VariantEncoding.md#encoding-types

@scovich scovich Sep 24, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly. I don't think the shredding spec directly says that, but it's implied because shredding is always presumed to start from binary encoded variant values and is a more efficient representation of the same. So throwing in random other types doesn't really make sense.

@scovich scovich Sep 25, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, I'm blind... the spec definitely directly says which parquet logical types are allowed for shredded columns -- there's a section for it, including a table:
https://github.com/apache/parquet-format/blob/master/VariantShredding.md#shredded-value-types

primitive_conversion_single_value!(UInt8Type, typed_value, index)
}
DataType::UInt16 => {
primitive_conversion_single_value!(UInt16Type, typed_value, index)
}
DataType::UInt32 => {
primitive_conversion_single_value!(UInt32Type, typed_value, index)
}
DataType::UInt64 => {
primitive_conversion_single_value!(UInt64Type, typed_value, index)
}
DataType::Float16 => {
primitive_conversion_single_value!(Float16Type, typed_value, index)
}
Expand Down Expand Up @@ -898,6 +869,14 @@ fn cast_to_binary_view_arrays(array: &dyn Array) -> Result<ArrayRef, ArrowError>
/// replaces all instances of Binary with BinaryView in a DataType
fn rewrite_to_view_types(data_type: &DataType) -> DataType {

@scovich scovich Sep 24, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we agree this is the right place for the checks, I should probably rename the function (and make it fallible)?

And also expand it to cover the exhaustive set of valid and invalid data types so there's no confusion about what's legal and what's forbidden. This can be done immediately, even if a given "valid" data type isn't yet supported -- the read will simply fail later on in such cases (exactly the same as already happens today).

@alamb alamb Sep 24, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

YesI agree checking the types up front as part of construction is 💯 and avoids the potential for errors later on in value methods

@scovich scovich Sep 24, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... when possible. Some of the new error checks I had to add are row-based, not column-based

match data_type {
// Unsigned integers are not allowed at all
DataType::UInt8 | DataType::UInt16 | DataType::UInt32 | DataType::UInt64 => {
panic!("Illegal shredded value type: {data_type:?}");

@alamb alamb Sep 24, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this would be a good place to return errors I think

@scovich scovich Sep 24, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, let me quickly fix that

@scovich scovich Sep 24, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

}
// UUID maps to 16-byte fixed-size binary; no other width is allowed
DataType::FixedSizeBinary(n) if *n != 16 => {
panic!("Illegal shredded value type: {data_type:?}");
}
DataType::Binary => DataType::BinaryView,
DataType::List(field) => DataType::List(rewrite_field_type(field)),
DataType::Struct(fields) => {
Expand Down
191 changes: 4 additions & 187 deletions parquet-variant-compute/src/variant_get.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -297,13 +297,13 @@ mod test {
use std::sync::Arc;

use arrow::array::{
Array, ArrayRef, AsArray, BinaryViewArray, BooleanArray, Date32Array, FixedSizeBinaryArray,
Float16Array, Float32Array, Float64Array, Int16Array, Int32Array, Int64Array, Int8Array,
StringArray, StructArray, UInt16Array, UInt32Array, UInt64Array, UInt8Array,
Array, ArrayRef, AsArray, BinaryViewArray, BooleanArray, Date32Array, Float16Array,
Float32Array, Float64Array, Int16Array, Int32Array, Int64Array, Int8Array, StringArray,
StructArray,
};
use arrow::buffer::NullBuffer;
use arrow::compute::CastOptions;
use arrow::datatypes::DataType::{Int16, Int32, Int64, UInt16, UInt32, UInt64, UInt8};
use arrow::datatypes::DataType::{Int16, Int32, Int64};
use arrow_schema::{DataType, Field, FieldRef, Fields};
use parquet_variant::{Variant, VariantPath, EMPTY_VARIANT_METADATA_BYTES};

Expand Down Expand Up @@ -438,26 +438,6 @@ mod test {
numeric_partially_shredded_test!(i64, partially_shredded_int64_variant_array);
}

#[test]
fn get_variant_partially_shredded_uint8_as_variant() {

@scovich scovich Sep 24, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure how exhaustive we want to be about negative testing as a replacement for all these unit tests I deleted?

@alamb alamb Sep 24, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need to worry too much about it. Let's just makes sure each error path is hit

numeric_partially_shredded_test!(u8, partially_shredded_uint8_variant_array);
}

#[test]
fn get_variant_partially_shredded_uint16_as_variant() {
numeric_partially_shredded_test!(u16, partially_shredded_uint16_variant_array);
}

#[test]
fn get_variant_partially_shredded_uint32_as_variant() {
numeric_partially_shredded_test!(u32, partially_shredded_uint32_variant_array);
}

#[test]
fn get_variant_partially_shredded_uint64_as_variant() {
numeric_partially_shredded_test!(u64, partially_shredded_uint64_variant_array);
}

#[test]
fn get_variant_partially_shredded_float16_as_variant() {
numeric_partially_shredded_test!(half::f16, partially_shredded_float16_variant_array);
Expand Down Expand Up @@ -490,23 +470,6 @@ mod test {
assert_eq!(result.value(3), Variant::from(false));
}

#[test]
fn get_variant_partially_shredded_fixed_size_binary_as_variant() {
let array = partially_shredded_fixed_size_binary_variant_array();
let options = GetOptions::new();
let result = variant_get(&array, options).unwrap();

// expect the result is a VariantArray
let result = VariantArray::try_new(&result).unwrap();
assert_eq!(result.len(), 4);

// Expect the values are the same as the original values
assert_eq!(result.value(0), Variant::from(&[1u8, 2u8, 3u8][..]));
assert!(!result.is_valid(1));
assert_eq!(result.value(2), Variant::from("n/a"));
assert_eq!(result.value(3), Variant::from(&[4u8, 5u8, 6u8][..]));
}

#[test]
fn get_variant_partially_shredded_utf8_as_variant() {
let array = partially_shredded_utf8_variant_array();
Expand Down Expand Up @@ -645,26 +608,6 @@ mod test {
numeric_perfectly_shredded_test!(i64, perfectly_shredded_int64_variant_array);
}

#[test]
fn get_variant_perfectly_shredded_uint8_as_variant() {
numeric_perfectly_shredded_test!(u8, perfectly_shredded_uint8_variant_array);
}

#[test]
fn get_variant_perfectly_shredded_uint16_as_variant() {
numeric_perfectly_shredded_test!(u16, perfectly_shredded_uint16_variant_array);
}

#[test]
fn get_variant_perfectly_shredded_uint32_as_variant() {
numeric_perfectly_shredded_test!(u32, perfectly_shredded_uint32_variant_array);
}

#[test]
fn get_variant_perfectly_shredded_uint64_as_variant() {
numeric_perfectly_shredded_test!(u64, perfectly_shredded_uint64_variant_array);
}

#[test]
fn get_variant_perfectly_shredded_float16_as_variant() {
numeric_perfectly_shredded_test!(half::f16, perfectly_shredded_float16_variant_array);
Expand Down Expand Up @@ -749,34 +692,6 @@ mod test {
Int64Array::from(vec![Some(1), Some(2), Some(3)])
);

perfectly_shredded_to_arrow_primitive_test!(
get_variant_perfectly_shredded_uint8_as_int8,
UInt8,
perfectly_shredded_uint8_variant_array,
UInt8Array::from(vec![Some(1), Some(2), Some(3)])
);

perfectly_shredded_to_arrow_primitive_test!(
get_variant_perfectly_shredded_uint16_as_uint16,
UInt16,
perfectly_shredded_uint16_variant_array,
UInt16Array::from(vec![Some(1), Some(2), Some(3)])
);

perfectly_shredded_to_arrow_primitive_test!(
get_variant_perfectly_shredded_uint32_as_uint32,
UInt32,
perfectly_shredded_uint32_variant_array,
UInt32Array::from(vec![Some(1), Some(2), Some(3)])
);

perfectly_shredded_to_arrow_primitive_test!(
get_variant_perfectly_shredded_uint64_as_uint64,
UInt64,
perfectly_shredded_uint64_variant_array,
UInt64Array::from(vec![Some(1), Some(2), Some(3)])
);

/// Return a VariantArray that represents a perfectly "shredded" variant
/// for the given typed value.
///
Expand Down Expand Up @@ -835,26 +750,6 @@ mod test {
Int64Array,
i64
);
numeric_perfectly_shredded_variant_array_fn!(
perfectly_shredded_uint8_variant_array,
UInt8Array,
u8
);
numeric_perfectly_shredded_variant_array_fn!(
perfectly_shredded_uint16_variant_array,
UInt16Array,
u16
);
numeric_perfectly_shredded_variant_array_fn!(
perfectly_shredded_uint32_variant_array,
UInt32Array,
u32
);
numeric_perfectly_shredded_variant_array_fn!(
perfectly_shredded_uint64_variant_array,
UInt64Array,
u64
);
numeric_perfectly_shredded_variant_array_fn!(
perfectly_shredded_float16_variant_array,
Float16Array,
Expand Down Expand Up @@ -963,26 +858,6 @@ mod test {
Int64Array,
i64
);
numeric_partially_shredded_variant_array_fn!(
partially_shredded_uint8_variant_array,
UInt8Array,
u8
);
numeric_partially_shredded_variant_array_fn!(
partially_shredded_uint16_variant_array,
UInt16Array,
u16
);
numeric_partially_shredded_variant_array_fn!(
partially_shredded_uint32_variant_array,
UInt32Array,
u32
);
numeric_partially_shredded_variant_array_fn!(
partially_shredded_uint64_variant_array,
UInt64Array,
u64
);
numeric_partially_shredded_variant_array_fn!(
partially_shredded_float16_variant_array,
Float16Array,
Expand Down Expand Up @@ -1043,64 +918,6 @@ mod test {
Arc::new(struct_array)
}

/// Return a VariantArray that represents a partially "shredded" variant for fixed size binary
fn partially_shredded_fixed_size_binary_variant_array() -> ArrayRef {
let (metadata, string_value) = {
let mut builder = parquet_variant::VariantBuilder::new();
builder.append_value("n/a");
builder.finish()
};

// Create the null buffer for the overall array
let nulls = NullBuffer::from(vec![
true, // row 0 non null
false, // row 1 is null
true, // row 2 non null
true, // row 3 non null
]);

// metadata is the same for all rows
let metadata = BinaryViewArray::from_iter_values(std::iter::repeat_n(&metadata, 4));

// See https://docs.google.com/document/d/1pw0AWoMQY3SjD7R4LgbPvMjG_xSCtXp3rZHkVp9jpZ4/edit?disco=AAABml8WQrY
// about why row1 is an empty but non null, value.
let values = BinaryViewArray::from(vec![
None, // row 0 is shredded, so no value
Some(b"" as &[u8]), // row 1 is null, so empty value
Some(&string_value), // copy the string value "N/A"
None, // row 3 is shredded, so no value
]);

// Create fixed size binary array with 3-byte values
let data = vec![
1u8, 2u8, 3u8, // row 0 is shredded
0u8, 0u8, 0u8, // row 1 is null (value doesn't matter)
0u8, 0u8, 0u8, // row 2 is a string (value doesn't matter)
4u8, 5u8, 6u8, // row 3 is shredded
];
let typed_value_nulls = arrow::buffer::NullBuffer::from(vec![
true, // row 0 has value
false, // row 1 is null
false, // row 2 is string
true, // row 3 has value
]);
let typed_value = FixedSizeBinaryArray::try_new(
3, // byte width
arrow::buffer::Buffer::from(data),
Some(typed_value_nulls),
)
.expect("should create fixed size binary array");

let struct_array = StructArrayBuilder::new()
.with_field("metadata", Arc::new(metadata), false)
.with_field("typed_value", Arc::new(typed_value), true)
.with_field("value", Arc::new(values), true)
.with_nulls(nulls)
.build();

Arc::new(struct_array)
}

/// Return a VariantArray that represents a partially "shredded" variant for UTF8
fn partially_shredded_utf8_variant_array() -> ArrayRef {
let (metadata, string_value) = {
Expand Down
Loading
Loading