ARROW-11422: [C#] add decimal support #9356

llerrowa · 2021-01-28T21:40:09Z

No description provided.

github-actions · 2021-01-28T21:40:36Z

Thanks for opening a pull request!

Could you open an issue for this pull request on JIRA?
https://issues.apache.org/jira/browse/ARROW

Then could you also rename pull request title in the following format?

ARROW-${JIRA_ID}: [${COMPONENT}] ${SUMMARY}

See also:

github-actions · 2021-01-28T21:43:38Z

https://issues.apache.org/jira/browse/ARROW-11422

emkornfield · 2021-02-02T04:15:08Z

I'm not familiar with the C# implementation but note decimals can now be 128 or 256 bit.

eerhardt

I don't believe PrimitiveArray<decimal> is the right thing to use here because the memory layout between C# and Arrow are different.

Check out the C++ or Java implementations to see how the Arrow decimal values are encoded.

csharp/src/Apache.Arrow/Types/DecimalType.cs

csharp/src/Apache.Arrow/Arrays/DecimalArray.cs

using a fixed size binary array

eerhardt

Looks like a good start. Here are my first round of comments.

csharp/src/Apache.Arrow/Flatbuf/BodyCompressionMethod.cs

csharp/test/Apache.Arrow.Tests/ArrowReaderVerifier.cs

csharp/src/Apache.Arrow/ArrowBuffer.Builder.cs

csharp/src/Apache.Arrow/DecimalUtility.cs

csharp/test/Apache.Arrow.Benchmarks/ArrowReaderBenchmark.cs

csharp/test/Apache.Arrow.Tests/TestData.cs

csharp/src/Apache.Arrow/Arrays/Decimal128Array.cs

csharp/src/Apache.Arrow/DecimalUtility.cs

expose ValueBuffer on FixedSizeBinaryArray and the visitor pattern

eerhardt

This is looking really good! Thanks for the good work here @suchagit.

I think after this round of feedback is addressed, it should be ready to be merged.

csharp/src/Apache.Arrow/DecimalUtility.cs

eerhardt · 2021-03-01T16:27:43Z

Can you also update

https://github.com/apache/arrow/blob/master/docs/source/status.rst

To check Decimal128 and Decimal256 support for C#?

Also, should be able to remove Decimal from the Not Implemented section of https://github.com/apache/arrow/blob/master/csharp/README.md

eerhardt

Some more comments on DecimalUtility. But I think this is really close. Thanks for all the good work.

csharp/src/Apache.Arrow/DecimalUtility.cs

eerhardt

I just have one tiny comment on a test. If necessary we can fix it in a follow up PR.

But this looks great! Thanks for the great contribution here and helping fill out the C# Arrow library.

I will merge this later today, or early tomorrow unless it receives more feedback.

eerhardt · 2021-03-10T17:52:42Z

csharp/test/Apache.Arrow.Tests/DecimalUtilityTests.cs

+                }
+                else
+                {
+                    builder.Append(d);


Would it be helpful to get the value back out and make sure it is the same?

Sure, that's done now

the decimal we append should match what we read back

eerhardt

Thanks again!

eerhardt · 2021-03-25T20:31:31Z

This fell of my radar, sorry! Merging.

Closes apache#9356 from suchagit/master Authored-by: Ali <alistair.worrell@gsacapital.com> Signed-off-by: Eric Erhardt <eric.erhardt@microsoft.com>

c# - add decimal support

2741a4d

github-actions bot added the Component: C# label Jan 28, 2021

llerrowa changed the title ~~[ARROW-11422] [C#] add decimal support~~ ARROW-11422: [C#] add decimal support Jan 28, 2021

emkornfield requested a review from eerhardt February 2, 2021 04:14

eerhardt reviewed Feb 2, 2021

View reviewed changes

csharp/src/Apache.Arrow/Types/DecimalType.cs Outdated Show resolved Hide resolved

csharp/src/Apache.Arrow/Arrays/DecimalArray.cs Outdated Show resolved Hide resolved

update flatc

8b739a5

jorgecarleitao force-pushed the master branch from d4608a9 to 356c300 Compare February 14, 2021 12:09

llerrowa added 2 commits February 15, 2021 19:45

support 128 and 256bit decimal types

ca3cca3

using a fixed size binary array

add licencing

fa2b898

eerhardt reviewed Feb 22, 2021

View reviewed changes

csharp/src/Apache.Arrow/DecimalUtility.cs Outdated Show resolved Hide resolved

eerhardt reviewed Feb 22, 2021

View reviewed changes

csharp/src/Apache.Arrow/DecimalUtility.cs Outdated Show resolved Hide resolved

llerrowa added 11 commits February 22, 2021 19:29

make auto generated enums internal and move to enum folder

18bc30d

combine decimal test methods into one for a FixedSizeBinaryArray

12fceb0

expose ValueBuffer on FixedSizeBinaryArray and the visitor pattern

make fixed size arrow buffer build internal and reuse for 64 byte call

840dcbd

decimal utility to internal

a9ecde5

remove redundant cast

f64b57e

test data to use different scale / precision for 128 / 256 bit decimal

ec088eb

else onto new line

f71c364

remove unused code

75151a8

support big and little endian

2c65ae1

rework get bytes

d49e995

update decimal scale in test data

a838421

eerhardt reviewed Mar 1, 2021

View reviewed changes

llerrowa added 2 commits March 2, 2021 13:16

rework decimal utility

c08dcd1

update support docs for c# decimal

c9dc454

llerrowa added 2 commits March 2, 2021 16:41

add decimal utility tests

e58d276

add licencing

39b7e8a

eerhardt reviewed Mar 8, 2021

View reviewed changes

llerrowa added 3 commits March 10, 2021 16:48

use string interpolation in exception messages

51cce12

decimal utility internal and rework tests

e708c18

avoid allocations for netcore

0550ec1

eerhardt approved these changes Mar 10, 2021

View reviewed changes

perform equality in decimal utility test

28a5880

the decimal we append should match what we read back

eerhardt approved these changes Mar 10, 2021

View reviewed changes

eerhardt closed this in 0bea590 Mar 25, 2021

dbeavon mentioned this pull request Apr 17, 2021

[FEATURE REQUEST]: Support System.Decimal in CreateDataFrame API dotnet/spark#818

Open

GeorgeAp pushed a commit to sirensolutions/arrow that referenced this pull request Jun 7, 2021

ARROW-11422: [C#] add decimal support

e9660fa

Closes apache#9356 from suchagit/master Authored-by: Ali <alistair.worrell@gsacapital.com> Signed-off-by: Eric Erhardt <eric.erhardt@microsoft.com>

michalursa pushed a commit to michalursa/arrow that referenced this pull request Jun 10, 2021

ARROW-11422: [C#] add decimal support

e638a91

Closes apache#9356 from suchagit/master Authored-by: Ali <alistair.worrell@gsacapital.com> Signed-off-by: Eric Erhardt <eric.erhardt@microsoft.com>

michalursa pushed a commit to michalursa/arrow that referenced this pull request Jun 13, 2021

ARROW-11422: [C#] add decimal support

98afd8a

Closes apache#9356 from suchagit/master Authored-by: Ali <alistair.worrell@gsacapital.com> Signed-off-by: Eric Erhardt <eric.erhardt@microsoft.com>

asfimport mentioned this pull request Mar 25, 2021

[C#] Add support for decimals #27310

Closed

ARROW-11422: [C#] add decimal support #9356

ARROW-11422: [C#] add decimal support #9356

Uh oh!

Conversation

llerrowa commented Jan 28, 2021

Uh oh!

github-actions bot commented Jan 28, 2021

Uh oh!

github-actions bot commented Jan 28, 2021

Uh oh!

emkornfield commented Feb 2, 2021

Uh oh!

eerhardt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

eerhardt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

eerhardt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

eerhardt commented Mar 1, 2021

Uh oh!

eerhardt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

eerhardt left a comment

Choose a reason for hiding this comment

Uh oh!

eerhardt Mar 10, 2021

Choose a reason for hiding this comment

Uh oh!

llerrowa Mar 10, 2021

Choose a reason for hiding this comment

Uh oh!

eerhardt left a comment

Choose a reason for hiding this comment

Uh oh!

eerhardt commented Mar 25, 2021

Uh oh!

Uh oh!