You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "scovich (via GitHub)" <gi...@apache.org> on 2025/09/25 15:26:41 UTC
[PR] Simpler shredding state [arrow-rs]
scovich opened a new pull request, #8444:
URL: https://github.com/apache/arrow-rs/pull/8444
* NOTE: Stacked on https://github.com/apache/arrow-rs/pull/8438, ignore all but the last commit
# Which issue does this PR close?
- Related to https://github.com/apache/arrow-rs/issues/8336
# Rationale for this change
While working on https://github.com/apache/arrow-rs/pull/8438, I noticed that the enum variants of `ShreddingState` actually made the code (a lot) more complex than if I just referenced the (optional) value and typed_value columns directly. That made me wonder if `ShreddingState` would be better as a simple two-field struct.
# What changes are included in this PR?
Change `ShreddingState` to a two-field struct and update the few call sites that noticed.
# Are these changes tested?
Existing tests cover what is mostly an internal change
# Are there any user-facing changes?
`ShreddingState` is pub and changed from enum to struct.
--
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] Simpler shredding state [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #8444:
URL: https://github.com/apache/arrow-rs/pull/8444#issuecomment-3339020030
🚀
--
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] Simpler shredding state [arrow-rs]
Posted by "klion26 (via GitHub)" <gi...@apache.org>.
klion26 commented on code in PR #8444:
URL: https://github.com/apache/arrow-rs/pull/8444#discussion_r2386989796
##########
parquet-variant-compute/src/variant_array.rs:
##########
@@ -627,47 +627,38 @@ impl From<ShreddedVariantFieldArray> for StructArray {
///
/// | value | typed_value | Meaning |
/// |----------|--------------|---------|
-/// | null | null | The value is missing; only valid for shredded object fields |
-/// | non-null | null | The value is present and may be any type, including `null` |
-/// | null | non-null | The value is present and is the shredded type |
-/// | non-null | non-null | The value is present and is a partially shredded object |
+/// | NULL | NULL | The value is missing; only valid for shredded object fields |
+/// | non-NULL | NULL | The value is present and may be any type, including [`Variant::Null`] |
+/// | NULL | non-NULL | The value is present and is the shredded type |
+/// | non-NULL | non-NULL | The value is present and is a partially shredded object |
+///
+///
+/// Applying the above rules to entire columns, we obtain the following:
+///
+/// | value | typed_value | Meaning |
+/// |--------|-------------|---------|
+/// | -- | -- | **Missing**: The value is always missing; only valid for shredded object fields |
Review Comment:
Thanks for this comment!
--
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] Simpler shredding state [arrow-rs]
Posted by "mbrobbel (via GitHub)" <gi...@apache.org>.
mbrobbel commented on code in PR #8444:
URL: https://github.com/apache/arrow-rs/pull/8444#discussion_r2379945077
##########
parquet-variant-compute/src/variant_array.rs:
##########
@@ -1022,7 +987,10 @@ mod test {
// Verify the shredding state is AllNull
assert!(matches!(
variant_array.shredding_state(),
- ShreddingState::AllNull
+ ShreddingState {
+ value: None,
+ typed_value: None
+ }
Review Comment:
Maybe derive `Default`?
--
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] Simpler shredding state [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on PR #8444:
URL: https://github.com/apache/arrow-rs/pull/8444#issuecomment-3338909359
Any reason not to merge?
--
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] Simpler shredding state [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on code in PR #8444:
URL: https://github.com/apache/arrow-rs/pull/8444#discussion_r2379960289
##########
parquet-variant-compute/src/variant_array.rs:
##########
@@ -1022,7 +987,10 @@ mod test {
// Verify the shredding state is AllNull
assert!(matches!(
variant_array.shredding_state(),
- ShreddingState::AllNull
+ ShreddingState {
+ value: None,
+ typed_value: None
+ }
Review Comment:
I mean, that would produce a physically correct outcome for these specific unit tests, but I don't think the concept of a "default" shredding state actually exists? Even if it did, it would be value=Some and typed_value=None (unshredded). Which is not at all a rust `Default` (needs an arg)
--
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] Simpler shredding state [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #8444:
URL: https://github.com/apache/arrow-rs/pull/8444#discussion_r2380225438
##########
parquet-variant-compute/src/variant_array.rs:
##########
@@ -627,47 +627,38 @@ impl From<ShreddedVariantFieldArray> for StructArray {
///
/// | value | typed_value | Meaning |
/// |----------|--------------|---------|
-/// | null | null | The value is missing; only valid for shredded object fields |
-/// | non-null | null | The value is present and may be any type, including `null` |
-/// | null | non-null | The value is present and is the shredded type |
-/// | non-null | non-null | The value is present and is a partially shredded object |
+/// | NULL | NULL | The value is missing; only valid for shredded object fields |
+/// | non-NULL | NULL | The value is present and may be any type, including [`Variant::Null`] |
+/// | NULL | non-NULL | The value is present and is the shredded type |
+/// | non-NULL | non-NULL | The value is present and is a partially shredded object |
+///
+///
+/// Applying the above rules to entire columns, we obtain the following:
Review Comment:
thank you for preserving this information
--
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] Simpler shredding state [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on code in PR #8444:
URL: https://github.com/apache/arrow-rs/pull/8444#discussion_r2379960289
##########
parquet-variant-compute/src/variant_array.rs:
##########
@@ -1022,7 +987,10 @@ mod test {
// Verify the shredding state is AllNull
assert!(matches!(
variant_array.shredding_state(),
- ShreddingState::AllNull
+ ShreddingState {
+ value: None,
+ typed_value: None
+ }
Review Comment:
I mean, that would produce a physically correct result, but I don't think the concept of a "default" shredding state actually exists? Even if it did, it would be value=Some and typed_value=None (unshredded). Which is not at all a rust `Default` (needs an arg)
--
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] Simpler shredding state [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on PR #8444:
URL: https://github.com/apache/arrow-rs/pull/8444#issuecomment-3335356821
CC @alamb @klion26 who almost certainly have opinions about this change
--
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] Simpler shredding state [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on code in PR #8444:
URL: https://github.com/apache/arrow-rs/pull/8444#discussion_r2379960289
##########
parquet-variant-compute/src/variant_array.rs:
##########
@@ -1022,7 +987,10 @@ mod test {
// Verify the shredding state is AllNull
assert!(matches!(
variant_array.shredding_state(),
- ShreddingState::AllNull
+ ShreddingState {
+ value: None,
+ typed_value: None
+ }
Review Comment:
That would produce a physically correct outcome for these specific unit tests, but I don't think the concept of a "default" shredding state actually exists? Even if it did, it would be value=Some and typed_value=None (unshredded). Which is not at all a rust `Default` (needs an arg)
--
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] Simpler shredding state [arrow-rs]
Posted by "mbrobbel (via GitHub)" <gi...@apache.org>.
mbrobbel merged PR #8444:
URL: https://github.com/apache/arrow-rs/pull/8444
--
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] Simpler shredding state [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on code in PR #8444:
URL: https://github.com/apache/arrow-rs/pull/8444#discussion_r2379972173
##########
parquet-variant-compute/src/variant_array.rs:
##########
@@ -627,47 +627,38 @@ impl From<ShreddedVariantFieldArray> for StructArray {
///
/// | value | typed_value | Meaning |
/// |----------|--------------|---------|
-/// | null | null | The value is missing; only valid for shredded object fields |
-/// | non-null | null | The value is present and may be any type, including `null` |
-/// | null | non-null | The value is present and is the shredded type |
-/// | non-null | non-null | The value is present and is a partially shredded object |
+/// | NULL | NULL | The value is missing; only valid for shredded object fields |
+/// | non-NULL | NULL | The value is present and may be any type, including [`Variant::Null`] |
+/// | NULL | non-NULL | The value is present and is the shredded type |
+/// | non-NULL | non-NULL | The value is present and is a partially shredded object |
+///
+///
+/// Applying the above rules to entire columns, we obtain the following:
+///
+/// | value | typed_value | Meaning |
+/// |--------|-------------|---------|
+/// | -- | -- | **Missing**: The value is always missing; only valid for shredded object fields |
+/// | exists | -- | **Unshredded**: If present, the value may be any type, including [`Variant::Null`]
+/// | -- | exists | **Perfectly shredded**: If present, the value is always the shredded type |
+/// | exists | exists | **Imperfectly shredded**: The value might (not) be present and might (not) be the shredded type |
+///
+/// NOTE: Partial shredding is a row-wise situation that can arise under imperfect shredding (a
+/// column-wise situation): When both columns exist (imperfect shredding) and the typed_value column
+/// is a struct, then both columns can be non-NULL for the same row if value is a variant object
+/// (partial shredding).
///
/// [Parquet Variant Shredding Spec]: https://github.com/apache/parquet-format/blob/master/VariantShredding.md#value-shredding
#[derive(Clone, Debug)]
-pub enum ShreddingState {
- /// This variant has no typed_value field
- Unshredded { value: BinaryViewArray },
- /// This variant has a typed_value field and no value field
- /// meaning it is the shredded type
- Typed { typed_value: ArrayRef },
- /// Imperfectly shredded: Shredded values reside in `typed_value` while those that failed to
- /// shred reside in `value`. Missing field values are NULL in both columns, while NULL primitive
- /// values have NULL `typed_value` and `Variant::Null` in `value`.
- ///
- /// NOTE: A partially shredded struct is a special kind of imperfect shredding, where
- /// `typed_value` and `value` are both non-NULL. The `typed_value` is a struct containing the
- /// subset of fields for which shredding was attempted (each field will then have its own value
- /// and/or typed_value sub-fields that indicate how shredding actually turned out). Meanwhile,
- /// the `value` is a variant object containing the subset of fields for which shredding was
- /// not even attempted.
- PartiallyShredded {
- value: BinaryViewArray,
- typed_value: ArrayRef,
- },
- /// All values are null, only metadata is present.
- ///
- /// This state occurs when neither `value` nor `typed_value` fields exist in the schema.
- /// Note: By strict spec interpretation, this should only be valid for shredded object fields,
- /// not top-level variants. However, we allow it and treat as Variant::Null for pragmatic
- /// handling of missing data.
- AllNull,
+pub struct ShreddingState {
+ value: Option<BinaryViewArray>,
+ typed_value: Option<ArrayRef>,
}
impl ShreddingState {
- /// try to create a new `ShreddingState` from the given `value` and `typed_value` fields
+ /// Create a new `ShreddingState` from the given `value` and `typed_value` fields
///
/// Note you can create a `ShreddingState` from a &[`StructArray`] using
- /// `ShreddingState::try_from(&struct_array)`, for example:
Review Comment:
`ShreddingState` constructor became infallible some time ago, and it changed to `impl From` instead of `TryFrom` at the same time. But this example kept working because of a blanket `impl TryFrom` in rust that leverages `From` under the hood.
--
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