-
Notifications
You must be signed in to change notification settings - Fork 824
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
fix: add missing precision overflow checking for cast_string_to_decimal
#4830
Conversation
cast_string_to_decimal
.and_then(|v| { | ||
T::validate_decimal_precision(v, precision) | ||
.is_ok() | ||
.then_some(v) | ||
}) |
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 believe that the original idea is to avoid this validation and leave the decision to the caller. But after several rounds of revamp on decimal, I'm not sure if that design idea is still kept and we want to add validation now.
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.
@viirya I noticed that casting from integers has this validation, which is inconsistent with the behavior of casting from strings.
There is an intuitive example in Datafusion.
DataFusion CLI v31.0.0
❯ select cast(1000 as decimal(10,8));
Optimizer rule 'simplify_expressions' failed
caused by
Arrow error: Invalid argument error: 100000000000 is too large to store in a Decimal128 of precision 10. Max is 9999999999
❯ select cast('1000' as decimal(10,8));
+--------------+
| Utf8("1000") |
+--------------+
| 10.00000000 |
+--------------+
If they could have a unified behavior, it would be preferable.
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 don't feel strongly here, casting in general does tend to check for overflow, and given it is parsing a string is unlikely to massively regress performance. I defer to you @viirya
arrow-cast/src/cast.rs
Outdated
@@ -8097,6 +8106,32 @@ mod tests { | |||
test_cast_string_to_decimal(array); | |||
} | |||
|
|||
#[test] | |||
fn test_cast_string_to_decimal128_precision_overflow() { |
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.
cast_string_to_decimal is not only for decimal128 but also for decimal256, do you want to add a test for decimal256 too?
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.
@viirya I think it is necessary, and a new test has been added for decimal256.
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.
Thank you.
…mal` (apache#4830) * fix: add missing precision overflow checking for `cast_string_to_decimal` * Add test_cast_string_to_decimal256_precision_overflow
Which issue does this PR close?
Closes #4829.
Rationale for this change
Perform a precision overflow check when casting from a string to a decimal.
What changes are included in this PR?
Are there any user-facing changes?
No