-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
GH-37730: [C#] throw OverflowException in DecimalUtility if fractionalPart is too large #37731
Conversation
|
@lidavidm - can you help with this one too, please? |
} | ||
if (fractionalPart > _maxDecimal || fractionalPart < _minDecimal) // decimal overflow, not much we can do here - C# needs a BigDecimal | ||
{ | ||
throw new OverflowException($"Value: {integerValue} is too big or too small to be represented as a decimal"); |
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.
The test cases confused me, but looking at it, it's not necessarily about too big or too small, but rather too precise?
Arrow represents 7.89 in decimal256(76,38)
as 789000000000000000000000000000000000000
integerPart is then 7
fractionalPart is 89000000000000000000000000000000000000
and that's just too many digits of precision for decimal
?
Which, it is literally too big, but that's a bit confusing unless you are thinking about the representation.
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.
Separately, I think it would be useful to differentiate the error messages here and possibly include the problematic 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.
updated in latest push
After merging your PR, Conbench analyzed the 5 benchmarking runs that have been run so far on merge-commit 629ecbd. There was 1 benchmark result with an error:
There were no benchmark performance regressions. 🎉 The full Conbench report has more details. It also includes information about 7 possible false positives for unstable benchmarks that are known to sometimes produce them. |
…ctionalPart is too large (apache#37731) ### Rationale for this change In BigQuery, the BIGNUMERIC supports very large precision and scale values, which causes an error in the DecimalUtility due to the `fractionalPart` of the value being larger than .NET's maximum decimal value. ### What changes are included in this PR? 1. Throws an OverflowException with the entire original value 2. Changes the original OverflowException to also include the original value (which can be parsed as a string by downstream drivers). ### Are these changes tested? Yes, tests are added for this scenario. ### Are there any user-facing changes? * Closes: apache#37730 Lead-authored-by: David Coe <[email protected]> Co-authored-by: davidhcoe <[email protected]> Signed-off-by: David Li <[email protected]>
…ctionalPart is too large (apache#37731) ### Rationale for this change In BigQuery, the BIGNUMERIC supports very large precision and scale values, which causes an error in the DecimalUtility due to the `fractionalPart` of the value being larger than .NET's maximum decimal value. ### What changes are included in this PR? 1. Throws an OverflowException with the entire original value 2. Changes the original OverflowException to also include the original value (which can be parsed as a string by downstream drivers). ### Are these changes tested? Yes, tests are added for this scenario. ### Are there any user-facing changes? * Closes: apache#37730 Lead-authored-by: David Coe <[email protected]> Co-authored-by: davidhcoe <[email protected]> Signed-off-by: David Li <[email protected]>
…ctionalPart is too large (apache#37731) ### Rationale for this change In BigQuery, the BIGNUMERIC supports very large precision and scale values, which causes an error in the DecimalUtility due to the `fractionalPart` of the value being larger than .NET's maximum decimal value. ### What changes are included in this PR? 1. Throws an OverflowException with the entire original value 2. Changes the original OverflowException to also include the original value (which can be parsed as a string by downstream drivers). ### Are these changes tested? Yes, tests are added for this scenario. ### Are there any user-facing changes? * Closes: apache#37730 Lead-authored-by: David Coe <[email protected]> Co-authored-by: davidhcoe <[email protected]> Signed-off-by: David Li <[email protected]>
Rationale for this change
In BigQuery, the BIGNUMERIC supports very large precision and scale values, which causes an error in the DecimalUtility due to the
fractionalPart
of the value being larger than .NET's maximum decimal value.What changes are included in this PR?
Are these changes tested?
Yes, tests are added for this scenario.
Are there any user-facing changes?