klion26 commented on code in PR #10320:
URL: https://github.com/apache/arrow-rs/pull/10320#discussion_r3680997779
##########
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)) {
Review Comment:
This logic can be moved into macro `handle_unshredded_case` and
`ValueOnlyUnshredVariantBuilder::append_row`
##########
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:
Why do we need to add this test
##########
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:
Does this need to panic? The callers of this function, `unshred_variant` and
`shredded_get_path`(in `variant_get`) both return `Result`
##########
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:
1. Is there any reason that `logical_nulls` is better than `nulls` here?
2. The match arms `None` and `null_count()` == 0 seem can be merged into
`ArrayRef::null_count()` == 0?
##########
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 Do we need to add some tests for decimal/timestamp/list/map? currently
they'are all covered by the macro `handle_unshredded_case` but they're differnt
enum of `UnshredVariantRowBuilder`
2 Do we need to add a test that returns `null` (not Variant::Null)
--
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]
