You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "sdf-jkl (via GitHub)" <gi...@apache.org> on 2026/07/11 03:38:38 UTC
[PR] Unshredded value non nullable 10315 [arrow-rs]
sdf-jkl opened a new pull request, #10320:
URL: https://github.com/apache/arrow-rs/pull/10320
# Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax.
-->
- Closes #10315.
- let's wait till #10318 ships
# Rationale for this change
- Parquet spec required unshredded `VariantArray` to be non-nullable. Other readers might reject our impl because `unshred_variant` and `variant_get as_type None` return nullable unshredded Variant.
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
# What changes are included in this PR?
- builds on #10318
- Add changes to `unshred_variant` and `variant_get` to return non-nullable unshredded Variant
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
# Are these changes tested?
- yes, added new unit tests and fixed some old ones
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
2. Serve as another way to document the expected behavior of the code
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
If this PR claims a performance improvement, please include evidence such as benchmark results.
-->
# Are there any user-facing changes?
- Yes, the nullability now conforms the spec
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
If there are any breaking changes to public APIs, please call them out.
-->
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Variant] Unshredded `VariantArray` producers annotate the `value` field as nullable (spec requires `required`) [arrow-rs]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey merged PR #10320:
URL: https://github.com/apache/arrow-rs/pull/10320
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Variant] Unshredded `VariantArray` producers annotate the `value` field as nullable (spec requires `required`) [arrow-rs]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on PR #10320:
URL: https://github.com/apache/arrow-rs/pull/10320#issuecomment-5223852339
thanks @sdf-jkl & @klion26
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Variant] Unshredded `VariantArray` producers annotate the `value` field as nullable (spec requires `required`) [arrow-rs]
Posted by "klion26 (via GitHub)" <gi...@apache.org>.
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Variant] Unshredded `VariantArray` producers annotate the `value` field as nullable (spec requires `required`) [arrow-rs]
Posted by "sdf-jkl (via GitHub)" <gi...@apache.org>.
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Variant] Unshredded `VariantArray` producers annotate the `value` field as nullable (spec requires `required`) [arrow-rs]
Posted by "sdf-jkl (via GitHub)" <gi...@apache.org>.
sdf-jkl commented on code in PR #10320:
URL: https://github.com/apache/arrow-rs/pull/10320#discussion_r3686636057
##########
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:
Thanks, added [here](https://github.com/apache/arrow-rs/pull/10320/commits/4184a5d896b75d280aa74606efa358887bffd043) 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.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Variant] Unshredded `VariantArray` producers annotate the `value` field as nullable (spec requires `required`) [arrow-rs]
Posted by "sdf-jkl (via GitHub)" <gi...@apache.org>.
sdf-jkl commented on PR #10320:
URL: https://github.com/apache/arrow-rs/pull/10320#issuecomment-5064474060
@klion26 Please take a look :pray:
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Variant] Unshredded `VariantArray` producers annotate the `value` field as nullable (spec requires `required`) [arrow-rs]
Posted by "sdf-jkl (via GitHub)" <gi...@apache.org>.
sdf-jkl commented on PR #10320:
URL: https://github.com/apache/arrow-rs/pull/10320#issuecomment-5172480553
Thanks @klion26 I addressed everything. Please take another look.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org