GH-25163: [C#] Support half-float arrays.#34618
Conversation
|
|
|
|
|
westonpace
left a comment
There was a problem hiding this comment.
This seems like a fairly minimal set of tests for a new types. Also, from some searching, it appears there are quite a few places we are visiting the various types:
RecordBatch.Builder.cs
ArrowArrayBuilderFactory.cs
ArrayBuilderTests.cs
ArrowReaderVerifier.cs
ArrowStreamWriter.cs
Do we need to add half-float support there?
Do you know if C# runs the integration tests (these are tests that read/write known golden IPC files from other implementations)? Although I'm not sure if we have half-float arrays in the IPC integration tests.
|
Thanks for the review @westonpace. I addressed your comments.
I have no idea. |
I'm not seeing half-float array in the current integration tests. Is that a test hole? |
eerhardt
left a comment
There was a problem hiding this comment.
Thanks for the contribution, @teo-tsirpanis!
I just have a few comments. This looks really good.
eerhardt
left a comment
There was a problem hiding this comment.
Thanks again for the contribution @teo-tsirpanis!
I'll merge this unless I see more feedback.
I don't think we need to hold this PR up for it but that does seem like something we'd like to fix at some point. i don't think too many implementations have half-float support at the moment so maybe it isn't surprising. |
|
Benchmark runs are scheduled for baseline = 196fbe5 and contender = 5e7e764. 5e7e764 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
### Rationale for this change .NET 5 introduced the [`System.Half`](https://devblogs.microsoft.com/dotnet/introducing-the-half-type/) type, which represents 16-bit floats. This PR adds support for them in Apache Arrow. ### What changes are included in this PR? I multi-targeted the `Apache.Arrow` project to .NET 6 (because .NET 5 is unsupported) and added a `HalfFloatArray` type with a very similar implementation as the other floating-point array types. I also updated the README. ### Are these changes tested? Yes. I also refactored the array tests to reduce duplication among the various numeric types. ### Are there any user-facing changes? Yes. * Closes: apache#25163 Lead-authored-by: Theodore Tsirpanis <teo@tsirpanis.gr> Co-authored-by: Eric Erhardt <eric.erhardt@microsoft.com> Signed-off-by: Eric Erhardt <eric.erhardt@microsoft.com>
Rationale for this change
.NET 5 introduced the
System.Halftype, which represents 16-bit floats. This PR adds support for them in Apache Arrow.What changes are included in this PR?
I multi-targeted the
Apache.Arrowproject to .NET 6 (because .NET 5 is unsupported) and added aHalfFloatArraytype with a very similar implementation as the other floating-point array types.I also updated the README.
Are these changes tested?
Yes. I also refactored the array tests to reduce duplication among the various numeric types.
Are there any user-facing changes?
Yes.