-
Notifications
You must be signed in to change notification settings - Fork 866
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
Round instead of Truncate while casting float to decimal #3000
Round instead of Truncate while casting float to decimal #3000
Conversation
Some(1.123_456_4), // round down | ||
Some(1.123_456_7), // round up |
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 change the test cases to
- round down
- round up
Some(1123456_i128), // round down | ||
Some(1123457_i128), // round up |
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.
and here's the result
arrow/src/compute/kernels/cast.rs
Outdated
#[test] | ||
#[cfg(not(feature = "force_validate"))] | ||
fn test_cast_f64_to_decimal128() { | ||
// to reproduce https://github.com/apache/arrow-rs/issues/2997 | ||
|
||
let decimal_type = DataType::Decimal128(18, 2); | ||
let array = Float64Array::from(vec![Some(0.0699999999)]); | ||
let array = Arc::new(array) as ArrayRef; | ||
generate_cast_test_case!( | ||
&array, | ||
Decimal128Array, | ||
&decimal_type, | ||
vec![Some(7_i128),] | ||
); | ||
} |
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.
to reproduce the issue #2997
thanks for the contribution, can you:
|
arrow/src/compute/kernels/cast.rs
Outdated
/// * Casting from `float32/float64` to `Decimal(precision, scale)` rounds to the `scale` decimals | ||
/// (i.e. casting 6.4999 to Decimal(10, 1) becomes 6.5). This is the breaking change from `26.0.0`. | ||
/// It used to truncate it instead of round (i.e. outputs 6.4 instead) |
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.
@jimexist i updated the doc here
let decimal_type = DataType::Decimal128(18, 2); | ||
let array = Float64Array::from(vec![ | ||
Some(0.0699999999), | ||
Some(0.0659999999), | ||
Some(0.0650000000), | ||
Some(0.0649999999), |
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.
@jimexist added the 0.065 case which will be rounded up to 0.07
thank you @jimexist |
Benchmark runs are scheduled for baseline = 24afac4 and contender = 61cf6f7. 61cf6f7 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
Which issue does this PR close?
Closes #2997
Rationale for this change
casting 0.06999999 to
Decimal(18, 2)
should be 0.07 instead of 0.06What changes are included in this PR?
add
.round()
before casting to intAre there any user-facing changes?
yes, we round the floating number instead of truncating it