Copilot commented on code in PR #8392:
URL: https://github.com/apache/arrow-rs/pull/8392#discussion_r2363930412
##########
parquet-variant-compute/src/variant_array.rs:
##########
@@ -129,7 +291,7 @@ impl VariantArray {
Ok(Self {
inner: inner.clone(),
metadata: metadata.clone(),
- shredding_state: ShreddingState::try_new(value, typed_value)?,
+ shredding_state: ShreddingState::new(value, typed_value),
Review Comment:
The `ShreddingState::new` method is called here but the old `try_new` method
was removed. Consider adding error handling if `ShreddingState::new` can
potentially fail, or document why this is safe by construction.
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -242,19 +242,15 @@ fn shredded_get_path(
/// quickly become annoying (and inefficient) to call `variant_get` for each
leaf value in a struct or
/// list and then try to assemble the results.
pub fn variant_get(input: &ArrayRef, options: GetOptions) -> Result<ArrayRef> {
- let variant_array: &VariantArray =
input.as_any().downcast_ref().ok_or_else(|| {
- ArrowError::InvalidArgumentError(
- "expected a VariantArray as the input for variant_get".to_owned(),
- )
- })?;
+ let variant_array = VariantArray::try_new(input)?;
Review Comment:
[nitpick] The function now creates a new `VariantArray` from the input
instead of downcasting. This could be inefficient if the input is already a
`VariantArray` wrapped as `ArrayRef`. Consider checking if the input can be
unwrapped directly before creating a new instance.
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -154,11 +154,11 @@ 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();
+ let mut shredding_state = input.shredding_state().clone();
let mut accumulated_nulls = input.inner().nulls().cloned();
let mut path_index = 0;
for path_element in path {
- match follow_shredded_path_element(shredding_state, path_element,
cast_options)? {
+ match follow_shredded_path_element(&shredding_state, path_element,
cast_options)? {
Review Comment:
[nitpick] The `shredding_state` is cloned here, which could be expensive if
it contains large arrays. Consider whether this clone is necessary or if
borrowing would suffice.
--
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-unsubscr...@arrow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
