You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "himadripal (via GitHub)" <gi...@apache.org> on 2024/12/04 17:28:37 UTC
[PR] decimal conversion looses value on lower precision, throws error now … [arrow-rs]
himadripal opened a new pull request, #6836:
URL: https://github.com/apache/arrow-rs/pull/6836
# Which issue does this PR close?
#6833
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
Closes #.
# Rationale for this change
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
# What changes are included in this PR?
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
# Are there any user-facing changes?
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!---
If there are any breaking changes to public APIs, please call them out.
-->
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: decimal conversion looses value on lower precision [arrow-rs]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #6836:
URL: https://github.com/apache/arrow-rs/pull/6836#issuecomment-2518139003
There is a regression in `test_cast_decimal128_to_decimal256_negative`. The new validation check is correctly throwing an error, but it looks like we also need to add this validation when creating decimal arrays since the current test is creating invalid arrays before the cast:
```rust
let array = vec![Some(i128::MAX), Some(i128::MIN)];
let input_decimal_array = create_decimal_array(array, 10, 3).unwrap();
```
I would expect this to fail, so we probably need to add the same validation there.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: decimal conversion looses value on lower precision [arrow-rs]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #6836:
URL: https://github.com/apache/arrow-rs/pull/6836#discussion_r1870561699
##########
arrow-cast/src/cast/decimal.rs:
##########
@@ -155,28 +163,24 @@ where
T: DecimalType,
T::Native: DecimalCast + ArrowNativeTypeOp,
{
- let array: PrimitiveArray<T> = match input_scale.cmp(&output_scale) {
- Ordering::Equal => {
- // the scale doesn't change, the native value don't need to be changed
- array.clone()
- }
- Ordering::Greater => convert_to_smaller_scale_decimal::<T, T>(
- array,
- input_scale,
- output_precision,
- output_scale,
- cast_options,
- )?,
- Ordering::Less => {
- // input_scale < output_scale
+ let array: PrimitiveArray<T> = if input_scale <= output_scale {
+ // input_scale <= output_scale
+ // the scale doesn't change, but precision may change and cause overflow
Review Comment:
Why don't we also check precision and skip `convert_to_bigger_or_equal_scale_decimal` if it won't overflow?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: decimal conversion looses value on lower precision [arrow-rs]
Posted by "himadripal (via GitHub)" <gi...@apache.org>.
himadripal commented on PR #6836:
URL: https://github.com/apache/arrow-rs/pull/6836#issuecomment-2523458834
Can anyone please let the build run - looks like its waiting for approval.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: decimal conversion looses value on lower precision [arrow-rs]
Posted by "himadripal (via GitHub)" <gi...@apache.org>.
himadripal commented on code in PR #6836:
URL: https://github.com/apache/arrow-rs/pull/6836#discussion_r1870625712
##########
arrow-cast/src/cast/decimal.rs:
##########
@@ -155,28 +163,24 @@ where
T: DecimalType,
T::Native: DecimalCast + ArrowNativeTypeOp,
{
- let array: PrimitiveArray<T> = match input_scale.cmp(&output_scale) {
- Ordering::Equal => {
- // the scale doesn't change, the native value don't need to be changed
- array.clone()
- }
- Ordering::Greater => convert_to_smaller_scale_decimal::<T, T>(
- array,
- input_scale,
- output_precision,
- output_scale,
- cast_options,
- )?,
- Ordering::Less => {
- // input_scale < output_scale
+ let array: PrimitiveArray<T> = if input_scale <= output_scale {
+ // input_scale <= output_scale
+ // the scale doesn't change, but precision may change and cause overflow
Review Comment:
lets assume, we are converting `12345` from (5, 3) to (6,3), then it needs to be `123450` - if we do array.clone() as per original code- wondering how it will add the 0 at the end.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: decimal conversion looses value on lower precision [arrow-rs]
Posted by "himadripal (via GitHub)" <gi...@apache.org>.
himadripal commented on code in PR #6836:
URL: https://github.com/apache/arrow-rs/pull/6836#discussion_r1870625712
##########
arrow-cast/src/cast/decimal.rs:
##########
@@ -155,28 +163,24 @@ where
T: DecimalType,
T::Native: DecimalCast + ArrowNativeTypeOp,
{
- let array: PrimitiveArray<T> = match input_scale.cmp(&output_scale) {
- Ordering::Equal => {
- // the scale doesn't change, the native value don't need to be changed
- array.clone()
- }
- Ordering::Greater => convert_to_smaller_scale_decimal::<T, T>(
- array,
- input_scale,
- output_precision,
- output_scale,
- cast_options,
- )?,
- Ordering::Less => {
- // input_scale < output_scale
+ let array: PrimitiveArray<T> = if input_scale <= output_scale {
+ // input_scale <= output_scale
+ // the scale doesn't change, but precision may change and cause overflow
Review Comment:
lets assume, we are converting this `12345` from (5, 3) to (6,3), then it needs to be `123450` - if we do array.clone() - wondering how it will add the 0 at the end.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: decimal conversion looses value on lower precision [arrow-rs]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on code in PR #6836:
URL: https://github.com/apache/arrow-rs/pull/6836#discussion_r1870235805
##########
arrow-cast/src/cast/decimal.rs:
##########
@@ -112,8 +112,19 @@ where
};
Ok(match cast_options.safe {
- true => array.unary_opt(f),
- false => array.try_unary(|x| f(x).ok_or_else(|| error(x)))?,
+ true => {
+ array.unary_opt(|x| {
+ f(x).filter(|v| O::is_valid_decimal_precision(*v, output_precision))
+ })
+ }
+ false => {
+ array.try_unary(|x| {
+ f(x).ok_or_else(|| error(x))
+ .and_then(|v|{
+ O:: validate_decimal_precision(v, output_precision).map(|_| v)
Review Comment:
Why not do this when computing the value (the code above), and only when this can fail?
(for example widening precision cannot fail, so no need to spend cycles on validating the produced values)
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: decimal conversion looses value on lower precision [arrow-rs]
Posted by "himadripal (via GitHub)" <gi...@apache.org>.
himadripal commented on code in PR #6836:
URL: https://github.com/apache/arrow-rs/pull/6836#discussion_r1870661505
##########
arrow-cast/src/cast/decimal.rs:
##########
@@ -155,28 +163,24 @@ where
T: DecimalType,
T::Native: DecimalCast + ArrowNativeTypeOp,
{
- let array: PrimitiveArray<T> = match input_scale.cmp(&output_scale) {
- Ordering::Equal => {
- // the scale doesn't change, the native value don't need to be changed
- array.clone()
- }
- Ordering::Greater => convert_to_smaller_scale_decimal::<T, T>(
- array,
- input_scale,
- output_precision,
- output_scale,
- cast_options,
- )?,
- Ordering::Less => {
- // input_scale < output_scale
+ let array: PrimitiveArray<T> = if input_scale <= output_scale {
+ // input_scale <= output_scale
+ // the scale doesn't change, but precision may change and cause overflow
Review Comment:
Yes, you are right. How to I get the input_precision to check against output_precision? fn signature only has output_precision
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: decimal conversion looses value on lower precision [arrow-rs]
Posted by "himadripal (via GitHub)" <gi...@apache.org>.
himadripal commented on code in PR #6836:
URL: https://github.com/apache/arrow-rs/pull/6836#discussion_r1871983398
##########
arrow-cast/src/cast/decimal.rs:
##########
@@ -155,28 +163,24 @@ where
T: DecimalType,
T::Native: DecimalCast + ArrowNativeTypeOp,
{
- let array: PrimitiveArray<T> = match input_scale.cmp(&output_scale) {
- Ordering::Equal => {
- // the scale doesn't change, the native value don't need to be changed
- array.clone()
- }
- Ordering::Greater => convert_to_smaller_scale_decimal::<T, T>(
- array,
- input_scale,
- output_precision,
- output_scale,
- cast_options,
- )?,
- Ordering::Less => {
- // input_scale < output_scale
+ let array: PrimitiveArray<T> = if input_scale <= output_scale {
+ // input_scale <= output_scale
+ // the scale doesn't change, but precision may change and cause overflow
Review Comment:
Done. @viirya please check.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: decimal conversion looses value on lower precision [arrow-rs]
Posted by "himadripal (via GitHub)" <gi...@apache.org>.
himadripal commented on code in PR #6836:
URL: https://github.com/apache/arrow-rs/pull/6836#discussion_r1870625712
##########
arrow-cast/src/cast/decimal.rs:
##########
@@ -155,28 +163,24 @@ where
T: DecimalType,
T::Native: DecimalCast + ArrowNativeTypeOp,
{
- let array: PrimitiveArray<T> = match input_scale.cmp(&output_scale) {
- Ordering::Equal => {
- // the scale doesn't change, the native value don't need to be changed
- array.clone()
- }
- Ordering::Greater => convert_to_smaller_scale_decimal::<T, T>(
- array,
- input_scale,
- output_precision,
- output_scale,
- cast_options,
- )?,
- Ordering::Less => {
- // input_scale < output_scale
+ let array: PrimitiveArray<T> = if input_scale <= output_scale {
+ // input_scale <= output_scale
+ // the scale doesn't change, but precision may change and cause overflow
Review Comment:
lets assume, we are converting this `12345` from (5, 3) -> (6,3), then it needs to be `123450` - if we do array.clone() - wondering how it will add the 0 at the end.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: decimal conversion looses value on lower precision [arrow-rs]
Posted by "himadripal (via GitHub)" <gi...@apache.org>.
himadripal commented on code in PR #6836:
URL: https://github.com/apache/arrow-rs/pull/6836#discussion_r1870625712
##########
arrow-cast/src/cast/decimal.rs:
##########
@@ -155,28 +163,24 @@ where
T: DecimalType,
T::Native: DecimalCast + ArrowNativeTypeOp,
{
- let array: PrimitiveArray<T> = match input_scale.cmp(&output_scale) {
- Ordering::Equal => {
- // the scale doesn't change, the native value don't need to be changed
- array.clone()
- }
- Ordering::Greater => convert_to_smaller_scale_decimal::<T, T>(
- array,
- input_scale,
- output_precision,
- output_scale,
- cast_options,
- )?,
- Ordering::Less => {
- // input_scale < output_scale
+ let array: PrimitiveArray<T> = if input_scale <= output_scale {
+ // input_scale <= output_scale
+ // the scale doesn't change, but precision may change and cause overflow
Review Comment:
lets assume, we are converting this `12345` from (5, 3) -> (6,3), then it need to be `123450` - if we do array.clone() - wondering how it will add the 0 at the end.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: decimal conversion looses value on lower precision [arrow-rs]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #6836:
URL: https://github.com/apache/arrow-rs/pull/6836#discussion_r1870647169
##########
arrow-cast/src/cast/decimal.rs:
##########
@@ -155,28 +163,24 @@ where
T: DecimalType,
T::Native: DecimalCast + ArrowNativeTypeOp,
{
- let array: PrimitiveArray<T> = match input_scale.cmp(&output_scale) {
- Ordering::Equal => {
- // the scale doesn't change, the native value don't need to be changed
- array.clone()
- }
- Ordering::Greater => convert_to_smaller_scale_decimal::<T, T>(
- array,
- input_scale,
- output_precision,
- output_scale,
- cast_options,
- )?,
- Ordering::Less => {
- // input_scale < output_scale
+ let array: PrimitiveArray<T> = if input_scale <= output_scale {
+ // input_scale <= output_scale
+ // the scale doesn't change, but precision may change and cause overflow
Review Comment:
12345 in (5, 3) is 12.345. When casting to (6, 3), it is still 12345, why it needs to be 123450? 123450 in (6, 3) is 123.45. If I understand it correctly.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: decimal conversion looses value on lower precision [arrow-rs]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #6836:
URL: https://github.com/apache/arrow-rs/pull/6836#issuecomment-2524156050
> Have we run any benchmarks (I'm not sure if any actually exist) to confirm this doesn't significantly regress performance.
>
> It seems unfortunate to be always performing overflow checks, when in many cases it should be possible to prove that precision overflow can't occur and need not be checked for
I created a simple benchmark for decimal casting in https://github.com/apache/arrow-rs/pull/6850.
Unsurprisingly, validating that the results are correct is slower than not validating the results.
## before
```
cast_decimal time: [45.281 ns 45.549 ns 45.871 ns]
```
## after (this PR)
```
cast_decimal time: [247.97 ns 248.78 ns 249.78 ns]
change: [+435.06% +439.47% +443.15%] (p = 0.00 < 0.05)
```
We currently have the config option of `safe` on or off:
```
pub struct CastOptions<'a> {
/// how to handle cast failures, either return NULL (safe=true) or return ERR (safe=false)
pub safe: bool,
```
So, yes, it is a performance regression, but the previous behavior was incorrect. This PR now makes this work as advertised.
@tustvold Is there a use case we need to support for faster casts without validating results per the CastOptions?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: decimal conversion looses value on lower precision [arrow-rs]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #6836:
URL: https://github.com/apache/arrow-rs/pull/6836#issuecomment-2518570636
> Have we run any benchmarks (I'm not sure if any actually exist) to confirm this doesn't significantly regress performance.
>
> It seems unfortunate to be always performing overflow checks, when in many cases it should be possible to prove that precision overflow can't occur and need not be checked for
I'll create a separate PR (probably tomorrow) to add some criterion benchmarks
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: decimal conversion looses value on lower precision [arrow-rs]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #6836:
URL: https://github.com/apache/arrow-rs/pull/6836#discussion_r1870669736
##########
arrow-cast/src/cast/decimal.rs:
##########
@@ -155,28 +163,24 @@ where
T: DecimalType,
T::Native: DecimalCast + ArrowNativeTypeOp,
{
- let array: PrimitiveArray<T> = match input_scale.cmp(&output_scale) {
- Ordering::Equal => {
- // the scale doesn't change, the native value don't need to be changed
- array.clone()
- }
- Ordering::Greater => convert_to_smaller_scale_decimal::<T, T>(
- array,
- input_scale,
- output_precision,
- output_scale,
- cast_options,
- )?,
- Ordering::Less => {
- // input_scale < output_scale
+ let array: PrimitiveArray<T> = if input_scale <= output_scale {
+ // input_scale <= output_scale
+ // the scale doesn't change, but precision may change and cause overflow
Review Comment:
Yea
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: decimal conversion looses value on lower precision [arrow-rs]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #6836:
URL: https://github.com/apache/arrow-rs/pull/6836#discussion_r1870012344
##########
arrow-cast/src/cast/mod.rs:
##########
@@ -9827,4 +9827,72 @@ mod tests {
"Cast non-nullable to non-nullable struct field returning null should fail",
);
}
+
+ #[test]
+ fn test_decimal_to_decimal_throw_error_on_precision_overflow_same_scale(){
+ let array = vec![Some(123456789)];
+ let array = create_decimal_array(array, 24, 2).unwrap();
+ println!("{:?}", array);
+ let input_type = DataType::Decimal128(24, 2);
+ let output_type = DataType::Decimal128(6, 2);
+ assert!(can_cast_types(&input_type, &output_type));
+ let mut options = CastOptions::default();
+ options.safe = false;
+
+ let result = cast_with_options(&array, &output_type, &options);
+
+ assert_eq!(result.unwrap_err().to_string(),
+ "InvalidArgumentError(123456789 is too large to store in a Decimal128 of precision 6. Max is 999999)");
Review Comment:
The error message format needs updating:
```
left: "Invalid argument error: 123456789 is too large to store in a Decimal256 of precision 6. Max is 999999"
right: "InvalidArgumentError(123456789 is too large to store in a Decimal256 of precision 6. Max is 999999)"
```
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] decimal conversion looses value on lower precision, throws error now … [arrow-rs]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #6836:
URL: https://github.com/apache/arrow-rs/pull/6836#discussion_r1870001891
##########
arrow-cast/src/cast/decimal.rs:
##########
@@ -112,8 +112,19 @@ where
};
Ok(match cast_options.safe {
Review Comment:
It seems strange to `match` on a boolean rather than just using an `if` statement. I know this is how the existing code was, but perhaps we could improve this while we are here.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: decimal conversion looses value on lower precision [arrow-rs]
Posted by "himadripal (via GitHub)" <gi...@apache.org>.
himadripal commented on code in PR #6836:
URL: https://github.com/apache/arrow-rs/pull/6836#discussion_r1870487495
##########
arrow-cast/src/cast/decimal.rs:
##########
@@ -112,8 +112,19 @@ where
};
Ok(match cast_options.safe {
- true => array.unary_opt(f),
- false => array.try_unary(|x| f(x).ok_or_else(|| error(x)))?,
+ true => {
+ array.unary_opt(|x| {
+ f(x).filter(|v| O::is_valid_decimal_precision(*v, output_precision))
+ })
+ }
+ false => {
+ array.try_unary(|x| {
+ f(x).ok_or_else(|| error(x))
+ .and_then(|v|{
+ O:: validate_decimal_precision(v, output_precision).map(|_| v)
Review Comment:
I thought about it, but then there are other examples where is it done this way, so kept it there. We can also do this as part of first error check point - inside this method
```rust
let error = cast_decimal_to_decimal_error::<I, O>(output_precision, output_scale);
```
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: decimal conversion looses value on lower precision [arrow-rs]
Posted by "himadripal (via GitHub)" <gi...@apache.org>.
himadripal commented on code in PR #6836:
URL: https://github.com/apache/arrow-rs/pull/6836#discussion_r1870615156
##########
arrow-cast/src/cast/decimal.rs:
##########
@@ -155,28 +163,24 @@ where
T: DecimalType,
T::Native: DecimalCast + ArrowNativeTypeOp,
{
- let array: PrimitiveArray<T> = match input_scale.cmp(&output_scale) {
- Ordering::Equal => {
- // the scale doesn't change, the native value don't need to be changed
- array.clone()
- }
- Ordering::Greater => convert_to_smaller_scale_decimal::<T, T>(
- array,
- input_scale,
- output_precision,
- output_scale,
- cast_options,
- )?,
- Ordering::Less => {
- // input_scale < output_scale
+ let array: PrimitiveArray<T> = if input_scale <= output_scale {
+ // input_scale <= output_scale
+ // the scale doesn't change, but precision may change and cause overflow
Review Comment:
won't it still has to go through the `convert_to_bigger_or_equal_scale_decimal` to add the zero at the end for bigger precision conversion. IMO, we can only skip the call if precision is less and scales are equal. Please correct me here.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: decimal conversion looses value on lower precision [arrow-rs]
Posted by "himadripal (via GitHub)" <gi...@apache.org>.
himadripal commented on code in PR #6836:
URL: https://github.com/apache/arrow-rs/pull/6836#discussion_r1870635145
##########
arrow-cast/src/cast/decimal.rs:
##########
@@ -155,28 +163,24 @@ where
T: DecimalType,
T::Native: DecimalCast + ArrowNativeTypeOp,
{
- let array: PrimitiveArray<T> = match input_scale.cmp(&output_scale) {
- Ordering::Equal => {
- // the scale doesn't change, the native value don't need to be changed
- array.clone()
- }
- Ordering::Greater => convert_to_smaller_scale_decimal::<T, T>(
- array,
- input_scale,
- output_precision,
- output_scale,
- cast_options,
- )?,
- Ordering::Less => {
- // input_scale < output_scale
+ let array: PrimitiveArray<T> = if input_scale <= output_scale {
+ // input_scale <= output_scale
+ // the scale doesn't change, but precision may change and cause overflow
Review Comment:
If this is correct, then we can return original array if precision is bigger and scale is equal.
```sql
select arrow_typeof(cast(cast(1.23 as decimal(10,3)) as decimal(12,3))),
cast(cast(1.23 as decimal(10,3)) as decimal(12,3));
----
Decimal128(12, 3) 1.23
```
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: decimal conversion looses value on lower precision [arrow-rs]
Posted by "himadripal (via GitHub)" <gi...@apache.org>.
himadripal commented on code in PR #6836:
URL: https://github.com/apache/arrow-rs/pull/6836#discussion_r1870635145
##########
arrow-cast/src/cast/decimal.rs:
##########
@@ -155,28 +163,24 @@ where
T: DecimalType,
T::Native: DecimalCast + ArrowNativeTypeOp,
{
- let array: PrimitiveArray<T> = match input_scale.cmp(&output_scale) {
- Ordering::Equal => {
- // the scale doesn't change, the native value don't need to be changed
- array.clone()
- }
- Ordering::Greater => convert_to_smaller_scale_decimal::<T, T>(
- array,
- input_scale,
- output_precision,
- output_scale,
- cast_options,
- )?,
- Ordering::Less => {
- // input_scale < output_scale
+ let array: PrimitiveArray<T> = if input_scale <= output_scale {
+ // input_scale <= output_scale
+ // the scale doesn't change, but precision may change and cause overflow
Review Comment:
decimal.slt - I added this and this is the output. If this is correct output, then I think we can return original array if precision is bigger and scale is equal.
```sql
select arrow_typeof(cast(cast(1.23 as decimal(10,3)) as decimal(12,3))),
cast(cast(1.23 as decimal(10,3)) as decimal(12,3));
----
Decimal128(12, 3) 1.23
```
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: decimal conversion looses value on lower precision [arrow-rs]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #6836:
URL: https://github.com/apache/arrow-rs/pull/6836#discussion_r1870621460
##########
arrow-cast/src/cast/decimal.rs:
##########
@@ -155,28 +163,24 @@ where
T: DecimalType,
T::Native: DecimalCast + ArrowNativeTypeOp,
{
- let array: PrimitiveArray<T> = match input_scale.cmp(&output_scale) {
- Ordering::Equal => {
- // the scale doesn't change, the native value don't need to be changed
- array.clone()
- }
- Ordering::Greater => convert_to_smaller_scale_decimal::<T, T>(
- array,
- input_scale,
- output_precision,
- output_scale,
- cast_options,
- )?,
- Ordering::Less => {
- // input_scale < output_scale
+ let array: PrimitiveArray<T> = if input_scale <= output_scale {
+ // input_scale <= output_scale
+ // the scale doesn't change, but precision may change and cause overflow
Review Comment:
For example, original code simply returns the original array for same scale. If the new precision is bigger, I think we can still do that, no?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: decimal conversion looses value on lower precision [arrow-rs]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #6836:
URL: https://github.com/apache/arrow-rs/pull/6836#discussion_r1870620081
##########
arrow-cast/src/cast/decimal.rs:
##########
@@ -155,28 +163,24 @@ where
T: DecimalType,
T::Native: DecimalCast + ArrowNativeTypeOp,
{
- let array: PrimitiveArray<T> = match input_scale.cmp(&output_scale) {
- Ordering::Equal => {
- // the scale doesn't change, the native value don't need to be changed
- array.clone()
- }
- Ordering::Greater => convert_to_smaller_scale_decimal::<T, T>(
- array,
- input_scale,
- output_precision,
- output_scale,
- cast_options,
- )?,
- Ordering::Less => {
- // input_scale < output_scale
+ let array: PrimitiveArray<T> = if input_scale <= output_scale {
+ // input_scale <= output_scale
+ // the scale doesn't change, but precision may change and cause overflow
Review Comment:
Oh, sorry, I mean to skip the check added in this PR.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: decimal conversion looses value on lower precision [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #6836:
URL: https://github.com/apache/arrow-rs/pull/6836#discussion_r1870088629
##########
arrow-cast/src/cast/decimal.rs:
##########
@@ -156,19 +179,9 @@ where
T::Native: DecimalCast + ArrowNativeTypeOp,
{
let array: PrimitiveArray<T> = match input_scale.cmp(&output_scale) {
Review Comment:
This could be changed to `<=`
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: decimal conversion looses value on lower precision [arrow-rs]
Posted by "himadripal (via GitHub)" <gi...@apache.org>.
himadripal commented on code in PR #6836:
URL: https://github.com/apache/arrow-rs/pull/6836#discussion_r1870635145
##########
arrow-cast/src/cast/decimal.rs:
##########
@@ -155,28 +163,24 @@ where
T: DecimalType,
T::Native: DecimalCast + ArrowNativeTypeOp,
{
- let array: PrimitiveArray<T> = match input_scale.cmp(&output_scale) {
- Ordering::Equal => {
- // the scale doesn't change, the native value don't need to be changed
- array.clone()
- }
- Ordering::Greater => convert_to_smaller_scale_decimal::<T, T>(
- array,
- input_scale,
- output_precision,
- output_scale,
- cast_options,
- )?,
- Ordering::Less => {
- // input_scale < output_scale
+ let array: PrimitiveArray<T> = if input_scale <= output_scale {
+ // input_scale <= output_scale
+ // the scale doesn't change, but precision may change and cause overflow
Review Comment:
If this is correct, then I think we can return original array if precision is bigger and scale is equal.
```sql
select arrow_typeof(cast(cast(1.23 as decimal(10,3)) as decimal(12,3))),
cast(cast(1.23 as decimal(10,3)) as decimal(12,3));
----
Decimal128(12, 3) 1.23
```
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: decimal conversion looses value on lower precision [arrow-rs]
Posted by "himadripal (via GitHub)" <gi...@apache.org>.
himadripal commented on code in PR #6836:
URL: https://github.com/apache/arrow-rs/pull/6836#discussion_r1870661505
##########
arrow-cast/src/cast/decimal.rs:
##########
@@ -155,28 +163,24 @@ where
T: DecimalType,
T::Native: DecimalCast + ArrowNativeTypeOp,
{
- let array: PrimitiveArray<T> = match input_scale.cmp(&output_scale) {
- Ordering::Equal => {
- // the scale doesn't change, the native value don't need to be changed
- array.clone()
- }
- Ordering::Greater => convert_to_smaller_scale_decimal::<T, T>(
- array,
- input_scale,
- output_precision,
- output_scale,
- cast_options,
- )?,
- Ordering::Less => {
- // input_scale < output_scale
+ let array: PrimitiveArray<T> = if input_scale <= output_scale {
+ // input_scale <= output_scale
+ // the scale doesn't change, but precision may change and cause overflow
Review Comment:
Yes, you are right. How to I get the input_precision to check against output_precision? fn signature only has output_precision, I can get it from the calling fn, its available there and change the signature. would that be fine?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: decimal conversion looses value on lower precision [arrow-rs]
Posted by "himadripal (via GitHub)" <gi...@apache.org>.
himadripal commented on code in PR #6836:
URL: https://github.com/apache/arrow-rs/pull/6836#discussion_r1870625712
##########
arrow-cast/src/cast/decimal.rs:
##########
@@ -155,28 +163,24 @@ where
T: DecimalType,
T::Native: DecimalCast + ArrowNativeTypeOp,
{
- let array: PrimitiveArray<T> = match input_scale.cmp(&output_scale) {
- Ordering::Equal => {
- // the scale doesn't change, the native value don't need to be changed
- array.clone()
- }
- Ordering::Greater => convert_to_smaller_scale_decimal::<T, T>(
- array,
- input_scale,
- output_precision,
- output_scale,
- cast_options,
- )?,
- Ordering::Less => {
- // input_scale < output_scale
+ let array: PrimitiveArray<T> = if input_scale <= output_scale {
+ // input_scale <= output_scale
+ // the scale doesn't change, but precision may change and cause overflow
Review Comment:
lets assume, we are converting this `12345` from (5, 3) to (6,3), then it needs to be `123450` - if we do array.clone() as per original code- wondering how it will add the 0 at the end.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: decimal conversion looses value on lower precision [arrow-rs]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #6836:
URL: https://github.com/apache/arrow-rs/pull/6836#discussion_r1870621460
##########
arrow-cast/src/cast/decimal.rs:
##########
@@ -155,28 +163,24 @@ where
T: DecimalType,
T::Native: DecimalCast + ArrowNativeTypeOp,
{
- let array: PrimitiveArray<T> = match input_scale.cmp(&output_scale) {
- Ordering::Equal => {
- // the scale doesn't change, the native value don't need to be changed
- array.clone()
- }
- Ordering::Greater => convert_to_smaller_scale_decimal::<T, T>(
- array,
- input_scale,
- output_precision,
- output_scale,
- cast_options,
- )?,
- Ordering::Less => {
- // input_scale < output_scale
+ let array: PrimitiveArray<T> = if input_scale <= output_scale {
+ // input_scale <= output_scale
+ // the scale doesn't change, but precision may change and cause overflow
Review Comment:
For example, original code simply returns the original array for same scale. If the precision is bigger, I think we can still do that, no?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: decimal conversion looses value on lower precision [arrow-rs]
Posted by "himadripal (via GitHub)" <gi...@apache.org>.
himadripal commented on code in PR #6836:
URL: https://github.com/apache/arrow-rs/pull/6836#discussion_r1870615156
##########
arrow-cast/src/cast/decimal.rs:
##########
@@ -155,28 +163,24 @@ where
T: DecimalType,
T::Native: DecimalCast + ArrowNativeTypeOp,
{
- let array: PrimitiveArray<T> = match input_scale.cmp(&output_scale) {
- Ordering::Equal => {
- // the scale doesn't change, the native value don't need to be changed
- array.clone()
- }
- Ordering::Greater => convert_to_smaller_scale_decimal::<T, T>(
- array,
- input_scale,
- output_precision,
- output_scale,
- cast_options,
- )?,
- Ordering::Less => {
- // input_scale < output_scale
+ let array: PrimitiveArray<T> = if input_scale <= output_scale {
+ // input_scale <= output_scale
+ // the scale doesn't change, but precision may change and cause overflow
Review Comment:
won't it still has to go through the `convert_to_bigger_or_equal_scale_decimal` to add the zero at the end for bigger precision conversion. IMO, we can only skip the call if precision is less and scales are equal. Please correct me here.
we can do check at the beginning like this
```rust
if (array.validate_decimal_precision().is_err())
```
but we need to return null for `safe=true` and throw error for `safe=false`
is that what you are suggesting?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: decimal conversion looses value on lower precision [arrow-rs]
Posted by "himadripal (via GitHub)" <gi...@apache.org>.
himadripal commented on code in PR #6836:
URL: https://github.com/apache/arrow-rs/pull/6836#discussion_r1870661505
##########
arrow-cast/src/cast/decimal.rs:
##########
@@ -155,28 +163,24 @@ where
T: DecimalType,
T::Native: DecimalCast + ArrowNativeTypeOp,
{
- let array: PrimitiveArray<T> = match input_scale.cmp(&output_scale) {
- Ordering::Equal => {
- // the scale doesn't change, the native value don't need to be changed
- array.clone()
- }
- Ordering::Greater => convert_to_smaller_scale_decimal::<T, T>(
- array,
- input_scale,
- output_precision,
- output_scale,
- cast_options,
- )?,
- Ordering::Less => {
- // input_scale < output_scale
+ let array: PrimitiveArray<T> = if input_scale <= output_scale {
+ // input_scale <= output_scale
+ // the scale doesn't change, but precision may change and cause overflow
Review Comment:
Yes, you are right. fn signature only has output_precision, I can get the input_precision from the calling fn, it's available there and change the signature. would that be fine?
##########
arrow-cast/src/cast/decimal.rs:
##########
@@ -155,28 +163,24 @@ where
T: DecimalType,
T::Native: DecimalCast + ArrowNativeTypeOp,
{
- let array: PrimitiveArray<T> = match input_scale.cmp(&output_scale) {
- Ordering::Equal => {
- // the scale doesn't change, the native value don't need to be changed
- array.clone()
- }
- Ordering::Greater => convert_to_smaller_scale_decimal::<T, T>(
- array,
- input_scale,
- output_precision,
- output_scale,
- cast_options,
- )?,
- Ordering::Less => {
- // input_scale < output_scale
+ let array: PrimitiveArray<T> = if input_scale <= output_scale {
+ // input_scale <= output_scale
+ // the scale doesn't change, but precision may change and cause overflow
Review Comment:
Yes, you are right. fn signature only has output_precision, I can get the input_precision from the calling fn, it's available there and change the signature to include input_precision. would that be fine?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: decimal conversion looses value on lower precision [arrow-rs]
Posted by "himadripal (via GitHub)" <gi...@apache.org>.
himadripal commented on PR #6836:
URL: https://github.com/apache/arrow-rs/pull/6836#issuecomment-2518886685
> > There is a regression in `test_cast_decimal128_to_decimal256_negative`. The new validation check is correctly throwing an error, but it looks like we also need to add this validation when creating decimal arrays since the current test is creating invalid arrays before the cast:
> > ```rust
> > let array = vec![Some(i128::MAX), Some(i128::MIN)];
> > let input_decimal_array = create_decimal_array(array, 10, 3).unwrap();
> > ```
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> > I would expect this to fail, so we probably need to add the same validation there.
>
> On second thoughts, it seems it was an intentional design decision not to validate this on array creation. Instead, an `array.validate_decimal_precision` method can optionally be called on the array to validate it after creation, so we should probably just update the test as needed (@alamb @tustvold perhaps you could correct me if I am wrong about this).
Changed the test to pass.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: decimal conversion looses value on lower precision [arrow-rs]
Posted by "himadripal (via GitHub)" <gi...@apache.org>.
himadripal commented on code in PR #6836:
URL: https://github.com/apache/arrow-rs/pull/6836#discussion_r1870625712
##########
arrow-cast/src/cast/decimal.rs:
##########
@@ -155,28 +163,24 @@ where
T: DecimalType,
T::Native: DecimalCast + ArrowNativeTypeOp,
{
- let array: PrimitiveArray<T> = match input_scale.cmp(&output_scale) {
- Ordering::Equal => {
- // the scale doesn't change, the native value don't need to be changed
- array.clone()
- }
- Ordering::Greater => convert_to_smaller_scale_decimal::<T, T>(
- array,
- input_scale,
- output_precision,
- output_scale,
- cast_options,
- )?,
- Ordering::Less => {
- // input_scale < output_scale
+ let array: PrimitiveArray<T> = if input_scale <= output_scale {
+ // input_scale <= output_scale
+ // the scale doesn't change, but precision may change and cause overflow
Review Comment:
lets assume, we are converting `12.345` from (5, 3) to (6,3), <strike> then it needs to be `123450` - if we do array.clone() as per original code- wondering how it will add the 0 at the end.</strike>. Then it should still be 12.345
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] decimal conversion looses value on lower precision, throws error now … [arrow-rs]
Posted by "himadripal (via GitHub)" <gi...@apache.org>.
himadripal commented on PR #6836:
URL: https://github.com/apache/arrow-rs/pull/6836#issuecomment-2518097749
@andygrove @viirya @alamb please take a look.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: decimal conversion looses value on lower precision [arrow-rs]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #6836:
URL: https://github.com/apache/arrow-rs/pull/6836#discussion_r1870561699
##########
arrow-cast/src/cast/decimal.rs:
##########
@@ -155,28 +163,24 @@ where
T: DecimalType,
T::Native: DecimalCast + ArrowNativeTypeOp,
{
- let array: PrimitiveArray<T> = match input_scale.cmp(&output_scale) {
- Ordering::Equal => {
- // the scale doesn't change, the native value don't need to be changed
- array.clone()
- }
- Ordering::Greater => convert_to_smaller_scale_decimal::<T, T>(
- array,
- input_scale,
- output_precision,
- output_scale,
- cast_options,
- )?,
- Ordering::Less => {
- // input_scale < output_scale
+ let array: PrimitiveArray<T> = if input_scale <= output_scale {
+ // input_scale <= output_scale
+ // the scale doesn't change, but precision may change and cause overflow
Review Comment:
Why don't we also check precision and skip `convert_to_bigger_or_equal_scale_decimal` if it won't overflow?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: decimal conversion looses value on lower precision [arrow-rs]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #6836:
URL: https://github.com/apache/arrow-rs/pull/6836#discussion_r1870561699
##########
arrow-cast/src/cast/decimal.rs:
##########
@@ -155,28 +163,24 @@ where
T: DecimalType,
T::Native: DecimalCast + ArrowNativeTypeOp,
{
- let array: PrimitiveArray<T> = match input_scale.cmp(&output_scale) {
- Ordering::Equal => {
- // the scale doesn't change, the native value don't need to be changed
- array.clone()
- }
- Ordering::Greater => convert_to_smaller_scale_decimal::<T, T>(
- array,
- input_scale,
- output_precision,
- output_scale,
- cast_options,
- )?,
- Ordering::Less => {
- // input_scale < output_scale
+ let array: PrimitiveArray<T> = if input_scale <= output_scale {
+ // input_scale <= output_scale
+ // the scale doesn't change, but precision may change and cause overflow
Review Comment:
Why don't we also check precision and skip <del>`convert_to_bigger_or_equal_scale_decimal`</del> overflow check if it won't overflow?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: decimal conversion looses value on lower precision [arrow-rs]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #6836:
URL: https://github.com/apache/arrow-rs/pull/6836#discussion_r1870620081
##########
arrow-cast/src/cast/decimal.rs:
##########
@@ -155,28 +163,24 @@ where
T: DecimalType,
T::Native: DecimalCast + ArrowNativeTypeOp,
{
- let array: PrimitiveArray<T> = match input_scale.cmp(&output_scale) {
- Ordering::Equal => {
- // the scale doesn't change, the native value don't need to be changed
- array.clone()
- }
- Ordering::Greater => convert_to_smaller_scale_decimal::<T, T>(
- array,
- input_scale,
- output_precision,
- output_scale,
- cast_options,
- )?,
- Ordering::Less => {
- // input_scale < output_scale
+ let array: PrimitiveArray<T> = if input_scale <= output_scale {
+ // input_scale <= output_scale
+ // the scale doesn't change, but precision may change and cause overflow
Review Comment:
Oh, sorry, I mean to skip the check added in this PR.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: decimal conversion looses value on lower precision [arrow-rs]
Posted by "himadripal (via GitHub)" <gi...@apache.org>.
himadripal commented on code in PR #6836:
URL: https://github.com/apache/arrow-rs/pull/6836#discussion_r1870635145
##########
arrow-cast/src/cast/decimal.rs:
##########
@@ -155,28 +163,24 @@ where
T: DecimalType,
T::Native: DecimalCast + ArrowNativeTypeOp,
{
- let array: PrimitiveArray<T> = match input_scale.cmp(&output_scale) {
- Ordering::Equal => {
- // the scale doesn't change, the native value don't need to be changed
- array.clone()
- }
- Ordering::Greater => convert_to_smaller_scale_decimal::<T, T>(
- array,
- input_scale,
- output_precision,
- output_scale,
- cast_options,
- )?,
- Ordering::Less => {
- // input_scale < output_scale
+ let array: PrimitiveArray<T> = if input_scale <= output_scale {
+ // input_scale <= output_scale
+ // the scale doesn't change, but precision may change and cause overflow
Review Comment:
decimal.slt - I added this and this is the output. Not sure if this is correct output.
```sql
select arrow_typeof(cast(cast(1.23 as decimal(10,3)) as decimal(12,3))),
cast(cast(1.23 as decimal(10,3)) as decimal(12,3));
----
Decimal128(12, 3) 1.23
```
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: decimal conversion looses value on lower precision [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on PR #6836:
URL: https://github.com/apache/arrow-rs/pull/6836#issuecomment-2525192111
> @tustvold Is there a use case we need to support for faster casts without validating results per the CastOptions?
No, the cast should be checked, apologies if that wasn't clear, my concern was the PR as originally formulated blindly performed the checked conversion regardless of the input type. Given the whole purpose of tracking precision is to avoid overflow checks, it seemed a little off.
I'll try to find some time to take another look at this PR as from a quick scan this looks to have been addressed
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: decimal conversion looses value on lower precision [arrow-rs]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #6836:
URL: https://github.com/apache/arrow-rs/pull/6836#issuecomment-2518166430
> There is a regression in `test_cast_decimal128_to_decimal256_negative`. The new validation check is correctly throwing an error, but it looks like we also need to add this validation when creating decimal arrays since the current test is creating invalid arrays before the cast:
>
> ```rust
> let array = vec![Some(i128::MAX), Some(i128::MIN)];
> let input_decimal_array = create_decimal_array(array, 10, 3).unwrap();
> ```
>
> I would expect this to fail, so we probably need to add the same validation there.
On second thoughts, it seems it was an intentional design decision not to validate this on array creation. Instead, an `array.validate_decimal_precision` method can optionally be called on the array to validate it aft er creation, so we should probably just update the test as needed (@alamb @tustvold perhaps you could correct me if I am wrong about this).
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: decimal conversion looses value on lower precision [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold merged PR #6836:
URL: https://github.com/apache/arrow-rs/pull/6836
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org