scovich commented on code in PR #9599:
URL: https://github.com/apache/arrow-rs/pull/9599#discussion_r3003317857
##########
parquet-variant-compute/src/shred_variant.rs:
##########
@@ -669,6 +703,12 @@ mod tests {
use std::sync::Arc;
use uuid::Uuid;
+ const NULL_VALUES: [NullValue; 3] = [
+ NullValue::TopLevelVariant,
+ NullValue::ObjectField,
+ NullValue::ArrayElement,
+ ];
+
Review Comment:
arrow-avro uses strum_macros crate, whose
[EnumIter](https://docs.rs/strum_macros/latest/strum_macros/derive.EnumIter.html)
seems relevant? Are we willing to take a dep on that crate tho?
##########
parquet-variant-compute/src/variant_to_arrow.rs:
##########
@@ -714,22 +707,22 @@ macro_rules! define_variant_to_primitive_builder {
}
fn append_value(&mut self, $value: &Variant<'_, '_>) ->
Result<bool> {
- if let Some(v) = $value_transform {
+ if let Some(v) = variant_cast_with_options(
+ $value,
+ self.cast_options,
+ |$value| $value_transform,
+ |value| {
+ format!(
+ "Failed to extract primitive of type {type_name}
from variant {value:?} at path VariantPath([])",
+ type_name = $type_name
+ )
+ },
+ )? {
self.builder.append_value(v);
Ok(true)
} else {
- if treat_invalid_as_null(self.cast_options, $value) {
- // Safe casting (or variant null): append null on
conversion failure
- self.builder.append_null();
- Ok(false)
- } else {
- // Unsafe casting: return error on conversion failure
- Err(ArrowError::CastError(format!(
- "Failed to extract primitive of type {} from
variant {:?} at path VariantPath([])",
- $type_name,
- $value
- )))
- }
+ self.builder.append_null();
+ Ok(false)
Review Comment:
All the call sites follow the same pattern:
```rust
if let Some(foo) = variant_cast_with_options(..., |value| { ... error
message ... })? {
... handle value ...
} else {
... handle NULL ...
}
```
Not only is it super bulky (each one spanning many lines), it's awkward
because we're passing a lambda whose result is returned and immediately
processed by `?`. It seems like we'd be better off just returning a placeholder
`Result<..., ()>` and building the error directly?
```rust
match variant_cast_with_options(...) {
Ok(Some(foo)) => { ... handle value ... }
Ok(None) => { ... handle NULL ... }
Err(()) => { return Err(...) }
}
```
or if we want to keep using `?`, just leverage `map_err`?
```rust
let value = variant_cast_with_options(...)
.map_err(|_| ... )?;
if let Some(foo) = value {
... handle value ...
} else {
... handle NULL ...
}
```
(but the three-way match seems more compact to me 🤷)
The goal is maximum readability... the current code is hard to read.
--
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]
