raghav-reglobe commented on code in PR #10598:
URL: https://github.com/apache/arrow-rs/pull/10598#discussion_r3744780189
##########
parquet-variant-compute/src/variant_array.rs:
##########
@@ -1152,17 +1152,13 @@ fn typed_value_to_variant(typed_value: &ArrayRef,
index: usize) -> Result<Varian
}
// todo other types here (note this is very similar to
cast_to_variant.rs)
// so it would be great to figure out how to share this code
- _ => {
- // We shouldn't panic in production code, but this is a
- // placeholder until we implement more types
- // https://github.com/apache/arrow-rs/issues/8091
- debug_assert!(
- false,
- "Unsupported typed_value type: {}",
- typed_value.data_type()
- );
- Ok(Variant::Null)
- }
+ //
+ // Returning Variant::Null here would silently misreport stored values
+ // as null in release builds
(https://github.com/apache/arrow-rs/issues/10597)
+ _ => Err(ArrowError::NotYetImplemented(format!(
Review Comment:
Still to implement: shredded objects (`Struct`) and shredded arrays (the
`List` family) — both legal per the shredding spec, and reachable today since
`shred_variant` itself produces them (the new test builds the `Struct` case
that way). Truly-illegal types can't reach this arm: `VariantArray::try_new`
already rejects them at construction via `canonicalize_and_verify_data_type`
(`Illegal shredded value type`, `InvalidArgumentError`). So by the time
`typed_value_to_variant` runs, the type is spec-legal and the only gap is the
missing conversion — which is why `NotYetImplemented` seemed like the accurate
classification. Implementing object/list here needs owned bytes (the borrowed
`Variant<'_, '_>` return can't express an assembled object), so I left that for
a follow-up.
--
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]
