From ce56d0725db09d42babcf572f6eb6cb999b5b250 Mon Sep 17 00:00:00 2001 From: sdf-jkl Date: Fri, 10 Jul 2026 16:46:30 -0400 Subject: [PATCH 1/5] make `value` mandatory field for `VariantArray`/`ShreddingState` --- parquet-variant-compute/src/from_json.rs | 6 +- parquet-variant-compute/src/shred_variant.rs | 90 +++--- .../src/unshred_variant.rs | 14 +- parquet-variant-compute/src/variant_array.rs | 276 +++++++----------- .../src/variant_array_builder.rs | 4 +- parquet-variant-compute/src/variant_get.rs | 182 +++++------- .../src/variant_to_arrow.rs | 4 +- parquet/tests/variant_integration.rs | 14 +- 8 files changed, 248 insertions(+), 342 deletions(-) diff --git a/parquet-variant-compute/src/from_json.rs b/parquet-variant-compute/src/from_json.rs index 8e8c0ea9cf98..93df1b281d4d 100644 --- a/parquet-variant-compute/src/from_json.rs +++ b/parquet-variant-compute/src/from_json.rs @@ -85,7 +85,7 @@ mod test { let variant_array = json_to_variant(&array_ref).unwrap(); let metadata_array = variant_array.metadata_column(); - let value_array = variant_array.value_column().expect("value column"); + let value_array = variant_array.value_column(); // Compare row 0 assert!(!variant_array.is_null(0)); @@ -134,7 +134,7 @@ mod test { let variant_array = json_to_variant(&array_ref).unwrap(); let metadata_array = variant_array.metadata_column(); - let value_array = variant_array.value_column().expect("value column"); + let value_array = variant_array.value_column(); // Compare row 0 assert!(!variant_array.is_null(0)); @@ -183,7 +183,7 @@ mod test { let variant_array = json_to_variant(&array_ref).unwrap(); let metadata_array = variant_array.metadata_column(); - let value_array = variant_array.value_column().expect("value column"); + let value_array = variant_array.value_column(); // Compare row 0 assert!(!variant_array.is_null(0)); diff --git a/parquet-variant-compute/src/shred_variant.rs b/parquet-variant-compute/src/shred_variant.rs index 48ecdc2bc282..389012ddc0bb 100644 --- a/parquet-variant-compute/src/shred_variant.rs +++ b/parquet-variant-compute/src/shred_variant.rs @@ -82,11 +82,6 @@ pub(crate) fn shred_variant_with_options( )); } - if array.value_column().is_none() { - // all-null case -- nothing to do. - return Ok(array.clone()); - }; - let mut builder = make_variant_to_shredded_variant_arrow_row_builder( as_type, cast_options, @@ -103,7 +98,7 @@ pub(crate) fn shred_variant_with_options( let (value, typed_value, nulls) = builder.finish()?; Ok(VariantArray::from_parts( array.metadata_column().clone(), - Some(Arc::new(value)), + Arc::new(value), Some(typed_value), nulls, )) @@ -452,11 +447,8 @@ impl<'a> VariantToShreddedObjectVariantRowBuilder<'a> { let mut builder = StructArrayBuilder::new(); for (field_name, typed_value_builder) in self.typed_value_builders { let (value, typed_value, nulls) = typed_value_builder.finish()?; - let array = ShreddedVariantFieldArray::from_parts( - Some(Arc::new(value)), - Some(typed_value), - nulls, - ); + let array = + ShreddedVariantFieldArray::from_parts(Arc::new(value), Some(typed_value), nulls); builder = builder.with_field(field_name, ArrayRef::from(array), false); } if let Some(nulls) = self.typed_value_nulls.finish() { @@ -701,7 +693,7 @@ impl VariantSchemaNode { mod tests { use super::*; use crate::VariantArrayBuilder; - use crate::variant_array::{binary_array_value, variant_from_arrays_at}; + use crate::variant_array::{all_null_value_column, binary_array_value, variant_from_arrays_at}; use arrow::array::{ Array, BinaryViewArray, FixedSizeBinaryArray, FixedSizeListArray, Float64Array, GenericListArray, GenericListViewArray, Int64Array, LargeBinaryArray, LargeStringArray, @@ -880,7 +872,7 @@ mod tests { ) { assert_eq!(array.len(), expected_len); - let fallback_value = array.value_column().unwrap(); + let fallback_value = array.value_column(); let fallback_metadata = array.metadata_column(); let array = downcast_list_like_array::(array); @@ -995,7 +987,7 @@ mod tests { } // Validate fallback variants for list elements that could not be shredded - let element_fallbacks = element_array.value_column().unwrap(); + let element_fallbacks = element_array.value_column(); assert_eq!(element_fallbacks.len(), expected_fallbacks.len()); for (idx, expected_fallback) in expected_fallbacks.iter().enumerate() { match expected_fallback { @@ -1123,7 +1115,7 @@ mod tests { typed_struct.column_by_name(field_name).unwrap(), ) .unwrap(); - assert!(field.value_column().unwrap().is_null(0)); + assert!(field.value_column().is_null(0)); assert!(field.typed_value_column().unwrap().is_null(0)); } } @@ -1135,11 +1127,10 @@ mod tests { // First create a valid VariantArray, then extract its parts to construct a shredded one let temp_array = VariantArray::from_iter(vec![Some(Variant::from("test"))]); let metadata = temp_array.metadata_column().clone(); - let value = temp_array.value_column().unwrap().clone(); + let value = temp_array.value_column().clone(); let typed_value = Arc::new(Int64Array::from(vec![42])) as ArrayRef; - let shredded_array = - VariantArray::from_parts(metadata, Some(value), Some(typed_value), None); + let shredded_array = VariantArray::from_parts(metadata, value, Some(typed_value), None); let result = shred_variant(&shredded_array, &DataType::Int64); assert!(matches!( @@ -1150,14 +1141,18 @@ mod tests { #[test] fn test_all_null_input() { - // Create VariantArray with no value field (all null case) - let metadata = Arc::new(BinaryViewArray::from_iter_values([&[1u8, 0u8]])); // minimal valid metadata - let all_null_array = VariantArray::from_parts(metadata, None, None, None); + // Create VariantArray whose value column is entirely null + let metadata = Arc::new(BinaryViewArray::from_iter_values([ + EMPTY_VARIANT_METADATA_BYTES, + ])); + let all_null_array = + VariantArray::from_parts(metadata, all_null_value_column(1), None, None); let result = shred_variant(&all_null_array, &DataType::Int64).unwrap(); - // Should return array with no value/typed_value fields - assert!(result.value_column().is_none()); - assert!(result.typed_value_column().is_none()); + // The row is valid but has no value, so it shreds to an explicit Variant::Null + // stored in the value column, with a null typed_value + assert!(result.typed_value_column().unwrap().is_null(0)); + assert_eq!(result.value(0), Variant::Null); } #[test] @@ -1259,7 +1254,7 @@ mod tests { // Verify structure let metadata_field = result.metadata_column(); - let value_field = result.value_column().unwrap(); + let value_field = result.value_column(); let typed_value_field = result .typed_value_column() .unwrap() @@ -1354,7 +1349,7 @@ mod tests { let result = shred_variant(&input, &DataType::LargeUtf8).unwrap(); let metadata = result.metadata_column(); - let value = result.value_column().unwrap(); + let value = result.value_column(); let typed_value = result .typed_value_column() .unwrap() @@ -1410,7 +1405,7 @@ mod tests { let result = shred_variant(&input, &DataType::LargeBinary).unwrap(); let metadata = result.metadata_column(); - let value = result.value_column().unwrap(); + let value = result.value_column(); let typed_value = result .typed_value_column() .unwrap() @@ -1660,20 +1655,20 @@ mod tests { // The first row should be shredded, so the `value` field should be null and the // `typed_value` field should contain the list assert!(result.is_valid(0)); - assert!(result.value_column().unwrap().is_null(0)); + assert!(result.value_column().is_null(0)); assert!(result.typed_value_column().unwrap().is_valid(0)); // The second row should not be shredded because the provided schema for shredding did not // match. Hence, the `value` field should contain the raw value and the `typed_value` field // should be null. assert!(result.is_valid(1)); - assert!(result.value_column().unwrap().is_valid(1)); + assert!(result.value_column().is_valid(1)); assert!(result.typed_value_column().unwrap().is_null(1)); // The third row should be shredded, so the `value` field should be null and the // `typed_value` field should contain the list assert!(result.is_valid(2)); - assert!(result.value_column().unwrap().is_null(2)); + assert!(result.value_column().is_null(2)); assert!(result.typed_value_column().unwrap().is_valid(2)); let typed_value = result.typed_value_column().unwrap(); @@ -1781,7 +1776,7 @@ mod tests { .as_any() .downcast_ref::() .unwrap(); - let outer_fallbacks = outer_elements.value_column().unwrap(); + let outer_fallbacks = outer_elements.value_column(); let outer_metadata = Arc::new(BinaryViewArray::from_iter_values(std::iter::repeat_n( EMPTY_VARIANT_METADATA_BYTES, @@ -1789,7 +1784,7 @@ mod tests { ))); let outer_variant = VariantArray::from_parts( outer_metadata, - Some(outer_fallbacks.clone()), + outer_fallbacks.clone(), Some(Arc::new(outer_values.clone())), None, ); @@ -1881,7 +1876,7 @@ mod tests { let id_field = ShreddedVariantFieldArray::try_new(element_objects.column_by_name("id").unwrap()) .unwrap(); - let id_values = id_field.value_column().unwrap(); + let id_values = id_field.value_column(); let id_typed_values = id_field .typed_value_column() .unwrap() @@ -1905,7 +1900,7 @@ mod tests { let name_field = ShreddedVariantFieldArray::try_new(element_objects.column_by_name("name").unwrap()) .unwrap(); - let name_values = name_field.value_column().unwrap(); + let name_values = name_field.value_column(); let name_typed_values = name_field .typed_value_column() .unwrap() @@ -1965,12 +1960,11 @@ mod tests { let result = shred_variant(&input, &target_schema).unwrap(); // Verify structure - assert!(result.value_column().is_some()); assert!(result.typed_value_column().is_some()); assert_eq!(result.len(), 9); let metadata = result.metadata_column(); - let value = result.value_column().unwrap(); + let value = result.value_column(); let typed_value = result .typed_value_column() .unwrap() @@ -1985,14 +1979,14 @@ mod tests { let age_field = ShreddedVariantFieldArray::try_new(typed_value.column_by_name("age").unwrap()).unwrap(); - let score_value = score_field.value_column().unwrap(); + let score_value = score_field.value_column(); let score_typed_value = score_field .typed_value_column() .unwrap() .as_any() .downcast_ref::() .unwrap(); - let age_value = age_field.value_column().unwrap(); + let age_value = age_field.value_column(); let age_typed_value = age_field .typed_value_column() .unwrap() @@ -2295,7 +2289,7 @@ mod tests { assert_eq!(result.len(), 5); // Access base value/typed_value columns - let value_field = result.value_column().unwrap(); + let value_field = result.value_column(); let typed_struct = result .typed_value_column() .unwrap() @@ -2331,7 +2325,7 @@ mod tests { EMPTY_VARIANT_METADATA_BYTES, scores_field.len(), ))), - Some(scores_field.value_column().unwrap().clone()), + scores_field.value_column().clone(), Some(scores_field.typed_value_column().unwrap().clone()), None, ), @@ -2360,7 +2354,7 @@ mod tests { .with_path("id", &DataType::Int32)? .build(); let result1 = shred_variant(&input, &schema1).unwrap(); - let value_field1 = result1.value_column().unwrap(); + let value_field1 = result1.value_column(); assert!(!value_field1.is_null(0)); // should contain {"age": 25, "score": 95.5} // Test with schema containing id and age fields @@ -2369,7 +2363,7 @@ mod tests { .with_path("age", &DataType::Int64)? .build(); let result2 = shred_variant(&input, &schema2).unwrap(); - let value_field2 = result2.value_column().unwrap(); + let value_field2 = result2.value_column(); assert!(!value_field2.is_null(0)); // should contain {"score": 95.5} // Test with schema containing all fields @@ -2379,7 +2373,7 @@ mod tests { .with_path("score", &DataType::Float64)? .build(); let result3 = shred_variant(&input, &schema3).unwrap(); - let value_field3 = result3.value_column().unwrap(); + let value_field3 = result3.value_column(); assert!(value_field3.is_null(0)); // fully shredded, no remaining fields Ok(()) @@ -2426,12 +2420,11 @@ mod tests { let result = shred_variant(&input, &target_schema).unwrap(); - assert!(result.value_column().is_some()); assert!(result.typed_value_column().is_some()); assert_eq!(result.len(), 6); let metadata = result.metadata_column(); - let value = result.value_column().unwrap(); + let value = result.value_column(); let typed_value = result .typed_value_column() .unwrap() @@ -2446,14 +2439,14 @@ mod tests { ShreddedVariantFieldArray::try_new(typed_value.column_by_name("session_id").unwrap()) .unwrap(); - let id_value = id_field.value_column().unwrap(); + let id_value = id_field.value_column(); let id_typed_value = id_field .typed_value_column() .unwrap() .as_any() .downcast_ref::() .unwrap(); - let session_id_value = session_id_field.value_column().unwrap(); + let session_id_value = session_id_field.value_column(); let session_id_typed_value = session_id_field .typed_value_column() .unwrap() @@ -2572,12 +2565,11 @@ mod tests { // Test output structure correctness assert_eq!(result.len(), input.len()); - assert!(result.value_column().is_some()); assert!(result.typed_value_column().is_some()); // For primitive shredding, verify that value and typed_value are never both non-null // (This rule applies to primitives; for objects, both can be non-null for partial shredding) - let value_field = result.value_column().unwrap(); + let value_field = result.value_column(); let typed_value_field = result .typed_value_column() .unwrap() diff --git a/parquet-variant-compute/src/unshred_variant.rs b/parquet-variant-compute/src/unshred_variant.rs index 30f861ce1e94..08c58fe0f532 100644 --- a/parquet-variant-compute/src/unshred_variant.rs +++ b/parquet-variant-compute/src/unshred_variant.rs @@ -59,7 +59,7 @@ use uuid::Uuid; /// - If unsupported data types are encountered in typed_value columns pub fn unshred_variant(array: &VariantArray) -> Result { // Check if already unshredded (optimization for common case) - if array.typed_value_column().is_none() && array.value_column().is_some() { + if array.typed_value_column().is_none() { return Ok(array.clone()); } @@ -89,7 +89,7 @@ pub fn unshred_variant(array: &VariantArray) -> Result { let value = value_builder.build()?; Ok(VariantArray::from_parts( metadata.clone(), - Some(Arc::new(value)), + Arc::new(value), None, nulls.cloned(), )) @@ -715,7 +715,7 @@ mod tests { Some("world"), ])); - let variant_array = VariantArray::from_parts(metadata, None, Some(typed_value), None); + let variant_array = VariantArray::perfectly_shredded(metadata, typed_value, None); let result = crate::unshred_variant(&variant_array).unwrap(); @@ -737,7 +737,7 @@ mod tests { Some("world"), ])); - let variant_array = VariantArray::from_parts(metadata, None, Some(typed_value), None); + let variant_array = VariantArray::perfectly_shredded(metadata, typed_value, None); let result = crate::unshred_variant(&variant_array).unwrap(); @@ -759,7 +759,7 @@ mod tests { &b"\xde\xad\xbe\xef"[..], ])); - let variant_array = VariantArray::from_parts(metadata, None, Some(typed_value), None); + let variant_array = VariantArray::perfectly_shredded(metadata, typed_value, None); let result = crate::unshred_variant(&variant_array).unwrap(); @@ -781,7 +781,7 @@ mod tests { &b"\xde\xad\xbe\xef"[..], ])); - let variant_array = VariantArray::from_parts(metadata, None, Some(typed_value), None); + let variant_array = VariantArray::perfectly_shredded(metadata, typed_value, None); let result = crate::unshred_variant(&variant_array).unwrap(); @@ -801,7 +801,7 @@ mod tests { let typed_value: ArrayRef = Arc::new(StringViewArray::from(vec![Some("hello")])); - let variant_array = VariantArray::from_parts(metadata, None, Some(typed_value), None); + let variant_array = VariantArray::perfectly_shredded(metadata, typed_value, None); let result = crate::unshred_variant(&variant_array); diff --git a/parquet-variant-compute/src/variant_array.rs b/parquet-variant-compute/src/variant_array.rs index 82cda7867f50..2e860d33b374 100644 --- a/parquet-variant-compute/src/variant_array.rs +++ b/parquet-variant-compute/src/variant_array.rs @@ -22,7 +22,7 @@ use crate::type_conversion::{ generic_conversion_single_value, generic_conversion_single_value_with_result, primitive_conversion_single_value, }; -use arrow::array::{Array, ArrayRef, AsArray, StructArray}; +use arrow::array::{Array, ArrayRef, AsArray, StructArray, new_null_array}; use arrow::buffer::NullBuffer; use arrow::compute::cast; use arrow::datatypes::{ @@ -63,6 +63,15 @@ pub(crate) fn variant_from_arrays_at<'m, 'v>( Some(Variant::new(metadata, value)) } +/// Returns an all-null binary `value` column of the given length. +/// +/// The shredding spec requires the `value` column to always be present in the +/// schema, so producers that have no unshredded values to store must still +/// emit an all-null column. See . +pub(crate) fn all_null_value_column(len: usize) -> ArrayRef { + new_null_array(&DataType::BinaryView, len) +} + /// Validates that an array has a binary-like data type. pub(crate) fn validate_binary_array(array: &dyn Array, field_name: &str) -> Result<()> { match array.data_type() { @@ -278,7 +287,7 @@ impl VariantArray { /// 1. A required field named `metadata` which is binary, large_binary, or /// binary_view /// - /// 2. An optional field named `value` that is binary, large_binary, or + /// 2. A required field named `value` that is binary, large_binary, or /// binary_view /// /// 3. An optional field named `typed_value` which can be any primitive type @@ -316,16 +325,27 @@ impl VariantArray { }) } + /// Construct a perfectly shredded `VariantArray`: every value is in + /// `typed_value` and the required `value` column is all-null. + #[cfg(test)] + pub(crate) fn perfectly_shredded( + metadata: ArrayRef, + typed_value: ArrayRef, + nulls: Option, + ) -> Self { + let value = all_null_value_column(typed_value.len()); + Self::from_parts(metadata, value, Some(typed_value), nulls) + } + pub(crate) fn from_parts( metadata: ArrayRef, - value: Option, + value: ArrayRef, typed_value: Option, nulls: Option, ) -> Self { - let mut builder = StructArrayBuilder::new().with_field("metadata", metadata.clone(), false); - if let Some(value) = value.clone() { - builder = builder.with_field("value", value, true); - } + let mut builder = StructArrayBuilder::new() + .with_field("metadata", metadata.clone(), false) + .with_field("value", value.clone(), true); if let Some(typed_value) = typed_value.clone() { builder = builder.with_field_ref(typed_value_field(&typed_value), typed_value); } @@ -396,25 +416,22 @@ 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 try_value(&self, index: usize) -> Result> { - match (self.typed_value_column(), self.value_column()) { + let value = self.value_column(); + match self.typed_value_column() { // Always prefer typed_value, if available - (Some(typed_value), value) if typed_value.is_valid(index) => { + Some(typed_value) if typed_value.is_valid(index) => { typed_value_to_variant(typed_value, value, index) } // Otherwise fall back to value, if available - (_, Some(value)) if value.is_valid(index) => variant_from_arrays_at( - &self.metadata, - value, - index, - ) - .ok_or_else(|| { - ArrowError::InvalidArgumentError(format!( - "metadata and value fields must be binary-like arrays, instead got {} and {}", - self.metadata.data_type(), - value.data_type() - )) - }), - // It is technically invalid for neither value nor typed_value fields to be available, + _ if value.is_valid(index) => variant_from_arrays_at(&self.metadata, value, index) + .ok_or_else(|| { + ArrowError::InvalidArgumentError(format!( + "metadata and value fields must be binary-like arrays, instead got {} and {}", + self.metadata.data_type(), + value.data_type() + )) + }), + // It is technically invalid for both value and typed_value to be null, // but the spec specifically requires readers to return Variant::Null in this case. _ => Ok(Variant::Null), } @@ -425,8 +442,8 @@ impl VariantArray { &self.metadata } - /// Return a reference to the `value` column of the [`StructArray`], if present - pub fn value_column(&self) -> Option<&ArrayRef> { + /// Return a reference to the `value` column of the [`StructArray`] + pub fn value_column(&self) -> &ArrayRef { self.shredding_state.value_column() } @@ -663,7 +680,7 @@ impl ShreddedVariantFieldArray { /// /// # Requirements of the `StructArray` /// - /// 1. An optional field named `value` that is binary, large_binary, or + /// 1. A required field named `value` that is binary, large_binary, or /// binary_view /// /// 2. An optional field named `typed_value` which can be any primitive type @@ -688,8 +705,8 @@ impl ShreddedVariantFieldArray { &self.shredding_state } - /// Return a reference to the `value` column of the [`StructArray`], if present - pub fn value_column(&self) -> Option<&ArrayRef> { + /// Return a reference to the `value` column of the [`StructArray`] + pub fn value_column(&self) -> &ArrayRef { self.shredding_state.value_column() } @@ -703,15 +720,20 @@ impl ShreddedVariantFieldArray { &self.inner } + /// Construct a perfectly shredded field: every value is in `typed_value` + /// and the required `value` column is all-null. + #[cfg(test)] + pub(crate) fn perfectly_shredded(typed_value: ArrayRef) -> Self { + let value = all_null_value_column(typed_value.len()); + Self::from_parts(value, Some(typed_value), None) + } + pub(crate) fn from_parts( - value: Option, + value: ArrayRef, typed_value: Option, nulls: Option, ) -> Self { - let mut builder = StructArrayBuilder::new(); - if let Some(value) = value.clone() { - builder = builder.with_field("value", value, true); - } + let mut builder = StructArrayBuilder::new().with_field("value", value.clone(), true); if let Some(typed_value) = typed_value.clone() { builder = builder.with_field_ref(typed_value_field(&typed_value), typed_value); } @@ -781,9 +803,10 @@ impl From for StructArray { /// Shredding Spec]. Shredding means that the actual value is stored in a typed /// `typed_field` instead of the generic `value` field. /// -/// Both value and typed_value are optional fields used together to encode a -/// single value. Values in the two fields must be interpreted according to the -/// following table (see [Parquet Variant Shredding Spec] for more details): +/// The `value` column is always present (the spec requires writers to emit +/// it); `typed_value` is optional. Values in the two columns must be +/// interpreted according to the following table (see [Parquet Variant +/// Shredding Spec] for more details): /// /// | value | typed_value | Meaning | /// |----------|--------------|---------| @@ -797,10 +820,12 @@ impl From for StructArray { /// /// | value | typed_value | Meaning | /// |--------|-------------|---------| -/// | -- | -- | **Missing**: The value is always missing; only valid for shredded object fields | /// | exists | -- | **Unshredded**: If present, the value may be any type, including [`Variant::Null`] -/// | -- | exists | **Perfectly shredded**: If present, the value is always the shredded type | -/// | exists | exists | **Imperfectly shredded**: The value might (not) be present and might (not) be the shredded type | +/// | exists | exists | **Shredded**: perfectly if `value` is all-null, otherwise imperfectly | +/// +/// Note the spec requires the `value` column to always be present in the +/// schema; structs without one are rejected +/// (see ). /// /// NOTE: Partial shredding is a row-wise situation that can arise under imperfect shredding (a /// column-wise situation): When both columns exist (imperfect shredding) and the typed_value column @@ -810,7 +835,7 @@ impl From for StructArray { /// [Parquet Variant Shredding Spec]: https://github.com/apache/parquet-format/blob/master/VariantShredding.md#value-shredding #[derive(Debug, Clone)] pub struct ShreddingState { - value: Option, + value: ArrayRef, typed_value: Option, } @@ -829,13 +854,13 @@ impl ShreddingState { /// let struct_array: StructArray = get_struct_array(); /// let shredding_state = ShreddingState::try_from(&struct_array).unwrap(); /// ``` - pub fn new(value: Option, typed_value: Option) -> Self { + pub fn new(value: ArrayRef, typed_value: Option) -> Self { Self { value, typed_value } } - /// Return a reference to the `value` column, if present - pub fn value_column(&self) -> Option<&ArrayRef> { - self.value.as_ref() + /// Return a reference to the `value` column + pub fn value_column(&self) -> &ArrayRef { + &self.value } /// Return a reference to the `typed_value` column, if present @@ -846,7 +871,7 @@ impl ShreddingState { /// Slice all the underlying arrays pub fn slice(&self, offset: usize, length: usize) -> Self { Self { - value: self.value.as_ref().map(|v| v.slice(offset, length)), + value: self.value.slice(offset, length), typed_value: self.typed_value.as_ref().map(|tv| tv.slice(offset, length)), } } @@ -856,15 +881,14 @@ impl TryFrom<&StructArray> for ShreddingState { type Error = ArrowError; fn try_from(inner_struct: &StructArray) -> Result { - // The `value` column need not exist, but if it does it must be a binary type. - let value = if let Some(value_col) = inner_struct.column_by_name("value") { - validate_binary_array(value_col.as_ref(), "value")?; - Some(value_col.clone()) - } else { - None + let Some(value) = inner_struct.column_by_name("value") else { + return Err(ArrowError::InvalidArgumentError( + "Invalid VariantArray: StructArray must contain a 'value' field".to_string(), + )); }; + validate_binary_array(value.as_ref(), "value")?; let typed_value = inner_struct.column_by_name("typed_value").cloned(); - Ok(ShreddingState::new(value, typed_value)) + Ok(ShreddingState::new(value.clone(), typed_value)) } } @@ -933,11 +957,11 @@ impl StructArrayBuilder { /// returns the non-null element at index as a Variant fn typed_value_to_variant<'a>( typed_value: &'a ArrayRef, - value: Option<&'a ArrayRef>, + value: &'a ArrayRef, index: usize, ) -> Result> { let data_type = typed_value.data_type(); - if value.is_some_and(|v| !matches!(data_type, DataType::Struct(_)) && v.is_valid(index)) { + if !matches!(data_type, DataType::Struct(_)) && value.is_valid(index) { // Only a partially shredded struct is allowed to have values for both columns panic!("Invalid variant, conflicting value and typed_value"); } @@ -1283,30 +1307,15 @@ mod test { } #[test] - fn all_null_missing_value_and_typed_value() { + fn invalid_missing_value() { let fields = Fields::from(vec![Field::new("metadata", DataType::BinaryView, false)]); let array = StructArray::new(fields, vec![make_binary_view_array()], None); - - // NOTE: By strict spec interpretation, this case (top-level variant with null/null) - // should be invalid, but we currently allow it and treat it as Variant::Null. - // This is a pragmatic decision to handle missing data gracefully. - let variant_array = VariantArray::try_new(&array).unwrap(); - - // Verify the shredding state is AllNull - assert!(matches!( - variant_array.shredding_state(), - ShreddingState { - value: None, - typed_value: None - } - )); - - // Verify that value() returns Variant::Null (compensating for spec violation) - for i in 0..variant_array.len() { - if variant_array.is_valid(i) { - assert_eq!(variant_array.value(i), parquet_variant::Variant::Null); - } - } + // The spec requires the `value` field to always be present in the schema + let err = VariantArray::try_new(&array); + assert_eq!( + err.unwrap_err().to_string(), + "Invalid argument error: Invalid VariantArray: StructArray must contain a 'value' field" + ); } #[test] @@ -1358,8 +1367,10 @@ mod test { EMPTY_VARIANT_METADATA_BYTES, typed_value.len(), )); + let value = new_null_array(&DataType::BinaryView, typed_value.len()); StructArrayBuilder::new() .with_field("metadata", Arc::new(metadata), false) + .with_field("value", value, true) .with_field("typed_value", typed_value, true) .build() } @@ -1400,88 +1411,27 @@ mod test { } #[test] - fn all_null_shredding_state() { - // Verify the shredding state is AllNull - assert!(matches!( - ShreddingState::new(None, None), - ShreddingState { - value: None, - typed_value: None - } - )); - } - - #[test] - fn all_null_variant_array_construction() { + fn all_null_value_column_is_valid_and_unshredded() { + // An all-null `value` column is accepted (only a *missing* column is + // rejected) and the array is unshredded (no typed_value column) let metadata = BinaryViewArray::from(vec![b"test" as &[u8]; 3]); - let nulls = NullBuffer::from(vec![false, false, false]); // all null - - let fields = Fields::from(vec![Field::new("metadata", DataType::BinaryView, false)]); - let struct_array = StructArray::new(fields, vec![Arc::new(metadata)], Some(nulls)); - - let variant_array = VariantArray::try_new(&struct_array).unwrap(); - - // Verify the shredding state is AllNull - assert!(matches!( - variant_array.shredding_state(), - ShreddingState { - value: None, - typed_value: None - } - )); - - // Verify all values are null - assert_eq!(variant_array.len(), 3); - assert!(!variant_array.is_valid(0)); - assert!(!variant_array.is_valid(1)); - assert!(!variant_array.is_valid(2)); - - // Verify that value() returns Variant::Null for all indices - for i in 0..variant_array.len() { - assert!( - !variant_array.is_valid(i), - "Expected value at index {i} to be null" - ); - } - } - - #[test] - fn value_field_present_but_all_null_should_be_unshredded() { - // This test demonstrates the issue: when a value field exists in schema - // but all its values are null, it should remain Unshredded, not AllNull - let metadata = BinaryViewArray::from(vec![b"test" as &[u8]; 3]); - - // Create a value field with all null values - let value_nulls = NullBuffer::from(vec![false, false, false]); // all null - let value_array = BinaryViewArray::from_iter_values(vec![""; 3]); - let value_data = value_array - .to_data() - .into_builder() - .nulls(Some(value_nulls)) - .build() - .unwrap(); - let value = BinaryViewArray::from(value_data); + let value = new_null_array(&DataType::BinaryView, 3); let fields = Fields::from(vec![ Field::new("metadata", DataType::BinaryView, false), - Field::new("value", DataType::BinaryView, true), // Field exists in schema + Field::new("value", DataType::BinaryView, true), ]); - let struct_array = StructArray::new( - fields, - vec![Arc::new(metadata), Arc::new(value)], - None, // struct itself is not null, just the value field is all null - ); + let struct_array = StructArray::new(fields, vec![Arc::new(metadata), value], None); let variant_array = VariantArray::try_new(&struct_array).unwrap(); + assert!(variant_array.typed_value_column().is_none()); - // This should be Unshredded, not AllNull, because value field exists in schema - assert!(matches!( - variant_array.shredding_state(), - ShreddingState { - value: Some(_), - typed_value: None - } - )); + // The rows are valid but have neither value nor typed_value; the spec + // requires readers to return Variant::Null in this case + for i in 0..variant_array.len() { + assert!(variant_array.is_valid(i)); + assert_eq!(variant_array.value(i), Variant::Null); + } } #[test] @@ -1715,15 +1665,8 @@ mod test { #[test] fn binary_typed_value_roundtrips() { // Verify that a shredded variant with Binary typed_value can be read back - let metadata: ArrayRef = Arc::new(BinaryViewArray::from_iter_values([ - EMPTY_VARIANT_METADATA_BYTES, - ])); let typed_value: ArrayRef = Arc::new(BinaryArray::from(vec![b"hello" as &[u8]])); - - let struct_array = StructArrayBuilder::new() - .with_field("metadata", metadata, false) - .with_field("typed_value", typed_value, true) - .build(); + let struct_array = make_variant_struct_with_typed_value(typed_value); let variant_array = VariantArray::try_new(&struct_array).unwrap(); assert_eq!(variant_array.value(0), Variant::from(b"hello" as &[u8])); @@ -1732,15 +1675,8 @@ mod test { #[test] fn large_binary_typed_value_roundtrips() { // Verify that a shredded variant with LargeBinary typed_value can be read back - let metadata: ArrayRef = Arc::new(BinaryViewArray::from_iter_values([ - EMPTY_VARIANT_METADATA_BYTES, - ])); let typed_value: ArrayRef = Arc::new(LargeBinaryArray::from(vec![b"world" as &[u8]])); - - let struct_array = StructArrayBuilder::new() - .with_field("metadata", metadata, false) - .with_field("typed_value", typed_value, true) - .build(); + let struct_array = make_variant_struct_with_typed_value(typed_value); let variant_array = VariantArray::try_new(&struct_array).unwrap(); assert_eq!(variant_array.value(0), Variant::from(b"world" as &[u8])); @@ -1750,16 +1686,10 @@ mod test { ($fn_name: ident, $invalid_typed_value: expr, $error_msg: literal) => { #[test] fn $fn_name() { - let metadata = BinaryViewArray::from_iter_values(std::iter::repeat_n( - EMPTY_VARIANT_METADATA_BYTES, - 1, - )); let invalid_typed_value = $invalid_typed_value; - let struct_array = StructArrayBuilder::new() - .with_field("metadata", Arc::new(metadata), false) - .with_field("typed_value", Arc::new(invalid_typed_value), true) - .build(); + let struct_array = + make_variant_struct_with_typed_value(Arc::new(invalid_typed_value)); let array: VariantArray = VariantArray::try_new(&struct_array) .expect("should create variant array") diff --git a/parquet-variant-compute/src/variant_array_builder.rs b/parquet-variant-compute/src/variant_array_builder.rs index f43e34d0cddf..bc2d7d370c4a 100644 --- a/parquet-variant-compute/src/variant_array_builder.rs +++ b/parquet-variant-compute/src/variant_array_builder.rs @@ -501,7 +501,7 @@ mod test { // the metadata and value fields of non shredded variants should not be null assert!(variant_array.metadata_column().nulls().is_none()); - assert!(variant_array.value_column().unwrap().nulls().is_none()); + assert!(variant_array.value_column().nulls().is_none()); let DataType::Struct(fields) = variant_array.data_type() else { panic!("Expected VariantArray to have Struct data type"); }; @@ -647,7 +647,7 @@ mod test { let array2 = VariantArray::from_parts( array.metadata_column().clone(), - Some(Arc::new(value_builder.build().unwrap())), + Arc::new(value_builder.build().unwrap()), None, None, ); diff --git a/parquet-variant-compute/src/variant_get.rs b/parquet-variant-compute/src/variant_get.rs index fc01ca8d3b20..a2e2bb2d07dd 100644 --- a/parquet-variant-compute/src/variant_get.rs +++ b/parquet-variant-compute/src/variant_get.rs @@ -28,6 +28,7 @@ use arrow_schema::{ArrowError, DataType, FieldRef}; use parquet_variant::{VariantPath, VariantPathElement}; use crate::ShreddingState; +use crate::variant_array::all_null_value_column; use crate::variant_to_arrow::make_variant_to_arrow_row_builder; use crate::{VariantArray, VariantType, unshred_variant}; @@ -37,8 +38,8 @@ use std::sync::Arc; pub(crate) enum ShreddedPathStep { /// Path step succeeded, return the new shredding state Success(ShreddingState), - /// The path element is not present in the `typed_value` column and there is no `value` column, - /// so we know it does not exist. It, and all paths under it, are all-NULL. + /// The path element is not present in the `typed_value` column and the `value` column is + /// all-null, so we know it does not exist. It, and all paths under it, are all-NULL. Missing, /// The path element is not present in the `typed_value` column and must be retrieved from the `value` /// column instead. The caller should be prepared to handle any value, including the requested @@ -70,8 +71,8 @@ fn take_list_like_index_as_shredding_state( let value_array = shredding_state.value_column(); let typed_array = shredding_state.typed_value_column(); - // If list elements have neither typed nor fallback value, this path step is missing. - if value_array.is_none() && typed_array.is_none() { + // If list elements have neither typed nor fallback values, this path step is missing. + if typed_array.is_none() && value_array.null_count() == value_array.len() { return Ok(None); } @@ -85,9 +86,7 @@ fn take_list_like_index_as_shredding_state( let index_array = UInt64Array::from(take_indices); // Gather both typed and fallback values at the requested element index. - let taken_value = value_array - .map(|value| take(value, &index_array, None)) - .transpose()?; + let taken_value = take(value_array, &index_array, None)?; let taken_typed = typed_array .map(|typed| take(typed, &index_array, None)) .transpose()?; @@ -115,11 +114,15 @@ pub(crate) fn follow_shredded_path_element( path_element: &VariantPathElement<'_>, _cast_options: &CastOptions, ) -> Result { - // If the requested path element is not present in `typed_value`, and `value` is missing, then + // If the requested path element is not present in `typed_value`, and `value` is all-null, then // we know it does not exist; it, and all paths under it, are all-NULL. - let missing_path_step = || match shredding_state.value_column() { - Some(_) => ShreddedPathStep::NotShredded, - None => ShreddedPathStep::Missing, + let missing_path_step = || { + let value = shredding_state.value_column(); + if value.null_count() == value.len() { + ShreddedPathStep::Missing + } else { + ShreddedPathStep::NotShredded + } }; let Some(typed_value) = shredding_state.typed_value_column() else { @@ -194,9 +197,7 @@ fn shredded_get_path( // Helper that creates a new VariantArray from the given nested value and typed_value columns, // properly accounting for accumulated nulls from path traversal let make_target_variant = - |value: Option, - typed_value: Option, - accumulated_nulls: Option| { + |value: ArrayRef, typed_value: Option, accumulated_nulls: Option| { let metadata = input.metadata_column().clone(); VariantArray::from_parts(metadata, value, typed_value, accumulated_nulls) }; @@ -291,7 +292,12 @@ fn shredded_get_path( // Propagating metadata is not necessary for an all-NULL array, but is cheaper than constructing // a new empty metadata array. (n * 3 bytes vs Arc bump) let metadata = input.metadata_column().clone(); - let arr = VariantArray::from_parts(metadata, None, None, all_nulls); + let arr = VariantArray::from_parts( + metadata, + all_null_value_column(num_rows), + None, + all_nulls, + ); return Ok(ArrayRef::from(arr)); } let arr = match as_field.map(|f| f.data_type()) { @@ -302,7 +308,7 @@ fn shredded_get_path( } ShreddedPathStep::NotShredded => { let target = make_target_variant( - shredding_state.value_column().cloned(), + shredding_state.value_column().clone(), None, accumulated_nulls, ); @@ -313,7 +319,7 @@ fn shredded_get_path( // Path exhausted! Create a new `VariantArray` for the location we landed on. let target = make_target_variant( - shredding_state.value_column().cloned(), + shredding_state.value_column().clone(), shredding_state.typed_value_column().cloned(), accumulated_nulls, ); @@ -381,14 +387,11 @@ fn try_perfect_shredding(variant_array: &VariantArray, as_field: &Field) -> Opti } let typed_value = variant_array.typed_value_column()?; - if typed_value.data_type() == as_field.data_type() - && variant_array - .value_column() - .is_none_or(|v| v.null_count() == v.len()) - { - // Here we need to gate against the case where the `typed_value` is null but data is in the `value` column. - // 1. If the `value` column is null, or - // 2. If every row in the `value` column is null + let value = variant_array.value_column(); + if typed_value.data_type() == as_field.data_type() && value.null_count() == value.len() { + // Here we need to gate against the case where the `typed_value` is null + // but data is in the `value` column: only an all-null `value` column + // qualifies as perfect shredding. // This is a perfect shredding, where the value is entirely shredded out, // so we can just return the typed value after merging the accumulated nulls. @@ -484,7 +487,9 @@ mod test { use std::sync::Arc; use super::{GetOptions, requested_field_is_shredded, variant_get}; - use crate::variant_array::{ShreddedVariantFieldArray, StructArrayBuilder}; + use crate::variant_array::{ + ShreddedVariantFieldArray, StructArrayBuilder, all_null_value_column, + }; use crate::{ ShreddedSchemaBuilder, VariantArray, VariantArrayBuilder, cast_to_variant, json_to_variant, shred_variant, @@ -1198,7 +1203,7 @@ mod test { EMPTY_VARIANT_METADATA_BYTES, typed_value.len(), )); - VariantArray::from_parts(Arc::new(metadata), None, Some(typed_value), None).into() + VariantArray::perfectly_shredded(Arc::new(metadata), typed_value, None).into() } }; } @@ -1868,7 +1873,7 @@ mod test { ArrayRef::from(VariantArray::from_parts( Arc::new(metadata), - None, + all_null_value_column(3), None, Some(nulls), )) @@ -2161,11 +2166,9 @@ mod test { let x_field_typed_value = Int32Array::from(vec![Some(1), Some(42)]); // For perfect shredding of the x field, no "value" column, only typed_value - let x_field_shredded = ShreddedVariantFieldArray::from_parts( - None, - Some(Arc::new(x_field_typed_value) as ArrayRef), - None, - ); + let x_field_shredded = ShreddedVariantFieldArray::perfectly_shredded(Arc::new( + x_field_typed_value, + ) as ArrayRef); // Create the main typed_value as a struct containing the "x" field let typed_value_fields = Fields::from(vec![Field::new( @@ -2183,7 +2186,7 @@ mod test { // Create the main VariantArray ArrayRef::from(VariantArray::from_parts( Arc::new(metadata_array), - Some(Arc::new(value_array)), + Arc::new(value_array), Some(Arc::new(typed_value_struct)), None, )) @@ -2454,7 +2457,7 @@ mod test { let result_variant = VariantArray::try_new(&result).unwrap(); assert!(result_variant.typed_value_column().is_none()); - assert!(result_variant.value_column().is_some()); + assert!(result_variant.value_column().null_count() < result_variant.len()); let expected_json: ArrayRef = Arc::new(StringArray::from(vec![ Some(r#"{"k":100000}"#), @@ -2618,11 +2621,9 @@ mod test { let x_field_typed_value = Int32Array::from(vec![Some(42), None]); // For the x field, only typed_value (perfect shredding when possible) - let x_field_shredded = ShreddedVariantFieldArray::from_parts( - None, - Some(Arc::new(x_field_typed_value) as ArrayRef), - None, - ); + let x_field_shredded = ShreddedVariantFieldArray::perfectly_shredded(Arc::new( + x_field_typed_value, + ) as ArrayRef); // Create the main typed_value as a struct containing the "x" field let typed_value_fields = Fields::from(vec![Field::new( @@ -2640,7 +2641,7 @@ mod test { // Build final VariantArray ArrayRef::from(VariantArray::from_parts( Arc::new(metadata_array), - Some(Arc::new(value_array)), + Arc::new(value_array), Some(Arc::new(typed_value_struct)), None, )) @@ -2698,11 +2699,8 @@ mod test { // Create the nested shredded structure // Level 2: x field (the deepest level) let x_typed_value = Int32Array::from(vec![Some(55), None]); - let x_field_shredded = ShreddedVariantFieldArray::from_parts( - None, - Some(Arc::new(x_typed_value) as ArrayRef), - None, - ); + let x_field_shredded = + ShreddedVariantFieldArray::perfectly_shredded(Arc::new(x_typed_value) as ArrayRef); // Level 1: a field containing x field + value field for fallbacks // The "a" field needs both typed_value (for shredded x) and value (for fallback cases) @@ -2730,7 +2728,7 @@ mod test { .unwrap(), ) as ArrayRef; let a_field_shredded = ShreddedVariantFieldArray::from_parts( - Some(Arc::new(a_value_array)), + Arc::new(a_value_array), Some(a_inner_typed_value), None, ); @@ -2751,7 +2749,7 @@ mod test { // Build final VariantArray ArrayRef::from(VariantArray::from_parts( Arc::new(metadata_array), - Some(Arc::new(value_array)), + Arc::new(value_array), Some(Arc::new(typed_value_struct)), None, )) @@ -2802,11 +2800,8 @@ mod test { // Level 3: x field (deepest level) let x_typed_value = Int32Array::from(vec![Some(100), None, None]); - let x_field_shredded = ShreddedVariantFieldArray::from_parts( - None, - Some(Arc::new(x_typed_value) as ArrayRef), - None, - ); + let x_field_shredded = + ShreddedVariantFieldArray::perfectly_shredded(Arc::new(x_typed_value) as ArrayRef); // Level 2: b field containing x field + value field let b_value_data = { @@ -2832,7 +2827,7 @@ mod test { .unwrap(), ) as ArrayRef; let b_field_shredded = ShreddedVariantFieldArray::from_parts( - Some(Arc::new(b_value_array)), + Arc::new(b_value_array), Some(b_inner_typed_value), None, ); @@ -2861,7 +2856,7 @@ mod test { .unwrap(), ) as ArrayRef; let a_field_shredded = ShreddedVariantFieldArray::from_parts( - Some(Arc::new(a_value_array)), + Arc::new(a_value_array), Some(a_inner_typed_value), None, ); @@ -2882,7 +2877,7 @@ mod test { // Build final VariantArray ArrayRef::from(VariantArray::from_parts( Arc::new(metadata_array), - Some(Arc::new(value_array)), + Arc::new(value_array), Some(Arc::new(typed_value_struct)), None, )) @@ -3634,27 +3629,21 @@ mod test { // Create shredded fields with different null patterns // Field "a": present in rows 0,3 (missing in rows 1,2,4) let a_field_typed_value = Int32Array::from(vec![Some(1), None, None, Some(1), None]); - let a_field_shredded = ShreddedVariantFieldArray::from_parts( - None, - Some(Arc::new(a_field_typed_value) as ArrayRef), - None, - ); + let a_field_shredded = ShreddedVariantFieldArray::perfectly_shredded(Arc::new( + a_field_typed_value, + ) as ArrayRef); // Field "b": present in rows 0,2 (missing in rows 1,3,4) let b_field_typed_value = Int32Array::from(vec![Some(2), None, Some(2), None, None]); - let b_field_shredded = ShreddedVariantFieldArray::from_parts( - None, - Some(Arc::new(b_field_typed_value) as ArrayRef), - None, - ); + let b_field_shredded = ShreddedVariantFieldArray::perfectly_shredded(Arc::new( + b_field_typed_value, + ) as ArrayRef); // Field "c": present in row 0 only (missing in all other rows) let c_field_typed_value = Int32Array::from(vec![Some(3), None, None, None, None]); - let c_field_shredded = ShreddedVariantFieldArray::from_parts( - None, - Some(Arc::new(c_field_typed_value) as ArrayRef), - None, - ); + let c_field_shredded = ShreddedVariantFieldArray::perfectly_shredded(Arc::new( + c_field_typed_value, + ) as ArrayRef); // Create main typed_value struct let typed_value_fields = Fields::from(vec![ @@ -3674,10 +3663,9 @@ mod test { .unwrap(); // Build final VariantArray with top-level nulls - ArrayRef::from(VariantArray::from_parts( + ArrayRef::from(VariantArray::perfectly_shredded( Arc::new(metadata_array), - None, - Some(Arc::new(typed_value_struct)), + Arc::new(typed_value_struct), Some(nulls), )) } @@ -3690,11 +3678,8 @@ mod test { // Create the inner level: contains typed_value with Int32 values // Row 0: has value 42, Row 1: inner null, Row 2: outer null, Row 3: top-level null let inner_typed_value = Int32Array::from(vec![Some(42), None, None, None]); // dummy value for row 2 - let inner = ShreddedVariantFieldArray::from_parts( - None, - Some(Arc::new(inner_typed_value) as ArrayRef), - None, - ); + let inner = + ShreddedVariantFieldArray::perfectly_shredded(Arc::new(inner_typed_value) as ArrayRef); let outer_typed_value_nulls = NullBuffer::from(vec![ true, // row 0: inner struct exists with typed_value=42 @@ -3707,11 +3692,8 @@ mod test { .with_nulls(outer_typed_value_nulls) .build(); - let outer = ShreddedVariantFieldArray::from_parts( - None, - Some(Arc::new(outer_typed_value) as ArrayRef), - None, - ); + let outer = + ShreddedVariantFieldArray::perfectly_shredded(Arc::new(outer_typed_value) as ArrayRef); let typed_value_nulls = NullBuffer::from(vec![ true, // row 0: inner struct exists with typed_value=42 @@ -3733,10 +3715,9 @@ mod test { true, // row 2: outer field NULL false, // row 3: top-level NULL ]); - ArrayRef::from(VariantArray::from_parts( + ArrayRef::from(VariantArray::perfectly_shredded( Arc::new(metadata_array), - None, - Some(Arc::new(typed_value)), + Arc::new(typed_value), Some(nulls), )) } @@ -3788,11 +3769,9 @@ mod test { // Create shredded field "x" (globally shredded - never appears in value field) // For top-level null row, the field still needs valid content (not null) let x_field_typed_value = Int32Array::from(vec![Some(1), Some(2), Some(3), Some(0)]); - let x_field_shredded = ShreddedVariantFieldArray::from_parts( - None, - Some(Arc::new(x_field_typed_value) as ArrayRef), - None, - ); + let x_field_shredded = ShreddedVariantFieldArray::perfectly_shredded(Arc::new( + x_field_typed_value, + ) as ArrayRef); // Create main typed_value struct (only contains shredded fields) let typed_value_struct = StructArrayBuilder::new() @@ -3804,7 +3783,7 @@ mod test { let variant_nulls = NullBuffer::from(vec![true, true, true, false]); // Row 3 is top-level null ArrayRef::from(VariantArray::from_parts( Arc::new(metadata_array), - Some(Arc::new(value_array)), + Arc::new(value_array), Some(Arc::new(typed_value_struct)), Some(variant_nulls), )) @@ -4507,7 +4486,7 @@ mod test { ])); let all_nulls_erased: ArrayRef = all_nulls_values.clone(); let all_nulls_field = - ShreddedVariantFieldArray::from_parts(None, Some(all_nulls_erased.clone()), None); + ShreddedVariantFieldArray::perfectly_shredded(all_nulls_erased.clone()); let all_nulls_type = all_nulls_field.data_type().clone(); let all_nulls_struct: ArrayRef = ArrayRef::from(all_nulls_field); @@ -4516,7 +4495,7 @@ mod test { Arc::new(Int32Array::from(vec![Some(10), None, Some(30)])); let some_nulls_erased: ArrayRef = some_nulls_values.clone(); let some_nulls_field = - ShreddedVariantFieldArray::from_parts(None, Some(some_nulls_erased.clone()), None); + ShreddedVariantFieldArray::perfectly_shredded(some_nulls_erased.clone()); let some_nulls_type = some_nulls_field.data_type().clone(); let some_nulls_struct: ArrayRef = ArrayRef::from(some_nulls_field); @@ -4524,8 +4503,7 @@ mod test { let inner_values: Arc = Arc::new(Int32Array::from(vec![Some(111), None, Some(333)])); let inner_erased: ArrayRef = inner_values.clone(); - let inner_field = - ShreddedVariantFieldArray::from_parts(None, Some(inner_erased.clone()), None); + let inner_field = ShreddedVariantFieldArray::perfectly_shredded(inner_erased.clone()); let inner_field_type = inner_field.data_type().clone(); let inner_struct_array: ArrayRef = ArrayRef::from(inner_field); @@ -4539,7 +4517,7 @@ mod test { ); let nested_struct_erased: ArrayRef = nested_struct.clone(); let struct_field = - ShreddedVariantFieldArray::from_parts(None, Some(nested_struct_erased.clone()), None); + ShreddedVariantFieldArray::perfectly_shredded(nested_struct_erased.clone()); let struct_field_type = struct_field.data_type().clone(); let struct_field_struct: ArrayRef = ArrayRef::from(struct_field); @@ -4559,10 +4537,9 @@ mod test { EMPTY_VARIANT_METADATA_BYTES, all_nulls_values.len(), )); - let variant_array: ArrayRef = VariantArray::from_parts( + let variant_array: ArrayRef = VariantArray::perfectly_shredded( Arc::new(metadata), - None, - Some(Arc::new(typed_value_struct)), + Arc::new(typed_value_struct), None, ) .into(); @@ -4926,8 +4903,7 @@ mod test { ))); let typed_value: ArrayRef = Arc::new($typed_value); let variant_array: ArrayRef = - VariantArray::from_parts(metadata, None, Some(typed_value), $parent_nulls) - .into(); + VariantArray::perfectly_shredded(metadata, typed_value, $parent_nulls).into(); let result = variant_get( &variant_array, diff --git a/parquet-variant-compute/src/variant_to_arrow.rs b/parquet-variant-compute/src/variant_to_arrow.rs index 9841da555da0..9598d7e71f9a 100644 --- a/parquet-variant-compute/src/variant_to_arrow.rs +++ b/parquet-variant-compute/src/variant_to_arrow.rs @@ -1001,7 +1001,7 @@ impl<'a> ListElementBuilder<'a> { Self::Shredded(b) => { let (value, typed_value, nulls) = b.finish()?; Ok(ArrayRef::from(ShreddedVariantFieldArray::from_parts( - Some(Arc::new(value)), + Arc::new(value), Some(typed_value), nulls, ))) @@ -1253,7 +1253,7 @@ impl VariantToBinaryVariantArrowRowBuilder { fn finish(mut self) -> Result { let variant_array = VariantArray::from_parts( self.metadata, - Some(Arc::new(self.builder.build()?)), + Arc::new(self.builder.build()?), None, // no typed_value column self.nulls.finish(), ); diff --git a/parquet/tests/variant_integration.rs b/parquet/tests/variant_integration.rs index b97b3f225485..b511bb735b9c 100644 --- a/parquet/tests/variant_integration.rs +++ b/parquet/tests/variant_integration.rs @@ -55,6 +55,8 @@ macro_rules! variant_test_case { // Notes // - case 3 is empty in cases.json for some reason // - cases 40, 42, 87, 127 and 128 are expected to fail always (they include invalid variants) +// - cases 41, 131 and 138 are expected to fail because their schemas omit the +// required `value` column (https://github.com/apache/arrow-rs/issues/10306) // - the remaining cases are expected to (eventually) pass variant_test_case!(1); @@ -104,7 +106,9 @@ variant_test_case!(38); variant_test_case!(39); // Is an error case (should be failing as the expected error message indicates) variant_test_case!(40, "both value and typed_value are non-null"); -variant_test_case!(41); +// Is an error case: the spec requires the `value` column to always be present +// (testArrayMissingValueColumn, https://github.com/apache/arrow-rs/issues/10306) +variant_test_case!(41, "StructArray must contain a 'value' field"); // Is an error case (should be failing as the expected error message indicates) variant_test_case!(42, "both value and typed_value are non-null"); // Is an error case (should be failing as the expected error message indicates) @@ -201,7 +205,9 @@ variant_test_case!(127, "Illegal shredded value type: UInt32"); variant_test_case!(128, "Expected object in value field"); variant_test_case!(129); variant_test_case!(130); -variant_test_case!(131); +// Is an error case: the spec requires the `value` column to always be present +// (testMissingValueColumn, https://github.com/apache/arrow-rs/issues/10306) +variant_test_case!(131, "StructArray must contain a 'value' field"); variant_test_case!(132); variant_test_case!(133); variant_test_case!(134); @@ -209,7 +215,9 @@ variant_test_case!(135); variant_test_case!(136); // Is an error case (should be failing as the expected error message indicates) variant_test_case!(137, "Illegal shredded value type: FixedSizeBinary(4)"); -variant_test_case!(138); +// Is an error case: the spec requires the `value` column to always be present +// (testShreddedObjectMissingValueColumn, https://github.com/apache/arrow-rs/issues/10306) +variant_test_case!(138, "StructArray must contain a 'value' field"); /// Test case definition structure matching the format from /// `parquet-testing/parquet_shredded/cases.json` From c0c9a8848240f4ca1265e980d46c23794adb50fa Mon Sep 17 00:00:00 2001 From: sdf-jkl Date: Fri, 10 Jul 2026 23:25:58 -0400 Subject: [PATCH 2/5] make `value` non-nullable for `variant_get as_type None`/`unshred_variant` --- .../src/unshred_variant.rs | 177 ++++++++++++++++-- parquet-variant-compute/src/variant_array.rs | 29 ++- parquet-variant-compute/src/variant_get.rs | 21 ++- .../src/variant_to_arrow.rs | 5 +- 4 files changed, 212 insertions(+), 20 deletions(-) diff --git a/parquet-variant-compute/src/unshred_variant.rs b/parquet-variant-compute/src/unshred_variant.rs index 08c58fe0f532..5f5b48332f91 100644 --- a/parquet-variant-compute/src/unshred_variant.rs +++ b/parquet-variant-compute/src/unshred_variant.rs @@ -25,6 +25,7 @@ use arrow::array::{ LargeBinaryArray, LargeStringArray, ListLikeArray, PrimitiveArray, StringArray, StringViewArray, StructArray, }; +use arrow::buffer::NullBuffer; use arrow::datatypes::{ ArrowPrimitiveType, DataType, Date32Type, Decimal32Type, Decimal64Type, Decimal128Type, DecimalType, Float32Type, Float64Type, Int8Type, Int16Type, Int32Type, Int64Type, @@ -58,18 +59,33 @@ use uuid::Uuid; /// - If the shredded data contains spec violations (e.g., field name conflicts) /// - If unsupported data types are encountered in typed_value columns pub fn unshred_variant(array: &VariantArray) -> Result { - // Check if already unshredded (optimization for common case) - if array.typed_value_column().is_none() { - return Ok(array.clone()); + let nulls = array.nulls(); + let metadata = array.metadata_column(); + let value_col = array.value_column(); + let typed_value_col = array.typed_value_column(); + + // Already unshredded: no data movement needed, but the output must annotate `value` as + // non-nullable per the spec. Inputs whose value-nulls are not all masked by the parent null + // buffer (spec-invalid "missing" rows) cannot be re-annotated and fall through to the row + // builder below, which materializes `Variant::Null` for such rows. + if typed_value_col.is_none() { + if value_field_is_non_nullable(array) { + return Ok(array.clone()); + } + if value_nulls_are_masked(value_col, nulls) { + return Ok(VariantArray::from_parts_unshredded( + metadata.clone(), + value_col.clone(), + nulls.cloned(), + )); + } } // NOTE: None/None at top-level is technically invalid, but the shredding spec requires us to // emit `Variant::Null` when a required value is missing. - let nulls = array.nulls(); let mut row_builder = UnshredVariantRowBuilder::try_new_opt(array.inner())? .unwrap_or_else(UnshredVariantRowBuilder::null); - let metadata = array.metadata_column(); let mut value_builder = VariantValueArrayBuilder::new(array.len()); for i in 0..array.len() { if array.is_null(i) { @@ -82,19 +98,43 @@ pub fn unshred_variant(array: &VariantArray) -> Result { })?; let metadata = VariantMetadata::try_new(metadata_bytes)?; let mut value_builder = value_builder.builder_ext(&metadata); - row_builder.append_row(&mut value_builder, &metadata, i)?; + if value_col.is_null(i) && typed_value_col.is_none_or(|tv| tv.is_null(i)) { + // Missing top-level value (spec-invalid): emit `Variant::Null` rather than the + // physical null the row builder would produce, which the non-nullable output + // `value` field could not represent. + value_builder.append_value(Variant::Null); + } else { + row_builder.append_row(&mut value_builder, &metadata, i)?; + } } } let value = value_builder.build()?; - Ok(VariantArray::from_parts( + Ok(VariantArray::from_parts_unshredded( metadata.clone(), Arc::new(value), - None, nulls.cloned(), )) } +fn value_field_is_non_nullable(array: &VariantArray) -> bool { + array + .inner() + .fields() + .find("value") + .is_some_and(|(_, field)| !field.is_nullable()) +} + +/// Returns true if every null in `value` is masked by a parent null, i.e. the column may be +/// annotated non-nullable. +fn value_nulls_are_masked(value: &ArrayRef, parent_nulls: Option<&NullBuffer>) -> bool { + match value.logical_nulls() { + None => true, + Some(value_nulls) if value_nulls.null_count() == 0 => true, + Some(value_nulls) => parent_nulls.is_some_and(|parent| parent.contains(&value_nulls)), + } +} + /// Row builder for converting shredded VariantArray rows back to unshredded form enum UnshredVariantRowBuilder<'a> { PrimitiveInt8(UnshredPrimitiveRowBuilder<'a, PrimitiveArray>), @@ -696,16 +736,24 @@ impl<'a, L: ListLikeArray> ListUnshredVariantBuilder<'a, L> { #[cfg(test)] mod tests { - use crate::VariantArray; + use crate::{VariantArray, VariantArrayBuilder, shred_variant}; use arrow::array::{ - ArrayRef, BinaryArray, BinaryViewArray, LargeBinaryArray, LargeStringArray, StringViewArray, + Array, ArrayRef, AsArray, BinaryArray, BinaryViewArray, Int64Array, LargeBinaryArray, + LargeStringArray, StringViewArray, }; - use parquet_variant::Variant; + use arrow::datatypes::DataType; + use parquet_variant::{EMPTY_VARIANT_METADATA_BYTES, Variant}; use std::sync::Arc; + /// Returns the nullability annotation of the `value` field + fn value_field_is_nullable(array: &VariantArray) -> bool { + let (_, field) = array.inner().fields().find("value").unwrap(); + field.is_nullable() + } + #[test] fn test_unshred_utf8view_typed_value() { - let metadata_bytes: &[u8] = &[0x01, 0x00, 0x00]; + let metadata_bytes = EMPTY_VARIANT_METADATA_BYTES; let metadata: ArrayRef = Arc::new(BinaryViewArray::from_iter_values(vec![metadata_bytes; 3])); @@ -727,7 +775,7 @@ mod tests { #[test] fn test_unshred_largeutf8_typed_value() { - let metadata_bytes: &[u8] = &[0x01, 0x00, 0x00]; + let metadata_bytes = EMPTY_VARIANT_METADATA_BYTES; let metadata: ArrayRef = Arc::new(BinaryViewArray::from_iter_values(vec![metadata_bytes; 3])); @@ -749,7 +797,7 @@ mod tests { #[test] fn test_unshred_binary_typed_value() { - let metadata_bytes: &[u8] = &[0x01, 0x00, 0x00]; + let metadata_bytes = EMPTY_VARIANT_METADATA_BYTES; let metadata: ArrayRef = Arc::new(BinaryViewArray::from_iter_values(vec![metadata_bytes; 3])); @@ -771,7 +819,7 @@ mod tests { #[test] fn test_unshred_largebinary_typed_value() { - let metadata_bytes: &[u8] = &[0x01, 0x00, 0x00]; + let metadata_bytes = EMPTY_VARIANT_METADATA_BYTES; let metadata: ArrayRef = Arc::new(BinaryViewArray::from_iter_values(vec![metadata_bytes; 3])); @@ -791,6 +839,105 @@ mod tests { assert_eq!(result.value(2), Variant::from(&b"\xde\xad\xbe\xef"[..])); } + #[test] + fn test_shred_unshred_round_trip_annotates_value_non_nullable() { + let mut builder = VariantArrayBuilder::new(1); + builder.append_variant(Variant::from(42i64)); + let original = builder.build(); + assert!(!value_field_is_nullable(&original)); + + let shredded = shred_variant(&original, &DataType::Int64).unwrap(); + assert!(value_field_is_nullable(&shredded)); // legal: typed_value present + + let unshredded = crate::unshred_variant(&shredded).unwrap(); + assert!(!value_field_is_nullable(&unshredded)); + assert_eq!(unshredded, original); + } + + #[test] + fn test_unshred_with_nulls_annotates_value_non_nullable() { + // a null row plus an unshreddable row, so the shredded input exercises parent + // nulls and both value/typed_value columns + let mut builder = VariantArrayBuilder::new(3); + builder.append_variant(Variant::from(1i64)); + builder.append_null(); + builder.append_variant(Variant::from("s")); + let original = builder.build(); + + let shredded = shred_variant(&original, &DataType::Int64).unwrap(); + let unshredded = crate::unshred_variant(&shredded).unwrap(); + + assert!(!value_field_is_nullable(&unshredded)); + assert_eq!(unshredded.len(), 3); + assert_eq!(unshredded.value(0), Variant::from(1i64)); + assert!(unshredded.is_null(1)); + assert_eq!(unshredded.value(2), Variant::from("s")); + } + + #[test] + fn test_unshred_already_unshredded_reannotates_nullable_value() { + let mut builder = VariantArrayBuilder::new(2); + builder.append_variant(Variant::from(42i64)); + builder.append_null(); + let original = builder.build(); + + // same data, but with the out-of-spec nullable `value` annotation + let nullable_input = VariantArray::from_parts( + original.metadata_column().clone(), + original.value_column().clone(), + None, + original.nulls().cloned(), + ); + assert!(value_field_is_nullable(&nullable_input)); + + let unshredded = crate::unshred_variant(&nullable_input).unwrap(); + assert!(!value_field_is_nullable(&unshredded)); + assert_eq!(unshredded, original); + } + + #[test] + fn test_unshred_missing_top_level_value_becomes_variant_null() { + let metadata_bytes = EMPTY_VARIANT_METADATA_BYTES; + let metadata: ArrayRef = + Arc::new(BinaryViewArray::from_iter_values(vec![metadata_bytes; 3])); + + // row 1 is null in both value and typed_value with a valid parent row: spec-invalid + // "missing" value, tolerated as Variant::Null (like `VariantArray::try_value`) + let typed_value: ArrayRef = Arc::new(Int64Array::from(vec![Some(1), None, Some(3)])); + let variant_array = VariantArray::perfectly_shredded(metadata, typed_value, None); + + let result = crate::unshred_variant(&variant_array).unwrap(); + + assert!(!value_field_is_nullable(&result)); + assert_eq!(result.inner().null_count(), 0); + assert_eq!(result.value(0), Variant::from(1i64)); + assert_eq!(result.value(1), Variant::Null); + assert_eq!(result.value(2), Variant::from(3i64)); + } + + #[test] + fn test_unshred_value_only_with_unmasked_nulls_materializes_variant_null() { + let mut builder = VariantArrayBuilder::new(1); + builder.append_variant(Variant::from(42i64)); + let single = builder.build(); + let metadata_bytes = single.metadata_column().as_binary_view().value(0); + let value_bytes = single.value_column().as_binary_view().value(0); + + // unshredded input whose `value` null is not masked by a parent null: cannot be + // re-annotated in place, so unshredding must materialize Variant::Null bytes + let metadata: ArrayRef = + Arc::new(BinaryViewArray::from_iter_values(vec![metadata_bytes; 2])); + let value: ArrayRef = Arc::new(BinaryViewArray::from(vec![Some(value_bytes), None])); + let input = VariantArray::from_parts(metadata, value, None, None); + + let result = crate::unshred_variant(&input).unwrap(); + + assert!(!value_field_is_nullable(&result)); + assert_eq!(result.inner().null_count(), 0); + assert_eq!(result.value(0), Variant::from(42i64)); + assert_eq!(result.value(1), Variant::Null); + } + #[test] fn test_unshred_returns_err_on_malformed_metadata() { // empty metadata bytes fail VariantMetadata's header parse. before this fix the diff --git a/parquet-variant-compute/src/variant_array.rs b/parquet-variant-compute/src/variant_array.rs index 2e860d33b374..25f1e155940b 100644 --- a/parquet-variant-compute/src/variant_array.rs +++ b/parquet-variant-compute/src/variant_array.rs @@ -337,15 +337,42 @@ impl VariantArray { Self::from_parts(metadata, value, Some(typed_value), nulls) } + /// Note: annotates `value` as nullable, which the spec only permits for shredded + /// variants. It is also needed by `variant_get`'s unshredded intermediates, whose + /// `value` column can contain unmasked nulls. Unshredded producers should use + /// [`Self::from_parts_unshredded`] instead. pub(crate) fn from_parts( metadata: ArrayRef, value: ArrayRef, typed_value: Option, nulls: Option, + ) -> Self { + Self::from_parts_with_nullable_value(metadata, value, typed_value, nulls, true) + } + + /// Construct an unshredded `VariantArray`, annotating `value` as non-nullable as the + /// spec requires when there is no `typed_value` column. + /// + /// # Panics + /// If `value` contains nulls not masked by `nulls`. + pub(crate) fn from_parts_unshredded( + metadata: ArrayRef, + value: ArrayRef, + nulls: Option, + ) -> Self { + Self::from_parts_with_nullable_value(metadata, value, None, nulls, false) + } + + fn from_parts_with_nullable_value( + metadata: ArrayRef, + value: ArrayRef, + typed_value: Option, + nulls: Option, + value_nullable: bool, ) -> Self { let mut builder = StructArrayBuilder::new() .with_field("metadata", metadata.clone(), false) - .with_field("value", value.clone(), true); + .with_field("value", value.clone(), value_nullable); if let Some(typed_value) = typed_value.clone() { builder = builder.with_field_ref(typed_value_field(&typed_value), typed_value); } diff --git a/parquet-variant-compute/src/variant_get.rs b/parquet-variant-compute/src/variant_get.rs index a2e2bb2d07dd..cdad4a58c1d5 100644 --- a/parquet-variant-compute/src/variant_get.rs +++ b/parquet-variant-compute/src/variant_get.rs @@ -292,10 +292,9 @@ fn shredded_get_path( // Propagating metadata is not necessary for an all-NULL array, but is cheaper than constructing // a new empty metadata array. (n * 3 bytes vs Arc bump) let metadata = input.metadata_column().clone(); - let arr = VariantArray::from_parts( + let arr = VariantArray::from_parts_unshredded( metadata, all_null_value_column(num_rows), - None, all_nulls, ); return Ok(ArrayRef::from(arr)); @@ -2427,6 +2426,22 @@ mod test { ); } + #[test] + fn test_variant_get_missing_path_as_variant_annotates_value_non_nullable() { + let (unshredded, shredded) = create_variant_get_as_variant_test_data(); + let variant_field = VariantArray::try_new(&unshredded).unwrap().field("result"); + + // indexing into a struct typed_value can never match: all-null variant output + let options = GetOptions::new_with_path(VariantPath::try_from("field_name[0]").unwrap()) + .with_as_type(Some(FieldRef::from(variant_field))); + let result = variant_get(&shredded, options).unwrap(); + let result_variant = VariantArray::try_new(&result).unwrap(); + + assert_eq!(result_variant.inner().null_count(), result_variant.len()); + let (_, value_field) = result_variant.inner().fields().find("value").unwrap(); + assert!(!value_field.is_nullable()); + } + fn create_variant_get_as_variant_test_data() -> (ArrayRef, ArrayRef) { let input_json: ArrayRef = Arc::new(StringArray::from(vec![ Some(r#"{"field_name": {"k": 100000}}"#), @@ -2458,6 +2473,8 @@ mod test { assert!(result_variant.typed_value_column().is_none()); assert!(result_variant.value_column().null_count() < result_variant.len()); + let (_, value_field) = result_variant.inner().fields().find("value").unwrap(); + assert!(!value_field.is_nullable()); let expected_json: ArrayRef = Arc::new(StringArray::from(vec![ Some(r#"{"k":100000}"#), diff --git a/parquet-variant-compute/src/variant_to_arrow.rs b/parquet-variant-compute/src/variant_to_arrow.rs index 9598d7e71f9a..94f61083ff7c 100644 --- a/parquet-variant-compute/src/variant_to_arrow.rs +++ b/parquet-variant-compute/src/variant_to_arrow.rs @@ -1251,10 +1251,11 @@ impl VariantToBinaryVariantArrowRowBuilder { } fn finish(mut self) -> Result { - let variant_array = VariantArray::from_parts( + // value-nulls are appended only alongside parent nulls, so the non-nullable + // `value` annotation is always valid here + let variant_array = VariantArray::from_parts_unshredded( self.metadata, Arc::new(self.builder.build()?), - None, // no typed_value column self.nulls.finish(), ); From 512a9f128874bc4d35200cf4bfbd95a3ff456d07 Mon Sep 17 00:00:00 2001 From: sdf-jkl Date: Thu, 30 Jul 2026 13:44:04 -0400 Subject: [PATCH 3/5] small api swap --- parquet-variant-compute/src/unshred_variant.rs | 8 +++----- parquet-variant-compute/src/variant_get.rs | 4 ++-- 2 files changed, 5 insertions(+), 7 deletions(-) diff --git a/parquet-variant-compute/src/unshred_variant.rs b/parquet-variant-compute/src/unshred_variant.rs index 5f5b48332f91..5992b331ac87 100644 --- a/parquet-variant-compute/src/unshred_variant.rs +++ b/parquet-variant-compute/src/unshred_variant.rs @@ -120,9 +120,8 @@ pub fn unshred_variant(array: &VariantArray) -> Result { fn value_field_is_non_nullable(array: &VariantArray) -> bool { array .inner() - .fields() - .find("value") - .is_some_and(|(_, field)| !field.is_nullable()) + .field_by_name("value") + .is_some_and(|field| !field.is_nullable()) } /// Returns true if every null in `value` is masked by a parent null, i.e. the column may be @@ -747,8 +746,7 @@ mod tests { /// Returns the nullability annotation of the `value` field fn value_field_is_nullable(array: &VariantArray) -> bool { - let (_, field) = array.inner().fields().find("value").unwrap(); - field.is_nullable() + array.inner().field_by_name("value").unwrap().is_nullable() } #[test] diff --git a/parquet-variant-compute/src/variant_get.rs b/parquet-variant-compute/src/variant_get.rs index f6fbbc2ba342..fd749c4ad299 100644 --- a/parquet-variant-compute/src/variant_get.rs +++ b/parquet-variant-compute/src/variant_get.rs @@ -2438,7 +2438,7 @@ mod test { let result_variant = VariantArray::try_new(&result).unwrap(); assert_eq!(result_variant.inner().null_count(), result_variant.len()); - let (_, value_field) = result_variant.inner().fields().find("value").unwrap(); + let value_field = result_variant.inner().field_by_name("value").unwrap(); assert!(!value_field.is_nullable()); } @@ -2473,7 +2473,7 @@ mod test { assert!(result_variant.typed_value_column().is_none()); assert!(result_variant.value_column().null_count() < result_variant.len()); - let (_, value_field) = result_variant.inner().fields().find("value").unwrap(); + let value_field = result_variant.inner().field_by_name("value").unwrap(); assert!(!value_field.is_nullable()); let expected_json: ArrayRef = Arc::new(StringArray::from(vec![ From 4184a5d896b75d280aa74606efa358887bffd043 Mon Sep 17 00:00:00 2001 From: sdf-jkl Date: Thu, 30 Jul 2026 18:18:11 -0400 Subject: [PATCH 4/5] addressing @klion26's review --- .../src/unshred_variant.rs | 137 +++++++++++++++--- 1 file changed, 119 insertions(+), 18 deletions(-) diff --git a/parquet-variant-compute/src/unshred_variant.rs b/parquet-variant-compute/src/unshred_variant.rs index 5992b331ac87..aac882789ffe 100644 --- a/parquet-variant-compute/src/unshred_variant.rs +++ b/parquet-variant-compute/src/unshred_variant.rs @@ -36,8 +36,8 @@ use arrow::temporal_conversions::time64us_to_time; use chrono::{DateTime, Utc}; use indexmap::IndexMap; use parquet_variant::{ - ObjectFieldBuilder, Variant, VariantBuilderExt, VariantDecimal4, VariantDecimal8, - VariantDecimal16, VariantDecimalType, VariantMetadata, + ListBuilder, ObjectBuilder, ObjectFieldBuilder, Variant, VariantBuilderExt, VariantDecimal4, + VariantDecimal8, VariantDecimal16, VariantDecimalType, VariantMetadata, }; use std::marker::PhantomData; use std::sync::Arc; @@ -67,7 +67,7 @@ pub fn unshred_variant(array: &VariantArray) -> Result { // Already unshredded: no data movement needed, but the output must annotate `value` as // non-nullable per the spec. Inputs whose value-nulls are not all masked by the parent null // buffer (spec-invalid "missing" rows) cannot be re-annotated and fall through to the row - // builder below, which materializes `Variant::Null` for such rows. + // loop below, whose top-level sink materializes `Variant::Null` for such rows. if typed_value_col.is_none() { if value_field_is_non_nullable(array) { return Ok(array.clone()); @@ -97,15 +97,8 @@ pub fn unshred_variant(array: &VariantArray) -> Result { ) })?; let metadata = VariantMetadata::try_new(metadata_bytes)?; - let mut value_builder = value_builder.builder_ext(&metadata); - if value_col.is_null(i) && typed_value_col.is_none_or(|tv| tv.is_null(i)) { - // Missing top-level value (spec-invalid): emit `Variant::Null` rather than the - // physical null the row builder would produce, which the non-nullable output - // `value` field could not represent. - value_builder.append_value(Variant::Null); - } else { - row_builder.append_row(&mut value_builder, &metadata, i)?; - } + let mut row_sink = TopLevelRowSink(value_builder.builder_ext(&metadata)); + row_builder.append_row(&mut row_sink, &metadata, i)?; } } @@ -127,10 +120,41 @@ fn value_field_is_non_nullable(array: &VariantArray) -> bool { /// Returns true if every null in `value` is masked by a parent null, i.e. the column may be /// annotated non-nullable. fn value_nulls_are_masked(value: &ArrayRef, parent_nulls: Option<&NullBuffer>) -> bool { - match value.logical_nulls() { - None => true, - Some(value_nulls) if value_nulls.null_count() == 0 => true, - Some(value_nulls) => parent_nulls.is_some_and(|parent| parent.contains(&value_nulls)), + value.null_count() == 0 + || parent_nulls + .zip(value.nulls()) + .is_some_and(|(parent, value_nulls)| parent.contains(value_nulls)) +} + +/// Wraps the sink that every top-level row is appended into. The row builders signal a missing +/// value (value and typed_value both NULL) by calling `append_null`, and this wrapper gives that +/// signal its top-level meaning: `Variant::Null`, because the non-nullable output `value` column +/// cannot hold a physical NULL. Array-level NULL rows are appended before the sink is built, so +/// they never reach it. Nested builders created via `try_new_object`/`try_new_list` are returned +/// unwrapped, so nested missing values keep their own semantics, e.g. [`ObjectFieldBuilder`] +/// omits the field. +struct TopLevelRowSink(B); + +impl VariantBuilderExt for TopLevelRowSink { + type State<'a> + = B::State<'a> + where + Self: 'a; + + fn append_null(&mut self) { + self.0.append_value(Variant::Null); + } + + fn append_value<'m, 'v>(&mut self, value: impl Into>) { + self.0.append_value(value); + } + + fn try_new_list(&mut self) -> Result>> { + self.0.try_new_list() + } + + fn try_new_object(&mut self) -> Result>> { + self.0.try_new_object() } } @@ -740,8 +764,10 @@ mod tests { Array, ArrayRef, AsArray, BinaryArray, BinaryViewArray, Int64Array, LargeBinaryArray, LargeStringArray, StringViewArray, }; - use arrow::datatypes::DataType; - use parquet_variant::{EMPTY_VARIANT_METADATA_BYTES, Variant}; + use arrow::datatypes::{DataType, Field, Fields, TimeUnit}; + use parquet_variant::{ + EMPTY_VARIANT_METADATA_BYTES, Variant, VariantBuilder, VariantDecimal8, + }; use std::sync::Arc; /// Returns the nullability annotation of the `value` field @@ -913,6 +939,81 @@ mod tests { assert_eq!(result.value(2), Variant::from(3i64)); } + /// Shreds `original` to `as_type`, then drops the parent null buffer so the parent-null row + /// becomes a spec-invalid "missing" row (value and typed_value both NULL with a valid + /// parent), and asserts unshredding turns exactly that row into `Variant::Null`. + fn assert_missing_row_unshreds_to_variant_null(original: &VariantArray, as_type: &DataType) { + let shredded = shred_variant(original, as_type).unwrap(); + // Row 0 must actually shred, so that its round trip below exercises the typed + // reconstruction path of this shape's row builder, not the value fallback. + assert!(shredded.typed_value_column().unwrap().is_valid(0)); + + // The parent-null row carries no metadata bytes, so give every row the metadata of row 0. + let metadata_bytes = shredded.metadata_column().as_binary_view().value(0); + let metadata: ArrayRef = Arc::new(BinaryViewArray::from_iter_values(std::iter::repeat_n( + metadata_bytes, + original.len(), + ))); + + let input = VariantArray::from_parts( + metadata, + shredded.value_column().clone(), + shredded.typed_value_column().cloned(), + None, + ); + + let result = crate::unshred_variant(&input).unwrap(); + assert!(!value_field_is_nullable(&result)); + assert_eq!(result.inner().null_count(), 0); + assert_eq!(result.value(0), original.value(0)); + assert_eq!(result.value(1), Variant::Null); + } + + /// Missing rows must become `Variant::Null` through every row-builder shape, since each + /// shape has its own expansion of `handle_unshredded_case`. + #[test] + fn test_unshred_missing_row_for_decimal_timestamp_object_list() { + let mut builder = VariantArrayBuilder::new(2); + builder.append_variant(Variant::from(VariantDecimal8::try_new(1234, 2).unwrap())); + builder.append_null(); + assert_missing_row_unshreds_to_variant_null(&builder.build(), &DataType::Decimal64(18, 2)); + + let mut builder = VariantArrayBuilder::new(2); + builder.append_variant(Variant::from(chrono::DateTime::from_timestamp(1, 0).unwrap())); + builder.append_null(); + assert_missing_row_unshreds_to_variant_null( + &builder.build(), + &DataType::Timestamp(TimeUnit::Microsecond, Some("UTC".into())), + ); + + let mut variant_builder = VariantBuilder::new(); + let mut object_builder = variant_builder.new_object(); + object_builder.insert("a", 1i64); + object_builder.finish(); + let (object_metadata, object_value) = variant_builder.finish(); + let mut builder = VariantArrayBuilder::new(2); + builder.append_variant(Variant::try_new(&object_metadata, &object_value).unwrap()); + builder.append_null(); + assert_missing_row_unshreds_to_variant_null( + &builder.build(), + &DataType::Struct(Fields::from(vec![Field::new("a", DataType::Int64, true)])), + ); + + let mut variant_builder = VariantBuilder::new(); + let mut list_builder = variant_builder.new_list(); + list_builder.append_value(1i64); + list_builder.append_value(2i64); + list_builder.finish(); + let (list_metadata, list_value) = variant_builder.finish(); + let mut builder = VariantArrayBuilder::new(2); + builder.append_variant(Variant::try_new(&list_metadata, &list_value).unwrap()); + builder.append_null(); + assert_missing_row_unshreds_to_variant_null( + &builder.build(), + &DataType::List(Arc::new(Field::new_list_field(DataType::Int64, true))), + ); + } + #[test] fn test_unshred_value_only_with_unmasked_nulls_materializes_variant_null() { let mut builder = VariantArrayBuilder::new(1); From 2059afce8896ee08755013636905eecbd3c32c4a Mon Sep 17 00:00:00 2001 From: sdf-jkl Date: Thu, 30 Jul 2026 18:23:28 -0400 Subject: [PATCH 5/5] fmt --- parquet-variant-compute/src/unshred_variant.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/parquet-variant-compute/src/unshred_variant.rs b/parquet-variant-compute/src/unshred_variant.rs index aac882789ffe..159402ec56c8 100644 --- a/parquet-variant-compute/src/unshred_variant.rs +++ b/parquet-variant-compute/src/unshred_variant.rs @@ -765,9 +765,7 @@ mod tests { LargeStringArray, StringViewArray, }; use arrow::datatypes::{DataType, Field, Fields, TimeUnit}; - use parquet_variant::{ - EMPTY_VARIANT_METADATA_BYTES, Variant, VariantBuilder, VariantDecimal8, - }; + use parquet_variant::{EMPTY_VARIANT_METADATA_BYTES, Variant, VariantBuilder, VariantDecimal8}; use std::sync::Arc; /// Returns the nullability annotation of the `value` field @@ -979,7 +977,9 @@ mod tests { assert_missing_row_unshreds_to_variant_null(&builder.build(), &DataType::Decimal64(18, 2)); let mut builder = VariantArrayBuilder::new(2); - builder.append_variant(Variant::from(chrono::DateTime::from_timestamp(1, 0).unwrap())); + builder.append_variant(Variant::from( + chrono::DateTime::from_timestamp(1, 0).unwrap(), + )); builder.append_null(); assert_missing_row_unshreds_to_variant_null( &builder.build(),