klion26 commented on code in PR #9518:
URL: https://github.com/apache/arrow-rs/pull/9518#discussion_r2894218164
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -836,22 +814,20 @@ mod test {
macro_rules! perfectly_shredded_variant_array_fn {
($func:ident, $typed_value_gen:expr) => {
fn $func() -> ArrayRef {
- // At the time of writing, the `VariantArrayBuilder` does not
support shredding.
- // so we must construct the array manually. see
https://github.com/apache/arrow-rs/issues/7895
+ // Prefer producing fixtures with shred_variant from
unshredded input.
+ // Fall back for non-shreddable test-only Arrow types (e.g.
LargeUtf8/LargeBinary/Null).
+ let typed_value: ArrayRef = Arc::new($typed_value_gen());
+ if let Ok(unshredded) = cast_to_variant(typed_value.as_ref()) {
Review Comment:
In the Rust world, which do we prefer,? The current and the following way.
```
if let Some(shredded) = cast_to_variant(typed_value.as_ref())
.ok()
and_then(|unshredded| {
shred_variant(&unshredded,
typed_value.data_type()).ok()
})
{
return shredded.into();
}
```
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -466,44 +468,20 @@ mod test {
macro_rules! partially_shredded_variant_array_gen {
($func_name:ident, $typed_value_array_gen: expr) => {
fn $func_name() -> ArrayRef {
- // At the time of writing, the `VariantArrayBuilder` does not
support shredding.
- // so we must construct the array manually. see
https://github.com/apache/arrow-rs/issues/7895
- let (metadata, string_value) = {
- let mut builder = parquet_variant::VariantBuilder::new();
- builder.append_value("n/a");
- builder.finish()
- };
-
- let nulls = NullBuffer::from(vec![
- true, // row 0 non null
- false, // row 1 is null
- true, // row 2 non null
- true, // row 3 non null
- ]);
-
- // metadata is the same for all rows
- let metadata =
BinaryViewArray::from_iter_values(std::iter::repeat_n(&metadata, 4));
-
- // See
https://docs.google.com/document/d/1pw0AWoMQY3SjD7R4LgbPvMjG_xSCtXp3rZHkVp9jpZ4/edit?disco=AAABml8WQrY
- // about why row1 is an empty but non null, value.
- let values = BinaryViewArray::from(vec![
- None, // row 0 is shredded, so no value
- Some(b"" as &[u8]), // row 1 is null, so empty value
(why?)
- Some(&string_value), // copy the string value "N/A"
- None, // row 3 is shredded, so no value
- ]);
-
- let typed_value = $typed_value_array_gen();
-
- let struct_array = StructArrayBuilder::new()
- .with_field("metadata", Arc::new(metadata), false)
- .with_field("typed_value", Arc::new(typed_value), true)
- .with_field("value", Arc::new(values), true)
- .with_nulls(nulls)
- .build();
- ArrayRef::from(
- VariantArray::try_new(&struct_array).expect("should create
variant array"),
- )
+ // Build the mixed input [typed, null, "n/a", typed] and let
shred_variant
+ // generate the shredded fixture for the requested type.
Review Comment:
Do we need to move this above the macro(L468) and enhance this like the
comment as L1625, add this, because that we'll still the shredded type to
*not* support `Utf8`("n/a" is `Utf8`)
##########
parquet-variant-compute/src/variant_get.rs:
##########
Review Comment:
Could you please also improve this, "n/a" is also string.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
