You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "davidhcoe (via GitHub)" <gi...@apache.org> on 2023/09/15 01:13:43 UTC
[GitHub] [arrow] davidhcoe opened a new pull request, #37731: GH-37730: [C#] throw OverflowException in DecimalUtility if fractionalPart is too large
davidhcoe opened a new pull request, #37731:
URL: https://github.com/apache/arrow/pull/37731
<!--
Thanks for opening a pull request!
If this is your first pull request you can find detailed information on how
to contribute here:
* [New Contributor's Guide](https://arrow.apache.org/docs/dev/developers/guide/step_by_step/pr_lifecycle.html#reviews-and-merge-of-the-pull-request)
* [Contributing Overview](https://arrow.apache.org/docs/dev/developers/overview.html)
If this is not a [minor PR](https://github.com/apache/arrow/blob/main/CONTRIBUTING.md#Minor-Fixes). Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose
Opening GitHub issues ahead of time contributes to the [Openness](http://theapacheway.com/open/#:~:text=Openness%20allows%20new%20users%20the,must%20happen%20in%20the%20open.) of the Apache Arrow project.
Then could you also rename the pull request title in the following format?
GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}
or
MINOR: [${COMPONENT}] ${SUMMARY}
In the case of PARQUET issues on JIRA the title also supports:
PARQUET-${JIRA_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}
-->
### 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.
-->
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?
<!--
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.
-->
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?
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
4. Serve as another way to document the expected behavior of the code
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
-->
Yes, tests are added for this scenario.
### 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 uncomment the line below and explain which changes are breaking.
-->
<!-- **This PR includes breaking changes to public APIs.** -->
<!--
Please uncomment the line below (and provide explanation) if the changes fix either (a) a security vulnerability, (b) a bug that caused incorrect or invalid data to be produced, or (c) a bug that causes a crash (even when the API contract is upheld). We use this to highlight fixes to issues that may affect users without their knowledge. For this reason, fixing bugs that cause errors don't count, since those are usually obvious.
-->
<!-- **This PR contains a "Critical Fix".** -->
--
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] GH-37730: [C#] throw OverflowException in DecimalUtility if fractionalPart is too large [arrow]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #37731:
URL: https://github.com/apache/arrow/pull/37731#discussion_r1348794675
##########
csharp/src/Apache.Arrow/DecimalUtility.cs:
##########
@@ -52,7 +52,11 @@ internal static decimal GetDecimal(in ArrowBuffer valueBuffer, int index, int sc
BigInteger integerPart = BigInteger.DivRem(integerValue, scaleBy, out BigInteger fractionalPart);
if (integerPart > _maxDecimal || integerPart < _minDecimal) // decimal overflow, not much we can do here - C# needs a BigDecimal
{
- throw new OverflowException($"Value: {integerPart} too big or too small to be represented as a decimal");
+ throw new OverflowException($"Value: {integerValue} is too big or too small to be represented as a decimal");
+ }
+ 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");
Review Comment:
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.
##########
csharp/src/Apache.Arrow/DecimalUtility.cs:
##########
@@ -52,7 +52,11 @@ internal static decimal GetDecimal(in ArrowBuffer valueBuffer, int index, int sc
BigInteger integerPart = BigInteger.DivRem(integerValue, scaleBy, out BigInteger fractionalPart);
if (integerPart > _maxDecimal || integerPart < _minDecimal) // decimal overflow, not much we can do here - C# needs a BigDecimal
{
- throw new OverflowException($"Value: {integerPart} too big or too small to be represented as a decimal");
+ throw new OverflowException($"Value: {integerValue} is too big or too small to be represented as a decimal");
+ }
+ 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");
Review Comment:
Separately, I think it would be useful to differentiate the error messages here and possibly include the problematic value
--
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] GH-37730: [C#] throw OverflowException in DecimalUtility if fractionalPart is too large [arrow]
Posted by "davidhcoe (via GitHub)" <gi...@apache.org>.
davidhcoe commented on PR #37731:
URL: https://github.com/apache/arrow/pull/37731#issuecomment-1750728294
@lidavidm - can you help with this one too, please?
--
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] GH-37730: [C#] throw OverflowException in DecimalUtility if fractionalPart is too large [arrow]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm merged PR #37731:
URL: https://github.com/apache/arrow/pull/37731
--
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] GH-37730: [C#] throw OverflowException in DecimalUtility if fractionalPart is too large [arrow]
Posted by "davidhcoe (via GitHub)" <gi...@apache.org>.
davidhcoe commented on code in PR #37731:
URL: https://github.com/apache/arrow/pull/37731#discussion_r1349176422
##########
csharp/src/Apache.Arrow/DecimalUtility.cs:
##########
@@ -52,7 +52,11 @@ internal static decimal GetDecimal(in ArrowBuffer valueBuffer, int index, int sc
BigInteger integerPart = BigInteger.DivRem(integerValue, scaleBy, out BigInteger fractionalPart);
if (integerPart > _maxDecimal || integerPart < _minDecimal) // decimal overflow, not much we can do here - C# needs a BigDecimal
{
- throw new OverflowException($"Value: {integerPart} too big or too small to be represented as a decimal");
+ throw new OverflowException($"Value: {integerValue} is too big or too small to be represented as a decimal");
+ }
+ 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");
Review Comment:
updated in latest push
--
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] GH-37730: [C#] throw OverflowException in DecimalUtility if fractionalPart is too large [arrow]
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #37731:
URL: https://github.com/apache/arrow/pull/37731#issuecomment-1751977073
After merging your PR, Conbench analyzed the 5 benchmarking runs that have been run so far on merge-commit 629ecbd8fd62173b62758282ca113a331940be4f.
There was 1 benchmark result with an error:
- Commit Run on `ec2-m5-4xlarge-us-east-2` at [2023-10-06 21:30:48Z](https://conbench.ursa.dev/compare/runs/ea8010470629456abd85802d26acc2a3...c0b25096d6174fc4956b931340f7afaf/)
- [`tpch` (R) with engine=arrow, format=native, language=R, memory_map=False, query_id=TPCH-15, scale_factor=10](https://conbench.ursa.dev/benchmark-results/065208d88c7773038000a1b68155d225)
There were no benchmark performance regressions. 🎉
The [full Conbench report](https://github.com/apache/arrow/runs/17502469300) has more details. It also includes information about 7 possible false positives for unstable benchmarks that are known to sometimes produce them.
--
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
[GitHub] [arrow] github-actions[bot] commented on pull request #37731: GH-37730: [C#] throw OverflowException in DecimalUtility if fractionalPart is too large
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #37731:
URL: https://github.com/apache/arrow/pull/37731#issuecomment-1720349971
:warning: GitHub issue #37730 **has been automatically assigned in GitHub** to PR creator.
--
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