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/29 17:38:00 UTC
[PR] [Variant] Introduce new BorrowedShreddingState concept [arrow-rs]
scovich opened a new pull request, #8499:
URL: https://github.com/apache/arrow-rs/pull/8499
# Which issue does this PR close?
- Relates to https://github.com/apache/arrow-rs/issues/8336
# Rationale for this change
While pathfinding support for unshredding variant objects and arrays, I ran into the same issue that `variant_get` already suffers -- `ShreddingState` is inconvenient because it owns the `value` and `typed_value` columns it refers to, even tho borrowing them often suffices.
# What changes are included in this PR?
Define a new `BorrowedShreddingState` which does what it says, and update `ShreddedPathStep` (in variant_get.rs) to use it.
Also, make the constructor fallible in order to correctly report casting failures if the `value` column is not a binary view.
# Are these changes tested?
Yes, existing tests cover this code.
# Are there any user-facing changes?
New `TryFrom` and `From` implementations.
`impl From<&StructArray> for ShreddingState` was replaced by a suitable `TryFrom`.
--
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] Introduce new BorrowedShreddingState concept [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #8499:
URL: https://github.com/apache/arrow-rs/pull/8499#discussion_r2388985412
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -152,7 +151,7 @@ fn shredded_get_path(
// Peel away the prefix of path elements that traverses the shredded parts of this variant
// column. Shredding will traverse the rest of the path on a per-row basis.
- let mut shredding_state = input.shredding_state().clone();
+ let mut shredding_state = input.shredding_state().borrow();
Review Comment:
🎉
##########
parquet-variant-compute/src/variant_array.rs:
##########
@@ -685,6 +670,14 @@ impl ShreddingState {
self.typed_value.as_ref()
}
+ /// Returns a borrowed version of this shredding state
Review 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] [Variant] Introduce new BorrowedShreddingState concept [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #8499:
URL: https://github.com/apache/arrow-rs/pull/8499#issuecomment-3348766514
I don't have much to add. FYI @codephage2020 @liamzwbao and @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] Introduce new BorrowedShreddingState concept [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on PR #8499:
URL: https://github.com/apache/arrow-rs/pull/8499#issuecomment-3348201073
CC @alamb
--
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] Introduce new BorrowedShreddingState concept [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on code in PR #8499:
URL: https://github.com/apache/arrow-rs/pull/8499#discussion_r2388736961
##########
parquet-variant-compute/src/variant_array.rs:
##########
@@ -694,14 +687,80 @@ impl ShreddingState {
}
}
-impl From<&StructArray> for ShreddingState {
- fn from(inner_struct: &StructArray) -> Self {
- let value = inner_struct
- .column_by_name("value")
- .and_then(|col| col.as_binary_view_opt().cloned());
- let typed_value = inner_struct.column_by_name("typed_value").cloned();
+/// Similar to [`ShreddingState`] except it holds borrowed references of the target arrays. Useful
+/// for avoiding clone operations when the caller does not need a self-standing shredding state.
+#[derive(Clone, Debug)]
+pub struct BorrowedShreddingState<'a> {
+ value: Option<&'a BinaryViewArray>,
+ typed_value: Option<&'a ArrayRef>,
+}
- ShreddingState::new(value, typed_value)
+impl<'a> BorrowedShreddingState<'a> {
+ /// 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:
+ ///
+ /// ```no_run
+ /// # use arrow::array::StructArray;
+ /// # use parquet_variant_compute::ShreddingState;
+ /// # fn get_struct_array() -> StructArray {
+ /// # unimplemented!()
+ /// # }
+ /// let struct_array: StructArray = get_struct_array();
+ /// let shredding_state = ShreddingState::try_from(&struct_array).unwrap();
Review Comment:
```suggestion
/// # use parquet_variant_compute::BorrowedShreddingState;
/// # fn get_struct_array() -> StructArray {
/// # unimplemented!()
/// # }
/// let struct_array: StructArray = get_struct_array();
/// let shredding_state = BorrowedShreddingState::try_from(&struct_array).unwrap();
```
##########
parquet-variant-compute/src/variant_array.rs:
##########
@@ -685,6 +670,14 @@ impl ShreddingState {
self.typed_value.as_ref()
}
+ /// Returns a borrowed version of this shredding state
+ pub fn borrow(&self) -> BorrowedShreddingState<'_> {
Review Comment:
I originally had an `impl From` instead, but this seems to be more readable/intuitive at call sites.
##########
parquet-variant-compute/src/variant_array.rs:
##########
@@ -272,26 +272,11 @@ impl VariantArray {
)));
};
- // Extract value and typed_value fields
- let value = if let Some(value_col) = inner.column_by_name("value") {
- if let Some(binary_view) = value_col.as_binary_view_opt() {
- Some(binary_view.clone())
- } else {
- return Err(ArrowError::NotYetImplemented(format!(
- "VariantArray 'value' field must be BinaryView, got {}",
- value_col.data_type()
- )));
- }
- } else {
- None
- };
- let typed_value = inner.column_by_name("typed_value").cloned();
Review Comment:
Now encapsulated by `ShreddingState::try_from`
--
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] Introduce new BorrowedShreddingState concept [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on PR #8499:
URL: https://github.com/apache/arrow-rs/pull/8499#issuecomment-3348354268
Should be a quickie, assuming reviewers agree with the premise.
--
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] Introduce new BorrowedShreddingState concept [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #8499:
URL: https://github.com/apache/arrow-rs/pull/8499#issuecomment-3350975609
🚀
--
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] Introduce new BorrowedShreddingState concept [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #8499:
URL: https://github.com/apache/arrow-rs/pull/8499
--
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