scovich commented on code in PR #9610:
URL: https://github.com/apache/arrow-rs/pull/9610#discussion_r3076231921
##########
parquet-variant-compute/src/shred_variant.rs:
##########
@@ -2213,7 +2221,10 @@ mod tests {
assert!(value_field.is_null(2));
assert!(value_field.is_valid(3));
assert_eq!(
- Variant::new(result.metadata_field().value(3),
value_field.value(3)),
+ Variant::new(
+ binary_array_value(result.metadata_field().as_ref(),
3).unwrap(),
+ binary_array_value(value_field.as_ref(), 3).unwrap()
+ ),
Review Comment:
`variant_from_arrays_at`?
(several more below)
##########
parquet-variant-compute/src/variant_array_builder.rs:
##########
@@ -639,7 +609,7 @@ mod test {
let array2 = VariantArray::from_parts(
array.metadata_field().clone(),
- Some(value_builder.build().unwrap()),
+ Some(Arc::new(value_builder.build().unwrap()) as ArrayRef),
Review Comment:
nit: Another unnecessary cast.
##########
parquet-variant-compute/src/unshred_variant.rs:
##########
@@ -347,7 +360,17 @@ trait AppendToVariantBuilder: Array {
macro_rules! handle_unshredded_case {
($self:expr, $builder:expr, $metadata:expr, $index:expr,
$partial_shredding:expr) => {{
let value = $self.value.as_ref().filter(|v| v.is_valid($index));
- let value = value.map(|v|
Variant::new_with_metadata($metadata.clone(), v.value($index)));
+ let value = value
+ .map(|v| {
+ let bytes = binary_array_value(v.as_ref(),
$index).ok_or_else(|| {
+ ArrowError::InvalidArgumentError(format!(
+ "value field must be a binary-like array, instead got
{}",
+ v.data_type(),
+ ))
+ })?;
+ Result::Ok(Variant::new_with_metadata($metadata.clone(),
bytes))
+ })
+ .transpose()?;
Review Comment:
Ah, we have a function returning Result, and the map+transpose is doing
`Option` -> `Option<Option>` -> `Option<Result>` -> `Result<Option>`.
Awkward/annoying mismatch.
It would be nice if `binary_array_value` returned Result instead of Option.
It's ~always an error AFAICT, only the error message changes (value, metadata,
etc). Then most call sites would be simpler. But this call site would still not
be very simple, so I'm not sure it's worth the effort to change that helper or
this site here.
##########
parquet-variant-compute/src/shred_variant.rs:
##########
@@ -96,7 +96,7 @@ pub fn shred_variant(array: &VariantArray, as_type:
&DataType) -> Result<Variant
let (value, typed_value, nulls) = builder.finish()?;
Ok(VariantArray::from_parts(
array.metadata_field().clone(),
- Some(value),
+ Some(Arc::new(value) as ArrayRef),
Review Comment:
Note: The main reason I pick on casts is because they're too powerful and
can easily be quietly misused. Ideally, a cast is only present if actually
required, as a clear marker to reviewer/reader that something unusual is going
on.
Similar story for unnecessary type annotations, except those are just
noise/bloat rather than a correctness risk.
My AI coding assistant sure loves splattering both all over the code it
writes tho, super annoying.
--
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]
