[Variant] Unshredded VariantArray producers annotate the value field as nullable (spec requires required) - #10320
Conversation
github-actions
Bot
added
parquet
sdf-jkl
changed the title
sdf-jkl
changed the title
VariantArray producers annotate the value field as nullable (spec requires required)
…dded-value-non-nullable-10315
|
@klion26 Please take a look 🙏 |
| 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)) { |
There was a problem hiding this comment.
This logic can be moved into macro handle_unshredded_case and ValueOnlyUnshredVariantBuilder::append_row
There was a problem hiding this comment.
Thanks, added here but with a twist. These methods also cover the nested fields, and a missing object field must stay absent instead of becoming Variant::Null (otherwise {"a": 3} would unshred as {"a": 3, "b": null} under a partially shredded schema). So the substitution lives in a sink wrapper (TopLevelRowSink) that only the top level uses.
| /// | ||
| /// # Panics | ||
| /// If `value` contains nulls not masked by `nulls`. | ||
| pub(crate) fn from_parts_unshredded( |
There was a problem hiding this comment.
Does this need to panic? The callers of this function, unshred_variant and shredded_get_path(in variant_get) both return Result
There was a problem hiding this 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.
| } | ||
|
|
||
| #[test] | ||
| fn test_variant_get_missing_path_as_variant_annotates_value_non_nullable() { |
There was a problem hiding this comment.
Why do we need to add this test
There was a problem hiding this 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.
| /// 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() { |
There was a problem hiding this comment.
- Is there any reason that
logical_nullsis better thannullshere? - The match arms
Noneandnull_count()== 0 seem can be merged intoArrayRef::null_count()== 0?
| } | ||
|
|
||
| #[test] | ||
| fn test_unshred_already_unshredded_reannotates_nullable_value() { |
There was a problem hiding this 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)
There was a problem hiding this comment.
- Thanks, added here
- That case is covered by
test_unshred_with_nulls_annotates_value_non_nullable: row 1 is a top-level NULL row and the test assertsunshredded.is_null(1)on the output.
…dded-value-non-nullable-10315
2 participants
Which issue does this PR close?
Closes [Variant] Unshredded
VariantArrayproducers annotate thevaluefield as nullable (spec requiresrequired) #10315.let's wait till [Variant] make
valuemandatory field forVariantArray/ShreddingState#10318 shipsRationale for this change
VariantArrayto be non-nullable. Other readers might reject our impl becauseunshred_variantandvariant_get as_type Nonereturn nullable unshredded Variant.What changes are included in this PR?
valuemandatory field forVariantArray/ShreddingState#10318unshred_variantandvariant_getto return non-nullable unshredded VariantAre these changes tested?
Are there any user-facing changes?