Skip to content

Fix panic on lossy decimal to float casting: round to saturation for overflows #7887

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 10 commits into from
Jul 23, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion arrow-cast/src/cast/decimal.rs
Original file line number Diff line number Diff line change
Expand Up @@ -614,7 +614,11 @@ where
Ok(Arc::new(value_builder.finish()))
}

// Cast the decimal array to floating-point array
/// Cast a decimal array to a floating point array.
///
/// Conversion is lossy and follows standard floating point semantics. Values
/// that exceed the representable range become `INFINITY` or `-INFINITY` without
/// returning an error.
pub(crate) fn cast_decimal_to_float<D: DecimalType, T: ArrowPrimitiveType, F>(
array: &dyn Array,
op: F,
Expand Down
37 changes: 36 additions & 1 deletion arrow-cast/src/cast/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -603,6 +603,8 @@ fn timestamp_to_date32<T: ArrowTimestampType>(
/// * Temporal to/from backing Primitive: zero-copy with data type change
/// * `Float32/Float64` to `Decimal(precision, scale)` rounds to the `scale` decimals
/// (i.e. casting `6.4999` to `Decimal(10, 1)` becomes `6.5`).
/// * `Decimal` to `Float32/Float64` is lossy and values outside the representable
/// range become `INFINITY` or `-INFINITY` without error.
///
/// Unsupported Casts (check with `can_cast_types` before calling):
/// * To or from `StructArray`
Expand Down Expand Up @@ -891,7 +893,7 @@ pub fn cast_with_options(
scale,
from_type,
to_type,
|x: i256| x.to_f64().unwrap(),
|x: i256| decimal256_to_f64(x),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to do something similar for Decimal128 above?

Copy link
Contributor

@scovich scovich Jul 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm having a hard time understanding the semantics of i256::to_64, but it seems to be a provided method of impl ToPrimitive for i256, which

tries to convert through to_i64(), and failing that through to_u64()

In contrast, the Decimal128 case above is doing a rust cast x as f64, which I would assume already handles this case Just Fine (and probably doesn't even need to resort to +/- INF, because f64 has a dynamic range of ~2048 powers of two (from 2**-1023 to 2**1023) -- far more than enough to handle even Decimal256 (which has dynamic range of only 512 powers of two (from 2**-255 to 2**255)

If we're anyway trying to convert the value to f64, it seems like we should fix this bug by following the advice from the docs for ToPrimitive::to_f64:

Types implementing this trait should override this method if they can represent a greater range.

This shouldn't actually be very difficult. Assuming the two i128 parts together form the halves of one giant two's complement value, something like this should do the trick, and would always return a finite value.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(just be careful -- the least-significant leading zero is the sign bit, and mustn't be shifted out; I'm not 100% certain my code above handles that correctly, could be off-by-one)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cast_options,
)
}
Expand Down Expand Up @@ -1993,6 +1995,17 @@ where
}
}

/// Convert a [`i256`] to `f64` saturating to infinity on overflow.
fn decimal256_to_f64(v: i256) -> f64 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Saturating to INF seems a better solution than panic'ing

v.to_f64().unwrap_or_else(|| {
if v.is_negative() {
f64::NEG_INFINITY
} else {
f64::INFINITY
}
})
}

fn cast_to_decimal<D, M>(
array: &dyn Array,
base: M,
Expand Down Expand Up @@ -8660,6 +8673,28 @@ mod tests {
"did not find expected error '{expected_error}' in actual error '{err}'"
);
}
#[test]
fn test_cast_decimal256_to_f64_overflow() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this need to cover negative infinity?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@klion26

Good catch.
I amended the test.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you also please either add or ensure there is an existing test for casting Decimal128 (i128::MIN and i128::MAX to f64)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update, I didn't notice that @kosiew added a follow on ticket to track this work:

// Test positive overflow (positive infinity)
let array = vec![Some(i256::MAX)];
let array = create_decimal256_array(array, 76, 2).unwrap();
let array = Arc::new(array) as ArrayRef;

let result = cast(&array, &DataType::Float64).unwrap();
let result = result.as_primitive::<Float64Type>();
assert!(result.value(0).is_infinite());
assert!(result.value(0) > 0.0); // Positive infinity

// Test negative overflow (negative infinity)
let array = vec![Some(i256::MIN)];
let array = create_decimal256_array(array, 76, 2).unwrap();
let array = Arc::new(array) as ArrayRef;

let result = cast(&array, &DataType::Float64).unwrap();
let result = result.as_primitive::<Float64Type>();
assert!(result.value(0).is_infinite());
assert!(result.value(0) < 0.0); // Negative infinity
}

#[test]
fn test_cast_decimal128_to_decimal128_negative_scale() {
Expand Down
Loading