scovich commented on code in PR #8354:
URL: https://github.com/apache/arrow-rs/pull/8354#discussion_r3184950678
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -43,17 +46,75 @@ pub(crate) enum ShreddedPathStep<'a> {
NotShredded,
}
+/// Build the next shredding state by taking one list-like element (at
`index`) per input row.
+///
+fn take_list_like_index_as_shredding_state<L: ListLikeArray + 'static>(
+ typed_value: &dyn Array,
+ index: usize,
+) -> Result<Option<ShreddingState>> {
+ let list_array = typed_value.as_any().downcast_ref::<L>().ok_or_else(|| {
+ ArrowError::ComputeError(format!(
+ "Expected array type '{}' while handling list-like path step, got
'{}'",
+ std::any::type_name::<L>(),
+ typed_value.data_type()
+ ))
+ })?;
+
+ let values = list_array.values();
+
+ let Some(struct_array) = values.as_any().downcast_ref::<StructArray>()
else {
Review Comment:
Ah... this is `Array::as_any`, which has sensible unwrapping for both
[&T](https://docs.rs/arrow-array/58.2.0/src/arrow_array/array/mod.rs.html#517)
and [Arc<dyn
Array>](https://docs.rs/arrow-array/58.2.0/src/arrow_array/array/mod.rs.html#433).
But if that's true then why not just `Array::as_struct_opt()` and be done
with it?
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -43,17 +46,75 @@ pub(crate) enum ShreddedPathStep<'a> {
NotShredded,
}
+/// Build the next shredding state by taking one list-like element (at
`index`) per input row.
+///
+fn take_list_like_index_as_shredding_state<L: ListLikeArray + 'static>(
+ typed_value: &dyn Array,
+ index: usize,
+) -> Result<Option<ShreddingState>> {
+ let list_array = typed_value.as_any().downcast_ref::<L>().ok_or_else(|| {
+ ArrowError::ComputeError(format!(
+ "Expected array type '{}' while handling list-like path step, got
'{}'",
+ std::any::type_name::<L>(),
+ typed_value.data_type()
+ ))
+ })?;
+
+ let values = list_array.values();
+
+ let Some(struct_array) = values.as_any().downcast_ref::<StructArray>()
else {
Review Comment:
This doesn't look right. `&dyn Array` and `Arc<dyn Array>` both `impl
Array`, so you have to (at least be prepared to) use `as_struct_opt()` here if
the direct downcast fails.
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -160,7 +253,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().borrow();
+ let mut shredding_state = input.shredding_state().clone();
Review Comment:
Doesn't this PR really hit the same question as the other? How much do we
care about the cost of all these Array clones? (because this code here _also_
gets rid of borrowed shredding state, replacing it with owned shredding state).
I'd prefer if we could take a step back to understand the big picture
better, it's all very convoluted right now.
I guess the main challenge here is, indexing into a `ListArray` requires a
`take` that produces a new array instance, so we can't "just" use a borrowed
shredding state? But can't we capture the owned value and then pass a
`borrow()` of it, like other callers do? But if everyone just calls `borrow()`
on an owned one, should we take ownership instead? (head hurts)
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -43,17 +46,75 @@ pub(crate) enum ShreddedPathStep<'a> {
NotShredded,
}
+/// Build the next shredding state by taking one list-like element (at
`index`) per input row.
+///
+fn take_list_like_index_as_shredding_state<L: ListLikeArray + 'static>(
+ typed_value: &dyn Array,
+ index: usize,
+) -> Result<Option<ShreddingState>> {
+ let list_array = typed_value.as_any().downcast_ref::<L>().ok_or_else(|| {
+ ArrowError::ComputeError(format!(
+ "Expected array type '{}' while handling list-like path step, got
'{}'",
+ std::any::type_name::<L>(),
+ typed_value.data_type()
+ ))
+ })?;
+
+ let values = list_array.values();
+
+ let Some(struct_array) = values.as_any().downcast_ref::<StructArray>()
else {
Review Comment:
Actually, I'm not sure how this even compiles when
[ListArray::values](https://docs.rs/arrow/latest/arrow/array/type.ListArray.html#method.values)
returns `&Arc<dyn Array>` and `Array: !Any`? Are we literally casting
`&Arc<_>` or `Arc<_>` as `Any`?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
