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/02/19 23:04:16 UTC
[PR] [WIP] Support Shredded Lists/Array in `variant_get` [arrow-rs]
sdf-jkl opened a new pull request, #8354:
URL: https://github.com/apache/arrow-rs/pull/8354
# Which issue does this PR close?
- Pathinding PR for #9443.
# Rationale for this change
We should be able to read lists using `variant_get`
# What changes are included in this PR?
# Are these changes tested?
I'm trying to start with some basic tests to do some TDD.
# Are there any user-facing changes?
--
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] Support Shredded Lists/Array in `variant_get` [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on code in PR #8354:
URL: https://github.com/apache/arrow-rs/pull/8354#discussion_r2997245874
##########
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:
Well that solves that, then! How should we sequence that and the removal of `BorrowedShreddingState`?
--
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] [WIP] Support Shredded Lists/Array in `variant_get` [arrow-rs]
Posted by "sdf-jkl (via GitHub)" <gi...@apache.org>.
sdf-jkl commented on code in PR #8354:
URL: https://github.com/apache/arrow-rs/pull/8354#discussion_r2836308961
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -97,14 +98,67 @@ pub(crate) fn follow_shredded_path_element<'a>(
})?;
let state = BorrowedShreddingState::try_from(struct_array)?;
- Ok(ShreddedPathStep::Success(state))
+ Ok(ShreddedPathStep::Success(state.into()))
}
- VariantPathElement::Index { .. } => {
+ VariantPathElement::Index { index } => {
// TODO: Support array indexing. Among other things, it will require slicing not
// only the array we have here, but also the corresponding metadata and null masks.
- Err(ArrowError::NotYetImplemented(
- "Pathing into shredded variant array index".into(),
- ))
+ let Some(list_array) = typed_value.as_any().downcast_ref::<GenericListArray<i32>>()
+ else {
+ // Downcast failure - if strict cast options are enabled, this should be an error
+ if !cast_options.safe {
+ return Err(ArrowError::CastError(format!(
+ "Cannot access index '{}' on non-list type: {}",
+ index,
+ typed_value.data_type()
+ )));
+ }
+ // With safe cast options, return NULL (missing_path_step)
+ return Ok(missing_path_step());
+ };
+
+ let offsets = list_array.offsets();
+ let values = list_array.values(); // This is a StructArray
+
+ let Some(struct_array) = values.as_any().downcast_ref::<StructArray>() else {
+ return Ok(missing_path_step());
Review Comment:
Right, it would mean that the `typed value` we are looking at is no longer a `Variant`, but a `PrimitiveArray`
--
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] [WIP] Support Shredded Lists/Array in `variant_get` [arrow-rs]
Posted by "sdf-jkl (via GitHub)" <gi...@apache.org>.
sdf-jkl commented on PR #8354:
URL: https://github.com/apache/arrow-rs/pull/8354#issuecomment-3444122321
Hey @scovich, I'm ready for another go when you are available, thanks.
--
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] [WIP] Support Shredded Lists/Array in `variant_get` [arrow-rs]
Posted by "sdf-jkl (via GitHub)" <gi...@apache.org>.
sdf-jkl commented on code in PR #8354:
URL: https://github.com/apache/arrow-rs/pull/8354#discussion_r2353515197
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -1010,7 +1010,101 @@ mod test {
let expected: ArrayRef = Arc::new(Int32Array::from(vec![Some(1), Some(42)]));
assert_eq!(&result, &expected);
}
+ /// This test manually constructs a shredded variant array representing lists
+ /// like ["comedy", "drama"], ["horror", null] and ["comedy", "drama", "romance"]
+ /// as VariantArray using variant_get.
+ #[test]
+ fn test_shredded_list_field_access() {
+ let array = shredded_list_variant_array();
+
+ // Test: Extract the 0 index field as VariantArray first
+ let options = GetOptions::new_with_path(VariantPath::from(0));
+ let result = variant_get(&array, options).unwrap();
+ let result_variant: &VariantArray = result.as_any().downcast_ref().unwrap();
+ assert_eq!(result_variant.len(), 3);
+
+ // Row 0: expect 0 index = "comedy"
+ assert_eq!(result_variant.value(0), Variant::String("comedy"));
+ // Row 1: expect 0 index = "horror"
+ assert_eq!(result_variant.value(1), Variant::String("horror"));
+ // Row 2: expect 0 index = "comedy"
+ assert_eq!(result_variant.value(2), Variant::String("comedy"));
+ }
+ /// Test extracting shredded list field with type conversion
+ #[test]
+ fn test_shredded_list_as_string() {
+ let array = shredded_list_variant_array();
+
+ // Test: Extract the 0 index values as StringArray (type conversion)
+ let field = Field::new("typed_value", DataType::Utf8, false);
+ let options = GetOptions::new_with_path(VariantPath::from(0))
+ .with_as_type(Some(FieldRef::from(field)));
+ let result = variant_get(&array, options).unwrap();
+
+ // Should get StringArray
+ let expected: ArrayRef = Arc::new(StringArray::from(vec![Some("comedy"), Some("drama")]));
+ assert_eq!(&result, &expected);
+ }
+ /// Helper function to create a shredded variant array representing lists
+ ///
+ /// This creates an array that represents:
+ /// Row 0: ["comedy", "drama"] ([0] is shredded, [1] is shredded - perfectly shredded)
+ /// Row 1: ["horror", null] ([0] is shredded, [1] is binary null - partially shredded)
+ /// Row 2: ["comedy", "drama", "romance"] (perfectly shredded)
+ ///
+ /// The physical layout follows the shredding spec where:
+ /// - metadata: contains list metadata
+ /// - typed_value: StructArray with 0 index value
+ /// - value: contains fallback for
+ fn shredded_list_variant_array() -> ArrayRef {
+ // Create the base metadata for lists
+
+ // Could add this as an api for VariantList, like VariantList::from()
+ fn build_list_metadata(vector: Vec<Variant>) -> (Vec<u8>, Vec<u8>) {
+ let mut builder = parquet_variant::VariantBuilder::new();
+ let mut list = builder.new_list();
+ for value in vector {
+ list.append_value(value);
+ }
+ list.finish();
+ builder.finish()
+ }
+ let (metadata1, _) =
+ build_list_metadata(vec![Variant::String("comedy"), Variant::String("drama")]);
+
+ let (metadata2, _) = build_list_metadata(vec![Variant::String("horror"), Variant::Null]);
+
+ let (metadata3, _) = build_list_metadata(vec![
+ Variant::String("comedy"),
+ Variant::String("drama"),
+ Variant::String("romance"),
+ ]);
+
+ // Create metadata array
+ let metadata_array =
+ BinaryViewArray::from_iter_values(vec![metadata1, metadata2, metadata3]);
+
+ // Create the untyped value array
+ let value_array = BinaryViewArray::from(vec![Variant::Null.as_u8_slice()]);
+ // Maybe I should try with an actual primitive array
+ let typed_value_array = StringArray::from(vec![
+ Some("comedy"),
+ Some("drama"),
+ Some("horror"),
+ Some("comedy"),
+ Some("drama"),
+ Some("romance"),
+ ]);
+ // Build the main VariantArray
+ let main_struct = crate::variant_array::StructArrayBuilder::new()
+ .with_field("metadata", Arc::new(metadata_array))
+ .with_field("value", Arc::new(value_array))
+ .with_field("typed_value", Arc::new(typed_value_array))
Review Comment:
Thanks for this too, I was under the wrong impression that the metadata encoding stores the offsets for the actual values. Reading your #8359 and rereading the Variant Encoding spec I see that the values offsets are within the value encoding itself.
So the outermost `typed_value` should be an `GenericListArray` of `element` - `VariantObjects` with {`value` and `typed_value` fields}?
--
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] [WIP] Support Shredded Lists/Array in `variant_get` [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on code in PR #8354:
URL: https://github.com/apache/arrow-rs/pull/8354#discussion_r2357571633
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -1122,64 +1121,109 @@ mod test {
let result = variant_get(&array, options).unwrap();
// Should get StringArray
- let expected: ArrayRef = Arc::new(StringArray::from(vec![Some("comedy"), Some("drama")]));
+ let expected: ArrayRef =
+ Arc::new(StringArray::from(vec![Some("comedy"), None, Some("drama")]));
assert_eq!(&result, &expected);
}
/// Helper function to create a shredded variant array representing lists
///
/// This creates an array that represents:
/// Row 0: ["comedy", "drama"] ([0] is shredded, [1] is shredded - perfectly shredded)
- /// Row 1: ["horror", null] ([0] is shredded, [1] is binary null - partially shredded)
- /// Row 2: ["comedy", "drama", "romance"] (perfectly shredded)
+ /// Row 1: ["horror", 123] ([0] is shredded, [1] is int - partially shredded)
///
/// The physical layout follows the shredding spec where:
/// - metadata: contains list metadata
/// - typed_value: StructArray with 0 index value
/// - value: contains fallback for
fn shredded_list_variant_array() -> ArrayRef {
- // Create the base metadata for lists
-
- // Could add this as an api for VariantList, like VariantList::from()
- fn build_list_metadata(vector: Vec<Variant>) -> (Vec<u8>, Vec<u8>) {
- let mut builder = parquet_variant::VariantBuilder::new();
- let mut list = builder.new_list();
- for value in vector {
- list.append_value(value);
- }
- list.finish();
- builder.finish()
- }
- let (metadata1, _) =
- build_list_metadata(vec![Variant::String("comedy"), Variant::String("drama")]);
+ // Create metadata array
+ let metadata_array =
+ BinaryViewArray::from_iter_values(std::iter::repeat_n(EMPTY_VARIANT_METADATA_BYTES, 2));
- let (metadata2, _) = build_list_metadata(vec![Variant::String("horror"), Variant::Null]);
+ // Building the typed_value ListArray
- let (metadata3, _) = build_list_metadata(vec![
- Variant::String("comedy"),
- Variant::String("drama"),
- Variant::String("romance"),
+ // Need a StructBuilder to create a ListBuilder
+ let fields = Fields::from(vec![
+ Field::new("value", DataType::Binary, true),
+ Field::new("typed_value", DataType::Utf8, true),
]);
+ let field_builders = vec![
+ make_builder(&DataType::Binary, 4),
+ make_builder(&DataType::Utf8, 4),
+ ];
+ let struct_builder = StructBuilder::new(fields, field_builders);
+
+ let mut builder = GenericListBuilder::<i32, StructBuilder>::new(struct_builder);
+
+ // Row 0 index 0
+ builder
+ .values()
+ .field_builder::<BinaryBuilder>(0)
+ .unwrap()
+ .append_null();
+ builder
+ .values()
+ .field_builder::<StringBuilder>(1)
+ .unwrap()
+ .append_value("comedy");
+ builder.values().append(true);
+
+ // Row 0 index 1
+ builder
+ .values()
+ .field_builder::<BinaryBuilder>(0)
+ .unwrap()
+ .append_null();
+ builder
+ .values()
+ .field_builder::<StringBuilder>(1)
+ .unwrap()
+ .append_value("drama");
+ builder.values().append(true);
+
+ // Next row
+ builder.append(true);
+
+ // Row 1 index 0
+ builder
+ .values()
+ .field_builder::<BinaryBuilder>(0)
+ .unwrap()
+ .append_null();
+ builder
+ .values()
+ .field_builder::<StringBuilder>(1)
+ .unwrap()
+ .append_value("horror");
+ builder.values().append(true);
+
+ // Row 1 index 1
+ let mut variant_builder = VariantBuilder::new();
+ variant_builder.append_value(123i32); // <------ couldn't find the right way to do it, used this as placeholder for binary
+ let (_, value) = variant_builder.finish();
+
+ builder
+ .values()
+ .field_builder::<BinaryBuilder>(0)
+ .unwrap()
+ .append_value(value);
+ builder
+ .values()
+ .field_builder::<StringBuilder>(1)
+ .unwrap()
+ .append_null();
+ builder.values().append(true);
+
+ // Next row
+ builder.append(true);
+
+ let typed_value_array = builder.finish();
- // Create metadata array
- let metadata_array =
- BinaryViewArray::from_iter_values(vec![metadata1, metadata2, metadata3]);
-
- // Create the untyped value array
- let value_array = BinaryViewArray::from(vec![Variant::Null.as_u8_slice()]);
- // Maybe I should try with an actual primitive array
- let typed_value_array = StringArray::from(vec![
- Some("comedy"),
- Some("drama"),
- Some("horror"),
- Some("comedy"),
- Some("drama"),
- Some("romance"),
- ]);
// Build the main VariantArray
let main_struct = crate::variant_array::StructArrayBuilder::new()
- .with_field("metadata", Arc::new(metadata_array))
- .with_field("value", Arc::new(value_array))
- .with_field("typed_value", Arc::new(typed_value_array))
+ .with_field("metadata", Arc::new(metadata_array), false)
+ // .with_field("value", Arc::new(value_array), true)
Review Comment:
There are two different `value` fields:
```
array_col: {
value: BINARY, -- for non-array variant objects
typed_value: {
elements: {
value: BINARY, -- for wrong-type array elements
typed_value: <ELEMENT TYPE>,
}
},
}
```
AFAIK, both those columns can be missing if every row contains a list with all-correct element types.
Note that variant arrays cannot contain (SQL) NULL tho -- so the list `[Some(1), None, Some(3)]` would produce a NULL `elements.typed_value` with `Variant::Null` in `elements.value`.
--
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] [WIP] Support Shredded Lists/Array in `variant_get` [arrow-rs]
Posted by "sdf-jkl (via GitHub)" <gi...@apache.org>.
sdf-jkl commented on code in PR #8354:
URL: https://github.com/apache/arrow-rs/pull/8354#discussion_r2361088726
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -1122,64 +1121,109 @@ mod test {
let result = variant_get(&array, options).unwrap();
// Should get StringArray
- let expected: ArrayRef = Arc::new(StringArray::from(vec![Some("comedy"), Some("drama")]));
+ let expected: ArrayRef =
+ Arc::new(StringArray::from(vec![Some("comedy"), None, Some("drama")]));
assert_eq!(&result, &expected);
}
/// Helper function to create a shredded variant array representing lists
///
/// This creates an array that represents:
/// Row 0: ["comedy", "drama"] ([0] is shredded, [1] is shredded - perfectly shredded)
- /// Row 1: ["horror", null] ([0] is shredded, [1] is binary null - partially shredded)
- /// Row 2: ["comedy", "drama", "romance"] (perfectly shredded)
+ /// Row 1: ["horror", 123] ([0] is shredded, [1] is int - partially shredded)
///
/// The physical layout follows the shredding spec where:
/// - metadata: contains list metadata
/// - typed_value: StructArray with 0 index value
/// - value: contains fallback for
fn shredded_list_variant_array() -> ArrayRef {
- // Create the base metadata for lists
-
- // Could add this as an api for VariantList, like VariantList::from()
- fn build_list_metadata(vector: Vec<Variant>) -> (Vec<u8>, Vec<u8>) {
- let mut builder = parquet_variant::VariantBuilder::new();
- let mut list = builder.new_list();
- for value in vector {
- list.append_value(value);
- }
- list.finish();
- builder.finish()
- }
- let (metadata1, _) =
- build_list_metadata(vec![Variant::String("comedy"), Variant::String("drama")]);
+ // Create metadata array
+ let metadata_array =
+ BinaryViewArray::from_iter_values(std::iter::repeat_n(EMPTY_VARIANT_METADATA_BYTES, 2));
- let (metadata2, _) = build_list_metadata(vec![Variant::String("horror"), Variant::Null]);
+ // Building the typed_value ListArray
- let (metadata3, _) = build_list_metadata(vec![
- Variant::String("comedy"),
- Variant::String("drama"),
- Variant::String("romance"),
+ // Need a StructBuilder to create a ListBuilder
+ let fields = Fields::from(vec![
+ Field::new("value", DataType::Binary, true),
+ Field::new("typed_value", DataType::Utf8, true),
]);
+ let field_builders = vec![
+ make_builder(&DataType::Binary, 4),
+ make_builder(&DataType::Utf8, 4),
+ ];
+ let struct_builder = StructBuilder::new(fields, field_builders);
+
+ let mut builder = GenericListBuilder::<i32, StructBuilder>::new(struct_builder);
+
+ // Row 0 index 0
+ builder
+ .values()
+ .field_builder::<BinaryBuilder>(0)
+ .unwrap()
+ .append_null();
+ builder
+ .values()
+ .field_builder::<StringBuilder>(1)
+ .unwrap()
+ .append_value("comedy");
+ builder.values().append(true);
+
+ // Row 0 index 1
+ builder
+ .values()
+ .field_builder::<BinaryBuilder>(0)
+ .unwrap()
+ .append_null();
+ builder
+ .values()
+ .field_builder::<StringBuilder>(1)
+ .unwrap()
+ .append_value("drama");
+ builder.values().append(true);
+
+ // Next row
+ builder.append(true);
+
+ // Row 1 index 0
+ builder
+ .values()
+ .field_builder::<BinaryBuilder>(0)
+ .unwrap()
+ .append_null();
+ builder
+ .values()
+ .field_builder::<StringBuilder>(1)
+ .unwrap()
+ .append_value("horror");
+ builder.values().append(true);
+
+ // Row 1 index 1
+ let mut variant_builder = VariantBuilder::new();
+ variant_builder.append_value(123i32); // <------ couldn't find the right way to do it, used this as placeholder for binary
+ let (_, value) = variant_builder.finish();
+
+ builder
+ .values()
+ .field_builder::<BinaryBuilder>(0)
+ .unwrap()
+ .append_value(value);
Review Comment:
Maybe it's possible to do something similar to `StructArrayBuilder`, but for `GenericListArray`?
Like
```rust
GenericListArrayBuilder::from(offsets, array: StructArray) //<- The input is always a struct array of
// {optional `value`, optional `typed_value`}
```
Use offsets to separate `StructArray` into separate `List`s
--
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] Support Shredded Lists/Array in `variant_get` [arrow-rs]
Posted by "sdf-jkl (via GitHub)" <gi...@apache.org>.
sdf-jkl commented on code in PR #8354:
URL: https://github.com/apache/arrow-rs/pull/8354#discussion_r3191059047
##########
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:
Fixed here https://github.com/apache/arrow-rs/pull/8354/commits/69167ea3af741da907e11078c8c8712046825de9. There are more places we could simplify this, but it's out of scope here.
--
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] Support Shredded Lists/Array in `variant_get` [arrow-rs]
Posted by "liamzwbao (via GitHub)" <gi...@apache.org>.
liamzwbao commented on code in PR #8354:
URL: https://github.com/apache/arrow-rs/pull/8354#discussion_r2870061437
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -96,15 +184,44 @@ pub(crate) fn follow_shredded_path_element<'a>(
))
})?;
- let state = BorrowedShreddingState::try_from(struct_array)?;
+ let state = ShreddingState::try_from(struct_array)?;
Ok(ShreddedPathStep::Success(state))
}
- VariantPathElement::Index { .. } => {
- // TODO: Support array indexing. Among other things, it will require slicing not
- // only the array we have here, but also the corresponding metadata and null masks.
- Err(ArrowError::NotYetImplemented(
- "Pathing into shredded variant array index".into(),
- ))
+ VariantPathElement::Index { index } => {
+ let state = if let Some(list_array) =
+ typed_value.as_any().downcast_ref::<GenericListArray<i32>>()
+ {
+ take_list_like_index_as_shredding_state(list_array, *index, cast_options)?
+ } else if let Some(list_array) =
+ typed_value.as_any().downcast_ref::<GenericListArray<i64>>()
+ {
+ take_list_like_index_as_shredding_state(list_array, *index, cast_options)?
+ } else if let Some(list_view_array) = typed_value
+ .as_any()
+ .downcast_ref::<GenericListViewArray<i32>>()
+ {
+ take_list_like_index_as_shredding_state(list_view_array, *index, cast_options)?
+ } else if let Some(list_view_array) = typed_value
+ .as_any()
+ .downcast_ref::<GenericListViewArray<i64>>()
+ {
+ take_list_like_index_as_shredding_state(list_view_array, *index, cast_options)?
+ } else if cast_options.safe {
Review Comment:
We could try modifying the `take_list_like_index_as_shredding_state`, then this part would be something like
```rs
let state = match typed_value.data_type() {
DataType::List(_) => take_list_like_index_as_shredding_state::<
GenericListArray<i32>,
>(typed_value, *index, cast_options)?,
DataType::LargeList(_) => take_list_like_index_as_shredding_state::<
GenericListArray<i64>,
>(typed_value, *index, cast_options)?,
DataType::ListView(_) => take_list_like_index_as_shredding_state::<
GenericListViewArray<i32>,
>(typed_value, *index, cast_options)?,
DataType::LargeListView(_) => take_list_like_index_as_shredding_state::<
GenericListViewArray<i64>,
>(typed_value, *index, cast_options)?,
```
--
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] [WIP] Support Shredded Lists/Array in `variant_get` [arrow-rs]
Posted by "sdf-jkl (via GitHub)" <gi...@apache.org>.
sdf-jkl commented on code in PR #8354:
URL: https://github.com/apache/arrow-rs/pull/8354#discussion_r2360242362
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -1122,64 +1121,109 @@ mod test {
let result = variant_get(&array, options).unwrap();
// Should get StringArray
- let expected: ArrayRef = Arc::new(StringArray::from(vec![Some("comedy"), Some("drama")]));
+ let expected: ArrayRef =
+ Arc::new(StringArray::from(vec![Some("comedy"), None, Some("drama")]));
assert_eq!(&result, &expected);
}
/// Helper function to create a shredded variant array representing lists
///
/// This creates an array that represents:
/// Row 0: ["comedy", "drama"] ([0] is shredded, [1] is shredded - perfectly shredded)
- /// Row 1: ["horror", null] ([0] is shredded, [1] is binary null - partially shredded)
- /// Row 2: ["comedy", "drama", "romance"] (perfectly shredded)
+ /// Row 1: ["horror", 123] ([0] is shredded, [1] is int - partially shredded)
///
/// The physical layout follows the shredding spec where:
/// - metadata: contains list metadata
/// - typed_value: StructArray with 0 index value
/// - value: contains fallback for
fn shredded_list_variant_array() -> ArrayRef {
- // Create the base metadata for lists
-
- // Could add this as an api for VariantList, like VariantList::from()
- fn build_list_metadata(vector: Vec<Variant>) -> (Vec<u8>, Vec<u8>) {
- let mut builder = parquet_variant::VariantBuilder::new();
- let mut list = builder.new_list();
- for value in vector {
- list.append_value(value);
- }
- list.finish();
- builder.finish()
- }
- let (metadata1, _) =
- build_list_metadata(vec![Variant::String("comedy"), Variant::String("drama")]);
+ // Create metadata array
+ let metadata_array =
+ BinaryViewArray::from_iter_values(std::iter::repeat_n(EMPTY_VARIANT_METADATA_BYTES, 2));
- let (metadata2, _) = build_list_metadata(vec![Variant::String("horror"), Variant::Null]);
+ // Building the typed_value ListArray
- let (metadata3, _) = build_list_metadata(vec![
- Variant::String("comedy"),
- Variant::String("drama"),
- Variant::String("romance"),
+ // Need a StructBuilder to create a ListBuilder
+ let fields = Fields::from(vec![
+ Field::new("value", DataType::Binary, true),
+ Field::new("typed_value", DataType::Utf8, true),
]);
+ let field_builders = vec![
+ make_builder(&DataType::Binary, 4),
+ make_builder(&DataType::Utf8, 4),
+ ];
+ let struct_builder = StructBuilder::new(fields, field_builders);
+
+ let mut builder = GenericListBuilder::<i32, StructBuilder>::new(struct_builder);
+
+ // Row 0 index 0
+ builder
+ .values()
+ .field_builder::<BinaryBuilder>(0)
+ .unwrap()
+ .append_null();
+ builder
+ .values()
+ .field_builder::<StringBuilder>(1)
+ .unwrap()
+ .append_value("comedy");
+ builder.values().append(true);
+
+ // Row 0 index 1
+ builder
+ .values()
+ .field_builder::<BinaryBuilder>(0)
+ .unwrap()
+ .append_null();
+ builder
+ .values()
+ .field_builder::<StringBuilder>(1)
+ .unwrap()
+ .append_value("drama");
+ builder.values().append(true);
+
+ // Next row
+ builder.append(true);
+
+ // Row 1 index 0
+ builder
+ .values()
+ .field_builder::<BinaryBuilder>(0)
+ .unwrap()
+ .append_null();
+ builder
+ .values()
+ .field_builder::<StringBuilder>(1)
+ .unwrap()
+ .append_value("horror");
+ builder.values().append(true);
+
+ // Row 1 index 1
+ let mut variant_builder = VariantBuilder::new();
+ variant_builder.append_value(123i32); // <------ couldn't find the right way to do it, used this as placeholder for binary
+ let (_, value) = variant_builder.finish();
+
+ builder
+ .values()
+ .field_builder::<BinaryBuilder>(0)
+ .unwrap()
+ .append_value(value);
Review Comment:
We could build the list of `element` - `StructArray` ["horror", 123] using the `StructArrayBuilder` :
```rust
// Building a single row struct array separately
let mut variant_value_builder = VariantValueArrayBuilder::new(4);
variant_value_builder.append_null();
variant_value_builder.append_value(Variant::from(123i32));
let mut struct_array_builder = StructArrayBuilder::new();
struct_array_builder.with_field("value", Arc::new(variant_value_builder.build().unwrap()), true);
struct_array_builder.with_field("typed_value", Arc::new(StringArray::from(vec![Some("horror"), None])), true);
let struct_array = struct_array_builder.build();
// How to build the GenericListArray?
```
But currently we have no way of constructing `GenericListArray` from existing `StructArray`s.
I wonder if we can implement `ArrayBuilder` trait for `VariantValueArrayBuilder` and build `GenericListArray` using the builders we allready have:
```rust
let field_builders = vec![
Box::new(VariantValueArrayBuilder::new()),
Box::new(StringBuilder::new()),
];
let struct_builder = StructBuilder::new(fields, field_builders);
let mut builder = GenericListBuilder::<i32, StructBuilder>::new(struct_builder);
```
--
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] Support Shredded Lists/Array in `variant_get` [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #8354:
URL: https://github.com/apache/arrow-rs/pull/8354#issuecomment-4533829502
đ -- thank you @sdf-jkl and @scovich
--
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] [WIP] Support Shredded Lists/Array in `variant_get` [arrow-rs]
Posted by "sdf-jkl (via GitHub)" <gi...@apache.org>.
sdf-jkl commented on code in PR #8354:
URL: https://github.com/apache/arrow-rs/pull/8354#discussion_r2836331532
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -97,14 +98,67 @@ pub(crate) fn follow_shredded_path_element<'a>(
})?;
let state = BorrowedShreddingState::try_from(struct_array)?;
- Ok(ShreddedPathStep::Success(state))
+ Ok(ShreddedPathStep::Success(state.into()))
}
- VariantPathElement::Index { .. } => {
+ VariantPathElement::Index { index } => {
// TODO: Support array indexing. Among other things, it will require slicing not
// only the array we have here, but also the corresponding metadata and null masks.
- Err(ArrowError::NotYetImplemented(
- "Pathing into shredded variant array index".into(),
- ))
+ let Some(list_array) = typed_value.as_any().downcast_ref::<GenericListArray<i32>>()
Review Comment:
Actually, it seems like not an issue right now because VariantArray does not support LargeList:
https://github.com/apache/arrow-rs/blob/25ad2d488fe19175b2cd15e86c3bc1153cb5a38b/parquet-variant-compute/src/variant_array.rs#L1184-L1187
--
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] Support Shredded Lists/Array in `variant_get` [arrow-rs]
Posted by "sdf-jkl (via GitHub)" <gi...@apache.org>.
sdf-jkl commented on code in PR #8354:
URL: https://github.com/apache/arrow-rs/pull/8354#discussion_r2997190461
##########
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:
@scovich the changes in this PR (#9610) will make value field an `ArrayRef`. This way there'd be no need for `Cow` or `BorrowedShreddingState`.
https://github.com/apache/arrow-rs/pull/9610/changes#diff-5f5ebb25cba94551493d3a9caa2ab4d94d19b1ce4ce3e3eac7662109a8966794R420
--
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] Support Shredded Lists/Array in `variant_get` [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on code in PR #8354:
URL: https://github.com/apache/arrow-rs/pull/8354#discussion_r2995841367
##########
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:
I'd defer to @alamb on how important this is -- I don't have a good sense of how expensive these shallow-deep clones are in practice, but I do know he was chasing a whole workstream of PR to avoid unnecessary ArrayData etc, which seems related.
--
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] Support Shredded Lists/Array in `variant_get` [arrow-rs]
Posted by "klion26 (via GitHub)" <gi...@apache.org>.
klion26 commented on code in PR #8354:
URL: https://github.com/apache/arrow-rs/pull/8354#discussion_r2853718840
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -1848,6 +1943,246 @@ mod test {
assert_eq!(&result, &expected);
}
+ /// This test uses a pre-shredded list array and validates index-path access.
+ #[test]
+ fn test_shredded_list_index_access() {
+ let array = shredded_list_variant_array();
+ // Test: Extract the 0 index field as VariantArray first
+ let options = GetOptions::new_with_path(VariantPath::from(0));
+ let result = variant_get(&array, options).unwrap();
+ let result_variant = VariantArray::try_new(&result).unwrap();
+ assert_eq!(result_variant.len(), 2);
+
+ // Row 0: expect 0 index = "comedy"
+ assert_eq!(result_variant.value(0), Variant::from("comedy"));
+ // Row 1: expect 0 index = "horror"
+ assert_eq!(result_variant.value(1), Variant::from("horror"));
+ }
+
+ /// Test extracting shredded list field with type conversion.
+ #[test]
+ fn test_shredded_list_as_string() {
+ let array = shredded_list_variant_array();
+ // Test: Extract the 0 index values as StringArray (type conversion)
+ let field = Field::new("typed_value", DataType::Utf8, false);
+ let options = GetOptions::new_with_path(VariantPath::from(0))
+ .with_as_type(Some(FieldRef::from(field)));
+ let result = variant_get(&array, options).unwrap();
+ // Should get StringArray
+ let expected: ArrayRef = Arc::new(StringArray::from(vec![Some("comedy"), Some("horror")]));
+ assert_eq!(&result, &expected);
+ }
+
+ #[test]
+ fn test_shredded_list_index_access_from_value_field() {
+ let array = shredded_list_variant_array();
+ // Index 1 maps to "drama" for row 0, and to fallback value 123 for row 1.
+ let options = GetOptions::new_with_path(VariantPath::from(1));
+ let result = variant_get(&array, options).unwrap();
+ let result_variant = VariantArray::try_new(&result).unwrap();
+
+ assert_eq!(result_variant.value(0), Variant::from("drama"));
+ assert_eq!(result_variant.value(1).as_int64(), Some(123));
+ }
+
+ #[test]
+ fn test_shredded_list_index_access_from_value_field_as_int64() {
+ let array = shredded_list_variant_array();
+ let field = Field::new("typed_value", DataType::Int64, true);
+ let options = GetOptions::new_with_path(VariantPath::from(1))
+ .with_as_type(Some(FieldRef::from(field)));
+ let result = variant_get(&array, options).unwrap();
+
+ // "drama" -> NULL, 123 -> 123.
+ let expected: ArrayRef = Arc::new(Int64Array::from(vec![None, Some(123)]));
+ assert_eq!(&result, &expected);
+ }
+
+ // The tests below are temp before: https://github.com/apache/arrow-rs/issues/9455
+ #[test]
+ fn test_shredded_list_index_out_of_bounds_unsafe_cast_errors() {
+ let options =
+ GetOptions::new_with_path(VariantPath::from(10)).with_cast_options(CastOptions {
+ safe: false,
+ ..Default::default()
+ });
+
+ let err = variant_get(&shredded_list_variant_array(), options.clone()).unwrap_err();
+ assert!(err.to_string().contains("Cannot access index '10'"));
+ }
+
+ #[test]
+ fn test_large_list_index_path_helper() {
+ let large_list = large_list_of_shredded_elements();
+ let state = take_list_index_as_shredding_state(&large_list, 1, &CastOptions::default())
+ .unwrap()
+ .unwrap();
+
+ let typed = state
+ .typed_value_field()
+ .unwrap()
+ .as_any()
+ .downcast_ref::<StringArray>()
+ .unwrap();
+ let value = state.value_field().unwrap();
Review Comment:
Do we need to assert the length of `typed/value` here?
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -42,16 +45,84 @@ pub(crate) enum ShreddedPathStep<'a> {
NotShredded,
}
+fn take_list_index_as_shredding_state<O: OffsetSizeTrait>(
+ list_array: &GenericListArray<O>,
+ index: usize,
+ cast_options: &CastOptions,
+) -> Result<Option<ShreddingState>> {
+ let offsets = list_array.offsets();
+ let values = list_array.values();
+
+ let Some(struct_array) = values.as_any().downcast_ref::<StructArray>() else {
+ return Ok(None);
+ };
+
+ let value_array = struct_array.column_by_name("value");
+ let typed_array = struct_array.column_by_name("typed_value");
+
+ // If list elements have neither typed nor fallback value, this path step is missing.
+ if value_array.is_none() && typed_array.is_none() {
+ return Ok(None);
+ }
+
+ let mut take_indices = Vec::with_capacity(list_array.len());
+ for row in 0..list_array.len() {
+ let start = offsets[row].as_usize();
+ let end = offsets[row + 1].as_usize();
+ let len = end - start;
+
+ if index < len {
+ let absolute_index = start.checked_add(index).ok_or_else(|| {
+ ArrowError::ComputeError("List index overflow while building take indices".into())
+ })?;
+ let absolute_index = u64::try_from(absolute_index)
+ .map_err(|_| ArrowError::ComputeError("List index does not fit into u64".into()))?;
+ take_indices.push(Some(absolute_index));
+ } else if cast_options.safe {
+ take_indices.push(None);
+ } else {
+ return Err(ArrowError::CastError(format!(
+ "Cannot access index '{}' for row {} with list length {}",
+ index, row, len
+ )));
Review Comment:
Do we need to add the behavior to the document or somewhere else?
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -1848,6 +1943,246 @@ mod test {
assert_eq!(&result, &expected);
}
+ /// This test uses a pre-shredded list array and validates index-path access.
+ #[test]
+ fn test_shredded_list_index_access() {
+ let array = shredded_list_variant_array();
+ // Test: Extract the 0 index field as VariantArray first
+ let options = GetOptions::new_with_path(VariantPath::from(0));
+ let result = variant_get(&array, options).unwrap();
+ let result_variant = VariantArray::try_new(&result).unwrap();
+ assert_eq!(result_variant.len(), 2);
+
+ // Row 0: expect 0 index = "comedy"
+ assert_eq!(result_variant.value(0), Variant::from("comedy"));
+ // Row 1: expect 0 index = "horror"
+ assert_eq!(result_variant.value(1), Variant::from("horror"));
+ }
+
+ /// Test extracting shredded list field with type conversion.
+ #[test]
+ fn test_shredded_list_as_string() {
+ let array = shredded_list_variant_array();
+ // Test: Extract the 0 index values as StringArray (type conversion)
+ let field = Field::new("typed_value", DataType::Utf8, false);
+ let options = GetOptions::new_with_path(VariantPath::from(0))
+ .with_as_type(Some(FieldRef::from(field)));
+ let result = variant_get(&array, options).unwrap();
+ // Should get StringArray
+ let expected: ArrayRef = Arc::new(StringArray::from(vec![Some("comedy"), Some("horror")]));
+ assert_eq!(&result, &expected);
+ }
+
+ #[test]
+ fn test_shredded_list_index_access_from_value_field() {
+ let array = shredded_list_variant_array();
+ // Index 1 maps to "drama" for row 0, and to fallback value 123 for row 1.
+ let options = GetOptions::new_with_path(VariantPath::from(1));
+ let result = variant_get(&array, options).unwrap();
+ let result_variant = VariantArray::try_new(&result).unwrap();
+
+ assert_eq!(result_variant.value(0), Variant::from("drama"));
+ assert_eq!(result_variant.value(1).as_int64(), Some(123));
+ }
+
+ #[test]
+ fn test_shredded_list_index_access_from_value_field_as_int64() {
+ let array = shredded_list_variant_array();
+ let field = Field::new("typed_value", DataType::Int64, true);
+ let options = GetOptions::new_with_path(VariantPath::from(1))
+ .with_as_type(Some(FieldRef::from(field)));
+ let result = variant_get(&array, options).unwrap();
+
+ // "drama" -> NULL, 123 -> 123.
+ let expected: ArrayRef = Arc::new(Int64Array::from(vec![None, Some(123)]));
+ assert_eq!(&result, &expected);
+ }
+
+ // The tests below are temp before: https://github.com/apache/arrow-rs/issues/9455
+ #[test]
+ fn test_shredded_list_index_out_of_bounds_unsafe_cast_errors() {
+ let options =
+ GetOptions::new_with_path(VariantPath::from(10)).with_cast_options(CastOptions {
+ safe: false,
+ ..Default::default()
+ });
+
+ let err = variant_get(&shredded_list_variant_array(), options.clone()).unwrap_err();
+ assert!(err.to_string().contains("Cannot access index '10'"));
+ }
+
+ #[test]
+ fn test_large_list_index_path_helper() {
+ let large_list = large_list_of_shredded_elements();
+ let state = take_list_index_as_shredding_state(&large_list, 1, &CastOptions::default())
+ .unwrap()
+ .unwrap();
+
+ let typed = state
+ .typed_value_field()
+ .unwrap()
+ .as_any()
+ .downcast_ref::<StringArray>()
+ .unwrap();
+ let value = state.value_field().unwrap();
+ assert_eq!(typed.value(0), "drama");
+ assert!(typed.is_null(1));
+ assert!(value.is_null(0));
+ assert!(value.is_valid(1));
+ }
+
+ #[test]
+ fn test_large_list_index_out_of_bounds_unsafe_cast_errors() {
+ let large_list = large_list_of_shredded_elements();
+ let err = take_list_index_as_shredding_state(
+ &large_list,
+ 10,
+ &CastOptions {
+ safe: false,
+ ..Default::default()
+ },
+ )
+ .unwrap_err();
+ assert!(err.to_string().contains("Cannot access index '10'"));
+ }
+
+ #[test]
+ fn test_shredded_list_in_struct_index_access() {
+ let array = shredded_struct_with_list_variant_array();
+ let options = GetOptions::new_with_path(VariantPath::try_from("a").unwrap().join(1));
Review Comment:
nit, now, this can also be `VariantPath::try_from("a[1]").unwrap()`
--
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] Support Shredded Lists/Array in `variant_get` [arrow-rs]
Posted by "sdf-jkl (via GitHub)" <gi...@apache.org>.
sdf-jkl commented on code in PR #8354:
URL: https://github.com/apache/arrow-rs/pull/8354#discussion_r2853987091
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -1848,6 +1943,246 @@ mod test {
assert_eq!(&result, &expected);
}
+ /// This test uses a pre-shredded list array and validates index-path access.
+ #[test]
+ fn test_shredded_list_index_access() {
+ let array = shredded_list_variant_array();
+ // Test: Extract the 0 index field as VariantArray first
+ let options = GetOptions::new_with_path(VariantPath::from(0));
+ let result = variant_get(&array, options).unwrap();
+ let result_variant = VariantArray::try_new(&result).unwrap();
+ assert_eq!(result_variant.len(), 2);
+
+ // Row 0: expect 0 index = "comedy"
+ assert_eq!(result_variant.value(0), Variant::from("comedy"));
+ // Row 1: expect 0 index = "horror"
+ assert_eq!(result_variant.value(1), Variant::from("horror"));
+ }
+
+ /// Test extracting shredded list field with type conversion.
+ #[test]
+ fn test_shredded_list_as_string() {
+ let array = shredded_list_variant_array();
+ // Test: Extract the 0 index values as StringArray (type conversion)
+ let field = Field::new("typed_value", DataType::Utf8, false);
+ let options = GetOptions::new_with_path(VariantPath::from(0))
+ .with_as_type(Some(FieldRef::from(field)));
+ let result = variant_get(&array, options).unwrap();
+ // Should get StringArray
+ let expected: ArrayRef = Arc::new(StringArray::from(vec![Some("comedy"), Some("horror")]));
+ assert_eq!(&result, &expected);
+ }
+
+ #[test]
+ fn test_shredded_list_index_access_from_value_field() {
+ let array = shredded_list_variant_array();
+ // Index 1 maps to "drama" for row 0, and to fallback value 123 for row 1.
+ let options = GetOptions::new_with_path(VariantPath::from(1));
+ let result = variant_get(&array, options).unwrap();
+ let result_variant = VariantArray::try_new(&result).unwrap();
+
+ assert_eq!(result_variant.value(0), Variant::from("drama"));
+ assert_eq!(result_variant.value(1).as_int64(), Some(123));
+ }
+
+ #[test]
+ fn test_shredded_list_index_access_from_value_field_as_int64() {
+ let array = shredded_list_variant_array();
+ let field = Field::new("typed_value", DataType::Int64, true);
+ let options = GetOptions::new_with_path(VariantPath::from(1))
+ .with_as_type(Some(FieldRef::from(field)));
+ let result = variant_get(&array, options).unwrap();
+
+ // "drama" -> NULL, 123 -> 123.
+ let expected: ArrayRef = Arc::new(Int64Array::from(vec![None, Some(123)]));
+ assert_eq!(&result, &expected);
+ }
+
+ // The tests below are temp before: https://github.com/apache/arrow-rs/issues/9455
+ #[test]
+ fn test_shredded_list_index_out_of_bounds_unsafe_cast_errors() {
+ let options =
+ GetOptions::new_with_path(VariantPath::from(10)).with_cast_options(CastOptions {
+ safe: false,
+ ..Default::default()
+ });
+
+ let err = variant_get(&shredded_list_variant_array(), options.clone()).unwrap_err();
+ assert!(err.to_string().contains("Cannot access index '10'"));
+ }
+
+ #[test]
+ fn test_large_list_index_path_helper() {
+ let large_list = large_list_of_shredded_elements();
+ let state = take_list_index_as_shredding_state(&large_list, 1, &CastOptions::default())
+ .unwrap()
+ .unwrap();
+
+ let typed = state
+ .typed_value_field()
+ .unwrap()
+ .as_any()
+ .downcast_ref::<StringArray>()
+ .unwrap();
+ let value = state.value_field().unwrap();
Review Comment:
I don't think I'll keep the test as is once we can use other `List`-like types in `variant_get`
--
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] [WIP] Support Shredded Lists/Array in `variant_get` [arrow-rs]
Posted by "sdf-jkl (via GitHub)" <gi...@apache.org>.
sdf-jkl commented on code in PR #8354:
URL: https://github.com/apache/arrow-rs/pull/8354#discussion_r2356743648
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -1122,64 +1121,109 @@ mod test {
let result = variant_get(&array, options).unwrap();
// Should get StringArray
- let expected: ArrayRef = Arc::new(StringArray::from(vec![Some("comedy"), Some("drama")]));
+ let expected: ArrayRef =
+ Arc::new(StringArray::from(vec![Some("comedy"), None, Some("drama")]));
assert_eq!(&result, &expected);
}
/// Helper function to create a shredded variant array representing lists
///
/// This creates an array that represents:
/// Row 0: ["comedy", "drama"] ([0] is shredded, [1] is shredded - perfectly shredded)
- /// Row 1: ["horror", null] ([0] is shredded, [1] is binary null - partially shredded)
- /// Row 2: ["comedy", "drama", "romance"] (perfectly shredded)
Review Comment:
This row doesn't bring any additional value for testing.
--
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] [WIP] Support Shredded Lists/Array in `variant_get` [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on PR #8354:
URL: https://github.com/apache/arrow-rs/pull/8354#issuecomment-3313837075
> > I'm not sure I understand how these unit tests will translate to `variant_get`?
>
> Could you elaborate please?
>
> I am currently trying to build just the Shredded `List` `VariantArray` test case, and while doing so learning how we could build them in `shred_variant` later. Once have a good way of building simple Shredded `List` `VariantArray` it will be easy to work on the rest of the unit tests for `variant_get`
No worries -- the current iteration does look it produces a correct shredded variant containing a list, so I should probably just be patient and let you finish!
--
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] [WIP] Support Shredded Lists/Array in `variant_get` [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on PR #8354:
URL: https://github.com/apache/arrow-rs/pull/8354#issuecomment-3338730079
> I made it work for a one of the simple tests and it doesn't go through with the second one because Variant to Arrow does not support utf8 yet.
>
> Do we have an issue tracking variant_to_arrow types support? If not, I can make one.
I'm not sure we have a tracking issue for utf8 support in variant_to_arrow, but I've also noticed that it's an annoying gap for unit testing (we all seem to reach for string values...)
--
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] [WIP] Support Shredded Lists/Array in `variant_get` [arrow-rs]
Posted by "sdf-jkl (via GitHub)" <gi...@apache.org>.
sdf-jkl commented on PR #8354:
URL: https://github.com/apache/arrow-rs/pull/8354#issuecomment-3930640104
I want to continue working here on #9443.
@klion26 I'll start addressing your old comments first.
--
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] Support Shredded Lists/Array in `variant_get` [arrow-rs]
Posted by "sdf-jkl (via GitHub)" <gi...@apache.org>.
sdf-jkl commented on code in PR #8354:
URL: https://github.com/apache/arrow-rs/pull/8354#discussion_r2989128373
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -92,13 +89,8 @@ fn take_list_like_index_as_shredding_state<L: ListLikeArray + 'static>(
ArrowError::ComputeError("List-like index does not fit into u64".into())
Review Comment:
Fixed here: https://github.com/apache/arrow-rs/pull/8354/commits/2135838994216e059c3c3ddc50f7f6dca30893f6. Thanks!
--
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] [WIP] Support Shredded Lists/Array in `variant_get` [arrow-rs]
Posted by "sdf-jkl (via GitHub)" <gi...@apache.org>.
sdf-jkl commented on code in PR #8354:
URL: https://github.com/apache/arrow-rs/pull/8354#discussion_r2836451583
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -97,14 +98,67 @@ pub(crate) fn follow_shredded_path_element<'a>(
})?;
let state = BorrowedShreddingState::try_from(struct_array)?;
- Ok(ShreddedPathStep::Success(state))
+ Ok(ShreddedPathStep::Success(state.into()))
}
- VariantPathElement::Index { .. } => {
+ VariantPathElement::Index { index } => {
// TODO: Support array indexing. Among other things, it will require slicing not
// only the array we have here, but also the corresponding metadata and null masks.
- Err(ArrowError::NotYetImplemented(
- "Pathing into shredded variant array index".into(),
- ))
+ let Some(list_array) = typed_value.as_any().downcast_ref::<GenericListArray<i32>>()
+ else {
+ // Downcast failure - if strict cast options are enabled, this should be an error
+ if !cast_options.safe {
+ return Err(ArrowError::CastError(format!(
+ "Cannot access index '{}' on non-list type: {}",
+ index,
+ typed_value.data_type()
+ )));
+ }
+ // With safe cast options, return NULL (missing_path_step)
+ return Ok(missing_path_step());
+ };
+
+ let offsets = list_array.offsets();
+ let values = list_array.values(); // This is a StructArray
+
+ let Some(struct_array) = values.as_any().downcast_ref::<StructArray>() else {
+ return Ok(missing_path_step());
+ };
+
+ let Some(typed_array) = struct_array.column_by_name("typed_value") else {
+ return Ok(missing_path_step());
+ };
+
+ // Build the list of indices to take
+ let mut take_indices = Vec::with_capacity(list_array.len());
+ for i in 0..list_array.len() {
+ let start = offsets[i] as usize;
+ let end = offsets[i + 1] as usize;
+ let len = end - start;
+
+ if *index < len {
+ take_indices.push(Some((start + index) as u32));
+ } else {
+ take_indices.push(None);
Review Comment:
https://github.com/apache/arrow-rs/pull/8354/commits/cfe7c00c41ad9565078561a7eb4d7ce50d4a3cd1
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -97,14 +98,67 @@ pub(crate) fn follow_shredded_path_element<'a>(
})?;
let state = BorrowedShreddingState::try_from(struct_array)?;
- Ok(ShreddedPathStep::Success(state))
+ Ok(ShreddedPathStep::Success(state.into()))
}
- VariantPathElement::Index { .. } => {
+ VariantPathElement::Index { index } => {
// TODO: Support array indexing. Among other things, it will require slicing not
// only the array we have here, but also the corresponding metadata and null masks.
- Err(ArrowError::NotYetImplemented(
- "Pathing into shredded variant array index".into(),
- ))
+ let Some(list_array) = typed_value.as_any().downcast_ref::<GenericListArray<i32>>()
+ else {
+ // Downcast failure - if strict cast options are enabled, this should be an error
+ if !cast_options.safe {
+ return Err(ArrowError::CastError(format!(
+ "Cannot access index '{}' on non-list type: {}",
+ index,
+ typed_value.data_type()
+ )));
+ }
+ // With safe cast options, return NULL (missing_path_step)
+ return Ok(missing_path_step());
+ };
+
+ let offsets = list_array.offsets();
+ let values = list_array.values(); // This is a StructArray
+
+ let Some(struct_array) = values.as_any().downcast_ref::<StructArray>() else {
+ return Ok(missing_path_step());
+ };
+
+ let Some(typed_array) = struct_array.column_by_name("typed_value") else {
+ return Ok(missing_path_step());
+ };
+
+ // Build the list of indices to take
+ let mut take_indices = Vec::with_capacity(list_array.len());
+ for i in 0..list_array.len() {
+ let start = offsets[i] as usize;
+ let end = offsets[i + 1] as usize;
+ let len = end - start;
+
+ if *index < len {
+ take_indices.push(Some((start + index) as u32));
+ } else {
+ take_indices.push(None);
Review Comment:
Done in https://github.com/apache/arrow-rs/pull/8354/commits/cfe7c00c41ad9565078561a7eb4d7ce50d4a3cd1
--
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] [WIP] Support Shredded Lists/Array in `variant_get` [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on PR #8354:
URL: https://github.com/apache/arrow-rs/pull/8354#issuecomment-3946970143
Looking for an early pass? Or is this `Ready for review` now?
--
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] Support Shredded Lists/Array in `variant_get` [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on PR #8354:
URL: https://github.com/apache/arrow-rs/pull/8354#issuecomment-3986828962
Everything looks good, code-wise -- nice and clean.
But there's still an open question of whether we intend to follow the jsonpath spec in our path step logic, as e.g. spark does?
https://github.com/apache/arrow-rs/pull/8354#discussion_r2842833615
The jsonpath spec requires `foo[100]` to return NULL if `foo` is not an array, and also requires returning NULL if `foo` has fewer than 101 elements. Similarly, `foo.bar` should return NULL if `foo` is not a struct and should also return NULL if `foo` has no field named `bar`. Safe casting would only influence actual casting decisions, e.g. a `variant_get` call that specifically requests a string and the requested path points to a struct.
In contrast, our current struct handling code currently returns an error if safe casting is disabled and:
* a `Field` path step encounters a "wrong" type (L169)
* an `Index` path step encounters a "wrong" type (L224)
* an `Index` path step is out of bounds (L99)
--
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] [WIP] Support Shredded Lists/Array in `variant_get` [arrow-rs]
Posted by "sdf-jkl (via GitHub)" <gi...@apache.org>.
sdf-jkl commented on code in PR #8354:
URL: https://github.com/apache/arrow-rs/pull/8354#discussion_r2373555195
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -100,12 +103,56 @@ pub(crate) fn follow_shredded_path_element(
Ok(ShreddedPathStep::Success(struct_array.into()))
}
- VariantPathElement::Index { .. } => {
+ VariantPathElement::Index { index } => {
// TODO: Support array indexing. Among other things, it will require slicing not
// only the array we have here, but also the corresponding metadata and null masks.
- Err(ArrowError::NotYetImplemented(
- "Pathing into shredded variant array index".into(),
- ))
+ let Some(list_array) = typed_value.as_any().downcast_ref::<GenericListArray<i64>>()// <- shouldn't be just i64
+ else {
+ // Downcast failure - if strict cast options are enabled, this should be an error
+ if !cast_options.safe {
+ return Err(ArrowError::CastError(format!(
+ "Cannot access index '{}' on non-list type: {}",
+ index,
+ typed_value.data_type()
+ )));
+ }
+ // With safe cast options, return NULL (missing_path_step)
+ return Ok(missing_path_step());
+ };
+
+ let offsets = list_array.offsets();
+ let list_len = list_array.len(); // number of lists
+ let values = list_array.values(); // This is a StructArray
+
+ let Some(struct_array) = values.as_any().downcast_ref::<StructArray>() else {
+ return Ok(missing_path_step());
+ };
+
+ let Some(field_array) = struct_array.column_by_name("typed_value") else {
+ return Ok(missing_path_step());
+ };
+
+ // Build the list of indices to take
+ let mut take_indices = Vec::with_capacity(list_len);
+ for i in 0..list_len {
+ let start = offsets[i] as usize;
+ let end = offsets[i + 1] as usize;
+ let len = end - start;
+
+ if *index < len {
+ take_indices.push(Some((start + index) as u32));
+ } else {
+ take_indices.push(None);
+ }
+ }
+
+ let index_array = UInt32Array::from(take_indices);
+
+ // Use Arrow compute kernel to gather elements
+ let taken = take(field_array, &index_array, None)?;
Review Comment:
You can see the basic idea here
--
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] Support Shredded Lists/Array in `variant_get` [arrow-rs]
Posted by "sdf-jkl (via GitHub)" <gi...@apache.org>.
sdf-jkl commented on code in PR #8354:
URL: https://github.com/apache/arrow-rs/pull/8354#discussion_r2912576417
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -1848,6 +1979,282 @@ mod test {
assert_eq!(&result, &expected);
}
+ /// This test uses a pre-shredded list array and validates index-path access.
+ #[test]
+ fn test_shredded_list_index_access() {
+ let array = shredded_list_variant_array();
+ // Test: Extract the 0 index field as VariantArray first
+ let options = GetOptions::new_with_path(VariantPath::from(0));
+ let result = variant_get(&array, options).unwrap();
+ let result_variant = VariantArray::try_new(&result).unwrap();
+ assert_eq!(result_variant.len(), 2);
+
+ // Row 0: expect 0 index = "comedy"
+ assert_eq!(result_variant.value(0), Variant::from("comedy"));
+ // Row 1: expect 0 index = "horror"
+ assert_eq!(result_variant.value(1), Variant::from("horror"));
+ }
+
+ /// Test extracting shredded list field with type conversion.
+ #[test]
+ fn test_shredded_list_as_string() {
+ let array = shredded_list_variant_array();
+ // Test: Extract the 0 index values as StringArray (type conversion)
+ let field = Field::new("typed_value", DataType::Utf8, false);
+ let options = GetOptions::new_with_path(VariantPath::from(0))
+ .with_as_type(Some(FieldRef::from(field)));
+ let result = variant_get(&array, options).unwrap();
+ // Should get StringArray
+ let expected: ArrayRef = Arc::new(StringArray::from(vec![Some("comedy"), Some("horror")]));
+ assert_eq!(&result, &expected);
+ }
+
+ #[test]
+ fn test_shredded_list_index_access_from_value_field() {
+ let array = shredded_list_variant_array();
+ // Index 1 maps to "drama" for row 0, and to fallback value 123 for row 1.
+ let options = GetOptions::new_with_path(VariantPath::from(1));
+ let result = variant_get(&array, options).unwrap();
+ let result_variant = VariantArray::try_new(&result).unwrap();
+
+ assert_eq!(result_variant.value(0), Variant::from("drama"));
+ assert_eq!(result_variant.value(1).as_int64(), Some(123));
+ }
+
+ #[test]
+ fn test_shredded_list_index_access_from_value_field_as_int64() {
+ let array = shredded_list_variant_array();
+ let field = Field::new("typed_value", DataType::Int64, true);
+ let options = GetOptions::new_with_path(VariantPath::from(1))
+ .with_as_type(Some(FieldRef::from(field)));
+ let result = variant_get(&array, options).unwrap();
+
+ // "drama" -> NULL, 123 -> 123.
+ let expected: ArrayRef = Arc::new(Int64Array::from(vec![None, Some(123)]));
+ assert_eq!(&result, &expected);
+ }
+
+ #[test]
+ fn test_shredded_list_index_out_of_bounds_unsafe_cast_errors() {
+ let options =
+ GetOptions::new_with_path(VariantPath::from(10)).with_cast_options(CastOptions {
+ safe: false,
+ ..Default::default()
+ });
+
+ let err = variant_get(&shredded_list_variant_array(), options.clone()).unwrap_err();
+ assert!(err.to_string().contains("Cannot access index '10'"));
+ }
+
+ #[test]
+ fn test_shredded_large_list_index_access_from_value_field() {
+ let array = shredded_large_list_variant_array();
+ // Index 1 maps to "drama" for row 0, and to fallback value 123 for row 1.
+ let options = GetOptions::new_with_path(VariantPath::from(1));
+ let result = variant_get(&array, options).unwrap();
+ let result_variant = VariantArray::try_new(&result).unwrap();
+
+ assert_eq!(result_variant.value(0), Variant::from("drama"));
+ assert_eq!(result_variant.value(1).as_int64(), Some(123));
+ }
+
+ #[test]
+ fn test_shredded_large_list_index_out_of_bounds_unsafe_cast_errors() {
+ let options =
+ GetOptions::new_with_path(VariantPath::from(10)).with_cast_options(CastOptions {
+ safe: false,
+ ..Default::default()
+ });
+
+ let err = variant_get(&shredded_large_list_variant_array(), options).unwrap_err();
+ assert!(err.to_string().contains("Cannot access index '10'"));
+ }
+
+ #[test]
+ fn test_shredded_list_view_index_access_from_value_field() {
+ let array = shredded_list_view_variant_array();
+ let options = GetOptions::new_with_path(VariantPath::from(1));
+ let result = variant_get(&array, options).unwrap();
+ let result_variant = VariantArray::try_new(&result).unwrap();
+
+ assert_eq!(result_variant.value(0), Variant::from("drama"));
+ assert_eq!(result_variant.value(1).as_int64(), Some(123));
+ }
+
+ #[test]
+ fn test_shredded_list_view_index_out_of_bounds_unsafe_cast_errors() {
+ let options =
+ GetOptions::new_with_path(VariantPath::from(10)).with_cast_options(CastOptions {
+ safe: false,
+ ..Default::default()
+ });
+
+ let err = variant_get(&shredded_list_view_variant_array(), options).unwrap_err();
+ assert!(err.to_string().contains("Cannot access index '10'"));
+ }
+
+ #[test]
+ fn test_shredded_large_list_view_index_access_from_value_field() {
+ let array = shredded_large_list_view_variant_array();
+ let options = GetOptions::new_with_path(VariantPath::from(1));
+ let result = variant_get(&array, options).unwrap();
+ let result_variant = VariantArray::try_new(&result).unwrap();
+
+ assert_eq!(result_variant.value(0), Variant::from("drama"));
+ assert_eq!(result_variant.value(1).as_int64(), Some(123));
+ }
+
+ #[test]
+ fn test_shredded_large_list_view_index_out_of_bounds_unsafe_cast_errors() {
+ let options =
+ GetOptions::new_with_path(VariantPath::from(10)).with_cast_options(CastOptions {
+ safe: false,
+ ..Default::default()
+ });
+
+ let err = variant_get(&shredded_large_list_view_variant_array(), options).unwrap_err();
+ assert!(err.to_string().contains("Cannot access index '10'"));
+ }
+
+ #[test]
+ fn test_shredded_list_in_struct_index_access() {
+ let array = shredded_struct_with_list_variant_array();
+ let options = GetOptions::new_with_path(VariantPath::try_from("a[1]").unwrap());
+ let result = variant_get(&array, options).unwrap();
+ let result_variant = VariantArray::try_new(&result).unwrap();
+
+ assert_eq!(result_variant.value(0), Variant::from("drama"));
+ assert_eq!(result_variant.value(1).as_int64(), Some(123));
+ }
+
+ #[test]
+ fn test_shredded_struct_in_list_field_access() {
+ let array = shredded_list_of_struct_variant_array();
+ let field = Field::new("x", DataType::Int32, true);
+ let path = VariantPath::from(0).join("x");
+ let options = GetOptions::new_with_path(path).with_as_type(Some(FieldRef::from(field)));
+ let result = variant_get(&array, options).unwrap();
+
+ let expected: ArrayRef = Arc::new(Int32Array::from(vec![Some(1), Some(3)]));
+ assert_eq!(&result, &expected);
+ }
+
+ #[test]
+ fn test_shredded_list_of_lists_index_access() {
+ let array = shredded_list_of_lists_variant_array();
+ let path = VariantPath::from(0).join(1);
+
+ let result = variant_get(&array, GetOptions::new_with_path(path.clone())).unwrap();
+ let result_variant = VariantArray::try_new(&result).unwrap();
+ assert_eq!(result_variant.value(0), Variant::from("b"));
+ assert_eq!(result_variant.value(1).as_int64(), Some(123));
+
+ let field = Field::new("typed_value", DataType::Int64, true);
+ let casted = variant_get(
+ &array,
+ GetOptions::new_with_path(path).with_as_type(Some(FieldRef::from(field))),
+ )
+ .unwrap();
+ let expected: ArrayRef = Arc::new(Int64Array::from(vec![None, Some(123)]));
+ assert_eq!(&casted, &expected);
+ }
+
+ /// Helper to create a shredded list variant array used by list index tests.
+ ///
+ /// Rows:
+ /// 1. `["comedy", "drama"]` (fully shred-able as `Utf8`)
+ /// 2. `["horror", 123]` (partially shredded, with fallback for the numeric element)
+ fn shredded_list_variant_array() -> ArrayRef {
+ let json_rows: ArrayRef = Arc::new(StringArray::from(vec![
+ Some(r#"["comedy", "drama"]"#),
+ Some(r#"["horror", 123]"#),
+ ]));
+ let input = json_to_variant(&json_rows).unwrap();
+
+ let list_schema = DataType::List(Arc::new(Field::new("item", DataType::Utf8, true)));
+ let shredded = shred_variant(&input, &list_schema).unwrap();
+ ArrayRef::from(shredded)
+ }
+
+ fn shredded_struct_with_list_variant_array() -> ArrayRef {
+ let json_rows: ArrayRef = Arc::new(StringArray::from(vec![
+ Some(r#"{"a": ["comedy", "drama"]}"#),
+ Some(r#"{"a": ["horror", 123]}"#),
+ ]));
+ let input = json_to_variant(&json_rows).unwrap();
+
+ let list_schema = DataType::List(Arc::new(Field::new("item", DataType::Utf8, true)));
+ let shredding_schema = ShreddedSchemaBuilder::default()
+ .with_path("a", &list_schema)
+ .unwrap()
+ .build();
+ let shredded = shred_variant(&input, &shredding_schema).unwrap();
+ ArrayRef::from(shredded)
+ }
+
+ fn shredded_list_of_struct_variant_array() -> ArrayRef {
+ let json_rows: ArrayRef = Arc::new(StringArray::from(vec![
+ Some(r#"[{"x": 1}, {"x": 2}]"#),
+ Some(r#"[{"x": 3}, {"y": 4}]"#),
+ ]));
+ let input = json_to_variant(&json_rows).unwrap();
+
+ let struct_type =
+ DataType::Struct(Fields::from(vec![Field::new("x", DataType::Int32, true)]));
+ let list_schema = DataType::List(Arc::new(Field::new("item", struct_type, true)));
+ let shredded = shred_variant(&input, &list_schema).unwrap();
+ ArrayRef::from(shredded)
+ }
+
+ fn shredded_list_of_lists_variant_array() -> ArrayRef {
+ let json_rows: ArrayRef = Arc::new(StringArray::from(vec![
+ Some(r#"[["a", "b"], ["c", "d"]]"#),
+ Some(r#"[["x", 123], ["y", "z"]]"#),
+ ]));
+ let input = json_to_variant(&json_rows).unwrap();
+
+ let inner_list = DataType::List(Arc::new(Field::new("item", DataType::Utf8, true)));
+ let outer_list = DataType::List(Arc::new(Field::new("item", inner_list, true)));
+ let shredded = shred_variant(&input, &outer_list).unwrap();
+ ArrayRef::from(shredded)
+ }
+
+ fn shredded_large_list_variant_array() -> ArrayRef {
+ let json_rows: ArrayRef = Arc::new(StringArray::from(vec![
+ Some(r#"["comedy", "drama"]"#),
+ Some(r#"["horror", 123]"#),
+ ]));
+ let input = json_to_variant(&json_rows).unwrap();
+
+ let list_schema = DataType::LargeList(Arc::new(Field::new("item", DataType::Utf8, true)));
+ let shredded = shred_variant(&input, &list_schema).unwrap();
+ ArrayRef::from(shredded)
+ }
+
+ fn shredded_list_view_variant_array() -> ArrayRef {
+ let json_rows: ArrayRef = Arc::new(StringArray::from(vec![
+ Some(r#"["comedy", "drama"]"#),
+ Some(r#"["horror", 123]"#),
+ ]));
+ let input = json_to_variant(&json_rows).unwrap();
+
+ let list_schema = DataType::ListView(Arc::new(Field::new("item", DataType::Utf8, true)));
+ let shredded = shred_variant(&input, &list_schema).unwrap();
+ ArrayRef::from(shredded)
+ }
+
+ fn shredded_large_list_view_variant_array() -> ArrayRef {
+ let json_rows: ArrayRef = Arc::new(StringArray::from(vec![
+ Some(r#"["comedy", "drama"]"#),
+ Some(r#"["horror", 123]"#),
+ ]));
+ let input = json_to_variant(&json_rows).unwrap();
+
+ let list_schema =
+ DataType::LargeListView(Arc::new(Field::new("item", DataType::Utf8, true)));
+ let shredded = shred_variant(&input, &list_schema).unwrap();
+ ArrayRef::from(shredded)
+ }
Review Comment:
Thanks, added the changes here https://github.com/apache/arrow-rs/pull/8354/commits/18d04d718103fd1334b54bc01cad24c8dcb9d132
--
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] Support Shredded Lists/Array in `variant_get` [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on PR #8354:
URL: https://github.com/apache/arrow-rs/pull/8354#issuecomment-3986833679
@alamb -- any opinions about supporting jsonpath semantics or not? Or ideas on who we should seek input 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] [WIP] Support Shredded Lists/Array in `variant_get` [arrow-rs]
Posted by "sdf-jkl (via GitHub)" <gi...@apache.org>.
sdf-jkl commented on code in PR #8354:
URL: https://github.com/apache/arrow-rs/pull/8354#discussion_r2836331532
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -97,14 +98,67 @@ pub(crate) fn follow_shredded_path_element<'a>(
})?;
let state = BorrowedShreddingState::try_from(struct_array)?;
- Ok(ShreddedPathStep::Success(state))
+ Ok(ShreddedPathStep::Success(state.into()))
}
- VariantPathElement::Index { .. } => {
+ VariantPathElement::Index { index } => {
// TODO: Support array indexing. Among other things, it will require slicing not
// only the array we have here, but also the corresponding metadata and null masks.
- Err(ArrowError::NotYetImplemented(
- "Pathing into shredded variant array index".into(),
- ))
+ let Some(list_array) = typed_value.as_any().downcast_ref::<GenericListArray<i32>>()
Review Comment:
Actually, it seems like not an issue right now because `VariantArray` does not support `LargeList`:
https://github.com/apache/arrow-rs/blob/25ad2d488fe19175b2cd15e86c3bc1153cb5a38b/parquet-variant-compute/src/variant_array.rs#L1184-L1187
--
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] [WIP] Support Shredded Lists/Array in `variant_get` [arrow-rs]
Posted by "sdf-jkl (via GitHub)" <gi...@apache.org>.
sdf-jkl commented on PR #8354:
URL: https://github.com/apache/arrow-rs/pull/8354#issuecomment-3608871614
@alamb @klion26 can you take a 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
Re: [PR] [WIP] Support Shredded Lists/Array in `variant_get` [arrow-rs]
Posted by "sdf-jkl (via GitHub)" <gi...@apache.org>.
sdf-jkl commented on code in PR #8354:
URL: https://github.com/apache/arrow-rs/pull/8354#discussion_r2608139858
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -97,14 +98,67 @@ pub(crate) fn follow_shredded_path_element<'a>(
})?;
let state = BorrowedShreddingState::try_from(struct_array)?;
- Ok(ShreddedPathStep::Success(state))
+ Ok(ShreddedPathStep::Success(state.into()))
}
- VariantPathElement::Index { .. } => {
+ VariantPathElement::Index { index } => {
// TODO: Support array indexing. Among other things, it will require slicing not
// only the array we have here, but also the corresponding metadata and null masks.
- Err(ArrowError::NotYetImplemented(
- "Pathing into shredded variant array index".into(),
- ))
+ let Some(list_array) = typed_value.as_any().downcast_ref::<GenericListArray<i32>>()
+ else {
+ // Downcast failure - if strict cast options are enabled, this should be an error
+ if !cast_options.safe {
+ return Err(ArrowError::CastError(format!(
+ "Cannot access index '{}' on non-list type: {}",
+ index,
+ typed_value.data_type()
+ )));
+ }
+ // With safe cast options, return NULL (missing_path_step)
+ return Ok(missing_path_step());
+ };
+
+ let offsets = list_array.offsets();
+ let values = list_array.values(); // This is a StructArray
+
+ let Some(struct_array) = values.as_any().downcast_ref::<StructArray>() else {
+ return Ok(missing_path_step());
+ };
+
+ let Some(typed_array) = struct_array.column_by_name("typed_value") else {
+ return Ok(missing_path_step());
+ };
+
+ // Build the list of indices to take
+ let mut take_indices = Vec::with_capacity(list_array.len());
+ for i in 0..list_array.len() {
+ let start = offsets[i] as usize;
+ let end = offsets[i + 1] as usize;
+ let len = end - start;
+
+ if *index < len {
+ take_indices.push(Some((start + index) as u32));
+ } else {
+ take_indices.push(None);
Review Comment:
Lists can be variable length within a `ListArray`, so I don't think it should be an error if the list if not long enough for an index.
```
(GenericListAray docs)
For example, the ListArray shown in the following diagram stores lists of strings. Note that [] represents an empty (length 0), but non NULL list.
âââââââââââââââ
â [A,B,C] â
âââââââââââââââ¤
â [] â
âââââââââââââââ¤
â NULL â
âââââââââââââââ¤
â [D] â
âââââââââââââââ¤
â [NULL, F] â
âââââââââââââââ
```
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -97,14 +98,67 @@ pub(crate) fn follow_shredded_path_element<'a>(
})?;
let state = BorrowedShreddingState::try_from(struct_array)?;
- Ok(ShreddedPathStep::Success(state))
+ Ok(ShreddedPathStep::Success(state.into()))
}
- VariantPathElement::Index { .. } => {
+ VariantPathElement::Index { index } => {
// TODO: Support array indexing. Among other things, it will require slicing not
// only the array we have here, but also the corresponding metadata and null masks.
- Err(ArrowError::NotYetImplemented(
- "Pathing into shredded variant array index".into(),
- ))
+ let Some(list_array) = typed_value.as_any().downcast_ref::<GenericListArray<i32>>()
+ else {
+ // Downcast failure - if strict cast options are enabled, this should be an error
+ if !cast_options.safe {
+ return Err(ArrowError::CastError(format!(
+ "Cannot access index '{}' on non-list type: {}",
+ index,
+ typed_value.data_type()
+ )));
+ }
+ // With safe cast options, return NULL (missing_path_step)
+ return Ok(missing_path_step());
+ };
+
+ let offsets = list_array.offsets();
+ let values = list_array.values(); // This is a StructArray
+
+ let Some(struct_array) = values.as_any().downcast_ref::<StructArray>() else {
+ return Ok(missing_path_step());
+ };
+
+ let Some(typed_array) = struct_array.column_by_name("typed_value") else {
+ return Ok(missing_path_step());
+ };
+
+ // Build the list of indices to take
+ let mut take_indices = Vec::with_capacity(list_array.len());
+ for i in 0..list_array.len() {
+ let start = offsets[i] as usize;
+ let end = offsets[i + 1] as usize;
+ let len = end - start;
+
+ if *index < len {
+ take_indices.push(Some((start + index) as u32));
+ } else {
+ take_indices.push(None);
Review Comment:
Lists can be variable length within a `ListArray`, so I don't think it should be an error if the list if not long enough for an index.
```
(GenericListArray docs)
For example, the ListArray shown in the following diagram stores lists of strings. Note that [] represents an empty (length 0), but non NULL list.
âââââââââââââââ
â [A,B,C] â
âââââââââââââââ¤
â [] â
âââââââââââââââ¤
â NULL â
âââââââââââââââ¤
â [D] â
âââââââââââââââ¤
â [NULL, F] â
âââââââââââââââ
```
--
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] [WIP] Support Shredded Lists/Array in `variant_get` [arrow-rs]
Posted by "klion26 (via GitHub)" <gi...@apache.org>.
klion26 commented on code in PR #8354:
URL: https://github.com/apache/arrow-rs/pull/8354#discussion_r2609654290
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -97,14 +98,67 @@ pub(crate) fn follow_shredded_path_element<'a>(
})?;
let state = BorrowedShreddingState::try_from(struct_array)?;
- Ok(ShreddedPathStep::Success(state))
+ Ok(ShreddedPathStep::Success(state.into()))
}
- VariantPathElement::Index { .. } => {
+ VariantPathElement::Index { index } => {
// TODO: Support array indexing. Among other things, it will require slicing not
// only the array we have here, but also the corresponding metadata and null masks.
- Err(ArrowError::NotYetImplemented(
- "Pathing into shredded variant array index".into(),
- ))
+ let Some(list_array) = typed_value.as_any().downcast_ref::<GenericListArray<i32>>()
+ else {
+ // Downcast failure - if strict cast options are enabled, this should be an error
+ if !cast_options.safe {
+ return Err(ArrowError::CastError(format!(
+ "Cannot access index '{}' on non-list type: {}",
+ index,
+ typed_value.data_type()
+ )));
+ }
+ // With safe cast options, return NULL (missing_path_step)
+ return Ok(missing_path_step());
+ };
+
+ let offsets = list_array.offsets();
+ let values = list_array.values(); // This is a StructArray
+
+ let Some(struct_array) = values.as_any().downcast_ref::<StructArray>() else {
+ return Ok(missing_path_step());
+ };
+
+ let Some(typed_array) = struct_array.column_by_name("typed_value") else {
+ return Ok(missing_path_step());
+ };
+
+ // Build the list of indices to take
+ let mut take_indices = Vec::with_capacity(list_array.len());
+ for i in 0..list_array.len() {
+ let start = offsets[i] as usize;
+ let end = offsets[i + 1] as usize;
+ let len = end - start;
+
+ if *index < len {
Review Comment:
Sorry for not describing it clearly.
The data`["comedy", "drama"], ["horro", 123]` translated into variant will be that
- `comedy`, `drama` and `horro` in the `typed_value` column,
- and `123` in the `value` column(it has an incompatible type).
Here, we retrieve all the results from the `typed_value` column(`take` in line 148), but `["hooro", 123](1)`(the second item in the list) here will return null(if `CastOptions::safe = true`) and `Err` (if `CastOptions::safe = false`) -- currently, we return null for both of the cases.
Seems there may be something more tricky here(maybe we need to have a design note for this as this [comment](https://github.com/apache/arrow-rs/issues/8082#issuecomment-3258435567)), such as
- if the target_type here we request is not `list`/`struct` then, we can use the logic like here, and respect the `CastOptions::safe`
- If we need to handle [variant nesting](https://github.com/apache/parquet-format/blob/master/VariantShredding.md#nesting) here or somewhere else?
- Here, I don't have any answer yet. I'll try to find some time next week for this.
--
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] [WIP] Support Shredded Lists/Array in `variant_get` [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on code in PR #8354:
URL: https://github.com/apache/arrow-rs/pull/8354#discussion_r2610915170
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -97,14 +98,67 @@ pub(crate) fn follow_shredded_path_element<'a>(
})?;
let state = BorrowedShreddingState::try_from(struct_array)?;
- Ok(ShreddedPathStep::Success(state))
+ Ok(ShreddedPathStep::Success(state.into()))
}
- VariantPathElement::Index { .. } => {
+ VariantPathElement::Index { index } => {
// TODO: Support array indexing. Among other things, it will require slicing not
// only the array we have here, but also the corresponding metadata and null masks.
- Err(ArrowError::NotYetImplemented(
- "Pathing into shredded variant array index".into(),
- ))
+ let Some(list_array) = typed_value.as_any().downcast_ref::<GenericListArray<i32>>()
+ else {
+ // Downcast failure - if strict cast options are enabled, this should be an error
+ if !cast_options.safe {
+ return Err(ArrowError::CastError(format!(
+ "Cannot access index '{}' on non-list type: {}",
+ index,
+ typed_value.data_type()
+ )));
+ }
+ // With safe cast options, return NULL (missing_path_step)
+ return Ok(missing_path_step());
+ };
+
+ let offsets = list_array.offsets();
+ let values = list_array.values(); // This is a StructArray
+
+ let Some(struct_array) = values.as_any().downcast_ref::<StructArray>() else {
+ return Ok(missing_path_step());
+ };
+
+ let Some(typed_array) = struct_array.column_by_name("typed_value") else {
+ return Ok(missing_path_step());
+ };
+
+ // Build the list of indices to take
+ let mut take_indices = Vec::with_capacity(list_array.len());
+ for i in 0..list_array.len() {
+ let start = offsets[i] as usize;
+ let end = offsets[i + 1] as usize;
+ let len = end - start;
+
+ if *index < len {
Review Comment:
NOTE: Other than (partially shredded) object fields, the shredding spec doesn't actually _require_ any other type to shred merely because a compatible `typed_value` column exists. We have to assume that e.g. `value` could contain i8, i32, and i64 values even if `typed_value` is a 64-bit int. And AFAIK, we also have to assume that `value` could contain a variant array even if `typed_value` is a list. Super annoying.
Maybe this code already handles that case, but I wanted to make sure to flag it.
--
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] [WIP] Support Shredded Lists/Array in `variant_get` [arrow-rs]
Posted by "sdf-jkl (via GitHub)" <gi...@apache.org>.
sdf-jkl commented on code in PR #8354:
URL: https://github.com/apache/arrow-rs/pull/8354#discussion_r2356745907
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -1122,64 +1121,109 @@ mod test {
let result = variant_get(&array, options).unwrap();
// Should get StringArray
- let expected: ArrayRef = Arc::new(StringArray::from(vec![Some("comedy"), Some("drama")]));
+ let expected: ArrayRef =
+ Arc::new(StringArray::from(vec![Some("comedy"), None, Some("drama")]));
assert_eq!(&result, &expected);
}
/// Helper function to create a shredded variant array representing lists
///
/// This creates an array that represents:
/// Row 0: ["comedy", "drama"] ([0] is shredded, [1] is shredded - perfectly shredded)
- /// Row 1: ["horror", null] ([0] is shredded, [1] is binary null - partially shredded)
- /// Row 2: ["comedy", "drama", "romance"] (perfectly shredded)
+ /// Row 1: ["horror", 123] ([0] is shredded, [1] is int - partially shredded)
///
/// The physical layout follows the shredding spec where:
/// - metadata: contains list metadata
/// - typed_value: StructArray with 0 index value
/// - value: contains fallback for
fn shredded_list_variant_array() -> ArrayRef {
- // Create the base metadata for lists
-
- // Could add this as an api for VariantList, like VariantList::from()
- fn build_list_metadata(vector: Vec<Variant>) -> (Vec<u8>, Vec<u8>) {
- let mut builder = parquet_variant::VariantBuilder::new();
- let mut list = builder.new_list();
- for value in vector {
- list.append_value(value);
- }
- list.finish();
- builder.finish()
- }
- let (metadata1, _) =
- build_list_metadata(vec![Variant::String("comedy"), Variant::String("drama")]);
+ // Create metadata array
+ let metadata_array =
+ BinaryViewArray::from_iter_values(std::iter::repeat_n(EMPTY_VARIANT_METADATA_BYTES, 2));
- let (metadata2, _) = build_list_metadata(vec![Variant::String("horror"), Variant::Null]);
+ // Building the typed_value ListArray
- let (metadata3, _) = build_list_metadata(vec![
- Variant::String("comedy"),
- Variant::String("drama"),
- Variant::String("romance"),
+ // Need a StructBuilder to create a ListBuilder
+ let fields = Fields::from(vec![
+ Field::new("value", DataType::Binary, true),
+ Field::new("typed_value", DataType::Utf8, true),
]);
+ let field_builders = vec![
+ make_builder(&DataType::Binary, 4),
+ make_builder(&DataType::Utf8, 4),
+ ];
+ let struct_builder = StructBuilder::new(fields, field_builders);
+
+ let mut builder = GenericListBuilder::<i32, StructBuilder>::new(struct_builder);
+
+ // Row 0 index 0
+ builder
+ .values()
+ .field_builder::<BinaryBuilder>(0)
+ .unwrap()
+ .append_null();
+ builder
+ .values()
+ .field_builder::<StringBuilder>(1)
+ .unwrap()
+ .append_value("comedy");
+ builder.values().append(true);
Review Comment:
If this is the right approach, I could make this cleaner with a helper function for appending each element.
--
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] Support Shredded Lists/Array in `variant_get` [arrow-rs]
Posted by "sdf-jkl (via GitHub)" <gi...@apache.org>.
sdf-jkl commented on code in PR #8354:
URL: https://github.com/apache/arrow-rs/pull/8354#discussion_r2854111039
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -1848,6 +1943,246 @@ mod test {
assert_eq!(&result, &expected);
}
+ /// This test uses a pre-shredded list array and validates index-path access.
+ #[test]
+ fn test_shredded_list_index_access() {
+ let array = shredded_list_variant_array();
+ // Test: Extract the 0 index field as VariantArray first
+ let options = GetOptions::new_with_path(VariantPath::from(0));
+ let result = variant_get(&array, options).unwrap();
+ let result_variant = VariantArray::try_new(&result).unwrap();
+ assert_eq!(result_variant.len(), 2);
+
+ // Row 0: expect 0 index = "comedy"
+ assert_eq!(result_variant.value(0), Variant::from("comedy"));
+ // Row 1: expect 0 index = "horror"
+ assert_eq!(result_variant.value(1), Variant::from("horror"));
+ }
+
+ /// Test extracting shredded list field with type conversion.
+ #[test]
+ fn test_shredded_list_as_string() {
+ let array = shredded_list_variant_array();
+ // Test: Extract the 0 index values as StringArray (type conversion)
+ let field = Field::new("typed_value", DataType::Utf8, false);
+ let options = GetOptions::new_with_path(VariantPath::from(0))
+ .with_as_type(Some(FieldRef::from(field)));
+ let result = variant_get(&array, options).unwrap();
+ // Should get StringArray
+ let expected: ArrayRef = Arc::new(StringArray::from(vec![Some("comedy"), Some("horror")]));
+ assert_eq!(&result, &expected);
+ }
+
+ #[test]
+ fn test_shredded_list_index_access_from_value_field() {
+ let array = shredded_list_variant_array();
+ // Index 1 maps to "drama" for row 0, and to fallback value 123 for row 1.
+ let options = GetOptions::new_with_path(VariantPath::from(1));
+ let result = variant_get(&array, options).unwrap();
+ let result_variant = VariantArray::try_new(&result).unwrap();
+
+ assert_eq!(result_variant.value(0), Variant::from("drama"));
+ assert_eq!(result_variant.value(1).as_int64(), Some(123));
+ }
+
+ #[test]
+ fn test_shredded_list_index_access_from_value_field_as_int64() {
+ let array = shredded_list_variant_array();
+ let field = Field::new("typed_value", DataType::Int64, true);
+ let options = GetOptions::new_with_path(VariantPath::from(1))
+ .with_as_type(Some(FieldRef::from(field)));
+ let result = variant_get(&array, options).unwrap();
+
+ // "drama" -> NULL, 123 -> 123.
+ let expected: ArrayRef = Arc::new(Int64Array::from(vec![None, Some(123)]));
+ assert_eq!(&result, &expected);
+ }
+
+ // The tests below are temp before: https://github.com/apache/arrow-rs/issues/9455
+ #[test]
+ fn test_shredded_list_index_out_of_bounds_unsafe_cast_errors() {
+ let options =
+ GetOptions::new_with_path(VariantPath::from(10)).with_cast_options(CastOptions {
+ safe: false,
+ ..Default::default()
+ });
+
+ let err = variant_get(&shredded_list_variant_array(), options.clone()).unwrap_err();
+ assert!(err.to_string().contains("Cannot access index '10'"));
+ }
+
+ #[test]
+ fn test_large_list_index_path_helper() {
+ let large_list = large_list_of_shredded_elements();
+ let state = take_list_index_as_shredding_state(&large_list, 1, &CastOptions::default())
+ .unwrap()
+ .unwrap();
+
+ let typed = state
+ .typed_value_field()
+ .unwrap()
+ .as_any()
+ .downcast_ref::<StringArray>()
+ .unwrap();
+ let value = state.value_field().unwrap();
+ assert_eq!(typed.value(0), "drama");
+ assert!(typed.is_null(1));
+ assert!(value.is_null(0));
+ assert!(value.is_valid(1));
+ }
+
+ #[test]
+ fn test_large_list_index_out_of_bounds_unsafe_cast_errors() {
+ let large_list = large_list_of_shredded_elements();
+ let err = take_list_index_as_shredding_state(
+ &large_list,
+ 10,
+ &CastOptions {
+ safe: false,
+ ..Default::default()
+ },
+ )
+ .unwrap_err();
+ assert!(err.to_string().contains("Cannot access index '10'"));
+ }
+
+ #[test]
+ fn test_shredded_list_in_struct_index_access() {
+ let array = shredded_struct_with_list_variant_array();
+ let options = GetOptions::new_with_path(VariantPath::try_from("a").unwrap().join(1));
Review Comment:
https://github.com/apache/arrow-rs/pull/8354/commits/cbfa058d527ed460bfd66f327624c26953c146ed
--
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] [WIP] Support Shredded Lists/Array in `variant_get` [arrow-rs]
Posted by "sdf-jkl (via GitHub)" <gi...@apache.org>.
sdf-jkl commented on code in PR #8354:
URL: https://github.com/apache/arrow-rs/pull/8354#discussion_r2836453945
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -97,14 +98,67 @@ pub(crate) fn follow_shredded_path_element<'a>(
})?;
let state = BorrowedShreddingState::try_from(struct_array)?;
- Ok(ShreddedPathStep::Success(state))
+ Ok(ShreddedPathStep::Success(state.into()))
}
- VariantPathElement::Index { .. } => {
+ VariantPathElement::Index { index } => {
// TODO: Support array indexing. Among other things, it will require slicing not
// only the array we have here, but also the corresponding metadata and null masks.
- Err(ArrowError::NotYetImplemented(
- "Pathing into shredded variant array index".into(),
- ))
+ let Some(list_array) = typed_value.as_any().downcast_ref::<GenericListArray<i32>>()
Review Comment:
Added in advance tests and support here: https://github.com/apache/arrow-rs/pull/8354/commits/cfe7c00c41ad9565078561a7eb4d7ce50d4a3cd1
--
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] [WIP] Support Shredded Lists/Array in `variant_get` [arrow-rs]
Posted by "sdf-jkl (via GitHub)" <gi...@apache.org>.
sdf-jkl commented on code in PR #8354:
URL: https://github.com/apache/arrow-rs/pull/8354#discussion_r2842997688
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -42,16 +45,84 @@ pub(crate) enum ShreddedPathStep<'a> {
NotShredded,
}
+fn take_list_index_as_shredding_state<O: OffsetSizeTrait>(
+ list_array: &GenericListArray<O>,
+ index: usize,
+ cast_options: &CastOptions,
+) -> Result<Option<ShreddingState>> {
+ let offsets = list_array.offsets();
+ let values = list_array.values();
+
+ let Some(struct_array) = values.as_any().downcast_ref::<StructArray>() else {
+ return Ok(None);
+ };
+
+ let value_array = struct_array.column_by_name("value");
+ let typed_array = struct_array.column_by_name("typed_value");
+
+ // If list elements have neither typed nor fallback value, this path step is missing.
+ if value_array.is_none() && typed_array.is_none() {
+ return Ok(None);
+ }
Review Comment:
Implemented here - https://github.com/apache/arrow-rs/pull/8354/commits/ccbf59b2e40f80274e5e5350b2b49d1c175cce5a
I used `ShreddingState` instead of `VariantArray`, because shredded fields don't have metadata needed for `VariantArray`
--
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] Support Shredded Lists/Array in `variant_get` [arrow-rs]
Posted by "sdf-jkl (via GitHub)" <gi...@apache.org>.
sdf-jkl commented on PR #8354:
URL: https://github.com/apache/arrow-rs/pull/8354#issuecomment-4521747585
@alamb @scovich finally unblocked!
--
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] Support Shredded Lists/Array in `variant_get` [arrow-rs]
Posted by "sdf-jkl (via GitHub)" <gi...@apache.org>.
sdf-jkl commented on code in PR #8354:
URL: https://github.com/apache/arrow-rs/pull/8354#discussion_r2997320009
##########
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:
We could push that PR forward, file a separate PR for `BorrowedShreddingState` and once they're done go back here.
--
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] Support Shredded Lists/Array in `variant_get` [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support Shredded Lists/Array in `variant_get` [arrow-rs]
Posted by "sdf-jkl (via GitHub)" <gi...@apache.org>.
sdf-jkl commented on code in PR #8354:
URL: https://github.com/apache/arrow-rs/pull/8354#discussion_r3190982505
##########
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:
Sounds good, I just copied the pattern from `follow_shredded_path_element` `VariantPathElement::Field` branch.
--
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] [WIP] Support Shredded Lists/Array in `variant_get` [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on code in PR #8354:
URL: https://github.com/apache/arrow-rs/pull/8354#discussion_r2357562447
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -1122,64 +1121,109 @@ mod test {
let result = variant_get(&array, options).unwrap();
// Should get StringArray
- let expected: ArrayRef = Arc::new(StringArray::from(vec![Some("comedy"), Some("drama")]));
+ let expected: ArrayRef =
+ Arc::new(StringArray::from(vec![Some("comedy"), None, Some("drama")]));
assert_eq!(&result, &expected);
}
/// Helper function to create a shredded variant array representing lists
///
/// This creates an array that represents:
/// Row 0: ["comedy", "drama"] ([0] is shredded, [1] is shredded - perfectly shredded)
- /// Row 1: ["horror", null] ([0] is shredded, [1] is binary null - partially shredded)
- /// Row 2: ["comedy", "drama", "romance"] (perfectly shredded)
+ /// Row 1: ["horror", 123] ([0] is shredded, [1] is int - partially shredded)
///
/// The physical layout follows the shredding spec where:
/// - metadata: contains list metadata
/// - typed_value: StructArray with 0 index value
/// - value: contains fallback for
fn shredded_list_variant_array() -> ArrayRef {
- // Create the base metadata for lists
-
- // Could add this as an api for VariantList, like VariantList::from()
- fn build_list_metadata(vector: Vec<Variant>) -> (Vec<u8>, Vec<u8>) {
- let mut builder = parquet_variant::VariantBuilder::new();
- let mut list = builder.new_list();
- for value in vector {
- list.append_value(value);
- }
- list.finish();
- builder.finish()
- }
- let (metadata1, _) =
- build_list_metadata(vec![Variant::String("comedy"), Variant::String("drama")]);
+ // Create metadata array
+ let metadata_array =
+ BinaryViewArray::from_iter_values(std::iter::repeat_n(EMPTY_VARIANT_METADATA_BYTES, 2));
- let (metadata2, _) = build_list_metadata(vec![Variant::String("horror"), Variant::Null]);
+ // Building the typed_value ListArray
- let (metadata3, _) = build_list_metadata(vec![
- Variant::String("comedy"),
- Variant::String("drama"),
- Variant::String("romance"),
+ // Need a StructBuilder to create a ListBuilder
+ let fields = Fields::from(vec![
+ Field::new("value", DataType::Binary, true),
+ Field::new("typed_value", DataType::Utf8, true),
]);
+ let field_builders = vec![
+ make_builder(&DataType::Binary, 4),
+ make_builder(&DataType::Utf8, 4),
+ ];
+ let struct_builder = StructBuilder::new(fields, field_builders);
+
+ let mut builder = GenericListBuilder::<i32, StructBuilder>::new(struct_builder);
+
+ // Row 0 index 0
+ builder
+ .values()
+ .field_builder::<BinaryBuilder>(0)
+ .unwrap()
+ .append_null();
+ builder
+ .values()
+ .field_builder::<StringBuilder>(1)
+ .unwrap()
+ .append_value("comedy");
+ builder.values().append(true);
+
+ // Row 0 index 1
+ builder
+ .values()
+ .field_builder::<BinaryBuilder>(0)
+ .unwrap()
+ .append_null();
+ builder
+ .values()
+ .field_builder::<StringBuilder>(1)
+ .unwrap()
+ .append_value("drama");
+ builder.values().append(true);
+
+ // Next row
+ builder.append(true);
+
+ // Row 1 index 0
+ builder
+ .values()
+ .field_builder::<BinaryBuilder>(0)
+ .unwrap()
+ .append_null();
+ builder
+ .values()
+ .field_builder::<StringBuilder>(1)
+ .unwrap()
+ .append_value("horror");
+ builder.values().append(true);
+
+ // Row 1 index 1
+ let mut variant_builder = VariantBuilder::new();
+ variant_builder.append_value(123i32); // <------ couldn't find the right way to do it, used this as placeholder for binary
+ let (_, value) = variant_builder.finish();
+
+ builder
+ .values()
+ .field_builder::<BinaryBuilder>(0)
+ .unwrap()
+ .append_value(value);
Review Comment:
Once https://github.com/apache/arrow-rs/pull/8360 merges (hopefully today), you can use a `VariantValueArrayBuilder` to create the values column. That requires keeping the builders separate and then using `VariantArray::from_parts` to assemble them into the final result.
See e.g. [VariantToBinaryVariantArrowRowBuilder](https://github.com/apache/arrow-rs/pull/8360/files#diff-5b1dcb31d6afd99fdac153e6e8d9cdec8aac81c0dfdcc13729de95951a409874R257-R288) in that same PR
--
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] [WIP] Support Shredded Lists/Array in `variant_get` [arrow-rs]
Posted by "sdf-jkl (via GitHub)" <gi...@apache.org>.
sdf-jkl commented on code in PR #8354:
URL: https://github.com/apache/arrow-rs/pull/8354#discussion_r2353395333
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -1010,7 +1010,101 @@ mod test {
let expected: ArrayRef = Arc::new(Int32Array::from(vec![Some(1), Some(42)]));
assert_eq!(&result, &expected);
}
+ /// This test manually constructs a shredded variant array representing lists
+ /// like ["comedy", "drama"], ["horror", null] and ["comedy", "drama", "romance"]
+ /// as VariantArray using variant_get.
+ #[test]
+ fn test_shredded_list_field_access() {
+ let array = shredded_list_variant_array();
+
+ // Test: Extract the 0 index field as VariantArray first
+ let options = GetOptions::new_with_path(VariantPath::from(0));
+ let result = variant_get(&array, options).unwrap();
+ let result_variant: &VariantArray = result.as_any().downcast_ref().unwrap();
+ assert_eq!(result_variant.len(), 3);
+
+ // Row 0: expect 0 index = "comedy"
+ assert_eq!(result_variant.value(0), Variant::String("comedy"));
+ // Row 1: expect 0 index = "horror"
+ assert_eq!(result_variant.value(1), Variant::String("horror"));
+ // Row 2: expect 0 index = "comedy"
+ assert_eq!(result_variant.value(2), Variant::String("comedy"));
+ }
+ /// Test extracting shredded list field with type conversion
+ #[test]
+ fn test_shredded_list_as_string() {
+ let array = shredded_list_variant_array();
+
+ // Test: Extract the 0 index values as StringArray (type conversion)
+ let field = Field::new("typed_value", DataType::Utf8, false);
+ let options = GetOptions::new_with_path(VariantPath::from(0))
+ .with_as_type(Some(FieldRef::from(field)));
+ let result = variant_get(&array, options).unwrap();
+
+ // Should get StringArray
+ let expected: ArrayRef = Arc::new(StringArray::from(vec![Some("comedy"), Some("drama")]));
+ assert_eq!(&result, &expected);
+ }
+ /// Helper function to create a shredded variant array representing lists
+ ///
+ /// This creates an array that represents:
+ /// Row 0: ["comedy", "drama"] ([0] is shredded, [1] is shredded - perfectly shredded)
+ /// Row 1: ["horror", null] ([0] is shredded, [1] is binary null - partially shredded)
+ /// Row 2: ["comedy", "drama", "romance"] (perfectly shredded)
+ ///
+ /// The physical layout follows the shredding spec where:
+ /// - metadata: contains list metadata
+ /// - typed_value: StructArray with 0 index value
+ /// - value: contains fallback for
+ fn shredded_list_variant_array() -> ArrayRef {
+ // Create the base metadata for lists
+
+ // Could add this as an api for VariantList, like VariantList::from()
+ fn build_list_metadata(vector: Vec<Variant>) -> (Vec<u8>, Vec<u8>) {
+ let mut builder = parquet_variant::VariantBuilder::new();
+ let mut list = builder.new_list();
+ for value in vector {
+ list.append_value(value);
+ }
+ list.finish();
+ builder.finish()
+ }
+ let (metadata1, _) =
+ build_list_metadata(vec![Variant::String("comedy"), Variant::String("drama")]);
Review Comment:
Thanks, I didn't realize that variant metadata only matters or variant values that contain objects before reading your PR #8359.
That means that for this specific example with non-nested lists that are not containing objects within them it's redundant.
--
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] [WIP] Support Shredded Lists/Array in `variant_get` [arrow-rs]
Posted by "sdf-jkl (via GitHub)" <gi...@apache.org>.
sdf-jkl commented on PR #8354:
URL: https://github.com/apache/arrow-rs/pull/8354#issuecomment-3325668781
Hey @scovich I see that your current implementation of `follow_shredded_path_element` for `VariantPathElement::Field` when following the shredded path is successful, it returns a `ShreddedPathStep::Success(field.shredding_state())` that holds a `ShreddingState::Typed` that holds a reference to the `typed_value` array. (That we later use for the next steps)
My question is: does `ShreddedPathStep::Success()` necessarily have to require the input `ShreddingState` to be a reference?
The reason I am asking is that since we use the output of `follow_shredded_path_element` to get the values from the shredded `VariantArray`, shouldn't we be free to drop the outer array once we extract the relevant `typed_value`?
The only way to work with list arrays I came up with so far, is to build new arrays with `arrow_select::take`, combining the path index and `GenericListArray` offsets.
But by using this method we create new arrays within the scope of the function and can't use a reference to the array in the `ShreddedPathStep::Success`.
Should we instead look for another way to represent a resulting array consisting of slices instead?
--
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] Support Shredded Lists/Array in `variant_get` [arrow-rs]
Posted by "sdf-jkl (via GitHub)" <gi...@apache.org>.
sdf-jkl commented on code in PR #8354:
URL: https://github.com/apache/arrow-rs/pull/8354#discussion_r2995850167
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -96,15 +171,33 @@ pub(crate) fn follow_shredded_path_element<'a>(
))
})?;
- let state = BorrowedShreddingState::try_from(struct_array)?;
+ let state = ShreddingState::try_from(struct_array)?;
Ok(ShreddedPathStep::Success(state))
}
- VariantPathElement::Index { .. } => {
- // TODO: Support array indexing. Among other things, it will require slicing not
- // only the array we have here, but also the corresponding metadata and null masks.
- Err(ArrowError::NotYetImplemented(
- "Pathing into shredded variant array index".into(),
- ))
+ VariantPathElement::Index { index } => {
+ let state = match typed_value.data_type() {
+ DataType::List(_) => take_list_like_index_as_shredding_state::<
+ GenericListArray<i32>,
+ >(typed_value.as_ref(), *index)?,
Review Comment:
Making it `take_list_index_state` works:
```rust
VariantPathElement::Index { index } => {
let state = match typed_value.data_type() {
DataType::List(_) => {
take_list_index_state::<GenericListArray<i32>>(typed_value.as_ref(), *index)?
}
DataType::LargeList(_) => {
take_list_index_state::<GenericListArray<i64>>(typed_value.as_ref(), *index)?
}
DataType::ListView(_) => take_list_index_state::<GenericListViewArray<i32>>(
typed_value.as_ref(),
*index,
)?,
DataType::LargeListView(_) => take_list_index_state::<GenericListViewArray<i64>>(
typed_value.as_ref(),
*index,
)?,
```
--
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] [WIP] Support Shredded Lists/Array in `variant_get` [arrow-rs]
Posted by "sdf-jkl (via GitHub)" <gi...@apache.org>.
sdf-jkl commented on code in PR #8354:
URL: https://github.com/apache/arrow-rs/pull/8354#discussion_r2836439402
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -97,14 +98,67 @@ pub(crate) fn follow_shredded_path_element<'a>(
})?;
let state = BorrowedShreddingState::try_from(struct_array)?;
- Ok(ShreddedPathStep::Success(state))
+ Ok(ShreddedPathStep::Success(state.into()))
}
- VariantPathElement::Index { .. } => {
+ VariantPathElement::Index { index } => {
// TODO: Support array indexing. Among other things, it will require slicing not
// only the array we have here, but also the corresponding metadata and null masks.
- Err(ArrowError::NotYetImplemented(
- "Pathing into shredded variant array index".into(),
- ))
+ let Some(list_array) = typed_value.as_any().downcast_ref::<GenericListArray<i32>>()
Review Comment:
I filed an issue to support other list types: #9455
--
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] [WIP] Support Shredded Lists/Array in `variant_get` [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on code in PR #8354:
URL: https://github.com/apache/arrow-rs/pull/8354#discussion_r2842790603
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -42,16 +45,84 @@ pub(crate) enum ShreddedPathStep<'a> {
NotShredded,
}
+fn take_list_index_as_shredding_state<O: OffsetSizeTrait>(
+ list_array: &GenericListArray<O>,
+ index: usize,
+ cast_options: &CastOptions,
+) -> Result<Option<ShreddingState>> {
+ let offsets = list_array.offsets();
+ let values = list_array.values();
+
+ let Some(struct_array) = values.as_any().downcast_ref::<StructArray>() else {
+ return Ok(None);
+ };
+
+ let value_array = struct_array.column_by_name("value");
+ let typed_array = struct_array.column_by_name("typed_value");
+
+ // If list elements have neither typed nor fallback value, this path step is missing.
+ if value_array.is_none() && typed_array.is_none() {
+ return Ok(None);
+ }
Review Comment:
How does this relate to the VariantArray helper? (should we use it)?
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -42,16 +45,84 @@ pub(crate) enum ShreddedPathStep<'a> {
NotShredded,
}
+fn take_list_index_as_shredding_state<O: OffsetSizeTrait>(
+ list_array: &GenericListArray<O>,
+ index: usize,
+ cast_options: &CastOptions,
+) -> Result<Option<ShreddingState>> {
+ let offsets = list_array.offsets();
+ let values = list_array.values();
+
+ let Some(struct_array) = values.as_any().downcast_ref::<StructArray>() else {
+ return Ok(None);
+ };
+
+ let value_array = struct_array.column_by_name("value");
+ let typed_array = struct_array.column_by_name("typed_value");
+
+ // If list elements have neither typed nor fallback value, this path step is missing.
+ if value_array.is_none() && typed_array.is_none() {
+ return Ok(None);
+ }
+
+ let mut take_indices = Vec::with_capacity(list_array.len());
+ for row in 0..list_array.len() {
+ let start = offsets[row].as_usize();
+ let end = offsets[row + 1].as_usize();
+ let len = end - start;
+
+ if index < len {
+ let absolute_index = start.checked_add(index).ok_or_else(|| {
+ ArrowError::ComputeError("List index overflow while building take indices".into())
+ })?;
+ let absolute_index = u64::try_from(absolute_index)
+ .map_err(|_| ArrowError::ComputeError("List index does not fit into u64".into()))?;
+ take_indices.push(Some(absolute_index));
+ } else if cast_options.safe {
+ take_indices.push(None);
+ } else {
+ return Err(ArrowError::CastError(format!(
+ "Cannot access index '{}' for row {} with list length {}",
+ index, row, len
+ )));
+ }
+ }
+
+ let index_array = UInt64Array::from(take_indices);
+
+ // Gather both typed and fallback values at the requested element index.
+ let taken_value = value_array
+ .map(|value| take(value, &index_array, None))
Review Comment:
aside: TIL about [take](https://docs.rs/arrow/latest/arrow/compute/fn.take.html). Very helpful here.
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -42,16 +45,84 @@ pub(crate) enum ShreddedPathStep<'a> {
NotShredded,
}
+fn take_list_index_as_shredding_state<O: OffsetSizeTrait>(
+ list_array: &GenericListArray<O>,
+ index: usize,
+ cast_options: &CastOptions,
+) -> Result<Option<ShreddingState>> {
+ let offsets = list_array.offsets();
+ let values = list_array.values();
+
+ let Some(struct_array) = values.as_any().downcast_ref::<StructArray>() else {
+ return Ok(None);
+ };
+
+ let value_array = struct_array.column_by_name("value");
+ let typed_array = struct_array.column_by_name("typed_value");
+
+ // If list elements have neither typed nor fallback value, this path step is missing.
+ if value_array.is_none() && typed_array.is_none() {
+ return Ok(None);
+ }
+
+ let mut take_indices = Vec::with_capacity(list_array.len());
+ for row in 0..list_array.len() {
+ let start = offsets[row].as_usize();
+ let end = offsets[row + 1].as_usize();
+ let len = end - start;
+
+ if index < len {
+ let absolute_index = start.checked_add(index).ok_or_else(|| {
+ ArrowError::ComputeError("List index overflow while building take indices".into())
+ })?;
+ let absolute_index = u64::try_from(absolute_index)
+ .map_err(|_| ArrowError::ComputeError("List index does not fit into u64".into()))?;
+ take_indices.push(Some(absolute_index));
+ } else if cast_options.safe {
+ take_indices.push(None);
+ } else {
+ return Err(ArrowError::CastError(format!(
+ "Cannot access index '{}' for row {} with list length {}",
+ index, row, len
+ )));
Review Comment:
We need to decide what the out of bounds semantics should be. For example, spark just returns NULL.
By way of comparison, spark and arrow-rs both return NULL for non-existent struct fields, which could be argued as analogous. Or maybe it's considered different and we want the error. Or maybe missing struct fields are also handled wrong?
(I'm comfortable with following spark semantics, but would love to hear others' thoughts)
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -96,15 +167,35 @@ pub(crate) fn follow_shredded_path_element<'a>(
))
})?;
- let state = BorrowedShreddingState::try_from(struct_array)?;
+ let state = ShreddingState::try_from(struct_array)?;
Ok(ShreddedPathStep::Success(state))
}
- VariantPathElement::Index { .. } => {
- // TODO: Support array indexing. Among other things, it will require slicing not
- // only the array we have here, but also the corresponding metadata and null masks.
- Err(ArrowError::NotYetImplemented(
- "Pathing into shredded variant array index".into(),
- ))
+ VariantPathElement::Index { index } => {
+ let state = if let Some(list_array) =
+ typed_value.as_any().downcast_ref::<GenericListArray<i32>>()
+ {
+ take_list_index_as_shredding_state(list_array, *index, cast_options)?
+ } else if let Some(list_array) =
+ typed_value.as_any().downcast_ref::<GenericListArray<i64>>()
+ {
+ take_list_index_as_shredding_state(list_array, *index, cast_options)?
+ } else {
+ // Downcast failure - if strict cast options are enabled, this should be an error
+ if !cast_options.safe {
+ return Err(ArrowError::CastError(format!(
Review Comment:
What about list view arrays?
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -96,15 +167,35 @@ pub(crate) fn follow_shredded_path_element<'a>(
))
})?;
- let state = BorrowedShreddingState::try_from(struct_array)?;
+ let state = ShreddingState::try_from(struct_array)?;
Ok(ShreddedPathStep::Success(state))
}
- VariantPathElement::Index { .. } => {
- // TODO: Support array indexing. Among other things, it will require slicing not
- // only the array we have here, but also the corresponding metadata and null masks.
- Err(ArrowError::NotYetImplemented(
- "Pathing into shredded variant array index".into(),
- ))
+ VariantPathElement::Index { index } => {
+ let state = if let Some(list_array) =
+ typed_value.as_any().downcast_ref::<GenericListArray<i32>>()
+ {
+ take_list_index_as_shredding_state(list_array, *index, cast_options)?
+ } else if let Some(list_array) =
+ typed_value.as_any().downcast_ref::<GenericListArray<i64>>()
+ {
+ take_list_index_as_shredding_state(list_array, *index, cast_options)?
+ } else {
+ // Downcast failure - if strict cast options are enabled, this should be an error
+ if !cast_options.safe {
+ return Err(ArrowError::CastError(format!(
Review Comment:
Also, why not follow the same else-if chain as above?
```rust
} else if cast_options.safe {
return Ok(...);
} else {
return Err(...);
}
```
--
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] [WIP] Support Shredded Lists/Array in `variant_get` [arrow-rs]
Posted by "sdf-jkl (via GitHub)" <gi...@apache.org>.
sdf-jkl commented on code in PR #8354:
URL: https://github.com/apache/arrow-rs/pull/8354#discussion_r2843510814
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -42,16 +45,84 @@ pub(crate) enum ShreddedPathStep<'a> {
NotShredded,
}
+fn take_list_index_as_shredding_state<O: OffsetSizeTrait>(
+ list_array: &GenericListArray<O>,
+ index: usize,
+ cast_options: &CastOptions,
+) -> Result<Option<ShreddingState>> {
+ let offsets = list_array.offsets();
+ let values = list_array.values();
+
+ let Some(struct_array) = values.as_any().downcast_ref::<StructArray>() else {
+ return Ok(None);
+ };
+
+ let value_array = struct_array.column_by_name("value");
+ let typed_array = struct_array.column_by_name("typed_value");
+
+ // If list elements have neither typed nor fallback value, this path step is missing.
+ if value_array.is_none() && typed_array.is_none() {
+ return Ok(None);
+ }
+
+ let mut take_indices = Vec::with_capacity(list_array.len());
+ for row in 0..list_array.len() {
+ let start = offsets[row].as_usize();
+ let end = offsets[row + 1].as_usize();
+ let len = end - start;
+
+ if index < len {
+ let absolute_index = start.checked_add(index).ok_or_else(|| {
+ ArrowError::ComputeError("List index overflow while building take indices".into())
+ })?;
+ let absolute_index = u64::try_from(absolute_index)
+ .map_err(|_| ArrowError::ComputeError("List index does not fit into u64".into()))?;
+ take_indices.push(Some(absolute_index));
+ } else if cast_options.safe {
+ take_indices.push(None);
+ } else {
+ return Err(ArrowError::CastError(format!(
+ "Cannot access index '{}' for row {} with list length {}",
+ index, row, len
+ )));
Review Comment:
I support following the spark semantics too
--
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] [WIP] Support Shredded Lists/Array in `variant_get` [arrow-rs]
Posted by "sdf-jkl (via GitHub)" <gi...@apache.org>.
sdf-jkl commented on code in PR #8354:
URL: https://github.com/apache/arrow-rs/pull/8354#discussion_r2835611344
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -1646,7 +1701,96 @@ mod test {
let expected: ArrayRef = Arc::new(Int32Array::from(vec![Some(1), Some(42)]));
assert_eq!(&result, &expected);
}
+ /// This test manually constructs a shredded variant array representing lists
+ /// like ["comedy", "drama"] and ["horror", 123]
+ /// as VariantArray using variant_get.
+ #[test]
+ fn test_shredded_list_index_access() {
+ let array = shredded_list_variant_array();
+ // Test: Extract the 0 index field as VariantArray first
+ let options = GetOptions::new_with_path(VariantPath::from(0));
+ let result = variant_get(&array, options).unwrap();
+ let result_variant = VariantArray::try_new(&result).unwrap();
+ assert_eq!(result_variant.len(), 2);
+ // Row 0: expect 0 index = "comedy"
+ assert_eq!(result_variant.value(0), Variant::from("comedy"));
Review Comment:
https://github.com/apache/arrow-rs/pull/8354/commits/cecd39f8d6020c4569e157c69c30bafbd48dbb62
--
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] [WIP] Support Shredded Lists/Array in `variant_get` [arrow-rs]
Posted by "sdf-jkl (via GitHub)" <gi...@apache.org>.
sdf-jkl commented on code in PR #8354:
URL: https://github.com/apache/arrow-rs/pull/8354#discussion_r2356741389
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -1122,64 +1121,109 @@ mod test {
let result = variant_get(&array, options).unwrap();
// Should get StringArray
- let expected: ArrayRef = Arc::new(StringArray::from(vec![Some("comedy"), Some("drama")]));
+ let expected: ArrayRef =
+ Arc::new(StringArray::from(vec![Some("comedy"), None, Some("drama")]));
assert_eq!(&result, &expected);
}
/// Helper function to create a shredded variant array representing lists
///
/// This creates an array that represents:
/// Row 0: ["comedy", "drama"] ([0] is shredded, [1] is shredded - perfectly shredded)
- /// Row 1: ["horror", null] ([0] is shredded, [1] is binary null - partially shredded)
- /// Row 2: ["comedy", "drama", "romance"] (perfectly shredded)
+ /// Row 1: ["horror", 123] ([0] is shredded, [1] is int - partially shredded)
///
/// The physical layout follows the shredding spec where:
/// - metadata: contains list metadata
/// - typed_value: StructArray with 0 index value
/// - value: contains fallback for
fn shredded_list_variant_array() -> ArrayRef {
- // Create the base metadata for lists
-
- // Could add this as an api for VariantList, like VariantList::from()
- fn build_list_metadata(vector: Vec<Variant>) -> (Vec<u8>, Vec<u8>) {
- let mut builder = parquet_variant::VariantBuilder::new();
- let mut list = builder.new_list();
- for value in vector {
- list.append_value(value);
- }
- list.finish();
- builder.finish()
- }
- let (metadata1, _) =
- build_list_metadata(vec![Variant::String("comedy"), Variant::String("drama")]);
+ // Create metadata array
+ let metadata_array =
+ BinaryViewArray::from_iter_values(std::iter::repeat_n(EMPTY_VARIANT_METADATA_BYTES, 2));
- let (metadata2, _) = build_list_metadata(vec![Variant::String("horror"), Variant::Null]);
+ // Building the typed_value ListArray
- let (metadata3, _) = build_list_metadata(vec![
- Variant::String("comedy"),
- Variant::String("drama"),
- Variant::String("romance"),
+ // Need a StructBuilder to create a ListBuilder
+ let fields = Fields::from(vec![
+ Field::new("value", DataType::Binary, true),
+ Field::new("typed_value", DataType::Utf8, true),
]);
+ let field_builders = vec![
+ make_builder(&DataType::Binary, 4),
+ make_builder(&DataType::Utf8, 4),
+ ];
+ let struct_builder = StructBuilder::new(fields, field_builders);
+
+ let mut builder = GenericListBuilder::<i32, StructBuilder>::new(struct_builder);
+
+ // Row 0 index 0
+ builder
+ .values()
+ .field_builder::<BinaryBuilder>(0)
+ .unwrap()
+ .append_null();
+ builder
+ .values()
+ .field_builder::<StringBuilder>(1)
+ .unwrap()
+ .append_value("comedy");
+ builder.values().append(true);
+
+ // Row 0 index 1
+ builder
+ .values()
+ .field_builder::<BinaryBuilder>(0)
+ .unwrap()
+ .append_null();
+ builder
+ .values()
+ .field_builder::<StringBuilder>(1)
+ .unwrap()
+ .append_value("drama");
+ builder.values().append(true);
+
+ // Next row
+ builder.append(true);
+
+ // Row 1 index 0
+ builder
+ .values()
+ .field_builder::<BinaryBuilder>(0)
+ .unwrap()
+ .append_null();
+ builder
+ .values()
+ .field_builder::<StringBuilder>(1)
+ .unwrap()
+ .append_value("horror");
+ builder.values().append(true);
+
+ // Row 1 index 1
+ let mut variant_builder = VariantBuilder::new();
+ variant_builder.append_value(123i32); // <------ couldn't find the right way to do it, used this as placeholder for binary
+ let (_, value) = variant_builder.finish();
+
+ builder
+ .values()
+ .field_builder::<BinaryBuilder>(0)
+ .unwrap()
+ .append_value(value);
Review Comment:
I couldn't figure out how to convert a Variant:Int32(123) into binary, so instead I appended the binary Variant value without the metadata.
--
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] [WIP] Support Shredded Lists/Array in `variant_get` [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on code in PR #8354:
URL: https://github.com/apache/arrow-rs/pull/8354#discussion_r2357565134
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -1122,64 +1121,109 @@ mod test {
let result = variant_get(&array, options).unwrap();
// Should get StringArray
- let expected: ArrayRef = Arc::new(StringArray::from(vec![Some("comedy"), Some("drama")]));
+ let expected: ArrayRef =
+ Arc::new(StringArray::from(vec![Some("comedy"), None, Some("drama")]));
assert_eq!(&result, &expected);
}
/// Helper function to create a shredded variant array representing lists
///
/// This creates an array that represents:
/// Row 0: ["comedy", "drama"] ([0] is shredded, [1] is shredded - perfectly shredded)
- /// Row 1: ["horror", null] ([0] is shredded, [1] is binary null - partially shredded)
- /// Row 2: ["comedy", "drama", "romance"] (perfectly shredded)
+ /// Row 1: ["horror", 123] ([0] is shredded, [1] is int - partially shredded)
///
/// The physical layout follows the shredding spec where:
/// - metadata: contains list metadata
/// - typed_value: StructArray with 0 index value
/// - value: contains fallback for
fn shredded_list_variant_array() -> ArrayRef {
- // Create the base metadata for lists
-
- // Could add this as an api for VariantList, like VariantList::from()
- fn build_list_metadata(vector: Vec<Variant>) -> (Vec<u8>, Vec<u8>) {
- let mut builder = parquet_variant::VariantBuilder::new();
- let mut list = builder.new_list();
- for value in vector {
- list.append_value(value);
- }
- list.finish();
- builder.finish()
- }
- let (metadata1, _) =
- build_list_metadata(vec![Variant::String("comedy"), Variant::String("drama")]);
+ // Create metadata array
+ let metadata_array =
+ BinaryViewArray::from_iter_values(std::iter::repeat_n(EMPTY_VARIANT_METADATA_BYTES, 2));
- let (metadata2, _) = build_list_metadata(vec![Variant::String("horror"), Variant::Null]);
+ // Building the typed_value ListArray
- let (metadata3, _) = build_list_metadata(vec![
- Variant::String("comedy"),
- Variant::String("drama"),
- Variant::String("romance"),
+ // Need a StructBuilder to create a ListBuilder
+ let fields = Fields::from(vec![
+ Field::new("value", DataType::Binary, true),
+ Field::new("typed_value", DataType::Utf8, true),
]);
+ let field_builders = vec![
+ make_builder(&DataType::Binary, 4),
+ make_builder(&DataType::Utf8, 4),
+ ];
+ let struct_builder = StructBuilder::new(fields, field_builders);
+
+ let mut builder = GenericListBuilder::<i32, StructBuilder>::new(struct_builder);
+
+ // Row 0 index 0
+ builder
+ .values()
+ .field_builder::<BinaryBuilder>(0)
+ .unwrap()
+ .append_null();
+ builder
+ .values()
+ .field_builder::<StringBuilder>(1)
+ .unwrap()
+ .append_value("comedy");
+ builder.values().append(true);
+
+ // Row 0 index 1
+ builder
+ .values()
+ .field_builder::<BinaryBuilder>(0)
+ .unwrap()
+ .append_null();
+ builder
+ .values()
+ .field_builder::<StringBuilder>(1)
+ .unwrap()
+ .append_value("drama");
+ builder.values().append(true);
+
+ // Next row
+ builder.append(true);
+
+ // Row 1 index 0
+ builder
+ .values()
+ .field_builder::<BinaryBuilder>(0)
+ .unwrap()
+ .append_null();
+ builder
+ .values()
+ .field_builder::<StringBuilder>(1)
+ .unwrap()
+ .append_value("horror");
+ builder.values().append(true);
+
+ // Row 1 index 1
+ let mut variant_builder = VariantBuilder::new();
+ variant_builder.append_value(123i32); // <------ couldn't find the right way to do it, used this as placeholder for binary
+ let (_, value) = variant_builder.finish();
+
+ builder
+ .values()
+ .field_builder::<BinaryBuilder>(0)
+ .unwrap()
+ .append_value(value);
Review Comment:
NOTE: The metadata column still needs a metadata entry for that row, else the different columns will have disagreeing row counts. Can append `EMPTY_VARIANT_METADATA_BYTES` if the value is primitive.
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -1122,64 +1121,109 @@ mod test {
let result = variant_get(&array, options).unwrap();
// Should get StringArray
- let expected: ArrayRef = Arc::new(StringArray::from(vec![Some("comedy"), Some("drama")]));
+ let expected: ArrayRef =
+ Arc::new(StringArray::from(vec![Some("comedy"), None, Some("drama")]));
assert_eq!(&result, &expected);
}
/// Helper function to create a shredded variant array representing lists
///
/// This creates an array that represents:
/// Row 0: ["comedy", "drama"] ([0] is shredded, [1] is shredded - perfectly shredded)
- /// Row 1: ["horror", null] ([0] is shredded, [1] is binary null - partially shredded)
- /// Row 2: ["comedy", "drama", "romance"] (perfectly shredded)
+ /// Row 1: ["horror", 123] ([0] is shredded, [1] is int - partially shredded)
///
/// The physical layout follows the shredding spec where:
/// - metadata: contains list metadata
/// - typed_value: StructArray with 0 index value
/// - value: contains fallback for
fn shredded_list_variant_array() -> ArrayRef {
- // Create the base metadata for lists
-
- // Could add this as an api for VariantList, like VariantList::from()
- fn build_list_metadata(vector: Vec<Variant>) -> (Vec<u8>, Vec<u8>) {
- let mut builder = parquet_variant::VariantBuilder::new();
- let mut list = builder.new_list();
- for value in vector {
- list.append_value(value);
- }
- list.finish();
- builder.finish()
- }
- let (metadata1, _) =
- build_list_metadata(vec![Variant::String("comedy"), Variant::String("drama")]);
+ // Create metadata array
+ let metadata_array =
+ BinaryViewArray::from_iter_values(std::iter::repeat_n(EMPTY_VARIANT_METADATA_BYTES, 2));
- let (metadata2, _) = build_list_metadata(vec![Variant::String("horror"), Variant::Null]);
+ // Building the typed_value ListArray
- let (metadata3, _) = build_list_metadata(vec![
- Variant::String("comedy"),
- Variant::String("drama"),
- Variant::String("romance"),
+ // Need a StructBuilder to create a ListBuilder
+ let fields = Fields::from(vec![
+ Field::new("value", DataType::Binary, true),
+ Field::new("typed_value", DataType::Utf8, true),
]);
+ let field_builders = vec![
+ make_builder(&DataType::Binary, 4),
+ make_builder(&DataType::Utf8, 4),
+ ];
+ let struct_builder = StructBuilder::new(fields, field_builders);
+
+ let mut builder = GenericListBuilder::<i32, StructBuilder>::new(struct_builder);
+
+ // Row 0 index 0
+ builder
+ .values()
+ .field_builder::<BinaryBuilder>(0)
+ .unwrap()
+ .append_null();
+ builder
+ .values()
+ .field_builder::<StringBuilder>(1)
+ .unwrap()
+ .append_value("comedy");
+ builder.values().append(true);
+
+ // Row 0 index 1
+ builder
+ .values()
+ .field_builder::<BinaryBuilder>(0)
+ .unwrap()
+ .append_null();
+ builder
+ .values()
+ .field_builder::<StringBuilder>(1)
+ .unwrap()
+ .append_value("drama");
+ builder.values().append(true);
+
+ // Next row
+ builder.append(true);
+
+ // Row 1 index 0
+ builder
+ .values()
+ .field_builder::<BinaryBuilder>(0)
+ .unwrap()
+ .append_null();
+ builder
+ .values()
+ .field_builder::<StringBuilder>(1)
+ .unwrap()
+ .append_value("horror");
+ builder.values().append(true);
+
+ // Row 1 index 1
+ let mut variant_builder = VariantBuilder::new();
+ variant_builder.append_value(123i32); // <------ couldn't find the right way to do it, used this as placeholder for binary
+ let (_, value) = variant_builder.finish();
+
+ builder
+ .values()
+ .field_builder::<BinaryBuilder>(0)
+ .unwrap()
+ .append_value(value);
Review Comment:
NOTE: The metadata column still needs a metadata entry for that row, else the different columns will have disagreeing row counts. Can append `EMPTY_VARIANT_METADATA_BYTES` if the corresponding value is primitive.
--
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] [WIP] Support Shredded Lists/Array in `variant_get` [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on code in PR #8354:
URL: https://github.com/apache/arrow-rs/pull/8354#discussion_r2357562447
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -1122,64 +1121,109 @@ mod test {
let result = variant_get(&array, options).unwrap();
// Should get StringArray
- let expected: ArrayRef = Arc::new(StringArray::from(vec![Some("comedy"), Some("drama")]));
+ let expected: ArrayRef =
+ Arc::new(StringArray::from(vec![Some("comedy"), None, Some("drama")]));
assert_eq!(&result, &expected);
}
/// Helper function to create a shredded variant array representing lists
///
/// This creates an array that represents:
/// Row 0: ["comedy", "drama"] ([0] is shredded, [1] is shredded - perfectly shredded)
- /// Row 1: ["horror", null] ([0] is shredded, [1] is binary null - partially shredded)
- /// Row 2: ["comedy", "drama", "romance"] (perfectly shredded)
+ /// Row 1: ["horror", 123] ([0] is shredded, [1] is int - partially shredded)
///
/// The physical layout follows the shredding spec where:
/// - metadata: contains list metadata
/// - typed_value: StructArray with 0 index value
/// - value: contains fallback for
fn shredded_list_variant_array() -> ArrayRef {
- // Create the base metadata for lists
-
- // Could add this as an api for VariantList, like VariantList::from()
- fn build_list_metadata(vector: Vec<Variant>) -> (Vec<u8>, Vec<u8>) {
- let mut builder = parquet_variant::VariantBuilder::new();
- let mut list = builder.new_list();
- for value in vector {
- list.append_value(value);
- }
- list.finish();
- builder.finish()
- }
- let (metadata1, _) =
- build_list_metadata(vec![Variant::String("comedy"), Variant::String("drama")]);
+ // Create metadata array
+ let metadata_array =
+ BinaryViewArray::from_iter_values(std::iter::repeat_n(EMPTY_VARIANT_METADATA_BYTES, 2));
- let (metadata2, _) = build_list_metadata(vec![Variant::String("horror"), Variant::Null]);
+ // Building the typed_value ListArray
- let (metadata3, _) = build_list_metadata(vec![
- Variant::String("comedy"),
- Variant::String("drama"),
- Variant::String("romance"),
+ // Need a StructBuilder to create a ListBuilder
+ let fields = Fields::from(vec![
+ Field::new("value", DataType::Binary, true),
+ Field::new("typed_value", DataType::Utf8, true),
]);
+ let field_builders = vec![
+ make_builder(&DataType::Binary, 4),
+ make_builder(&DataType::Utf8, 4),
+ ];
+ let struct_builder = StructBuilder::new(fields, field_builders);
+
+ let mut builder = GenericListBuilder::<i32, StructBuilder>::new(struct_builder);
+
+ // Row 0 index 0
+ builder
+ .values()
+ .field_builder::<BinaryBuilder>(0)
+ .unwrap()
+ .append_null();
+ builder
+ .values()
+ .field_builder::<StringBuilder>(1)
+ .unwrap()
+ .append_value("comedy");
+ builder.values().append(true);
+
+ // Row 0 index 1
+ builder
+ .values()
+ .field_builder::<BinaryBuilder>(0)
+ .unwrap()
+ .append_null();
+ builder
+ .values()
+ .field_builder::<StringBuilder>(1)
+ .unwrap()
+ .append_value("drama");
+ builder.values().append(true);
+
+ // Next row
+ builder.append(true);
+
+ // Row 1 index 0
+ builder
+ .values()
+ .field_builder::<BinaryBuilder>(0)
+ .unwrap()
+ .append_null();
+ builder
+ .values()
+ .field_builder::<StringBuilder>(1)
+ .unwrap()
+ .append_value("horror");
+ builder.values().append(true);
+
+ // Row 1 index 1
+ let mut variant_builder = VariantBuilder::new();
+ variant_builder.append_value(123i32); // <------ couldn't find the right way to do it, used this as placeholder for binary
+ let (_, value) = variant_builder.finish();
+
+ builder
+ .values()
+ .field_builder::<BinaryBuilder>(0)
+ .unwrap()
+ .append_value(value);
Review Comment:
Once https://github.com/apache/arrow-rs/pull/8360 merges (hopefully today), you can use a `VariantValueArrayBuilder` to create the values column. That requires keeping the builders separate and then using a `StructArrayBuilder` to assemble them into the final struct.
See e.g. [VariantToBinaryVariantArrowRowBuilder](https://github.com/apache/arrow-rs/pull/8360/files#diff-5b1dcb31d6afd99fdac153e6e8d9cdec8aac81c0dfdcc13729de95951a409874R257-R288) in that same PR
--
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] [WIP] Support Shredded Lists/Array in `variant_get` [arrow-rs]
Posted by "sdf-jkl (via GitHub)" <gi...@apache.org>.
sdf-jkl commented on PR #8354:
URL: https://github.com/apache/arrow-rs/pull/8354#issuecomment-3336028976
Hey @scovich I made it work for a one of the simple tests and it doesn't go through with the second one because Variant to Arrow does not support utf8 yet.
Do we have an issue tracking variant_to_arrow types support? If not, I can make one.
--
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] [WIP] Support Shredded Lists/Array in `variant_get` [arrow-rs]
Posted by "sdf-jkl (via GitHub)" <gi...@apache.org>.
sdf-jkl commented on code in PR #8354:
URL: https://github.com/apache/arrow-rs/pull/8354#discussion_r2608074262
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -97,14 +98,67 @@ pub(crate) fn follow_shredded_path_element<'a>(
})?;
let state = BorrowedShreddingState::try_from(struct_array)?;
- Ok(ShreddedPathStep::Success(state))
+ Ok(ShreddedPathStep::Success(state.into()))
}
- VariantPathElement::Index { .. } => {
+ VariantPathElement::Index { index } => {
// TODO: Support array indexing. Among other things, it will require slicing not
// only the array we have here, but also the corresponding metadata and null masks.
- Err(ArrowError::NotYetImplemented(
- "Pathing into shredded variant array index".into(),
- ))
+ let Some(list_array) = typed_value.as_any().downcast_ref::<GenericListArray<i32>>()
Review Comment:
Yes, I wanted to add two cases for i32 and i64 lists.
--
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] Support Shredded Lists/Array in `variant_get` [arrow-rs]
Posted by "sdf-jkl (via GitHub)" <gi...@apache.org>.
sdf-jkl commented on code in PR #8354:
URL: https://github.com/apache/arrow-rs/pull/8354#discussion_r2997380963
##########
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:
Or push this one as is and fix it later
--
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] [WIP] Support Shredded Lists/Array in `variant_get` [arrow-rs]
Posted by "sdf-jkl (via GitHub)" <gi...@apache.org>.
sdf-jkl commented on code in PR #8354:
URL: https://github.com/apache/arrow-rs/pull/8354#discussion_r2356737405
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -1122,64 +1121,109 @@ mod test {
let result = variant_get(&array, options).unwrap();
// Should get StringArray
- let expected: ArrayRef = Arc::new(StringArray::from(vec![Some("comedy"), Some("drama")]));
+ let expected: ArrayRef =
+ Arc::new(StringArray::from(vec![Some("comedy"), None, Some("drama")]));
assert_eq!(&result, &expected);
}
/// Helper function to create a shredded variant array representing lists
///
/// This creates an array that represents:
/// Row 0: ["comedy", "drama"] ([0] is shredded, [1] is shredded - perfectly shredded)
- /// Row 1: ["horror", null] ([0] is shredded, [1] is binary null - partially shredded)
- /// Row 2: ["comedy", "drama", "romance"] (perfectly shredded)
+ /// Row 1: ["horror", 123] ([0] is shredded, [1] is int - partially shredded)
///
/// The physical layout follows the shredding spec where:
/// - metadata: contains list metadata
/// - typed_value: StructArray with 0 index value
/// - value: contains fallback for
fn shredded_list_variant_array() -> ArrayRef {
- // Create the base metadata for lists
-
- // Could add this as an api for VariantList, like VariantList::from()
- fn build_list_metadata(vector: Vec<Variant>) -> (Vec<u8>, Vec<u8>) {
- let mut builder = parquet_variant::VariantBuilder::new();
- let mut list = builder.new_list();
- for value in vector {
- list.append_value(value);
- }
- list.finish();
- builder.finish()
- }
- let (metadata1, _) =
- build_list_metadata(vec![Variant::String("comedy"), Variant::String("drama")]);
+ // Create metadata array
+ let metadata_array =
+ BinaryViewArray::from_iter_values(std::iter::repeat_n(EMPTY_VARIANT_METADATA_BYTES, 2));
- let (metadata2, _) = build_list_metadata(vec![Variant::String("horror"), Variant::Null]);
+ // Building the typed_value ListArray
- let (metadata3, _) = build_list_metadata(vec![
- Variant::String("comedy"),
- Variant::String("drama"),
- Variant::String("romance"),
+ // Need a StructBuilder to create a ListBuilder
+ let fields = Fields::from(vec![
+ Field::new("value", DataType::Binary, true),
+ Field::new("typed_value", DataType::Utf8, true),
]);
+ let field_builders = vec![
+ make_builder(&DataType::Binary, 4),
+ make_builder(&DataType::Utf8, 4),
+ ];
+ let struct_builder = StructBuilder::new(fields, field_builders);
+
+ let mut builder = GenericListBuilder::<i32, StructBuilder>::new(struct_builder);
Review Comment:
I hope I got it right this time. Building a `GenericListArray` of `StructArrays` of `element` two fields.
--
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] [WIP] Support Shredded Lists/Array in `variant_get` [arrow-rs]
Posted by "sdf-jkl (via GitHub)" <gi...@apache.org>.
sdf-jkl commented on PR #8354:
URL: https://github.com/apache/arrow-rs/pull/8354#issuecomment-3312703599
> I'm not sure I understand how these unit tests will translate to `variant_get`?
Could you elaborate please?
--
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] Support Shredded Lists/Array in `variant_get` [arrow-rs]
Posted by "sdf-jkl (via GitHub)" <gi...@apache.org>.
sdf-jkl commented on code in PR #8354:
URL: https://github.com/apache/arrow-rs/pull/8354#discussion_r2874193911
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -96,15 +184,44 @@ pub(crate) fn follow_shredded_path_element<'a>(
))
})?;
- let state = BorrowedShreddingState::try_from(struct_array)?;
+ let state = ShreddingState::try_from(struct_array)?;
Ok(ShreddedPathStep::Success(state))
}
- VariantPathElement::Index { .. } => {
- // TODO: Support array indexing. Among other things, it will require slicing not
- // only the array we have here, but also the corresponding metadata and null masks.
- Err(ArrowError::NotYetImplemented(
- "Pathing into shredded variant array index".into(),
- ))
+ VariantPathElement::Index { index } => {
+ let state = if let Some(list_array) =
+ typed_value.as_any().downcast_ref::<GenericListArray<i32>>()
+ {
+ take_list_like_index_as_shredding_state(list_array, *index, cast_options)?
+ } else if let Some(list_array) =
+ typed_value.as_any().downcast_ref::<GenericListArray<i64>>()
+ {
+ take_list_like_index_as_shredding_state(list_array, *index, cast_options)?
+ } else if let Some(list_view_array) = typed_value
+ .as_any()
+ .downcast_ref::<GenericListViewArray<i32>>()
+ {
+ take_list_like_index_as_shredding_state(list_view_array, *index, cast_options)?
+ } else if let Some(list_view_array) = typed_value
+ .as_any()
+ .downcast_ref::<GenericListViewArray<i64>>()
+ {
+ take_list_like_index_as_shredding_state(list_view_array, *index, cast_options)?
+ } else if cast_options.safe {
Review Comment:
Thanks, fixed in https://github.com/apache/arrow-rs/pull/8354/commits/279b6347e92a3beeb046ea13678a7714e32b0acc
--
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] [WIP] Support Shredded Lists/Array in `variant_get` [arrow-rs]
Posted by "sdf-jkl (via GitHub)" <gi...@apache.org>.
sdf-jkl commented on PR #8354:
URL: https://github.com/apache/arrow-rs/pull/8354#issuecomment-3700598515
@liamzwbao taking over with #8082
--
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] [WIP] Support Shredded Lists/Array in `variant_get` [arrow-rs]
Posted by "sdf-jkl (via GitHub)" <gi...@apache.org>.
sdf-jkl closed pull request #8354: [WIP] Support Shredded Lists/Array in `variant_get`
URL: https://github.com/apache/arrow-rs/pull/8354
--
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] Support Shredded Lists/Array in `variant_get` [arrow-rs]
Posted by "sdf-jkl (via GitHub)" <gi...@apache.org>.
sdf-jkl commented on code in PR #8354:
URL: https://github.com/apache/arrow-rs/pull/8354#discussion_r2995322497
##########
parquet-variant/src/path.rs:
##########
@@ -346,5 +346,14 @@ mod tests {
err.to_string(),
"Parser error: Invalid token in bracket request: `abc`. Expected a quoted string or a number(e.g., `['field']` or `[123]`)"
);
+
+ // Out-of-range integer indexes are invalid path tokens.
+ let too_large_index = (usize::MAX as u128) + 1;
+ let err = VariantPath::try_from(format!("[{too_large_index}]").as_str()).unwrap_err();
+ assert!(
+ err.to_string()
+ .contains("Parser error: Invalid token in bracket request"),
+ "{err}"
+ );
Review Comment:
We have these two:
- https://github.com/apache/arrow-rs/pull/8354/changes#diff-9c960f865bc6facbaa5b8aa37bf6d918a2413799be4d7eaff5ddb421408b64feR1875 index out of bounds returns Null
- https://github.com/apache/arrow-rs/pull/8354/changes#diff-9c960f865bc6facbaa5b8aa37bf6d918a2413799be4d7eaff5ddb421408b64feR2797 index on non-list returns Null
##########
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:
`BorrowedShreddingState` is still used in unshred_variant.rs, but that's it:
https://github.com/apache/arrow-rs/blob/398962ec67bc777eca1c635c8ef01a9c634530eb/parquet-variant-compute/src/unshred_variant.rs#L172
--------
I tried using `Cow` before, but I was `Cow`ing the whole `ShreddingState`, which was unnecessary.
I switched to `.clone` because value: `Option<BinaryViewArray>` is almost zero copy:
```rust
pub type BinaryViewArray = GenericByteViewArray<BinaryViewType>;
// ...
pub struct GenericByteViewArray<T>
where
T: ByteViewType + ?Sized,
{
data_type: DataType, // 24 bytes
views: ScalarBuffer<u128>, // zero copy cloning
buffers: Arc<[Buffer]>, // zero copy cloning
phantom: PhantomData<T>, // zero sized
nulls: Option<NullBuffer>, // mostly zero copy cloning + 3usize
}
```
`Cow`ing just the `BinaryViewArray` should work.
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -96,15 +171,33 @@ pub(crate) fn follow_shredded_path_element<'a>(
))
})?;
- let state = BorrowedShreddingState::try_from(struct_array)?;
+ let state = ShreddingState::try_from(struct_array)?;
Ok(ShreddedPathStep::Success(state))
}
- VariantPathElement::Index { .. } => {
- // TODO: Support array indexing. Among other things, it will require slicing not
- // only the array we have here, but also the corresponding metadata and null masks.
- Err(ArrowError::NotYetImplemented(
- "Pathing into shredded variant array index".into(),
- ))
+ VariantPathElement::Index { index } => {
+ let state = match typed_value.data_type() {
+ DataType::List(_) => take_list_like_index_as_shredding_state::<
+ GenericListArray<i32>,
+ >(typed_value.as_ref(), *index)?,
Review Comment:
`take_list_like_index_state`?
Last one still won't fit đĸ
```rust
VariantPathElement::Index { index } => {
let state = match typed_value.data_type() {
DataType::List(_) => take_list_like_index_state::<GenericListArray<i32>>(
typed_value.as_ref(),
*index,
)?,
DataType::LargeList(_) => take_list_like_index_state::<GenericListArray<i64>>(
typed_value.as_ref(),
*index,
)?,
DataType::ListView(_) => take_list_like_index_state::<GenericListViewArray<i32>>(
typed_value.as_ref(),
*index,
)?,
DataType::LargeListView(_) => take_list_like_index_state::<
GenericListViewArray<i64>,
>(typed_value.as_ref(), *index)?,
_ => {
```
--
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] [WIP] Support Shredded Lists/Array in `variant_get` [arrow-rs]
Posted by "klion26 (via GitHub)" <gi...@apache.org>.
klion26 commented on code in PR #8354:
URL: https://github.com/apache/arrow-rs/pull/8354#discussion_r2609654290
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -97,14 +98,67 @@ pub(crate) fn follow_shredded_path_element<'a>(
})?;
let state = BorrowedShreddingState::try_from(struct_array)?;
- Ok(ShreddedPathStep::Success(state))
+ Ok(ShreddedPathStep::Success(state.into()))
}
- VariantPathElement::Index { .. } => {
+ VariantPathElement::Index { index } => {
// TODO: Support array indexing. Among other things, it will require slicing not
// only the array we have here, but also the corresponding metadata and null masks.
- Err(ArrowError::NotYetImplemented(
- "Pathing into shredded variant array index".into(),
- ))
+ let Some(list_array) = typed_value.as_any().downcast_ref::<GenericListArray<i32>>()
+ else {
+ // Downcast failure - if strict cast options are enabled, this should be an error
+ if !cast_options.safe {
+ return Err(ArrowError::CastError(format!(
+ "Cannot access index '{}' on non-list type: {}",
+ index,
+ typed_value.data_type()
+ )));
+ }
+ // With safe cast options, return NULL (missing_path_step)
+ return Ok(missing_path_step());
+ };
+
+ let offsets = list_array.offsets();
+ let values = list_array.values(); // This is a StructArray
+
+ let Some(struct_array) = values.as_any().downcast_ref::<StructArray>() else {
+ return Ok(missing_path_step());
+ };
+
+ let Some(typed_array) = struct_array.column_by_name("typed_value") else {
+ return Ok(missing_path_step());
+ };
+
+ // Build the list of indices to take
+ let mut take_indices = Vec::with_capacity(list_array.len());
+ for i in 0..list_array.len() {
+ let start = offsets[i] as usize;
+ let end = offsets[i + 1] as usize;
+ let len = end - start;
+
+ if *index < len {
Review Comment:
Sorry for not describing it clearly.
The data`["comedy", "drama"], ["horro", 123]` translated into variant will be that
- `comedy`, `drama` and `horro` in the `typed_value` column,
- and `123` in the `value` column(it has an incompatible type).
Here, we retrieve all the results from the `typed_value` column(`take` in line 148), but `["hooro", 123](1)`(the second item in the list) here will return null(if `CastOptions::safe = true`) and `Err` (if `CastOptions::safe = false`) -- currently, we return null for both of the cases.
Seems there may be something more tricky here(maybe we need to have a design note for this), such as
- if the target_type here we request is not `list`/`struct` then, we can use the logic like here, and respect the `CastOptions::safe`
- If we need to handle [variant nesting](https://github.com/apache/parquet-format/blob/master/VariantShredding.md#nesting) here or somewhere else?
- Here, I don't have any answer yet. I'll try to find some time next week for this.
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -97,14 +98,67 @@ pub(crate) fn follow_shredded_path_element<'a>(
})?;
let state = BorrowedShreddingState::try_from(struct_array)?;
- Ok(ShreddedPathStep::Success(state))
+ Ok(ShreddedPathStep::Success(state.into()))
}
- VariantPathElement::Index { .. } => {
+ VariantPathElement::Index { index } => {
// TODO: Support array indexing. Among other things, it will require slicing not
// only the array we have here, but also the corresponding metadata and null masks.
- Err(ArrowError::NotYetImplemented(
- "Pathing into shredded variant array index".into(),
- ))
+ let Some(list_array) = typed_value.as_any().downcast_ref::<GenericListArray<i32>>()
+ else {
+ // Downcast failure - if strict cast options are enabled, this should be an error
+ if !cast_options.safe {
+ return Err(ArrowError::CastError(format!(
+ "Cannot access index '{}' on non-list type: {}",
+ index,
+ typed_value.data_type()
+ )));
+ }
+ // With safe cast options, return NULL (missing_path_step)
+ return Ok(missing_path_step());
+ };
+
+ let offsets = list_array.offsets();
+ let values = list_array.values(); // This is a StructArray
+
+ let Some(struct_array) = values.as_any().downcast_ref::<StructArray>() else {
+ return Ok(missing_path_step());
+ };
+
+ let Some(typed_array) = struct_array.column_by_name("typed_value") else {
+ return Ok(missing_path_step());
+ };
+
+ // Build the list of indices to take
+ let mut take_indices = Vec::with_capacity(list_array.len());
+ for i in 0..list_array.len() {
+ let start = offsets[i] as usize;
+ let end = offsets[i + 1] as usize;
+ let len = end - start;
+
+ if *index < len {
+ take_indices.push(Some((start + index) as u32));
+ } else {
+ take_indices.push(None);
Review Comment:
Yes, the list could have a different length, but do we need to return `Err` if there is an `OutOfBound` occurs (like the `CastOptions:safe=false`), so that the caller knows this.
--
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] Support Shredded Lists/Array in `variant_get` [arrow-rs]
Posted by "sdf-jkl (via GitHub)" <gi...@apache.org>.
sdf-jkl commented on code in PR #8354:
URL: https://github.com/apache/arrow-rs/pull/8354#discussion_r2854059033
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -96,15 +167,35 @@ pub(crate) fn follow_shredded_path_element<'a>(
))
})?;
- let state = BorrowedShreddingState::try_from(struct_array)?;
+ let state = ShreddingState::try_from(struct_array)?;
Ok(ShreddedPathStep::Success(state))
}
- VariantPathElement::Index { .. } => {
- // TODO: Support array indexing. Among other things, it will require slicing not
- // only the array we have here, but also the corresponding metadata and null masks.
- Err(ArrowError::NotYetImplemented(
- "Pathing into shredded variant array index".into(),
- ))
+ VariantPathElement::Index { index } => {
+ let state = if let Some(list_array) =
+ typed_value.as_any().downcast_ref::<GenericListArray<i32>>()
+ {
+ take_list_index_as_shredding_state(list_array, *index, cast_options)?
+ } else if let Some(list_array) =
+ typed_value.as_any().downcast_ref::<GenericListArray<i64>>()
+ {
+ take_list_index_as_shredding_state(list_array, *index, cast_options)?
+ } else {
+ // Downcast failure - if strict cast options are enabled, this should be an error
+ if !cast_options.safe {
+ return Err(ArrowError::CastError(format!(
Review Comment:
> Also, why not follow the same else-if chain as above?
>
> } else if cast_options.safe {
> return Ok(...);
> } else {
> return Err(...);
> }
Kept it similar to `VariantPathElement::Field` above
--
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] Support Shredded Lists/Array in `variant_get` [arrow-rs]
Posted by "sdf-jkl (via GitHub)" <gi...@apache.org>.
sdf-jkl commented on code in PR #8354:
URL: https://github.com/apache/arrow-rs/pull/8354#discussion_r2854059033
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -96,15 +167,35 @@ pub(crate) fn follow_shredded_path_element<'a>(
))
})?;
- let state = BorrowedShreddingState::try_from(struct_array)?;
+ let state = ShreddingState::try_from(struct_array)?;
Ok(ShreddedPathStep::Success(state))
}
- VariantPathElement::Index { .. } => {
- // TODO: Support array indexing. Among other things, it will require slicing not
- // only the array we have here, but also the corresponding metadata and null masks.
- Err(ArrowError::NotYetImplemented(
- "Pathing into shredded variant array index".into(),
- ))
+ VariantPathElement::Index { index } => {
+ let state = if let Some(list_array) =
+ typed_value.as_any().downcast_ref::<GenericListArray<i32>>()
+ {
+ take_list_index_as_shredding_state(list_array, *index, cast_options)?
+ } else if let Some(list_array) =
+ typed_value.as_any().downcast_ref::<GenericListArray<i64>>()
+ {
+ take_list_index_as_shredding_state(list_array, *index, cast_options)?
+ } else {
+ // Downcast failure - if strict cast options are enabled, this should be an error
+ if !cast_options.safe {
+ return Err(ArrowError::CastError(format!(
Review Comment:
> Also, why not follow the same else-if chain as above?
>
> } else if cast_options.safe {
> return Ok(...);
> } else {
> return Err(...);
> }
I kept it similar to `VariantPathElement::Field` above. Changed in https://github.com/apache/arrow-rs/pull/8354/commits/cf94d439ed910197c0780c73c8b42eabf16db51d
--
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] [WIP] Support Shredded Lists/Array in `variant_get` [arrow-rs]
Posted by "sdf-jkl (via GitHub)" <gi...@apache.org>.
sdf-jkl commented on PR #8354:
URL: https://github.com/apache/arrow-rs/pull/8354#issuecomment-3946986045
@scovich an early pass to check the test coverage should do.
Once the test coverage is complete and the code passes, it will be `Ready for review`
--
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] Support Shredded Lists/Array in `variant_get` [arrow-rs]
Posted by "sdf-jkl (via GitHub)" <gi...@apache.org>.
sdf-jkl commented on code in PR #8354:
URL: https://github.com/apache/arrow-rs/pull/8354#discussion_r3184722881
##########
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:
@scovich this should be ready without having to wait for #9791
--
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] Support Shredded Lists/Array in `variant_get` [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #8354:
URL: https://github.com/apache/arrow-rs/pull/8354#issuecomment-4104436183
> @alamb -- any opinions about supporting jsonpath semantics or not? Or ideas on who we should seek input from?
In my opinion supporing jsonpath sounds like a good idea in general
The usecase I know of (and have) for variant_get is to mirror spark's variant_get function: https://docs.databricks.com/aws/en/sql/language-manual/functions/variant_get
So having the arrow-rs implementation match sounds good too
--
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] Support Shredded Lists/Array in `variant_get` [arrow-rs]
Posted by "liamzwbao (via GitHub)" <gi...@apache.org>.
liamzwbao commented on code in PR #8354:
URL: https://github.com/apache/arrow-rs/pull/8354#discussion_r2893430812
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -1848,6 +1979,282 @@ mod test {
assert_eq!(&result, &expected);
}
+ /// This test uses a pre-shredded list array and validates index-path access.
+ #[test]
+ fn test_shredded_list_index_access() {
+ let array = shredded_list_variant_array();
+ // Test: Extract the 0 index field as VariantArray first
+ let options = GetOptions::new_with_path(VariantPath::from(0));
+ let result = variant_get(&array, options).unwrap();
+ let result_variant = VariantArray::try_new(&result).unwrap();
+ assert_eq!(result_variant.len(), 2);
+
+ // Row 0: expect 0 index = "comedy"
+ assert_eq!(result_variant.value(0), Variant::from("comedy"));
+ // Row 1: expect 0 index = "horror"
+ assert_eq!(result_variant.value(1), Variant::from("horror"));
+ }
+
+ /// Test extracting shredded list field with type conversion.
+ #[test]
+ fn test_shredded_list_as_string() {
+ let array = shredded_list_variant_array();
+ // Test: Extract the 0 index values as StringArray (type conversion)
+ let field = Field::new("typed_value", DataType::Utf8, false);
+ let options = GetOptions::new_with_path(VariantPath::from(0))
+ .with_as_type(Some(FieldRef::from(field)));
+ let result = variant_get(&array, options).unwrap();
+ // Should get StringArray
+ let expected: ArrayRef = Arc::new(StringArray::from(vec![Some("comedy"), Some("horror")]));
+ assert_eq!(&result, &expected);
+ }
+
+ #[test]
+ fn test_shredded_list_index_access_from_value_field() {
+ let array = shredded_list_variant_array();
+ // Index 1 maps to "drama" for row 0, and to fallback value 123 for row 1.
+ let options = GetOptions::new_with_path(VariantPath::from(1));
+ let result = variant_get(&array, options).unwrap();
+ let result_variant = VariantArray::try_new(&result).unwrap();
+
+ assert_eq!(result_variant.value(0), Variant::from("drama"));
+ assert_eq!(result_variant.value(1).as_int64(), Some(123));
+ }
+
+ #[test]
+ fn test_shredded_list_index_access_from_value_field_as_int64() {
+ let array = shredded_list_variant_array();
+ let field = Field::new("typed_value", DataType::Int64, true);
+ let options = GetOptions::new_with_path(VariantPath::from(1))
+ .with_as_type(Some(FieldRef::from(field)));
+ let result = variant_get(&array, options).unwrap();
+
+ // "drama" -> NULL, 123 -> 123.
+ let expected: ArrayRef = Arc::new(Int64Array::from(vec![None, Some(123)]));
+ assert_eq!(&result, &expected);
+ }
+
+ #[test]
+ fn test_shredded_list_index_out_of_bounds_unsafe_cast_errors() {
+ let options =
+ GetOptions::new_with_path(VariantPath::from(10)).with_cast_options(CastOptions {
+ safe: false,
+ ..Default::default()
+ });
+
+ let err = variant_get(&shredded_list_variant_array(), options.clone()).unwrap_err();
+ assert!(err.to_string().contains("Cannot access index '10'"));
+ }
+
+ #[test]
+ fn test_shredded_large_list_index_access_from_value_field() {
+ let array = shredded_large_list_variant_array();
+ // Index 1 maps to "drama" for row 0, and to fallback value 123 for row 1.
+ let options = GetOptions::new_with_path(VariantPath::from(1));
+ let result = variant_get(&array, options).unwrap();
+ let result_variant = VariantArray::try_new(&result).unwrap();
+
+ assert_eq!(result_variant.value(0), Variant::from("drama"));
+ assert_eq!(result_variant.value(1).as_int64(), Some(123));
+ }
+
+ #[test]
+ fn test_shredded_large_list_index_out_of_bounds_unsafe_cast_errors() {
+ let options =
+ GetOptions::new_with_path(VariantPath::from(10)).with_cast_options(CastOptions {
+ safe: false,
+ ..Default::default()
+ });
+
+ let err = variant_get(&shredded_large_list_variant_array(), options).unwrap_err();
+ assert!(err.to_string().contains("Cannot access index '10'"));
+ }
+
+ #[test]
+ fn test_shredded_list_view_index_access_from_value_field() {
+ let array = shredded_list_view_variant_array();
+ let options = GetOptions::new_with_path(VariantPath::from(1));
+ let result = variant_get(&array, options).unwrap();
+ let result_variant = VariantArray::try_new(&result).unwrap();
+
+ assert_eq!(result_variant.value(0), Variant::from("drama"));
+ assert_eq!(result_variant.value(1).as_int64(), Some(123));
+ }
+
+ #[test]
+ fn test_shredded_list_view_index_out_of_bounds_unsafe_cast_errors() {
+ let options =
+ GetOptions::new_with_path(VariantPath::from(10)).with_cast_options(CastOptions {
+ safe: false,
+ ..Default::default()
+ });
+
+ let err = variant_get(&shredded_list_view_variant_array(), options).unwrap_err();
+ assert!(err.to_string().contains("Cannot access index '10'"));
+ }
+
+ #[test]
+ fn test_shredded_large_list_view_index_access_from_value_field() {
+ let array = shredded_large_list_view_variant_array();
+ let options = GetOptions::new_with_path(VariantPath::from(1));
+ let result = variant_get(&array, options).unwrap();
+ let result_variant = VariantArray::try_new(&result).unwrap();
+
+ assert_eq!(result_variant.value(0), Variant::from("drama"));
+ assert_eq!(result_variant.value(1).as_int64(), Some(123));
+ }
+
+ #[test]
+ fn test_shredded_large_list_view_index_out_of_bounds_unsafe_cast_errors() {
+ let options =
+ GetOptions::new_with_path(VariantPath::from(10)).with_cast_options(CastOptions {
+ safe: false,
+ ..Default::default()
+ });
+
+ let err = variant_get(&shredded_large_list_view_variant_array(), options).unwrap_err();
+ assert!(err.to_string().contains("Cannot access index '10'"));
+ }
+
+ #[test]
+ fn test_shredded_list_in_struct_index_access() {
+ let array = shredded_struct_with_list_variant_array();
+ let options = GetOptions::new_with_path(VariantPath::try_from("a[1]").unwrap());
+ let result = variant_get(&array, options).unwrap();
+ let result_variant = VariantArray::try_new(&result).unwrap();
+
+ assert_eq!(result_variant.value(0), Variant::from("drama"));
+ assert_eq!(result_variant.value(1).as_int64(), Some(123));
+ }
+
+ #[test]
+ fn test_shredded_struct_in_list_field_access() {
+ let array = shredded_list_of_struct_variant_array();
+ let field = Field::new("x", DataType::Int32, true);
+ let path = VariantPath::from(0).join("x");
+ let options = GetOptions::new_with_path(path).with_as_type(Some(FieldRef::from(field)));
+ let result = variant_get(&array, options).unwrap();
+
+ let expected: ArrayRef = Arc::new(Int32Array::from(vec![Some(1), Some(3)]));
+ assert_eq!(&result, &expected);
+ }
+
+ #[test]
+ fn test_shredded_list_of_lists_index_access() {
+ let array = shredded_list_of_lists_variant_array();
+ let path = VariantPath::from(0).join(1);
+
+ let result = variant_get(&array, GetOptions::new_with_path(path.clone())).unwrap();
+ let result_variant = VariantArray::try_new(&result).unwrap();
+ assert_eq!(result_variant.value(0), Variant::from("b"));
+ assert_eq!(result_variant.value(1).as_int64(), Some(123));
+
+ let field = Field::new("typed_value", DataType::Int64, true);
+ let casted = variant_get(
+ &array,
+ GetOptions::new_with_path(path).with_as_type(Some(FieldRef::from(field))),
+ )
+ .unwrap();
+ let expected: ArrayRef = Arc::new(Int64Array::from(vec![None, Some(123)]));
+ assert_eq!(&casted, &expected);
+ }
+
+ /// Helper to create a shredded list variant array used by list index tests.
+ ///
+ /// Rows:
+ /// 1. `["comedy", "drama"]` (fully shred-able as `Utf8`)
+ /// 2. `["horror", 123]` (partially shredded, with fallback for the numeric element)
+ fn shredded_list_variant_array() -> ArrayRef {
+ let json_rows: ArrayRef = Arc::new(StringArray::from(vec![
+ Some(r#"["comedy", "drama"]"#),
+ Some(r#"["horror", 123]"#),
+ ]));
+ let input = json_to_variant(&json_rows).unwrap();
+
+ let list_schema = DataType::List(Arc::new(Field::new("item", DataType::Utf8, true)));
+ let shredded = shred_variant(&input, &list_schema).unwrap();
+ ArrayRef::from(shredded)
+ }
+
+ fn shredded_struct_with_list_variant_array() -> ArrayRef {
+ let json_rows: ArrayRef = Arc::new(StringArray::from(vec![
+ Some(r#"{"a": ["comedy", "drama"]}"#),
+ Some(r#"{"a": ["horror", 123]}"#),
+ ]));
+ let input = json_to_variant(&json_rows).unwrap();
+
+ let list_schema = DataType::List(Arc::new(Field::new("item", DataType::Utf8, true)));
+ let shredding_schema = ShreddedSchemaBuilder::default()
+ .with_path("a", &list_schema)
+ .unwrap()
+ .build();
+ let shredded = shred_variant(&input, &shredding_schema).unwrap();
+ ArrayRef::from(shredded)
+ }
+
+ fn shredded_list_of_struct_variant_array() -> ArrayRef {
+ let json_rows: ArrayRef = Arc::new(StringArray::from(vec![
+ Some(r#"[{"x": 1}, {"x": 2}]"#),
+ Some(r#"[{"x": 3}, {"y": 4}]"#),
+ ]));
+ let input = json_to_variant(&json_rows).unwrap();
+
+ let struct_type =
+ DataType::Struct(Fields::from(vec![Field::new("x", DataType::Int32, true)]));
+ let list_schema = DataType::List(Arc::new(Field::new("item", struct_type, true)));
+ let shredded = shred_variant(&input, &list_schema).unwrap();
+ ArrayRef::from(shredded)
+ }
+
+ fn shredded_list_of_lists_variant_array() -> ArrayRef {
+ let json_rows: ArrayRef = Arc::new(StringArray::from(vec![
+ Some(r#"[["a", "b"], ["c", "d"]]"#),
+ Some(r#"[["x", 123], ["y", "z"]]"#),
+ ]));
+ let input = json_to_variant(&json_rows).unwrap();
+
+ let inner_list = DataType::List(Arc::new(Field::new("item", DataType::Utf8, true)));
+ let outer_list = DataType::List(Arc::new(Field::new("item", inner_list, true)));
+ let shredded = shred_variant(&input, &outer_list).unwrap();
+ ArrayRef::from(shredded)
+ }
+
+ fn shredded_large_list_variant_array() -> ArrayRef {
+ let json_rows: ArrayRef = Arc::new(StringArray::from(vec![
+ Some(r#"["comedy", "drama"]"#),
+ Some(r#"["horror", 123]"#),
+ ]));
+ let input = json_to_variant(&json_rows).unwrap();
+
+ let list_schema = DataType::LargeList(Arc::new(Field::new("item", DataType::Utf8, true)));
+ let shredded = shred_variant(&input, &list_schema).unwrap();
+ ArrayRef::from(shredded)
+ }
+
+ fn shredded_list_view_variant_array() -> ArrayRef {
+ let json_rows: ArrayRef = Arc::new(StringArray::from(vec![
+ Some(r#"["comedy", "drama"]"#),
+ Some(r#"["horror", 123]"#),
+ ]));
+ let input = json_to_variant(&json_rows).unwrap();
+
+ let list_schema = DataType::ListView(Arc::new(Field::new("item", DataType::Utf8, true)));
+ let shredded = shred_variant(&input, &list_schema).unwrap();
+ ArrayRef::from(shredded)
+ }
+
+ fn shredded_large_list_view_variant_array() -> ArrayRef {
+ let json_rows: ArrayRef = Arc::new(StringArray::from(vec![
+ Some(r#"["comedy", "drama"]"#),
+ Some(r#"["horror", 123]"#),
+ ]));
+ let input = json_to_variant(&json_rows).unwrap();
+
+ let list_schema =
+ DataType::LargeListView(Arc::new(Field::new("item", DataType::Utf8, true)));
+ let shredded = shred_variant(&input, &list_schema).unwrap();
+ ArrayRef::from(shredded)
+ }
Review Comment:
nit: These helpers looks pretty similar other than the schema definition, we could consider generalize them. This would also help the callers of these helpers to be merged in a single test if they just differ in the list types
--
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] Support Shredded Lists/Array in `variant_get` [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on PR #8354:
URL: https://github.com/apache/arrow-rs/pull/8354#issuecomment-4111635492
Filed https://github.com/apache/arrow-rs/issues/9606 to track the jsonpath semantics -- it goes beyond just this PR.
--
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] Support Shredded Lists/Array in `variant_get` [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on code in PR #8354:
URL: https://github.com/apache/arrow-rs/pull/8354#discussion_r2984638988
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -92,13 +89,8 @@ fn take_list_like_index_as_shredding_state<L: ListLikeArray + 'static>(
ArrowError::ComputeError("List-like index does not fit into u64".into())
Review Comment:
If we're going for jsonpath semantics, should array index overflow also produce NULL instead of error?
On the other hand, one could argue that `foo[18446744073709551616]` (a 65-bit number) is an invalid jsonpath because it could never produce a valid array element, regardless of the underlying data?
But on the other hand, these overflow errors are not due to the jsonpath syntax at all -- they're due to overflow in the internal data structures. But actually... how could these overflows ever arise in the first place? We know `row_range.end` is a valid `usize`, as are `index` and `row_range.len()`, and because `index < len`, we know `row_range.begin + index` is _also_ a valid `usize`. So the first `checked_add` can never fail. And the conversion from `usize` to `u64` also cannot fail on any hardware that exists today (I don't know of any hardware with 128-bit pointers, at least).
Overall, I wonder if the whole thing is just:
```rust
let index = (index < row_range.len()).then(|| row_range.start + index as u64);
take_indices.push(index);
```
--
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] Support Shredded Lists/Array in `variant_get` [arrow-rs]
Posted by "sdf-jkl (via GitHub)" <gi...@apache.org>.
sdf-jkl commented on code in PR #8354:
URL: https://github.com/apache/arrow-rs/pull/8354#discussion_r2854115953
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -42,16 +45,84 @@ pub(crate) enum ShreddedPathStep<'a> {
NotShredded,
}
+fn take_list_index_as_shredding_state<O: OffsetSizeTrait>(
+ list_array: &GenericListArray<O>,
+ index: usize,
+ cast_options: &CastOptions,
+) -> Result<Option<ShreddingState>> {
+ let offsets = list_array.offsets();
+ let values = list_array.values();
+
+ let Some(struct_array) = values.as_any().downcast_ref::<StructArray>() else {
+ return Ok(None);
+ };
+
+ let value_array = struct_array.column_by_name("value");
+ let typed_array = struct_array.column_by_name("typed_value");
+
+ // If list elements have neither typed nor fallback value, this path step is missing.
+ if value_array.is_none() && typed_array.is_none() {
+ return Ok(None);
+ }
+
+ let mut take_indices = Vec::with_capacity(list_array.len());
+ for row in 0..list_array.len() {
+ let start = offsets[row].as_usize();
+ let end = offsets[row + 1].as_usize();
+ let len = end - start;
+
+ if index < len {
+ let absolute_index = start.checked_add(index).ok_or_else(|| {
+ ArrowError::ComputeError("List index overflow while building take indices".into())
+ })?;
+ let absolute_index = u64::try_from(absolute_index)
+ .map_err(|_| ArrowError::ComputeError("List index does not fit into u64".into()))?;
+ take_indices.push(Some(absolute_index));
+ } else if cast_options.safe {
+ take_indices.push(None);
+ } else {
+ return Err(ArrowError::CastError(format!(
+ "Cannot access index '{}' for row {} with list length {}",
+ index, row, len
+ )));
Review Comment:
Added to docs here https://github.com/apache/arrow-rs/pull/8354/commits/91589adc8fb7812d37e1cb0525072b1d6ea56354
--
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] Support Shredded Lists/Array in `variant_get` [arrow-rs]
Posted by "liamzwbao (via GitHub)" <gi...@apache.org>.
liamzwbao commented on code in PR #8354:
URL: https://github.com/apache/arrow-rs/pull/8354#discussion_r2870042661
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -96,15 +184,44 @@ pub(crate) fn follow_shredded_path_element<'a>(
))
})?;
- let state = BorrowedShreddingState::try_from(struct_array)?;
+ let state = ShreddingState::try_from(struct_array)?;
Ok(ShreddedPathStep::Success(state))
}
- VariantPathElement::Index { .. } => {
- // TODO: Support array indexing. Among other things, it will require slicing not
- // only the array we have here, but also the corresponding metadata and null masks.
- Err(ArrowError::NotYetImplemented(
- "Pathing into shredded variant array index".into(),
- ))
+ VariantPathElement::Index { index } => {
+ let state = if let Some(list_array) =
+ typed_value.as_any().downcast_ref::<GenericListArray<i32>>()
+ {
+ take_list_like_index_as_shredding_state(list_array, *index, cast_options)?
+ } else if let Some(list_array) =
+ typed_value.as_any().downcast_ref::<GenericListArray<i64>>()
+ {
+ take_list_like_index_as_shredding_state(list_array, *index, cast_options)?
+ } else if let Some(list_view_array) = typed_value
+ .as_any()
+ .downcast_ref::<GenericListViewArray<i32>>()
+ {
+ take_list_like_index_as_shredding_state(list_view_array, *index, cast_options)?
+ } else if let Some(list_view_array) = typed_value
+ .as_any()
+ .downcast_ref::<GenericListViewArray<i64>>()
+ {
+ take_list_like_index_as_shredding_state(list_view_array, *index, cast_options)?
+ } else if cast_options.safe {
Review Comment:
Since we can inspect the type from `typed_value.data_type()`, maybe it is better to match on that first and downcast only to the corresponding list type. This avoids unnecessary downcast attempts. For example, `LargeListView` no longer needs to try the first three list variants.
--
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] [WIP] Support Shredded Lists/Array in `variant_get` [arrow-rs]
Posted by "sdf-jkl (via GitHub)" <gi...@apache.org>.
sdf-jkl commented on code in PR #8354:
URL: https://github.com/apache/arrow-rs/pull/8354#discussion_r2356743648
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -1122,64 +1121,109 @@ mod test {
let result = variant_get(&array, options).unwrap();
// Should get StringArray
- let expected: ArrayRef = Arc::new(StringArray::from(vec![Some("comedy"), Some("drama")]));
+ let expected: ArrayRef =
+ Arc::new(StringArray::from(vec![Some("comedy"), None, Some("drama")]));
assert_eq!(&result, &expected);
}
/// Helper function to create a shredded variant array representing lists
///
/// This creates an array that represents:
/// Row 0: ["comedy", "drama"] ([0] is shredded, [1] is shredded - perfectly shredded)
- /// Row 1: ["horror", null] ([0] is shredded, [1] is binary null - partially shredded)
- /// Row 2: ["comedy", "drama", "romance"] (perfectly shredded)
Review Comment:
This row doesn't bring any additional value for testing, so I removed it.
--
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] [WIP] Support Shredded Lists/Array in `variant_get` [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on code in PR #8354:
URL: https://github.com/apache/arrow-rs/pull/8354#discussion_r2357562447
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -1122,64 +1121,109 @@ mod test {
let result = variant_get(&array, options).unwrap();
// Should get StringArray
- let expected: ArrayRef = Arc::new(StringArray::from(vec![Some("comedy"), Some("drama")]));
+ let expected: ArrayRef =
+ Arc::new(StringArray::from(vec![Some("comedy"), None, Some("drama")]));
assert_eq!(&result, &expected);
}
/// Helper function to create a shredded variant array representing lists
///
/// This creates an array that represents:
/// Row 0: ["comedy", "drama"] ([0] is shredded, [1] is shredded - perfectly shredded)
- /// Row 1: ["horror", null] ([0] is shredded, [1] is binary null - partially shredded)
- /// Row 2: ["comedy", "drama", "romance"] (perfectly shredded)
+ /// Row 1: ["horror", 123] ([0] is shredded, [1] is int - partially shredded)
///
/// The physical layout follows the shredding spec where:
/// - metadata: contains list metadata
/// - typed_value: StructArray with 0 index value
/// - value: contains fallback for
fn shredded_list_variant_array() -> ArrayRef {
- // Create the base metadata for lists
-
- // Could add this as an api for VariantList, like VariantList::from()
- fn build_list_metadata(vector: Vec<Variant>) -> (Vec<u8>, Vec<u8>) {
- let mut builder = parquet_variant::VariantBuilder::new();
- let mut list = builder.new_list();
- for value in vector {
- list.append_value(value);
- }
- list.finish();
- builder.finish()
- }
- let (metadata1, _) =
- build_list_metadata(vec![Variant::String("comedy"), Variant::String("drama")]);
+ // Create metadata array
+ let metadata_array =
+ BinaryViewArray::from_iter_values(std::iter::repeat_n(EMPTY_VARIANT_METADATA_BYTES, 2));
- let (metadata2, _) = build_list_metadata(vec![Variant::String("horror"), Variant::Null]);
+ // Building the typed_value ListArray
- let (metadata3, _) = build_list_metadata(vec![
- Variant::String("comedy"),
- Variant::String("drama"),
- Variant::String("romance"),
+ // Need a StructBuilder to create a ListBuilder
+ let fields = Fields::from(vec![
+ Field::new("value", DataType::Binary, true),
+ Field::new("typed_value", DataType::Utf8, true),
]);
+ let field_builders = vec![
+ make_builder(&DataType::Binary, 4),
+ make_builder(&DataType::Utf8, 4),
+ ];
+ let struct_builder = StructBuilder::new(fields, field_builders);
+
+ let mut builder = GenericListBuilder::<i32, StructBuilder>::new(struct_builder);
+
+ // Row 0 index 0
+ builder
+ .values()
+ .field_builder::<BinaryBuilder>(0)
+ .unwrap()
+ .append_null();
+ builder
+ .values()
+ .field_builder::<StringBuilder>(1)
+ .unwrap()
+ .append_value("comedy");
+ builder.values().append(true);
+
+ // Row 0 index 1
+ builder
+ .values()
+ .field_builder::<BinaryBuilder>(0)
+ .unwrap()
+ .append_null();
+ builder
+ .values()
+ .field_builder::<StringBuilder>(1)
+ .unwrap()
+ .append_value("drama");
+ builder.values().append(true);
+
+ // Next row
+ builder.append(true);
+
+ // Row 1 index 0
+ builder
+ .values()
+ .field_builder::<BinaryBuilder>(0)
+ .unwrap()
+ .append_null();
+ builder
+ .values()
+ .field_builder::<StringBuilder>(1)
+ .unwrap()
+ .append_value("horror");
+ builder.values().append(true);
+
+ // Row 1 index 1
+ let mut variant_builder = VariantBuilder::new();
+ variant_builder.append_value(123i32); // <------ couldn't find the right way to do it, used this as placeholder for binary
+ let (_, value) = variant_builder.finish();
+
+ builder
+ .values()
+ .field_builder::<BinaryBuilder>(0)
+ .unwrap()
+ .append_value(value);
Review Comment:
Once https://github.com/apache/arrow-rs/pull/8360 merges (hopefully today), you can use a `VariantValueArrayBuilder` to create the values column. That requires keeping the builders separate and then using `VariantArray::from_parts` to assemble their output into the final result.
See e.g. [VariantToBinaryVariantArrowRowBuilder](https://github.com/apache/arrow-rs/pull/8360/files#diff-5b1dcb31d6afd99fdac153e6e8d9cdec8aac81c0dfdcc13729de95951a409874R257-R288) in that same PR
--
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] [WIP] Support Shredded Lists/Array in `variant_get` [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on code in PR #8354:
URL: https://github.com/apache/arrow-rs/pull/8354#discussion_r2360963397
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -1122,64 +1121,109 @@ mod test {
let result = variant_get(&array, options).unwrap();
// Should get StringArray
- let expected: ArrayRef = Arc::new(StringArray::from(vec![Some("comedy"), Some("drama")]));
+ let expected: ArrayRef =
+ Arc::new(StringArray::from(vec![Some("comedy"), None, Some("drama")]));
assert_eq!(&result, &expected);
}
/// Helper function to create a shredded variant array representing lists
///
/// This creates an array that represents:
/// Row 0: ["comedy", "drama"] ([0] is shredded, [1] is shredded - perfectly shredded)
- /// Row 1: ["horror", null] ([0] is shredded, [1] is binary null - partially shredded)
- /// Row 2: ["comedy", "drama", "romance"] (perfectly shredded)
+ /// Row 1: ["horror", 123] ([0] is shredded, [1] is int - partially shredded)
///
/// The physical layout follows the shredding spec where:
/// - metadata: contains list metadata
/// - typed_value: StructArray with 0 index value
/// - value: contains fallback for
fn shredded_list_variant_array() -> ArrayRef {
- // Create the base metadata for lists
-
- // Could add this as an api for VariantList, like VariantList::from()
- fn build_list_metadata(vector: Vec<Variant>) -> (Vec<u8>, Vec<u8>) {
- let mut builder = parquet_variant::VariantBuilder::new();
- let mut list = builder.new_list();
- for value in vector {
- list.append_value(value);
- }
- list.finish();
- builder.finish()
- }
- let (metadata1, _) =
- build_list_metadata(vec![Variant::String("comedy"), Variant::String("drama")]);
+ // Create metadata array
+ let metadata_array =
+ BinaryViewArray::from_iter_values(std::iter::repeat_n(EMPTY_VARIANT_METADATA_BYTES, 2));
- let (metadata2, _) = build_list_metadata(vec![Variant::String("horror"), Variant::Null]);
+ // Building the typed_value ListArray
- let (metadata3, _) = build_list_metadata(vec![
- Variant::String("comedy"),
- Variant::String("drama"),
- Variant::String("romance"),
+ // Need a StructBuilder to create a ListBuilder
+ let fields = Fields::from(vec![
+ Field::new("value", DataType::Binary, true),
+ Field::new("typed_value", DataType::Utf8, true),
]);
+ let field_builders = vec![
+ make_builder(&DataType::Binary, 4),
+ make_builder(&DataType::Utf8, 4),
+ ];
+ let struct_builder = StructBuilder::new(fields, field_builders);
+
+ let mut builder = GenericListBuilder::<i32, StructBuilder>::new(struct_builder);
+
+ // Row 0 index 0
+ builder
+ .values()
+ .field_builder::<BinaryBuilder>(0)
+ .unwrap()
+ .append_null();
+ builder
+ .values()
+ .field_builder::<StringBuilder>(1)
+ .unwrap()
+ .append_value("comedy");
+ builder.values().append(true);
+
+ // Row 0 index 1
+ builder
+ .values()
+ .field_builder::<BinaryBuilder>(0)
+ .unwrap()
+ .append_null();
+ builder
+ .values()
+ .field_builder::<StringBuilder>(1)
+ .unwrap()
+ .append_value("drama");
+ builder.values().append(true);
+
+ // Next row
+ builder.append(true);
+
+ // Row 1 index 0
+ builder
+ .values()
+ .field_builder::<BinaryBuilder>(0)
+ .unwrap()
+ .append_null();
+ builder
+ .values()
+ .field_builder::<StringBuilder>(1)
+ .unwrap()
+ .append_value("horror");
+ builder.values().append(true);
+
+ // Row 1 index 1
+ let mut variant_builder = VariantBuilder::new();
+ variant_builder.append_value(123i32); // <------ couldn't find the right way to do it, used this as placeholder for binary
+ let (_, value) = variant_builder.finish();
+
+ builder
+ .values()
+ .field_builder::<BinaryBuilder>(0)
+ .unwrap()
+ .append_value(value);
Review Comment:
... but I don't know how easily we could fit a normal array builder into our shredding-specific builder code?
--
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] [WIP] Support Shredded Lists/Array in `variant_get` [arrow-rs]
Posted by "sdf-jkl (via GitHub)" <gi...@apache.org>.
sdf-jkl commented on code in PR #8354:
URL: https://github.com/apache/arrow-rs/pull/8354#discussion_r2608123758
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -97,14 +98,67 @@ pub(crate) fn follow_shredded_path_element<'a>(
})?;
let state = BorrowedShreddingState::try_from(struct_array)?;
- Ok(ShreddedPathStep::Success(state))
+ Ok(ShreddedPathStep::Success(state.into()))
}
- VariantPathElement::Index { .. } => {
+ VariantPathElement::Index { index } => {
// TODO: Support array indexing. Among other things, it will require slicing not
// only the array we have here, but also the corresponding metadata and null masks.
- Err(ArrowError::NotYetImplemented(
- "Pathing into shredded variant array index".into(),
- ))
+ let Some(list_array) = typed_value.as_any().downcast_ref::<GenericListArray<i32>>()
+ else {
+ // Downcast failure - if strict cast options are enabled, this should be an error
+ if !cast_options.safe {
+ return Err(ArrowError::CastError(format!(
+ "Cannot access index '{}' on non-list type: {}",
+ index,
+ typed_value.data_type()
+ )));
+ }
+ // With safe cast options, return NULL (missing_path_step)
+ return Ok(missing_path_step());
+ };
+
+ let offsets = list_array.offsets();
+ let values = list_array.values(); // This is a StructArray
+
+ let Some(struct_array) = values.as_any().downcast_ref::<StructArray>() else {
+ return Ok(missing_path_step());
+ };
+
+ let Some(typed_array) = struct_array.column_by_name("typed_value") else {
+ return Ok(missing_path_step());
+ };
+
+ // Build the list of indices to take
+ let mut take_indices = Vec::with_capacity(list_array.len());
+ for i in 0..list_array.len() {
+ let start = offsets[i] as usize;
+ let end = offsets[i + 1] as usize;
+ let len = end - start;
+
+ if *index < len {
Review Comment:
This test is running variant_get with the `as_type` parameter, specifying which type we are looking for.
The example `VariantArray` is shredded by `String` type, therefore a `String` value cannot be outside the `typed_value` column and will return a null in this test case.
Given the ["comedy", "drama"], ["horror" 123] `ListArray` if we try take the value from index 1 instead 0, we will get:
```rust
thread 'variant_get::test::test_shredded_list_as_string' (51616) panicked at parquet-variant-compute\src\variant_get.rs:1735:9:
assertion `left == right` failed
left: StringArray
[
"drama",
null,
]
right: StringArray
[
"comedy",
"horror",
]
```
--
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] [WIP] Support Shredded Lists/Array in `variant_get` [arrow-rs]
Posted by "sdf-jkl (via GitHub)" <gi...@apache.org>.
sdf-jkl commented on code in PR #8354:
URL: https://github.com/apache/arrow-rs/pull/8354#discussion_r2607973173
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -97,14 +98,67 @@ pub(crate) fn follow_shredded_path_element<'a>(
})?;
let state = BorrowedShreddingState::try_from(struct_array)?;
- Ok(ShreddedPathStep::Success(state))
+ Ok(ShreddedPathStep::Success(state.into()))
}
- VariantPathElement::Index { .. } => {
+ VariantPathElement::Index { index } => {
// TODO: Support array indexing. Among other things, it will require slicing not
// only the array we have here, but also the corresponding metadata and null masks.
- Err(ArrowError::NotYetImplemented(
- "Pathing into shredded variant array index".into(),
- ))
+ let Some(list_array) = typed_value.as_any().downcast_ref::<GenericListArray<i32>>()
+ else {
+ // Downcast failure - if strict cast options are enabled, this should be an error
+ if !cast_options.safe {
+ return Err(ArrowError::CastError(format!(
+ "Cannot access index '{}' on non-list type: {}",
+ index,
+ typed_value.data_type()
+ )));
+ }
+ // With safe cast options, return NULL (missing_path_step)
+ return Ok(missing_path_step());
+ };
+
+ let offsets = list_array.offsets();
+ let values = list_array.values(); // This is a StructArray
+
+ let Some(struct_array) = values.as_any().downcast_ref::<StructArray>() else {
+ return Ok(missing_path_step());
+ };
+
+ let Some(typed_array) = struct_array.column_by_name("typed_value") else {
+ return Ok(missing_path_step());
+ };
+
+ // Build the list of indices to take
+ let mut take_indices = Vec::with_capacity(list_array.len());
+ for i in 0..list_array.len() {
+ let start = offsets[i] as usize;
+ let end = offsets[i + 1] as usize;
+ let len = end - start;
+
+ if *index < len {
+ take_indices.push(Some((start + index) as u32));
+ } else {
+ take_indices.push(None);
+ }
+ }
+
+ let index_array = UInt32Array::from(take_indices);
+
+ // Use Arrow compute kernel to gather elements
+ let taken = take(typed_array, &index_array, None)?;
Review Comment:
Yes, I believe this is the inconvenient part of working with non-contiguous data.
--
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] [WIP] Support Shredded Lists/Array in `variant_get` [arrow-rs]
Posted by "sdf-jkl (via GitHub)" <gi...@apache.org>.
sdf-jkl commented on code in PR #8354:
URL: https://github.com/apache/arrow-rs/pull/8354#discussion_r2842862447
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -96,15 +167,35 @@ pub(crate) fn follow_shredded_path_element<'a>(
))
})?;
- let state = BorrowedShreddingState::try_from(struct_array)?;
+ let state = ShreddingState::try_from(struct_array)?;
Ok(ShreddedPathStep::Success(state))
}
- VariantPathElement::Index { .. } => {
- // TODO: Support array indexing. Among other things, it will require slicing not
- // only the array we have here, but also the corresponding metadata and null masks.
- Err(ArrowError::NotYetImplemented(
- "Pathing into shredded variant array index".into(),
- ))
+ VariantPathElement::Index { index } => {
+ let state = if let Some(list_array) =
+ typed_value.as_any().downcast_ref::<GenericListArray<i32>>()
+ {
+ take_list_index_as_shredding_state(list_array, *index, cast_options)?
+ } else if let Some(list_array) =
+ typed_value.as_any().downcast_ref::<GenericListArray<i64>>()
+ {
+ take_list_index_as_shredding_state(list_array, *index, cast_options)?
+ } else {
+ // Downcast failure - if strict cast options are enabled, this should be an error
+ if !cast_options.safe {
+ return Err(ArrowError::CastError(format!(
Review Comment:
I postponed adding match arms for other List-like types till we push #9457
--
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] [WIP] Support Shredded Lists/Array in `variant_get` [arrow-rs]
Posted by "sdf-jkl (via GitHub)" <gi...@apache.org>.
sdf-jkl commented on PR #8354:
URL: https://github.com/apache/arrow-rs/pull/8354#issuecomment-3946733061
@klion26 @scovich please review when available. thanks!
--
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] [WIP] Support Shredded Lists/Array in `variant_get` [arrow-rs]
Posted by "sdf-jkl (via GitHub)" <gi...@apache.org>.
sdf-jkl commented on code in PR #8354:
URL: https://github.com/apache/arrow-rs/pull/8354#discussion_r2836308961
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -97,14 +98,67 @@ pub(crate) fn follow_shredded_path_element<'a>(
})?;
let state = BorrowedShreddingState::try_from(struct_array)?;
- Ok(ShreddedPathStep::Success(state))
+ Ok(ShreddedPathStep::Success(state.into()))
}
- VariantPathElement::Index { .. } => {
+ VariantPathElement::Index { index } => {
// TODO: Support array indexing. Among other things, it will require slicing not
// only the array we have here, but also the corresponding metadata and null masks.
- Err(ArrowError::NotYetImplemented(
- "Pathing into shredded variant array index".into(),
- ))
+ let Some(list_array) = typed_value.as_any().downcast_ref::<GenericListArray<i32>>()
+ else {
+ // Downcast failure - if strict cast options are enabled, this should be an error
+ if !cast_options.safe {
+ return Err(ArrowError::CastError(format!(
+ "Cannot access index '{}' on non-list type: {}",
+ index,
+ typed_value.data_type()
+ )));
+ }
+ // With safe cast options, return NULL (missing_path_step)
+ return Ok(missing_path_step());
+ };
+
+ let offsets = list_array.offsets();
+ let values = list_array.values(); // This is a StructArray
+
+ let Some(struct_array) = values.as_any().downcast_ref::<StructArray>() else {
+ return Ok(missing_path_step());
Review Comment:
It could still be a valid `Variant`, but not shredded at a certain depth.
--
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] Support Shredded Lists/Array in `variant_get` [arrow-rs]
Posted by "sdf-jkl (via GitHub)" <gi...@apache.org>.
sdf-jkl commented on PR #8354:
URL: https://github.com/apache/arrow-rs/pull/8354#issuecomment-4090816912
@alamb following up on this.
Please check https://github.com/apache/arrow-rs/pull/8354#issuecomment-3986833679
--
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] Support Shredded Lists/Array in `variant_get` [arrow-rs]
Posted by "sdf-jkl (via GitHub)" <gi...@apache.org>.
sdf-jkl commented on code in PR #8354:
URL: https://github.com/apache/arrow-rs/pull/8354#discussion_r2854115953
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -42,16 +45,84 @@ pub(crate) enum ShreddedPathStep<'a> {
NotShredded,
}
+fn take_list_index_as_shredding_state<O: OffsetSizeTrait>(
+ list_array: &GenericListArray<O>,
+ index: usize,
+ cast_options: &CastOptions,
+) -> Result<Option<ShreddingState>> {
+ let offsets = list_array.offsets();
+ let values = list_array.values();
+
+ let Some(struct_array) = values.as_any().downcast_ref::<StructArray>() else {
+ return Ok(None);
+ };
+
+ let value_array = struct_array.column_by_name("value");
+ let typed_array = struct_array.column_by_name("typed_value");
+
+ // If list elements have neither typed nor fallback value, this path step is missing.
+ if value_array.is_none() && typed_array.is_none() {
+ return Ok(None);
+ }
+
+ let mut take_indices = Vec::with_capacity(list_array.len());
+ for row in 0..list_array.len() {
+ let start = offsets[row].as_usize();
+ let end = offsets[row + 1].as_usize();
+ let len = end - start;
+
+ if index < len {
+ let absolute_index = start.checked_add(index).ok_or_else(|| {
+ ArrowError::ComputeError("List index overflow while building take indices".into())
+ })?;
+ let absolute_index = u64::try_from(absolute_index)
+ .map_err(|_| ArrowError::ComputeError("List index does not fit into u64".into()))?;
+ take_indices.push(Some(absolute_index));
+ } else if cast_options.safe {
+ take_indices.push(None);
+ } else {
+ return Err(ArrowError::CastError(format!(
+ "Cannot access index '{}' for row {} with list length {}",
+ index, row, len
+ )));
Review Comment:
https://github.com/apache/arrow-rs/pull/8354/commits/91589adc8fb7812d37e1cb0525072b1d6ea56354
--
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] Support Shredded Lists/Array in `variant_get` [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on code in PR #8354:
URL: https://github.com/apache/arrow-rs/pull/8354#discussion_r2990140186
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -96,15 +171,33 @@ pub(crate) fn follow_shredded_path_element<'a>(
))
})?;
- let state = BorrowedShreddingState::try_from(struct_array)?;
+ let state = ShreddingState::try_from(struct_array)?;
Ok(ShreddedPathStep::Success(state))
}
- VariantPathElement::Index { .. } => {
- // TODO: Support array indexing. Among other things, it will require slicing not
- // only the array we have here, but also the corresponding metadata and null masks.
- Err(ArrowError::NotYetImplemented(
- "Pathing into shredded variant array index".into(),
- ))
+ VariantPathElement::Index { index } => {
+ let state = match typed_value.data_type() {
+ DataType::List(_) => take_list_like_index_as_shredding_state::<
+ GenericListArray<i32>,
+ >(typed_value.as_ref(), *index)?,
Review Comment:
Aside: It just makes my eyes hurt when `fmt` does stuff like this. But I don't know a way to make it better, unless we want to drastically shorten the function name âšī¸
##########
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:
This is ultimately cloning an `ArrayRef` (cheap) `BinaryViewArray` (not expensive, but not cheap either -- has to go through `ArrayData`). Should we change `ShreddingState::value` to `Option<Cow<'a, BinaryViewArray>>` to avoid unnecessary cloning for struct-only paths? Then we could get rid of `BorrowedShreddingState` entirely -- which BTW I think this PR currently leaves as dead code (but no clippy warnings because it's `pub`)
##########
parquet-variant/src/path.rs:
##########
@@ -346,5 +346,14 @@ mod tests {
err.to_string(),
"Parser error: Invalid token in bracket request: `abc`. Expected a quoted string or a number(e.g., `['field']` or `[123]`)"
);
+
+ // Out-of-range integer indexes are invalid path tokens.
+ let too_large_index = (usize::MAX as u128) + 1;
+ let err = VariantPath::try_from(format!("[{too_large_index}]").as_str()).unwrap_err();
+ assert!(
+ err.to_string()
+ .contains("Parser error: Invalid token in bracket request"),
+ "{err}"
+ );
Review Comment:
Nice touch!
Do we have similar unit tests for verifying runtime behavior of a given jsonpath when the underlying data doesn't "fit"? Or would that be part of the larger fix?
--
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] [WIP] Support Shredded Lists/Array in `variant_get` [arrow-rs]
Posted by "klion26 (via GitHub)" <gi...@apache.org>.
klion26 commented on PR #8354:
URL: https://github.com/apache/arrow-rs/pull/8354#issuecomment-3610646355
> @alamb @klion26 can you take a look?
Thanks for the remainder, I'll check it later today or tomorrow.
--
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] [WIP] Support Shredded Lists/Array in `variant_get` [arrow-rs]
Posted by "klion26 (via GitHub)" <gi...@apache.org>.
klion26 commented on code in PR #8354:
URL: https://github.com/apache/arrow-rs/pull/8354#discussion_r2591683489
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -97,14 +98,67 @@ pub(crate) fn follow_shredded_path_element<'a>(
})?;
let state = BorrowedShreddingState::try_from(struct_array)?;
- Ok(ShreddedPathStep::Success(state))
+ Ok(ShreddedPathStep::Success(state.into()))
}
- VariantPathElement::Index { .. } => {
+ VariantPathElement::Index { index } => {
// TODO: Support array indexing. Among other things, it will require slicing not
// only the array we have here, but also the corresponding metadata and null masks.
- Err(ArrowError::NotYetImplemented(
- "Pathing into shredded variant array index".into(),
- ))
+ let Some(list_array) = typed_value.as_any().downcast_ref::<GenericListArray<i32>>()
+ else {
+ // Downcast failure - if strict cast options are enabled, this should be an error
+ if !cast_options.safe {
+ return Err(ArrowError::CastError(format!(
+ "Cannot access index '{}' on non-list type: {}",
+ index,
+ typed_value.data_type()
+ )));
+ }
+ // With safe cast options, return NULL (missing_path_step)
+ return Ok(missing_path_step());
+ };
+
+ let offsets = list_array.offsets();
+ let values = list_array.values(); // This is a StructArray
+
+ let Some(struct_array) = values.as_any().downcast_ref::<StructArray>() else {
+ return Ok(missing_path_step());
+ };
+
+ let Some(typed_array) = struct_array.column_by_name("typed_value") else {
+ return Ok(missing_path_step());
+ };
+
+ // Build the list of indices to take
+ let mut take_indices = Vec::with_capacity(list_array.len());
+ for i in 0..list_array.len() {
+ let start = offsets[i] as usize;
+ let end = offsets[i + 1] as usize;
+ let len = end - start;
+
+ if *index < len {
+ take_indices.push(Some((start + index) as u32));
+ } else {
+ take_indices.push(None);
+ }
+ }
+
+ let index_array = UInt32Array::from(take_indices);
+
+ // Use Arrow compute kernel to gather elements
+ let taken = take(typed_array, &index_array, None)?;
Review Comment:
Seems this will create a new array, not sure if we can use some "view" here to avoid creating the new array here.
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -97,14 +98,67 @@ pub(crate) fn follow_shredded_path_element<'a>(
})?;
let state = BorrowedShreddingState::try_from(struct_array)?;
- Ok(ShreddedPathStep::Success(state))
+ Ok(ShreddedPathStep::Success(state.into()))
}
- VariantPathElement::Index { .. } => {
+ VariantPathElement::Index { index } => {
// TODO: Support array indexing. Among other things, it will require slicing not
// only the array we have here, but also the corresponding metadata and null masks.
- Err(ArrowError::NotYetImplemented(
- "Pathing into shredded variant array index".into(),
- ))
+ let Some(list_array) = typed_value.as_any().downcast_ref::<GenericListArray<i32>>()
+ else {
+ // Downcast failure - if strict cast options are enabled, this should be an error
+ if !cast_options.safe {
+ return Err(ArrowError::CastError(format!(
+ "Cannot access index '{}' on non-list type: {}",
+ index,
+ typed_value.data_type()
+ )));
+ }
+ // With safe cast options, return NULL (missing_path_step)
+ return Ok(missing_path_step());
+ };
+
+ let offsets = list_array.offsets();
+ let values = list_array.values(); // This is a StructArray
+
+ let Some(struct_array) = values.as_any().downcast_ref::<StructArray>() else {
+ return Ok(missing_path_step());
+ };
+
+ let Some(typed_array) = struct_array.column_by_name("typed_value") else {
+ return Ok(missing_path_step());
+ };
+
+ // Build the list of indices to take
+ let mut take_indices = Vec::with_capacity(list_array.len());
+ for i in 0..list_array.len() {
+ let start = offsets[i] as usize;
+ let end = offsets[i + 1] as usize;
+ let len = end - start;
+
+ if *index < len {
+ take_indices.push(Some((start + index) as u32));
+ } else {
+ take_indices.push(None);
Review Comment:
Does this mean `OutOfBound` in the current list? Currently, we'll return `null`, not sure if we need to return an error in this case. Return an error or not, maybe we can log the behavior somewhere.
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -97,14 +98,67 @@ pub(crate) fn follow_shredded_path_element<'a>(
})?;
let state = BorrowedShreddingState::try_from(struct_array)?;
- Ok(ShreddedPathStep::Success(state))
+ Ok(ShreddedPathStep::Success(state.into()))
}
- VariantPathElement::Index { .. } => {
+ VariantPathElement::Index { index } => {
// TODO: Support array indexing. Among other things, it will require slicing not
// only the array we have here, but also the corresponding metadata and null masks.
- Err(ArrowError::NotYetImplemented(
- "Pathing into shredded variant array index".into(),
- ))
+ let Some(list_array) = typed_value.as_any().downcast_ref::<GenericListArray<i32>>()
+ else {
+ // Downcast failure - if strict cast options are enabled, this should be an error
+ if !cast_options.safe {
+ return Err(ArrowError::CastError(format!(
+ "Cannot access index '{}' on non-list type: {}",
+ index,
+ typed_value.data_type()
+ )));
+ }
+ // With safe cast options, return NULL (missing_path_step)
+ return Ok(missing_path_step());
+ };
+
+ let offsets = list_array.offsets();
+ let values = list_array.values(); // This is a StructArray
+
+ let Some(struct_array) = values.as_any().downcast_ref::<StructArray>() else {
+ return Ok(missing_path_step());
Review Comment:
Double-checking here,
From the [variant shredding spec](https://github.com/apache/parquet-format/blob/master/VariantShredding.md#arrays), the element is a required group list. Does this `return Ok(missing_path_step());` mean the input is not a valid variant?
```
optional group tags (VARIANT) {
required binary metadata;
optional binary value;
optional group typed_value (LIST) { # must be optional to allow a null list
repeated group list {
required group element { # shredded element
optional binary value;
optional binary typed_value (STRING);
}
}
}
}
```
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -1646,7 +1701,96 @@ mod test {
let expected: ArrayRef = Arc::new(Int32Array::from(vec![Some(1), Some(42)]));
assert_eq!(&result, &expected);
}
+ /// This test manually constructs a shredded variant array representing lists
+ /// like ["comedy", "drama"] and ["horror", 123]
+ /// as VariantArray using variant_get.
+ #[test]
+ fn test_shredded_list_index_access() {
+ let array = shredded_list_variant_array();
+ // Test: Extract the 0 index field as VariantArray first
+ let options = GetOptions::new_with_path(VariantPath::from(0));
+ let result = variant_get(&array, options).unwrap();
+ let result_variant = VariantArray::try_new(&result).unwrap();
+ assert_eq!(result_variant.len(), 2);
+ // Row 0: expect 0 index = "comedy"
+ assert_eq!(result_variant.value(0), Variant::from("comedy"));
+ // Row 1: expect 0 index = "horror"
+ assert_eq!(result_variant.value(1), Variant::from("horror"));
+ }
Review Comment:
We may need to add a blank line between tests/functions(above and below) for better display.
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -97,14 +98,67 @@ pub(crate) fn follow_shredded_path_element<'a>(
})?;
let state = BorrowedShreddingState::try_from(struct_array)?;
- Ok(ShreddedPathStep::Success(state))
+ Ok(ShreddedPathStep::Success(state.into()))
}
- VariantPathElement::Index { .. } => {
+ VariantPathElement::Index { index } => {
// TODO: Support array indexing. Among other things, it will require slicing not
// only the array we have here, but also the corresponding metadata and null masks.
- Err(ArrowError::NotYetImplemented(
- "Pathing into shredded variant array index".into(),
- ))
+ let Some(list_array) = typed_value.as_any().downcast_ref::<GenericListArray<i32>>()
+ else {
+ // Downcast failure - if strict cast options are enabled, this should be an error
+ if !cast_options.safe {
+ return Err(ArrowError::CastError(format!(
+ "Cannot access index '{}' on non-list type: {}",
+ index,
+ typed_value.data_type()
+ )));
+ }
+ // With safe cast options, return NULL (missing_path_step)
+ return Ok(missing_path_step());
+ };
+
+ let offsets = list_array.offsets();
+ let values = list_array.values(); // This is a StructArray
+
+ let Some(struct_array) = values.as_any().downcast_ref::<StructArray>() else {
+ return Ok(missing_path_step());
+ };
+
+ let Some(typed_array) = struct_array.column_by_name("typed_value") else {
+ return Ok(missing_path_step());
+ };
+
+ // Build the list of indices to take
+ let mut take_indices = Vec::with_capacity(list_array.len());
+ for i in 0..list_array.len() {
+ let start = offsets[i] as usize;
+ let end = offsets[i + 1] as usize;
+ let len = end - start;
+
+ if *index < len {
Review Comment:
Please correct me if I'm wrong. Here we _assert that **all**_ the values will be `typed_value` column, and use the indices collected here to retrieve the final value.
What if the value is located in the `value` column instead of the `typed_value` column? (change the test `test_shredded_list_as_string` from `VariantPath::from(0)` to `VariantPath::from(1)` can see this)
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -1646,7 +1701,96 @@ mod test {
let expected: ArrayRef = Arc::new(Int32Array::from(vec![Some(1), Some(42)]));
assert_eq!(&result, &expected);
}
+ /// This test manually constructs a shredded variant array representing lists
+ /// like ["comedy", "drama"] and ["horror", 123]
+ /// as VariantArray using variant_get.
+ #[test]
+ fn test_shredded_list_index_access() {
+ let array = shredded_list_variant_array();
+ // Test: Extract the 0 index field as VariantArray first
+ let options = GetOptions::new_with_path(VariantPath::from(0));
+ let result = variant_get(&array, options).unwrap();
+ let result_variant = VariantArray::try_new(&result).unwrap();
+ assert_eq!(result_variant.len(), 2);
+ // Row 0: expect 0 index = "comedy"
+ assert_eq!(result_variant.value(0), Variant::from("comedy"));
Review Comment:
Do we need to cover the case that `list[index]` located in the `value` field of the input variant(such as `VariantPath::from(1)` here for `["horror", 123]`)
Maybe we can add some more test cases
- the request index value located in the `typed_value` column
- the request index value located in the `value` column
- some nest struct(list in struct, or struct in list)
- ...
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -97,14 +98,67 @@ pub(crate) fn follow_shredded_path_element<'a>(
})?;
let state = BorrowedShreddingState::try_from(struct_array)?;
- Ok(ShreddedPathStep::Success(state))
+ Ok(ShreddedPathStep::Success(state.into()))
}
- VariantPathElement::Index { .. } => {
+ VariantPathElement::Index { index } => {
// TODO: Support array indexing. Among other things, it will require slicing not
// only the array we have here, but also the corresponding metadata and null masks.
- Err(ArrowError::NotYetImplemented(
- "Pathing into shredded variant array index".into(),
- ))
+ let Some(list_array) = typed_value.as_any().downcast_ref::<GenericListArray<i32>>()
Review Comment:
Is there any chance the list length exceeds `i32`?
--
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] [WIP] Support Shredded Lists/Array in `variant_get` [arrow-rs]
Posted by "sdf-jkl (via GitHub)" <gi...@apache.org>.
sdf-jkl commented on code in PR #8354:
URL: https://github.com/apache/arrow-rs/pull/8354#discussion_r2461286479
##########
parquet-variant-compute/src/variant_array.rs:
##########
@@ -870,6 +870,40 @@ impl From<BorrowedShreddingState<'_>> for ShreddingState {
}
}
+pub enum ShreddingStateCow<'a> {
+ Owned(ShreddingState),
+ Borrowed(BorrowedShreddingState<'a>),
+}
+
+impl<'a> From<ShreddingState> for ShreddingStateCow<'a> {
+ fn from(s: ShreddingState) -> Self {
+ Self::Owned(s)
+ }
+}
+impl<'a> From<BorrowedShreddingState<'a>> for ShreddingStateCow<'a> {
+ fn from(s: BorrowedShreddingState<'a>) -> Self {
+ Self::Borrowed(s)
+ }
+}
+
+impl<'a> ShreddingStateCow<'a> {
+ /// Always gives the caller a borrowed view, even if we own internally.
+ pub fn as_view(&self) -> BorrowedShreddingState<'_> {
+ match self {
+ ShreddingStateCow::Borrowed(b) => b.clone(),
+ ShreddingStateCow::Owned(o) => o.borrow(),
+ }
+ }
+
+ /// Materialize ownership when the caller needs to keep it.
+ pub fn into_owned(self) -> ShreddingState {
+ match self {
+ ShreddingStateCow::Borrowed(b) => b.into(),
+ ShreddingStateCow::Owned(o) => o,
+ }
+ }
+}
Review Comment:
Here is the new `ShreddingStateCow` enum implementation
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -151,20 +205,21 @@ 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 = ShreddingStateCow::Borrowed(input.shredding_state().borrow());
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.as_view(), path_element, cast_options)?
+ {
ShreddedPathStep::Success(state) => {
// Union nulls from the typed_value we just accessed
- if let Some(typed_value) = shredding_state.typed_value_field() {
+ if let Some(typed_value) = shredding_state.as_view().typed_value_field() {
accumulated_nulls = arrow::buffer::NullBuffer::union(
accumulated_nulls.as_ref(),
typed_value.nulls(),
);
}
- shredding_state = state;
+ shredding_state = ShreddingStateCow::Owned(state.into_owned());
Review Comment:
Here I could not come up with a way to make the `shredding_state` for the next `path_element` be ither borrowed or owned depending on the `follow_shredded_path_element` output.
Made it `into_owned()` just to pass the borrow checker.
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -97,14 +98,67 @@ pub(crate) fn follow_shredded_path_element<'a>(
})?;
let state = BorrowedShreddingState::try_from(struct_array)?;
- Ok(ShreddedPathStep::Success(state))
+ Ok(ShreddedPathStep::Success(state.into()))
}
- VariantPathElement::Index { .. } => {
+ VariantPathElement::Index { index } => {
// TODO: Support array indexing. Among other things, it will require slicing not
// only the array we have here, but also the corresponding metadata and null masks.
- Err(ArrowError::NotYetImplemented(
- "Pathing into shredded variant array index".into(),
- ))
+ let Some(list_array) = typed_value.as_any().downcast_ref::<GenericListArray<i32>>()
+ else {
+ // Downcast failure - if strict cast options are enabled, this should be an error
+ if !cast_options.safe {
+ return Err(ArrowError::CastError(format!(
+ "Cannot access index '{}' on non-list type: {}",
+ index,
+ typed_value.data_type()
+ )));
+ }
+ // With safe cast options, return NULL (missing_path_step)
+ return Ok(missing_path_step());
+ };
+
+ let offsets = list_array.offsets();
+ let values = list_array.values(); // This is a StructArray
+
+ let Some(struct_array) = values.as_any().downcast_ref::<StructArray>() else {
+ return Ok(missing_path_step());
+ };
+
+ let Some(typed_array) = struct_array.column_by_name("typed_value") else {
+ return Ok(missing_path_step());
+ };
+
+ // Build the list of indices to take
+ let mut take_indices = Vec::with_capacity(list_array.len());
+ for i in 0..list_array.len() {
+ let start = offsets[i] as usize;
+ let end = offsets[i + 1] as usize;
+ let len = end - start;
+
+ if *index < len {
+ take_indices.push(Some((start + index) as u32));
+ } else {
+ take_indices.push(None);
+ }
+ }
+
+ let index_array = UInt32Array::from(take_indices);
+
+ // Use Arrow compute kernel to gather elements
+ let taken = take(typed_array, &index_array, None)?;
+
+ let metadata_array = BinaryViewArray::from_iter_values(std::iter::repeat_n(
+ EMPTY_VARIANT_METADATA_BYTES,
+ taken.len(),
+ ));
+
+ let struct_array = &StructArrayBuilder::new()
+ .with_field("metadata", Arc::new(metadata_array), false)
+ .with_field("typed_value", taken, true)
+ .build();
+
+ let state = ShreddingState::try_from(struct_array)?;
+ Ok(ShreddedPathStep::Success(state.into()))
}
}
}
Review Comment:
When we use `variant_get` on `Struct` `Variant Array`'s it's relatively easy to extract the `typed_value`. For example, if we extract a.b because on the inside it's just:
```
VariantArray{
StructArray{
"typed_value":
StructArray{
"typed_value": PrimiteArray, <- We can directly borrow the value into
ShreddingState::Success() because the needed values in the array are contiguous
"value": VariantArray
```
But if we try to extract "typed_value" from a `List` `VariantArray` it gets more complicated. For example, extracting 0.0:
```
VariantArray{
StructArray{
"typed_value":
ListArray{
Offsets
StructArray{
"typed_value": PrimiteArray, <- but the values are now not contiguous, and the
output array can only be extracted using offsets, no borrow available
"value": VariantArray
```
Because of this issue the output of `follow_shredded_path_element` -> ShreddedPathStep::Success can end up receiving `BorrowedShreddingState` or owned `ShreddingState`.
To make this work I added a `ShreddingStateCow` enum and made it the `ShreddedPathStep::Success` input.
--
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] [WIP] Support Shredded Lists/Array in `variant_get` [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on code in PR #8354:
URL: https://github.com/apache/arrow-rs/pull/8354#discussion_r2354378630
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -1010,7 +1010,101 @@ mod test {
let expected: ArrayRef = Arc::new(Int32Array::from(vec![Some(1), Some(42)]));
assert_eq!(&result, &expected);
}
+ /// This test manually constructs a shredded variant array representing lists
+ /// like ["comedy", "drama"], ["horror", null] and ["comedy", "drama", "romance"]
+ /// as VariantArray using variant_get.
+ #[test]
+ fn test_shredded_list_field_access() {
+ let array = shredded_list_variant_array();
+
+ // Test: Extract the 0 index field as VariantArray first
+ let options = GetOptions::new_with_path(VariantPath::from(0));
+ let result = variant_get(&array, options).unwrap();
+ let result_variant: &VariantArray = result.as_any().downcast_ref().unwrap();
+ assert_eq!(result_variant.len(), 3);
+
+ // Row 0: expect 0 index = "comedy"
+ assert_eq!(result_variant.value(0), Variant::String("comedy"));
+ // Row 1: expect 0 index = "horror"
+ assert_eq!(result_variant.value(1), Variant::String("horror"));
+ // Row 2: expect 0 index = "comedy"
+ assert_eq!(result_variant.value(2), Variant::String("comedy"));
+ }
+ /// Test extracting shredded list field with type conversion
+ #[test]
+ fn test_shredded_list_as_string() {
+ let array = shredded_list_variant_array();
+
+ // Test: Extract the 0 index values as StringArray (type conversion)
+ let field = Field::new("typed_value", DataType::Utf8, false);
+ let options = GetOptions::new_with_path(VariantPath::from(0))
+ .with_as_type(Some(FieldRef::from(field)));
+ let result = variant_get(&array, options).unwrap();
+
+ // Should get StringArray
+ let expected: ArrayRef = Arc::new(StringArray::from(vec![Some("comedy"), Some("drama")]));
+ assert_eq!(&result, &expected);
+ }
+ /// Helper function to create a shredded variant array representing lists
+ ///
+ /// This creates an array that represents:
+ /// Row 0: ["comedy", "drama"] ([0] is shredded, [1] is shredded - perfectly shredded)
+ /// Row 1: ["horror", null] ([0] is shredded, [1] is binary null - partially shredded)
+ /// Row 2: ["comedy", "drama", "romance"] (perfectly shredded)
+ ///
+ /// The physical layout follows the shredding spec where:
+ /// - metadata: contains list metadata
+ /// - typed_value: StructArray with 0 index value
+ /// - value: contains fallback for
+ fn shredded_list_variant_array() -> ArrayRef {
+ // Create the base metadata for lists
+
+ // Could add this as an api for VariantList, like VariantList::from()
+ fn build_list_metadata(vector: Vec<Variant>) -> (Vec<u8>, Vec<u8>) {
+ let mut builder = parquet_variant::VariantBuilder::new();
+ let mut list = builder.new_list();
+ for value in vector {
+ list.append_value(value);
+ }
+ list.finish();
+ builder.finish()
+ }
+ let (metadata1, _) =
+ build_list_metadata(vec![Variant::String("comedy"), Variant::String("drama")]);
+
+ let (metadata2, _) = build_list_metadata(vec![Variant::String("horror"), Variant::Null]);
+
+ let (metadata3, _) = build_list_metadata(vec![
+ Variant::String("comedy"),
+ Variant::String("drama"),
+ Variant::String("romance"),
+ ]);
+
+ // Create metadata array
+ let metadata_array =
+ BinaryViewArray::from_iter_values(vec![metadata1, metadata2, metadata3]);
+
+ // Create the untyped value array
+ let value_array = BinaryViewArray::from(vec![Variant::Null.as_u8_slice()]);
+ // Maybe I should try with an actual primitive array
+ let typed_value_array = StringArray::from(vec![
+ Some("comedy"),
+ Some("drama"),
+ Some("horror"),
+ Some("comedy"),
+ Some("drama"),
+ Some("romance"),
+ ]);
+ // Build the main VariantArray
+ let main_struct = crate::variant_array::StructArrayBuilder::new()
+ .with_field("metadata", Arc::new(metadata_array))
+ .with_field("value", Arc::new(value_array))
+ .with_field("typed_value", Arc::new(typed_value_array))
Review Comment:
yes, exactly! And `element` is non-nullable (**), while the two children are nullable.
(**) As always, in arrow, it can still have null entries, but only if its parent is already NULL for the same row (so nobody can ever observe a non-null element)
--
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] [WIP] Support Shredded Lists/Array in `variant_get` [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on code in PR #8354:
URL: https://github.com/apache/arrow-rs/pull/8354#discussion_r2357565134
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -1122,64 +1121,109 @@ mod test {
let result = variant_get(&array, options).unwrap();
// Should get StringArray
- let expected: ArrayRef = Arc::new(StringArray::from(vec![Some("comedy"), Some("drama")]));
+ let expected: ArrayRef =
+ Arc::new(StringArray::from(vec![Some("comedy"), None, Some("drama")]));
assert_eq!(&result, &expected);
}
/// Helper function to create a shredded variant array representing lists
///
/// This creates an array that represents:
/// Row 0: ["comedy", "drama"] ([0] is shredded, [1] is shredded - perfectly shredded)
- /// Row 1: ["horror", null] ([0] is shredded, [1] is binary null - partially shredded)
- /// Row 2: ["comedy", "drama", "romance"] (perfectly shredded)
+ /// Row 1: ["horror", 123] ([0] is shredded, [1] is int - partially shredded)
///
/// The physical layout follows the shredding spec where:
/// - metadata: contains list metadata
/// - typed_value: StructArray with 0 index value
/// - value: contains fallback for
fn shredded_list_variant_array() -> ArrayRef {
- // Create the base metadata for lists
-
- // Could add this as an api for VariantList, like VariantList::from()
- fn build_list_metadata(vector: Vec<Variant>) -> (Vec<u8>, Vec<u8>) {
- let mut builder = parquet_variant::VariantBuilder::new();
- let mut list = builder.new_list();
- for value in vector {
- list.append_value(value);
- }
- list.finish();
- builder.finish()
- }
- let (metadata1, _) =
- build_list_metadata(vec![Variant::String("comedy"), Variant::String("drama")]);
+ // Create metadata array
+ let metadata_array =
+ BinaryViewArray::from_iter_values(std::iter::repeat_n(EMPTY_VARIANT_METADATA_BYTES, 2));
- let (metadata2, _) = build_list_metadata(vec![Variant::String("horror"), Variant::Null]);
+ // Building the typed_value ListArray
- let (metadata3, _) = build_list_metadata(vec![
- Variant::String("comedy"),
- Variant::String("drama"),
- Variant::String("romance"),
+ // Need a StructBuilder to create a ListBuilder
+ let fields = Fields::from(vec![
+ Field::new("value", DataType::Binary, true),
+ Field::new("typed_value", DataType::Utf8, true),
]);
+ let field_builders = vec![
+ make_builder(&DataType::Binary, 4),
+ make_builder(&DataType::Utf8, 4),
+ ];
+ let struct_builder = StructBuilder::new(fields, field_builders);
+
+ let mut builder = GenericListBuilder::<i32, StructBuilder>::new(struct_builder);
+
+ // Row 0 index 0
+ builder
+ .values()
+ .field_builder::<BinaryBuilder>(0)
+ .unwrap()
+ .append_null();
+ builder
+ .values()
+ .field_builder::<StringBuilder>(1)
+ .unwrap()
+ .append_value("comedy");
+ builder.values().append(true);
+
+ // Row 0 index 1
+ builder
+ .values()
+ .field_builder::<BinaryBuilder>(0)
+ .unwrap()
+ .append_null();
+ builder
+ .values()
+ .field_builder::<StringBuilder>(1)
+ .unwrap()
+ .append_value("drama");
+ builder.values().append(true);
+
+ // Next row
+ builder.append(true);
+
+ // Row 1 index 0
+ builder
+ .values()
+ .field_builder::<BinaryBuilder>(0)
+ .unwrap()
+ .append_null();
+ builder
+ .values()
+ .field_builder::<StringBuilder>(1)
+ .unwrap()
+ .append_value("horror");
+ builder.values().append(true);
+
+ // Row 1 index 1
+ let mut variant_builder = VariantBuilder::new();
+ variant_builder.append_value(123i32); // <------ couldn't find the right way to do it, used this as placeholder for binary
+ let (_, value) = variant_builder.finish();
+
+ builder
+ .values()
+ .field_builder::<BinaryBuilder>(0)
+ .unwrap()
+ .append_value(value);
Review Comment:
NOTE: The metadata column still needs a metadata entry for that row, else the different columns will have disagreeing row counts. Can use `EMPTY_VARIANT_METADATA_BYTES` if needed.
--
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] [WIP] Support Shredded Lists/Array in `variant_get` [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on code in PR #8354:
URL: https://github.com/apache/arrow-rs/pull/8354#discussion_r2360950977
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -1122,64 +1121,109 @@ mod test {
let result = variant_get(&array, options).unwrap();
// Should get StringArray
- let expected: ArrayRef = Arc::new(StringArray::from(vec![Some("comedy"), Some("drama")]));
+ let expected: ArrayRef =
+ Arc::new(StringArray::from(vec![Some("comedy"), None, Some("drama")]));
assert_eq!(&result, &expected);
}
/// Helper function to create a shredded variant array representing lists
///
/// This creates an array that represents:
/// Row 0: ["comedy", "drama"] ([0] is shredded, [1] is shredded - perfectly shredded)
- /// Row 1: ["horror", null] ([0] is shredded, [1] is binary null - partially shredded)
- /// Row 2: ["comedy", "drama", "romance"] (perfectly shredded)
+ /// Row 1: ["horror", 123] ([0] is shredded, [1] is int - partially shredded)
///
/// The physical layout follows the shredding spec where:
/// - metadata: contains list metadata
/// - typed_value: StructArray with 0 index value
/// - value: contains fallback for
fn shredded_list_variant_array() -> ArrayRef {
- // Create the base metadata for lists
-
- // Could add this as an api for VariantList, like VariantList::from()
- fn build_list_metadata(vector: Vec<Variant>) -> (Vec<u8>, Vec<u8>) {
- let mut builder = parquet_variant::VariantBuilder::new();
- let mut list = builder.new_list();
- for value in vector {
- list.append_value(value);
- }
- list.finish();
- builder.finish()
- }
- let (metadata1, _) =
- build_list_metadata(vec![Variant::String("comedy"), Variant::String("drama")]);
+ // Create metadata array
+ let metadata_array =
+ BinaryViewArray::from_iter_values(std::iter::repeat_n(EMPTY_VARIANT_METADATA_BYTES, 2));
- let (metadata2, _) = build_list_metadata(vec![Variant::String("horror"), Variant::Null]);
+ // Building the typed_value ListArray
- let (metadata3, _) = build_list_metadata(vec![
- Variant::String("comedy"),
- Variant::String("drama"),
- Variant::String("romance"),
+ // Need a StructBuilder to create a ListBuilder
+ let fields = Fields::from(vec![
+ Field::new("value", DataType::Binary, true),
+ Field::new("typed_value", DataType::Utf8, true),
]);
+ let field_builders = vec![
+ make_builder(&DataType::Binary, 4),
+ make_builder(&DataType::Utf8, 4),
+ ];
+ let struct_builder = StructBuilder::new(fields, field_builders);
+
+ let mut builder = GenericListBuilder::<i32, StructBuilder>::new(struct_builder);
+
+ // Row 0 index 0
+ builder
+ .values()
+ .field_builder::<BinaryBuilder>(0)
+ .unwrap()
+ .append_null();
+ builder
+ .values()
+ .field_builder::<StringBuilder>(1)
+ .unwrap()
+ .append_value("comedy");
+ builder.values().append(true);
+
+ // Row 0 index 1
+ builder
+ .values()
+ .field_builder::<BinaryBuilder>(0)
+ .unwrap()
+ .append_null();
+ builder
+ .values()
+ .field_builder::<StringBuilder>(1)
+ .unwrap()
+ .append_value("drama");
+ builder.values().append(true);
+
+ // Next row
+ builder.append(true);
+
+ // Row 1 index 0
+ builder
+ .values()
+ .field_builder::<BinaryBuilder>(0)
+ .unwrap()
+ .append_null();
+ builder
+ .values()
+ .field_builder::<StringBuilder>(1)
+ .unwrap()
+ .append_value("horror");
+ builder.values().append(true);
+
+ // Row 1 index 1
+ let mut variant_builder = VariantBuilder::new();
+ variant_builder.append_value(123i32); // <------ couldn't find the right way to do it, used this as placeholder for binary
+ let (_, value) = variant_builder.finish();
+
+ builder
+ .values()
+ .field_builder::<BinaryBuilder>(0)
+ .unwrap()
+ .append_value(value);
Review Comment:
Interesting... it wouldn't work for `VariantArrayBuilder` because `VariantArray` has to stop implementing `Array`. But `VariantValueArrayBuilder` produces a `BinaryViewArray` as output, so in theory there's nothing to stop it implementing the trait?
--
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] [WIP] Support Shredded Lists/Array in `variant_get` [arrow-rs]
Posted by "sdf-jkl (via GitHub)" <gi...@apache.org>.
sdf-jkl commented on code in PR #8354:
URL: https://github.com/apache/arrow-rs/pull/8354#discussion_r2356732960
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -1122,64 +1121,109 @@ mod test {
let result = variant_get(&array, options).unwrap();
// Should get StringArray
- let expected: ArrayRef = Arc::new(StringArray::from(vec![Some("comedy"), Some("drama")]));
+ let expected: ArrayRef =
+ Arc::new(StringArray::from(vec![Some("comedy"), None, Some("drama")]));
assert_eq!(&result, &expected);
}
/// Helper function to create a shredded variant array representing lists
///
/// This creates an array that represents:
/// Row 0: ["comedy", "drama"] ([0] is shredded, [1] is shredded - perfectly shredded)
- /// Row 1: ["horror", null] ([0] is shredded, [1] is binary null - partially shredded)
- /// Row 2: ["comedy", "drama", "romance"] (perfectly shredded)
+ /// Row 1: ["horror", 123] ([0] is shredded, [1] is int - partially shredded)
///
/// The physical layout follows the shredding spec where:
/// - metadata: contains list metadata
/// - typed_value: StructArray with 0 index value
/// - value: contains fallback for
fn shredded_list_variant_array() -> ArrayRef {
- // Create the base metadata for lists
-
- // Could add this as an api for VariantList, like VariantList::from()
- fn build_list_metadata(vector: Vec<Variant>) -> (Vec<u8>, Vec<u8>) {
- let mut builder = parquet_variant::VariantBuilder::new();
- let mut list = builder.new_list();
- for value in vector {
- list.append_value(value);
- }
- list.finish();
- builder.finish()
- }
- let (metadata1, _) =
- build_list_metadata(vec![Variant::String("comedy"), Variant::String("drama")]);
+ // Create metadata array
+ let metadata_array =
+ BinaryViewArray::from_iter_values(std::iter::repeat_n(EMPTY_VARIANT_METADATA_BYTES, 2));
- let (metadata2, _) = build_list_metadata(vec![Variant::String("horror"), Variant::Null]);
+ // Building the typed_value ListArray
- let (metadata3, _) = build_list_metadata(vec![
- Variant::String("comedy"),
- Variant::String("drama"),
- Variant::String("romance"),
+ // Need a StructBuilder to create a ListBuilder
+ let fields = Fields::from(vec![
+ Field::new("value", DataType::Binary, true),
+ Field::new("typed_value", DataType::Utf8, true),
]);
+ let field_builders = vec![
+ make_builder(&DataType::Binary, 4),
+ make_builder(&DataType::Utf8, 4),
+ ];
+ let struct_builder = StructBuilder::new(fields, field_builders);
+
+ let mut builder = GenericListBuilder::<i32, StructBuilder>::new(struct_builder);
+
+ // Row 0 index 0
+ builder
+ .values()
+ .field_builder::<BinaryBuilder>(0)
+ .unwrap()
+ .append_null();
+ builder
+ .values()
+ .field_builder::<StringBuilder>(1)
+ .unwrap()
+ .append_value("comedy");
+ builder.values().append(true);
+
+ // Row 0 index 1
+ builder
+ .values()
+ .field_builder::<BinaryBuilder>(0)
+ .unwrap()
+ .append_null();
+ builder
+ .values()
+ .field_builder::<StringBuilder>(1)
+ .unwrap()
+ .append_value("drama");
+ builder.values().append(true);
+
+ // Next row
+ builder.append(true);
+
+ // Row 1 index 0
+ builder
+ .values()
+ .field_builder::<BinaryBuilder>(0)
+ .unwrap()
+ .append_null();
+ builder
+ .values()
+ .field_builder::<StringBuilder>(1)
+ .unwrap()
+ .append_value("horror");
+ builder.values().append(true);
+
+ // Row 1 index 1
+ let mut variant_builder = VariantBuilder::new();
+ variant_builder.append_value(123i32); // <------ couldn't find the right way to do it, used this as placeholder for binary
+ let (_, value) = variant_builder.finish();
+
+ builder
+ .values()
+ .field_builder::<BinaryBuilder>(0)
+ .unwrap()
+ .append_value(value);
+ builder
+ .values()
+ .field_builder::<StringBuilder>(1)
+ .unwrap()
+ .append_null();
+ builder.values().append(true);
+
+ // Next row
+ builder.append(true);
+
+ let typed_value_array = builder.finish();
- // Create metadata array
- let metadata_array =
- BinaryViewArray::from_iter_values(vec![metadata1, metadata2, metadata3]);
-
- // Create the untyped value array
- let value_array = BinaryViewArray::from(vec![Variant::Null.as_u8_slice()]);
- // Maybe I should try with an actual primitive array
- let typed_value_array = StringArray::from(vec![
- Some("comedy"),
- Some("drama"),
- Some("horror"),
- Some("comedy"),
- Some("drama"),
- Some("romance"),
- ]);
// Build the main VariantArray
let main_struct = crate::variant_array::StructArrayBuilder::new()
- .with_field("metadata", Arc::new(metadata_array))
- .with_field("value", Arc::new(value_array))
- .with_field("typed_value", Arc::new(typed_value_array))
+ .with_field("metadata", Arc::new(metadata_array), false)
+ // .with_field("value", Arc::new(value_array), true)
Review Comment:
I think we don't need the `value` array since every value is a list in the array.
--
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] Support Shredded Lists/Array in `variant_get` [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #8354:
URL: https://github.com/apache/arrow-rs/pull/8354
--
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