Check overflow while casting floating point value to decimal128 #3021
Conversation
arrow-cast/src/cast.rs
Outdated
| if cast_options.safe { | ||
| let iter = array.iter().map(|v| { | ||
| v.and_then(|v| { | ||
| let mul_v = (mul * v.as_()).round() as i128; |
There was a problem hiding this comment.
If we don't need to validate against precision, the multiplication of f64 itself won't overflow.
There was a problem hiding this comment.
The only case that might be "overflow" here is an infinity value. After converting to i128, it is i128::MAX that is over the range of decimal 128.
arrow-cast/src/cast.rs
Outdated
| if precision <= DECIMAL128_MAX_PRECISION | ||
| && (mul_v > MAX_DECIMAL_FOR_EACH_PRECISION[precision as usize - 1] | ||
| || mul_v < MIN_DECIMAL_FOR_EACH_PRECISION[precision as usize - 1]) |
There was a problem hiding this comment.
I don't think we need to validate precision, as we don't in other places. This is explicitly an opt-in, we only need to error if the underlying value is truncated/overflows - i.e. data loss has occurred.
arrow-cast/src/cast.rs
Outdated
| &DataType::Decimal128(38, 30), | ||
| &CastOptions { safe: false }, | ||
| ); | ||
| assert!(casted_array.is_err()); |
There was a problem hiding this comment.
Typically we do something like
let err = operation.unwrap_err().to_string();
assert!(err.contains("EXPECTED), "{}", err)
To avoid false positives
arrow-cast/src/cast.rs
Outdated
| v | ||
| ))) | ||
| } else { | ||
| Ok(mul_v as i128) |
There was a problem hiding this comment.
I'm still confused as to how this can't overflow?
If the float value was i128::MAX any scale greater than 1 would result in this conversion overflowing?
There was a problem hiding this comment.
Converting a floating value larger than i128::MAX to i128 will get i128::MAX.
There was a problem hiding this comment.
It won't overflow and won't wrapping back to smaller value.
There was a problem hiding this comment.
I believe we also return errors for truncation when casting from say u64 to u32? I think it would a bit surprising to return an error for wrapping overflow but not saturating?
arrow-cast/src/cast.rs
Outdated
| ))) | ||
| } else { | ||
| let integer = mul_v as i128; | ||
| if integer == i128::MAX || integer == i128::MIN { |
There was a problem hiding this comment.
Can we use TryInto here, or some other fallible conversion instead of as, I think this will be faster and also avoid a false positive on i128::MAX?
There was a problem hiding this comment.
Do you think we need to implement From<f64> for i128?
error[E0277]: the trait bound `i128: From<f64>` is not satisfied
There was a problem hiding this comment.
Oh... It seems support for this is unstable rust-lang/rust#67057
Perhaps we could use https://docs.rs/num/latest/num/trait.ToPrimitive.html ?
This appears to be correctly checked https://docs.rs/num-traits/0.2.14/src/num_traits/cast.rs.html#310. We could also copy this logic
| .try_unary::<_, Decimal128Type, _>(|v| { | ||
| mul.mul_checked(v.as_()).and_then(|value| { | ||
| let mul_v = value.round(); | ||
| let integer: i128 = mul_v.to_i128().ok_or_else(|| { |
There was a problem hiding this comment.
It covers f64::INFINITY, f64::MAX, etc. cases.
Co-authored-by: Raphael Taylor-Davies <[email protected]>
|
Benchmark runs are scheduled for baseline = 4f525fe and contender = 108e7d2. 108e7d2 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
|
Thanks @tustvold |
Which issue does this PR close?
Closes #3020.
Rationale for this change
What changes are included in this PR?
Are there any user-facing changes?