klion26 commented on code in PR #8719:
URL: https://github.com/apache/arrow-rs/pull/8719#discussion_r2465526907
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -3584,4 +3587,53 @@ mod test {
"Failed to cast to Decimal256(precision=76, scale=39) from variant
Decimal16"
));
}
+
+
perfectly_shredded_variant_array_fn!(perfectly_shredded_invalid_time_variant_array,
|| {
+ // 86401000000 is invalid for Time64Microsecond (max is 86400000000)
+ Time64MicrosecondArray::from(vec![
+ Some(86401000000),
+ Some(86401000000),
+ Some(86401000000),
+ ])
+ });
+
+ #[test]
+ fn test_variant_get_error_when_cast_failure_and_safe_false() {
+ let variant_array = perfectly_shredded_invalid_time_variant_array();
+
+ let field = Field::new("result",
DataType::Time64(TimeUnit::Microsecond), true);
+ let cast_options = CastOptions {
+ safe: false, // Will error on cast failure
+ ..Default::default()
+ };
+ let options = GetOptions::new()
+ .with_as_type(Some(FieldRef::from(field)))
+ .with_cast_options(cast_options);
+ let err = variant_get(&variant_array, options).unwrap_err();
+ assert!(
+ err.to_string().contains(
+ "Cast error: Cast failed: Invalid microsecond from midnight:
86401000000"
+ )
+ );
+ }
+
+ #[test]
+ fn test_variant_get_return_null_when_cast_failure_and_safe_true() {
+ let variant_array = perfectly_shredded_invalid_time_variant_array();
+
+ let field = Field::new("result",
DataType::Time64(TimeUnit::Microsecond), true);
+ let cast_options = CastOptions {
+ safe: true, // Will Variant::Null on cast failure
+ ..Default::default()
+ };
+ let options = GetOptions::new()
+ .with_as_type(Some(FieldRef::from(field)))
+ .with_cast_options(cast_options);
+ let result = variant_get(&variant_array, options).unwrap();
+ assert_eq!(3, result.len());
+
+ for i in 0..3 {
Review Comment:
The result is all null instead of an Array of `Variant::Null` because
- `builder.append_value(target.try_value(i)).unwrap_or(Variant::Null)` in
`shredded_get_path` will call `builder.append_value` with `Variant::Null`
- the builder is `VariantToPrimitiveArrowRowBuilder`, and will call
`T::from_variant(value)` in `builder.append_value`
- then will call `PrimitiveFromVariant` for `Time64Microsecond`
- This will `Variant::as_time_utc` currently, and will return `None` as the
input is not `Variant::Time(_)`
Not sure if we need to change the return value of `variant_get` here from
`result.is_null(i)` to `Variant::Null`
--
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]
