klion26 commented on code in PR #8719:
URL: https://github.com/apache/arrow-rs/pull/8719#discussion_r2468075130
##########
parquet-variant-compute/src/type_conversion.rs:
##########
@@ -302,6 +302,19 @@ macro_rules! generic_conversion_single_value {
}
pub(crate) use generic_conversion_single_value;
+macro_rules! generic_conversion_single_value_with_result {
+ ($t:ty, $method:ident, $cast_fn:expr, $input:expr, $index:expr) => {{
+ let arr = $input.$method::<$t>();
+ let v = arr.value($index);
+ match ($cast_fn)(v) {
+ Ok(var) => Ok(Variant::from(var)),
+ Err(e) => Err(ArrowError::CastError(format!("Cast failed: {e}"))),
Review Comment:
Fixed
##########
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:
Seems we need to return an arrow null instead of `Variant::Null` here, as
the doc of `CastOptions`, `/// If true, return error on conversion failure. If
false, insert null for failed conversions.`, seems the return
--
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]
