Jefffrey commented on code in PR #10707:
URL: https://github.com/apache/arrow-rs/pull/10707#discussion_r3800857972
##########
arrow-cast/src/cast/mod.rs:
##########
@@ -348,6 +348,20 @@ pub fn cast(array: &dyn Array, to_type: &DataType) ->
Result<ArrayRef, ArrowErro
cast_with_options(array, to_type, &CastOptions::default())
}
+/// Convert an integer to a decimal native value without wrapping.
+///
+/// `AsPrimitive` / `as` silently truncates when the source is wider than `M`
+/// (for example `5_000_000_000i64 as i32`). All integer sources fit in `i128`
+/// losslessly, which [`DecimalCast`] then converts to the decimal native type
+/// with a range check.
Review Comment:
```suggestion
/// with a range check. For types that always fit (e.g. `i64` to
`Decimal128`) this
/// should get optimized to being equivalent to `i64 as i128`.
```
##########
arrow-cast/src/cast/mod.rs:
##########
@@ -360,43 +374,62 @@ fn cast_integer_to_decimal<
cast_options: &CastOptions,
) -> Result<ArrayRef, ArrowError>
where
- <T as ArrowPrimitiveType>::Native: AsPrimitive<M>,
- M: ArrowNativeTypeOp,
+ <T as ArrowPrimitiveType>::Native: ArrowNativeTypeOp + Into<i128>,
+ M: ArrowNativeTypeOp + DecimalCast,
{
- let scale_factor = base.pow_checked(scale.unsigned_abs() as
u32).map_err(|_| {
+ let overflow = |v: T::Native| {
ArrowError::CastError(format!(
- "Cannot cast to {:?}({}, {}). The scale causes overflow.",
+ "Cannot cast to {}({precision}, {scale}). Overflowing on {v:?}",
D::PREFIX,
- precision,
- scale,
))
- })?;
+ };
let array = if scale < 0 {
- match cast_options.safe {
- true => array.unary_opt::<_, D>(|v| {
- v.as_()
- .div_checked(scale_factor)
+ // Compute the scale factor once in the source type. Scaling before the
+ // checked conversion permits values that only fit the decimal native
+ // type after scaling.
+ let scale_factor = T::Native::usize_as(10)
+ .pow_checked(scale.unsigned_abs() as u32)
+ .ok();
+
+ match (scale_factor, cast_options.safe) {
+ (Some(scale_factor), true) => array.unary_opt::<_, D>(|v| {
+ v.div_checked(scale_factor)
.ok()
+ .and_then(integer_to_decimal_native::<_, M>)
.and_then(|v| (D::is_valid_decimal_precision(v,
precision)).then_some(v))
}),
- false => array.try_unary::<_, D, _>(|v| {
- v.as_()
- .div_checked(scale_factor)
+ (Some(scale_factor), false) => array.try_unary::<_, D, _>(|v| {
+ v.div_checked(scale_factor)
+ .ok()
+ .and_then(integer_to_decimal_native::<_, M>)
+ .ok_or_else(|| overflow(v))
.and_then(|v| D::validate_decimal_precision(v, precision,
scale).map(|()| v))
})?,
+ // A scale factor that overflows the source type is larger than all
+ // source values, so integer division produces zero.
Review Comment:
```suggestion
// A scale factor that overflows the source type is larger than
all
// source values, so integer division produces zero.
//
// For a well formed decimal scale, this path should never be
reachable.
```
##########
arrow-cast/src/cast/mod.rs:
##########
@@ -10622,6 +10647,121 @@ mod tests {
assert!(casted_array.is_err());
}
+ #[test]
+ fn test_cast_integer_to_decimal32_does_not_truncate() {
+ let array = Int64Array::from(vec![5_000_000_000i64, 10_000_000_000,
42]);
+ let safe = CastOptions {
+ safe: true,
+ format_options: FormatOptions::default(),
+ };
+ let unsafe_opts = CastOptions {
+ safe: false,
+ format_options: FormatOptions::default(),
+ };
+
+ let result = cast_with_options(&array, &DataType::Decimal32(9, 0),
&safe).unwrap();
+ let result = result.as_primitive::<Decimal32Type>();
+ assert!(
+ result.is_null(0),
+ "5e9 must not wrap to {}",
+ result.value(0)
+ );
+ assert!(result.is_null(1));
+ assert_eq!(result.value(2), 42);
+
+ let err = cast_with_options(&array, &DataType::Decimal32(9, 0),
&unsafe_opts)
+ .unwrap_err()
+ .to_string();
+ assert!(
+ err.contains("5000000000"),
+ "unsafe error should report the original value, got {err}"
+ );
+ assert!(
+ !err.contains("705032704"),
Review Comment:
could we assert more of the error message; this isnt clearly obvious that
we're getting the error we expect
##########
arrow-cast/src/cast/mod.rs:
##########
@@ -2410,14 +2443,6 @@ fn cast_to_decimal<D, M>(
where
D: DecimalType + ArrowPrimitiveType<Native = M>,
M: ArrowNativeTypeOp + DecimalCast,
- u8: num_traits::AsPrimitive<M>,
- u16: num_traits::AsPrimitive<M>,
- u32: num_traits::AsPrimitive<M>,
- u64: num_traits::AsPrimitive<M>,
- i8: num_traits::AsPrimitive<M>,
- i16: num_traits::AsPrimitive<M>,
- i32: num_traits::AsPrimitive<M>,
- i64: num_traits::AsPrimitive<M>,
Review Comment:
nice 👍
--
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]
