[Variant] Support Binary/LargeBinary children #9610
+331
−233
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -96,7 +96,7 @@ pub fn shred_variant(array: &VariantArray, as_type: &DataType) -> Result<Variant | |
| let (value, typed_value, nulls) = builder.finish()?; | ||
| Ok(VariantArray::from_parts( | ||
| array.metadata_field().clone(), | ||
| Some(Arc::new(value) as ArrayRef), | ||
| Some(typed_value), | ||
| nulls, | ||
| )) | ||
|
|
@@ -443,8 +443,11 @@ 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) as ArrayRef), | ||
| Some(typed_value), | ||
| nulls, | ||
| ); | ||
| builder = builder.with_field(field_name, ArrayRef::from(array), false); | ||
| } | ||
| if let Some(nulls) = self.typed_value_nulls.finish() { | ||
|
|
@@ -689,6 +692,7 @@ impl VariantSchemaNode { | |
| mod tests { | ||
| use super::*; | ||
| use crate::VariantArrayBuilder; | ||
| use crate::variant_array::binary_array_value; | ||
| use arrow::array::{ | ||
| Array, BinaryViewArray, FixedSizeBinaryArray, Float64Array, GenericListArray, | ||
| GenericListViewArray, Int64Array, LargeBinaryArray, LargeStringArray, ListArray, | ||
|
|
@@ -867,7 +871,8 @@ mod tests { | |
| ) { | ||
| assert_eq!(array.len(), expected_len); | ||
|
|
||
| let fallback_value = array.value_field().unwrap(); | ||
| let fallback_metadata = array.metadata_field(); | ||
| let array = downcast_list_like_array::<O>(array); | ||
|
|
||
| assert_eq!( | ||
|
|
@@ -887,7 +892,7 @@ mod tests { | |
| ); | ||
| assert_eq!( | ||
| array.len(), | ||
| fallback_value.len(), | ||
| "fallbacks value field should match array length" | ||
| ); | ||
|
|
||
|
|
@@ -902,28 +907,33 @@ mod tests { | |
| // Successfully shredded: typed list value present, no fallback value | ||
| assert!(array.is_valid(idx)); | ||
| assert_eq!(array.value_size(idx), *len); | ||
| assert!(fallback_value.is_null(idx)); | ||
| } | ||
| None => { | ||
| // Unable to shred: typed list value absent, fallback should carry the variant | ||
| assert!(array.is_null(idx)); | ||
| assert_eq!(array.value_size(idx), O::zero()); | ||
| match expected_fallback { | ||
| Some(expected_variant) => { | ||
| assert!(fallback_value.is_valid(idx)); | ||
| let metadata_bytes = | ||
| binary_array_value(fallback_metadata.as_ref(), idx).unwrap(); | ||
| let metadata_bytes = | ||
| if fallback_metadata.is_valid(idx) && !metadata_bytes.is_empty() { | ||
| metadata_bytes | ||
| } else { | ||
| EMPTY_VARIANT_METADATA_BYTES | ||
| }; | ||
| assert_eq!( | ||
| Variant::new( | ||
| metadata_bytes, | ||
| binary_array_value(fallback_value.as_ref(), idx).unwrap() | ||
| ), | ||
| expected_variant.clone() | ||
| ); | ||
| } | ||
| None => { | ||
| assert!(fallback_value.is_null(idx)); | ||
| } | ||
| } | ||
| } | ||
|
|
@@ -983,7 +993,10 @@ mod tests { | |
| Some(expected_variant) => { | ||
| assert!(element_fallbacks.is_valid(idx)); | ||
| assert_eq!( | ||
| Variant::new( | ||
| EMPTY_VARIANT_METADATA_BYTES, | ||
| binary_array_value(element_fallbacks.as_ref(), idx).unwrap() | ||
| ), | ||
| expected_variant.clone() | ||
| ); | ||
| } | ||
|
|
@@ -1129,7 +1142,7 @@ 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]])) as ArrayRef; // minimal valid metadata | ||
| let all_null_array = VariantArray::from_parts(metadata, None, None, None); | ||
| let result = shred_variant(&all_null_array, &DataType::Int64).unwrap(); | ||
|
|
||
|
|
@@ -1243,7 +1256,10 @@ mod tests { | |
| assert!(!value_field.is_null(1)); // value should contain original | ||
| assert!(typed_value_field.is_null(1)); // typed_value should be null | ||
| assert_eq!( | ||
| Variant::new( | ||
| binary_array_value(metadata_field.as_ref(), 1).unwrap(), | ||
| binary_array_value(value_field.as_ref(), 1).unwrap() | ||
| ), | ||
| Variant::from("hello") | ||
| ); | ||
|
|
||
|
|
@@ -1259,7 +1275,10 @@ mod tests { | |
| assert!(!result.is_null(4)); | ||
| assert!(!value_field.is_null(4)); // should contain Variant::Null | ||
| assert_eq!( | ||
| Variant::new( | ||
| binary_array_value(metadata_field.as_ref(), 4).unwrap(), | ||
| binary_array_value(value_field.as_ref(), 4).unwrap() | ||
| ), | ||
| Variant::Null | ||
| ); | ||
| assert!(typed_value_field.is_null(4)); | ||
|
|
@@ -1336,7 +1355,10 @@ mod tests { | |
| assert!(value.is_valid(1)); | ||
| assert!(typed_value.is_null(1)); | ||
| assert_eq!( | ||
| Variant::new( | ||
| binary_array_value(metadata.as_ref(), 1).unwrap(), | ||
| binary_array_value(value.as_ref(), 1).unwrap() | ||
| ), | ||
| Variant::from(42i64) | ||
| ); | ||
|
|
||
|
|
@@ -1350,7 +1372,10 @@ mod tests { | |
| assert!(value.is_valid(3)); | ||
| assert!(typed_value.is_null(3)); | ||
| assert_eq!( | ||
| Variant::new( | ||
| binary_array_value(metadata.as_ref(), 3).unwrap(), | ||
| binary_array_value(value.as_ref(), 3).unwrap() | ||
| ), | ||
| Variant::Null | ||
| ); | ||
|
|
||
|
|
@@ -1392,7 +1417,10 @@ mod tests { | |
| assert!(value.is_valid(1)); | ||
| assert!(typed_value.is_null(1)); | ||
| assert_eq!( | ||
| Variant::new( | ||
| binary_array_value(metadata.as_ref(), 1).unwrap(), | ||
| binary_array_value(value.as_ref(), 1).unwrap() | ||
| ), | ||
| Variant::from("not_binary") | ||
| ); | ||
|
|
||
|
|
@@ -1406,7 +1434,10 @@ mod tests { | |
| assert!(value.is_valid(3)); | ||
| assert!(typed_value.is_null(3)); | ||
| assert_eq!( | ||
| Variant::new( | ||
| binary_array_value(metadata.as_ref(), 3).unwrap(), | ||
| binary_array_value(value.as_ref(), 3).unwrap() | ||
| ), | ||
|
Contributor
There was a problem hiding this comment. There are a lot of these, spread across several test modules. Do you think it would be helpful to define a test helper function that takes a pair of arrays (applying deref) plus an index, and encapsulates the binary array value+unwrap calls? assert_eq!(
variant_from_arrays_at(metadata, value, 3),
Ok(Variant::Null)
); |
||
| Variant::Null | ||
| ); | ||
|
|
||
|
|
@@ -1682,14 +1713,14 @@ mod tests { | |
| .unwrap(); | ||
| let outer_fallbacks = outer_elements.value_field().unwrap(); | ||
|
|
||
| let outer_metadata = Arc::new(BinaryViewArray::from_iter_values(std::iter::repeat_n( | ||
| EMPTY_VARIANT_METADATA_BYTES, | ||
| outer_elements.len(), | ||
| ))) as ArrayRef; | ||
| let outer_variant = VariantArray::from_parts( | ||
| outer_metadata, | ||
| Some(outer_fallbacks.clone()), | ||
| Some(Arc::new(outer_values.clone()) as ArrayRef), | ||
| None, | ||
| ); | ||
|
|
||
|
|
@@ -1792,7 +1823,10 @@ mod tests { | |
| // null is stored as Variant::Null in values | ||
| assert!(id_values.is_valid(1)); | ||
| assert_eq!( | ||
| Variant::new( | ||
| EMPTY_VARIANT_METADATA_BYTES, | ||
| binary_array_value(id_values.as_ref(), 1).unwrap() | ||
| ), | ||
| Variant::Null | ||
| ); | ||
| assert!(id_typed_values.is_null(1)); | ||
|
|
@@ -1866,7 +1900,6 @@ mod tests { | |
| assert_eq!(result.len(), 9); | ||
|
|
||
| let metadata = result.metadata_field(); | ||
| let value = result.value_field().unwrap(); | ||
| let typed_value = result | ||
| .typed_value_field() | ||
|
|
@@ -1882,24 +1915,14 @@ mod tests { | |
| let age_field = | ||
| ShreddedVariantFieldArray::try_new(typed_value.column_by_name("age").unwrap()).unwrap(); | ||
|
|
||
| let score_value = score_field.value_field().unwrap(); | ||
| let score_typed_value = score_field | ||
| .typed_value_field() | ||
| .unwrap() | ||
| .as_any() | ||
| .downcast_ref::<Float64Array>() | ||
| .unwrap(); | ||
| let age_value = age_field.value_field().unwrap(); | ||
| let age_typed_value = age_field | ||
| .typed_value_field() | ||
| .unwrap() | ||
|
|
@@ -1918,10 +1941,13 @@ mod tests { | |
| } | ||
| fn get_value<'m, 'v>( | ||
| i: usize, | ||
| metadata: &'m dyn Array, | ||
| value: &'v dyn Array, | ||
| ) -> Variant<'m, 'v> { | ||
| Variant::new( | ||
| binary_array_value(metadata, i).unwrap(), | ||
| binary_array_value(value, i).unwrap(), | ||
| ) | ||
| } | ||
| let expect = |i, expected_result: Option<ShreddedValue<ShreddedStruct>>| { | ||
| match expected_result { | ||
|
|
@@ -1933,7 +1959,10 @@ mod tests { | |
| match expected_value { | ||
| Some(expected_value) => { | ||
| assert!(value.is_valid(i)); | ||
| assert_eq!( | ||
| expected_value, | ||
| get_value(i, metadata.as_ref(), value.as_ref()) | ||
| ); | ||
| } | ||
| None => { | ||
| assert!(value.is_null(i)); | ||
|
|
@@ -1952,7 +1981,7 @@ mod tests { | |
| assert!(score_value.is_valid(i)); | ||
| assert_eq!( | ||
| expected_score_value, | ||
| get_value(i, metadata.as_ref(), score_value.as_ref()) | ||
|
Contributor
There was a problem hiding this comment. Does deref coercion somehow not kick in here?
Contributor
There was a problem hiding this comment. Ah... that only works for concrete types, not dyn trait refs. Blech. |
||
| ); | ||
| } | ||
| None => { | ||
|
|
@@ -1973,7 +2002,7 @@ mod tests { | |
| assert!(age_value.is_valid(i)); | ||
| assert_eq!( | ||
| expected_age_value, | ||
| get_value(i, metadata.as_ref(), age_value.as_ref()) | ||
| ); | ||
| } | ||
| None => { | ||
|
|
@@ -2114,7 +2143,7 @@ mod tests { | |
| // Helper to correctly create a variant object using a row's existing metadata | ||
| let object_with_foo_field = |i| { | ||
| use parquet_variant::{ParentState, ValueBuilder, VariantMetadata}; | ||
| let metadata = VariantMetadata::new(binary_array_value(metadata.as_ref(), i).unwrap()); | ||
| let mut metadata_builder = ReadOnlyMetadataBuilder::new(&metadata); | ||
| let mut value_builder = ValueBuilder::new(); | ||
| let state = ParentState::variant(&mut value_builder, &mut metadata_builder); | ||
|
|
@@ -2213,7 +2242,10 @@ mod tests { | |
| assert!(value_field.is_null(2)); | ||
| assert!(value_field.is_valid(3)); | ||
| assert_eq!( | ||
| Variant::new( | ||
| binary_array_value(result.metadata_field().as_ref(), 3).unwrap(), | ||
| binary_array_value(value_field.as_ref(), 3).unwrap() | ||
| ), | ||
|
AdamGS marked this conversation as resolved.
Outdated
|
||
| Variant::from("not an object") | ||
| ); | ||
| assert!(value_field.is_null(4)); | ||
|
|
@@ -2231,10 +2263,10 @@ mod tests { | |
| .unwrap(); | ||
| assert_list_structure_and_elements::<Int64Type, i32>( | ||
| &VariantArray::from_parts( | ||
| Arc::new(BinaryViewArray::from_iter_values(std::iter::repeat_n( | ||
| EMPTY_VARIANT_METADATA_BYTES, | ||
| scores_field.len(), | ||
| ))) as ArrayRef, | ||
| Some(scores_field.value_field().unwrap().clone()), | ||
| Some(scores_field.typed_value_field().unwrap().clone()), | ||
| None, | ||
|
|
@@ -2350,24 +2382,14 @@ mod tests { | |
| ShreddedVariantFieldArray::try_new(typed_value.column_by_name("session_id").unwrap()) | ||
| .unwrap(); | ||
|
|
||
| let id_value = id_field.value_field().unwrap(); | ||
| let id_typed_value = id_field | ||
| .typed_value_field() | ||
| .unwrap() | ||
| .as_any() | ||
| .downcast_ref::<FixedSizeBinaryArray>() | ||
| .unwrap(); | ||
| let session_id_value = session_id_field.value_field().unwrap(); | ||
| let session_id_typed_value = session_id_field | ||
| .typed_value_field() | ||
| .unwrap() | ||
|
|
@@ -2404,7 +2426,10 @@ mod tests { | |
| assert_eq!(session_id_typed_value.value(1), mock_uuid_3.as_bytes()); | ||
|
|
||
| // Verify the value field contains the name field | ||
| let row_1_variant = Variant::new( | ||
| binary_array_value(metadata.as_ref(), 1).unwrap(), | ||
| binary_array_value(value.as_ref(), 1).unwrap(), | ||
| ); | ||
| let Variant::Object(obj) = row_1_variant else { | ||
| panic!("Expected object"); | ||
| }; | ||
|
|
@@ -2436,7 +2461,10 @@ mod tests { | |
|
|
||
| assert!(session_id_value.is_valid(3)); // type mismatch, stored in value | ||
| assert!(session_id_typed_value.is_null(3)); | ||
| let session_id_variant = Variant::new( | ||
| binary_array_value(metadata.as_ref(), 3).unwrap(), | ||
| binary_array_value(session_id_value.as_ref(), 3).unwrap(), | ||
| ); | ||
| assert_eq!(session_id_variant, Variant::from("not-a-uuid")); | ||
|
|
||
| // Row 4: Type mismatch - id is int64, not UUID | ||
|
|
@@ -2447,7 +2475,10 @@ mod tests { | |
|
|
||
| assert!(id_value.is_valid(4)); // type mismatch, stored in value | ||
| assert!(id_typed_value.is_null(4)); | ||
| let id_variant = Variant::new( | ||
| binary_array_value(metadata.as_ref(), 4).unwrap(), | ||
| binary_array_value(id_value.as_ref(), 4).unwrap(), | ||
| ); | ||
| assert_eq!(id_variant, Variant::from(12345i64)); | ||
|
|
||
| assert!(session_id_value.is_null(4)); | ||
|
|
||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I'm pretty sure the cast is unnecessary (automatic coercion by compiler)
(several more below)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've cleaned up all the ones I've added, but without tooling support for this lint its mostly a style thing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: The main reason I pick on casts is because they're too powerful and can easily be quietly misused. Ideally, a cast is only present if actually required, as a clear marker to reviewer/reader that something unusual is going on.
Similar story for unnecessary type annotations, except those are just noise/bloat rather than a correctness risk.
My AI coding assistant sure loves splattering both all over the code it writes tho, super annoying.