sdf-jkl commented on code in PR #10598:
URL: https://github.com/apache/arrow-rs/pull/10598#discussion_r3752409423
##########
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!(
+ "VariantArray does not yet support typed_value type: {}",
+ typed_value.data_type()
+ ))),
Review Comment:
These types being composite make `try_value`/`value` returning borrowed
Variant for them painful.
We should leave a suggestion to unshred the array before extracting the
value for now and decide if we want to proceed with this or defer.
```suggestion
//
// Composite shredded values may require combining `value` and
// `typed_value` and allocating new encoded bytes. `try_value`
returns
// borrowed Variant, so callers must unshred the array first.
_ => Err(ArrowError::NotYetImplemented(format!(
"VariantArray::try_value cannot materialize typed_value of type {} \
as a borrowed Variant; call unshred_variant first",
typed_value.data_type()
))),
```
##########
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:
@raghav-reglobe thanks for catching this. I think I was working on smth
related to this earlier, but it was not documented in an issue.
This PR can close the bug of incorrectly returning Null for unsupported
types. I will create a separate issue to track supporting composite types for
`typed_value_to_variant`
##########
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)
Review Comment:
We shouldn't add a timeline narrating comment about what used to be there.
--
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]
