Skip to content
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-38351: [C#] Add SqlDecimal support to Decimal128Array #38481

Merged
merged 6 commits into from
Oct 27, 2023

Conversation

CurtHagenlocher
Copy link
Contributor

@CurtHagenlocher CurtHagenlocher commented Oct 26, 2023

What changes are included in this PR?

Adds support for reading and writing System.Data.SqlTypes.SqlDecimal against Decimal128Array.

Are these changes tested?

Yes.

Are there any user-facing changes?

Adds functions to the API.

@github-actions
Copy link

⚠️ GitHub issue #38351 has been automatically assigned in GitHub to PR creator.

@github-actions github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Oct 26, 2023
Copy link
Contributor

@davidhcoe davidhcoe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In SqlDecimal, the max precision and scale are both 38. In Snowflake, the max is NUMERIC(38,37). Can you include a test with these max values?

@davidhcoe
Copy link
Contributor

looks good after adding the tests

@CurtHagenlocher
Copy link
Contributor Author

r-project.org is down, causing the Docker build to fail. If it doesn't come back reasonably soon, I may commit anyway.

@CurtHagenlocher
Copy link
Contributor Author

The good news for me is that the unrelated build failure gave me more time to think about the code I thought was wrong and to come up with a test which showed that it indeed was wrong... .

@CurtHagenlocher CurtHagenlocher merged commit 1633f8d into apache:main Oct 27, 2023
8 checks passed
@CurtHagenlocher CurtHagenlocher removed the awaiting committer review Awaiting committer review label Oct 27, 2023
@conbench-apache-arrow
Copy link

After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 1633f8d.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 4 possible false positives for unstable benchmarks that are known to sometimes produce them.

@CurtHagenlocher CurtHagenlocher deleted the dev/curth/SqlDecimal128 branch October 30, 2023 16:36
CurtHagenlocher pushed a commit to apache/arrow-adbc that referenced this pull request Oct 31, 2023
This PR:

- Adds support for SqlDecimal for Decimal128 (follow up to
apache/arrow#38481)
- Treats Decimal256 values as string (follow up to
apache/arrow#38508)
- Adds a new DecimalBehavior to the Client to allow the caller to
determine how to treat decimal values
- Standardizes the test frameworks to Xunit

Addresses #1230

---------

Co-authored-by: David Coe <coedavid@umich.edu>
vleslief-ms pushed a commit to vleslief-ms/arrow-adbc that referenced this pull request Nov 9, 2023
This PR:

- Adds support for SqlDecimal for Decimal128 (follow up to
apache/arrow#38481)
- Treats Decimal256 values as string (follow up to
apache/arrow#38508)
- Adds a new DecimalBehavior to the Client to allow the caller to
determine how to treat decimal values
- Standardizes the test frameworks to Xunit

Addresses apache#1230

---------

Co-authored-by: David Coe <coedavid@umich.edu>
loicalleyne pushed a commit to loicalleyne/arrow that referenced this pull request Nov 13, 2023
…e#38481)

### What changes are included in this PR?

Adds support for reading and writing System.Data.SqlTypes.SqlDecimal against Decimal128Array.

### Are these changes tested?

Yes.

### Are there any user-facing changes?

Adds functions to the API.
* Closes: apache#38351

Authored-by: Curt Hagenlocher <curt@hagenlocher.org>
Signed-off-by: Curt Hagenlocher <curt@hagenlocher.org>
vleslief-ms pushed a commit to vleslief-ms/arrow-adbc that referenced this pull request Nov 15, 2023
This PR:

- Adds support for SqlDecimal for Decimal128 (follow up to
apache/arrow#38481)
- Treats Decimal256 values as string (follow up to
apache/arrow#38508)
- Adds a new DecimalBehavior to the Client to allow the caller to
determine how to treat decimal values
- Standardizes the test frameworks to Xunit

Addresses apache#1230

---------

Co-authored-by: David Coe <coedavid@umich.edu>
dgreiss pushed a commit to dgreiss/arrow that referenced this pull request Feb 19, 2024
…e#38481)

### What changes are included in this PR?

Adds support for reading and writing System.Data.SqlTypes.SqlDecimal against Decimal128Array.

### Are these changes tested?

Yes.

### Are there any user-facing changes?

Adds functions to the API.
* Closes: apache#38351

Authored-by: Curt Hagenlocher <curt@hagenlocher.org>
Signed-off-by: Curt Hagenlocher <curt@hagenlocher.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[C#] Better support for decimal values which exceed the range of the BCL's System.Decimal in Decimal128
2 participants