scovich commented on code in PR #9599:
URL: https://github.com/apache/arrow-rs/pull/9599#discussion_r2975321510
##########
parquet-variant-compute/src/shred_variant.rs:
##########
@@ -285,26 +320,26 @@ impl<'a> VariantToShreddedArrayVariantRowBuilder<'a> {
// If the variant is an array, value must be null.
match variant {
Variant::List(list) => {
+ self.nulls.append_non_null();
self.value_builder.append_null();
self.typed_value_builder
.append_value(&Variant::List(list))?;
Ok(true)
}
other => {
+ self.nulls.append_non_null();
self.value_builder.append_value(other);
self.typed_value_builder.append_null()?;
Ok(false)
}
}
}
- fn finish(self) -> Result<(BinaryViewArray, ArrayRef, Option<NullBuffer>)>
{
+ fn finish(mut self) -> Result<(BinaryViewArray, ArrayRef,
Option<NullBuffer>)> {
Review Comment:
The `mut` is needed now because `self.nulls.finish()` takes `&mut self`
rather than `self` like the other two?
##########
parquet-variant-compute/src/shred_variant.rs:
##########
@@ -881,7 +914,9 @@ mod tests {
expected_variant.clone()
);
}
- None => unreachable!(),
+ None => {
+ assert!(fallbacks.0.is_null(idx));
Review Comment:
This `is_null` is checking for `Variant::Null`, correct?
Which is the spec-mandated "null" value for a variant array element?
##########
parquet-variant-compute/src/shred_variant.rs:
##########
@@ -1338,13 +1421,7 @@ mod tests {
5,
&[0, 3, 6, 6, 6, 6],
&[Some(3), Some(3), None, None, Some(0)],
- &[
- None,
- None,
- Some(Variant::from("not a list")),
- Some(Variant::Null),
- None,
- ],
+ &[None, None, Some(Variant::from("not a list")), None, None],
Review Comment:
Can you help me understand how to interpret these calls to
`assert_list_structure_and_elements`? Several months ago I felt like I
understood variant shredding nulls well, but I'm rusty now.
I _think_ the switch from `Variant::Null` fallback to true NULL is because
the top-level variant array itself is missing? But I thought we were only
missing support for "null" array elements before? Was the support for "null"
top-level elements actually incorrect before?
##########
parquet-variant-compute/src/shred_variant.rs:
##########
@@ -271,11 +303,14 @@ impl<'a> VariantToShreddedArrayVariantRowBuilder<'a> {
cast_options,
capacity,
)?,
+ nulls: NullBufferBuilder::new(capacity),
+ null_mode,
})
}
fn append_null(&mut self) -> Result<()> {
- self.value_builder.append_value(Variant::Null);
+ self.null_mode.append_to_struct_nulls(&mut self.nulls);
+ self.null_mode.append_to_value(&mut self.value_builder);
Review Comment:
This is a behavior change (fix), right? We no longer hard-wire Variant null,
but instead use the mode (derived from parent of this entry) to decide its
behavior?
--
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]
