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 7753fce0f674..495728b9a4af 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..ef72359ed733 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 @@ -308,24 +317,36 @@ impl VariantArray { }; validate_binary_array(metadata_col.as_ref(), "metadata")?; + let shredding_state = ShreddingState::try_from(inner)?; + + // `try_from` synthesizes an all-null `value` when the input omits it. Rebuild the inner + // struct so `inner()` and write-back carry the column too. + if inner.column_by_name("value").is_none() { + return Ok(Self::from_parts( + metadata_col.clone(), + shredding_state.value_column().clone(), + shredding_state.typed_value_column().cloned(), + inner.nulls().cloned(), + )); + } + // Note these clones are cheap, they just bump the ref count Ok(Self { inner: inner.clone(), metadata: metadata_col.clone(), - shredding_state: ShreddingState::try_from(inner)?, + shredding_state, }) } 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 +417,26 @@ 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) => { - typed_value_to_variant(typed_value, value, index) + Some(typed_value) if typed_value.is_valid(index) => { + if !matches!(typed_value.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"); + } + typed_value_to_variant(typed_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 +447,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 +685,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 @@ -676,10 +698,22 @@ impl ShreddedVariantFieldArray { )); }; + let shredding_state = ShreddingState::try_from(inner_struct)?; + + // `try_from` synthesizes an all-null `value` when the input omits it. Rebuild the inner + // struct so `inner()` and write-back carry the column too (see `VariantArray::try_new`). + if inner_struct.column_by_name("value").is_none() { + return Ok(Self::from_parts( + shredding_state.value_column().clone(), + shredding_state.typed_value_column().cloned(), + inner_struct.nulls().cloned(), + )); + } + // Note this clone is cheap, it just bumps the ref count Ok(Self { inner: inner_struct.clone(), - shredding_state: ShreddingState::try_from(inner_struct)?, + shredding_state, }) } @@ -688,8 +722,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() } @@ -704,14 +738,11 @@ impl ShreddedVariantFieldArray { } 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 +812,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 +829,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 +844,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 +863,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 +880,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,14 +890,21 @@ 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 typed_value = inner_struct.column_by_name("typed_value").cloned(); + let value = match inner_struct.column_by_name("value") { + Some(value) => { + validate_binary_array(value.as_ref(), "value")?; + value.clone() + } + // Lenient read: a shredded group may omit the spec-required `value`. Synthesize an + // all-null one so the model always has a `value`. + None if typed_value.is_some() => all_null_value_column(inner_struct.len()), + None => { + return Err(ArrowError::InvalidArgumentError( + "Invalid VariantArray: StructArray must contain a 'value' field".to_string(), + )); + } + }; Ok(ShreddingState::new(value, typed_value)) } } @@ -931,16 +972,8 @@ 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>, - index: usize, -) -> Result> { +fn typed_value_to_variant(typed_value: &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)) { - // Only a partially shredded struct is allowed to have values for both columns - panic!("Invalid variant, conflicting value and typed_value"); - } match data_type { DataType::Null => Ok(Variant::Null), DataType::Boolean => { @@ -1244,6 +1277,28 @@ fn canonicalize_and_verify_field(field: &Arc) -> Result, + ) -> Self { + let value = all_null_value_column(typed_value.len()); + Self::from_parts(metadata, value, Some(typed_value), nulls) + } +} + +#[cfg(test)] +impl ShreddedVariantFieldArray { + 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) + } +} + #[cfg(test)] mod test { use crate::VariantArrayBuilder; @@ -1283,30 +1338,36 @@ mod test { } #[test] - fn all_null_missing_value_and_typed_value() { - let fields = Fields::from(vec![Field::new("metadata", DataType::BinaryView, false)]); - let array = StructArray::new(fields, vec![make_binary_view_array()], None); + fn read_missing_value_column() { + // With `typed_value` present, a missing `value` is read leniently: an all-null `value` is + // synthesized and normalized into the inner struct (so a write-back emits it), and values + // still decode from `typed_value`. + let typed_value = Arc::new(Int64Array::from(vec![Some(1), None, Some(3)])) as ArrayRef; + let metadata = + BinaryViewArray::from_iter_values(std::iter::repeat_n(EMPTY_VARIANT_METADATA_BYTES, 3)); + let struct_array = StructArrayBuilder::new() + .with_field("metadata", Arc::new(metadata), false) + .with_field("typed_value", typed_value, true) + .build(); + assert!(struct_array.column_by_name("value").is_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 - } - )); + let variant_array = VariantArray::try_new(&struct_array).unwrap(); + assert_eq!(variant_array.value_column().len(), 3); + assert_eq!(variant_array.value_column().null_count(), 3); + assert!(variant_array.inner().column_by_name("value").is_some()); + assert!(variant_array.typed_value_column().is_some()); + assert_eq!(variant_array.value(0), Variant::from(1i64)); + assert_eq!(variant_array.value(1), Variant::Null); + assert_eq!(variant_array.value(2), Variant::from(3i64)); - // 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); - } - } + // With no `typed_value` either, there is nothing to read, so `value` stays required. + let fields = Fields::from(vec![Field::new("metadata", DataType::BinaryView, false)]); + let metadata_only = StructArray::new(fields, vec![make_binary_view_array()], None); + let err = VariantArray::try_new(&metadata_only); + assert_eq!( + err.unwrap_err().to_string(), + "Invalid argument error: Invalid VariantArray: StructArray must contain a 'value' field" + ); } #[test] @@ -1358,8 +1419,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 +1463,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 +1717,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 +1727,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 +1738,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 59407c617c39..bd28f5c8b7b1 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), )) @@ -4665,7 +4644,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); @@ -4674,7 +4653,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); @@ -4682,8 +4661,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); @@ -4697,7 +4675,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); @@ -4717,10 +4695,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(); @@ -5084,8 +5061,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 8022ded25a67..d7c61b7653af 100644 --- a/parquet-variant-compute/src/variant_to_arrow.rs +++ b/parquet-variant-compute/src/variant_to_arrow.rs @@ -1152,7 +1152,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, ))) @@ -1404,7 +1404,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 f4525185f639..e06426a7a80b 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 omit the spec-required `value` column. They are read leniently by +// synthesizing an all-null `value` (https://github.com/apache/arrow-rs/issues/10306) // - the remaining cases are expected to (eventually) pass variant_test_case!(1); @@ -104,6 +106,8 @@ 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"); +// Schema omits the spec-required `value` column. Read leniently by synthesizing an +// all-null `value` (testArrayMissingValueColumn, https://github.com/apache/arrow-rs/issues/10306) variant_test_case!(41); // 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"); @@ -201,6 +205,8 @@ 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); +// Schema omits the spec-required `value` column. Read leniently by synthesizing an +// all-null `value` (testMissingValueColumn, https://github.com/apache/arrow-rs/issues/10306) variant_test_case!(131); variant_test_case!(132); variant_test_case!(133); @@ -209,6 +215,8 @@ 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)"); +// Schema omits the spec-required `value` column. Read leniently by synthesizing an +// all-null `value` (testShreddedObjectMissingValueColumn, https://github.com/apache/arrow-rs/issues/10306) variant_test_case!(138); /// Test case definition structure matching the format from