sdf-jkl commented on code in PR #10320:
URL: https://github.com/apache/arrow-rs/pull/10320#discussion_r3686649730
##########
parquet-variant-compute/src/variant_array.rs:
##########
@@ -338,15 +338,42 @@ impl VariantArray {
})
}
+ /// 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<ArrayRef>,
nulls: Option<NullBuffer>,
+ ) -> 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(
Review Comment:
The panic is not new logic in this function, it comes from
`StructArray::new` inside `StructArrayBuilder::build`, which validates that a
non-nullable field has no unmasked nulls. The doc comment just makes that
existing behavior visible.
I kept it as a panic rather than a `Result` because it can only fire on an
internal bug, never on bad input data.
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -2427,6 +2426,22 @@ mod test {
);
}
+ #[test]
+ fn test_variant_get_missing_path_as_variant_annotates_value_non_nullable()
{
Review Comment:
This PR changes two production call sites, not just `unshred_variant`. The
missing-path branch of `shredded_get_path` used to build its all-NULL output
with a nullable `value` field and now uses `from_parts_unshredded`. This test
pins that second call site, without it only the `unshred_variant` paths would
be covered.
##########
parquet-variant-compute/src/unshred_variant.rs:
##########
@@ -82,19 +98,43 @@ pub fn unshred_variant(array: &VariantArray) ->
Result<VariantArray> {
})?;
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() {
Review Comment:
Applied
[here](https://github.com/apache/arrow-rs/pull/10320/commits/4184a5d896b75d280aa74606efa358887bffd043)
##########
parquet-variant-compute/src/unshred_variant.rs:
##########
@@ -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() {
Review Comment:
1. Thanks, added
[here](https://github.com/apache/arrow-rs/pull/10320/commits/4184a5d896b75d280aa74606efa358887bffd043)
2. That case is covered by
`test_unshred_with_nulls_annotates_value_non_nullable`: row 1 is a top-level
NULL row and the test asserts `unshredded.is_null(1)` on the output.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
