-
Notifications
You must be signed in to change notification settings - Fork 980
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
Changes from all commits
cb41f13
89d360e
cf9268d
8ca4168
1bbfae3
c658501
8e74ff2
3217813
ea83122
05ace0c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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` | ||
|
@@ -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), | ||
cast_options, | ||
) | ||
} | ||
|
@@ -1993,6 +1995,17 @@ where | |
} | ||
} | ||
|
||
/// Convert a [`i256`] to `f64` saturating to infinity on overflow. | ||
fn decimal256_to_f64(v: i256) -> f64 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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, | ||
|
@@ -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() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this need to cover negative infinity? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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() { | ||
|
There was a problem hiding this comment.
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?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
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, whichIn contrast, the
Decimal128
case above is doing a rust castx 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 evenDecimal256
(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: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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i256::to_f64
to handle overflow #7980 to track this idea