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.

Sign up for GitHub

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

Jump to bottom

GH-25163: [C#] Support half-float arrays. #34618

Merged
merged 9 commits into from
Mar 27, 2023
Merged

GH-25163: [C#] Support half-float arrays. #34618

merged 9 commits into from
Mar 27, 2023

Conversation

teo-tsirpanis
Copy link
Contributor

@teo-tsirpanis teo-tsirpanis commented Mar 17, 2023 •
edited by github-actions bot
Loading

Rationale for this change

.NET 5 introduced the System.Half 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.

@github-actions
Copy link

@github-actions
Copy link

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

@teo-tsirpanis teo-tsirpanis changed the title GH-25163: [csharp] Support half-float arrays. GH-25163: [C#] Support half-float arrays. Mar 17, 2023
@github-actions
Copy link

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

@teo-tsirpanis teo-tsirpanis mentioned this pull request Mar 17, 2023
Closed
@github-actions github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Mar 17, 2023
westonpace
westonpace reviewed Mar 22, 2023
View reviewed changes
Copy link
Member

@westonpace westonpace left a comment

Choose a reason for hiding this comment

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

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.

@github-actions github-actions bot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Mar 22, 2023
@github-actions github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Mar 22, 2023
@teo-tsirpanis
Copy link
Contributor Author

Thanks for the review @westonpace. I addressed your comments.

Do you know if C# runs the integration tests […]?

I have no idea.

@github-actions github-actions bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Mar 24, 2023
@eerhardt
Copy link
Contributor

Although I'm not sure if we have half-float arrays in the IPC integration tests.

I'm not seeing half-float array in the current integration tests. Is that a test hole?

eerhardt
eerhardt reviewed Mar 25, 2023
View reviewed changes
Copy link
Contributor

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution, @teo-tsirpanis!

I just have a few comments. This looks really good.

@github-actions github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Mar 25, 2023
@teo-tsirpanis teo-tsirpanis requested a review from eerhardt March 25, 2023 12:07
eerhardt
eerhardt reviewed Mar 25, 2023
View reviewed changes
@github-actions github-actions bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Mar 25, 2023
eerhardt
eerhardt approved these changes Mar 25, 2023
View reviewed changes
Copy link
Contributor

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

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

Thanks again for the contribution @teo-tsirpanis!

I'll merge this unless I see more feedback.

@github-actions github-actions bot added awaiting merge Awaiting merge and removed awaiting changes Awaiting changes labels Mar 25, 2023
@westonpace
Copy link
Member

I'm not seeing half-float array in the current integration tests. Is that a test hole?

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.

@eerhardt eerhardt merged commit 5e7e764 into apache:main Mar 27, 2023
@teo-tsirpanis teo-tsirpanis deleted the net6 branch March 27, 2023 21:46
@ursabot
Copy link

ursabot commented Mar 28, 2023

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.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Failed ⬇️0.44% ⬆️0.03%] test-mac-arm
[Finished ⬇️0.0% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.92% ⬆️0.0%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] 5e7e7642 ec2-t3-xlarge-us-east-2
[Failed] 5e7e7642 test-mac-arm
[Finished] 5e7e7642 ursa-i9-9960x
[Finished] 5e7e7642 ursa-thinkcentre-m75q
[Finished] 196fbe5f ec2-t3-xlarge-us-east-2
[Failed] 196fbe5f test-mac-arm
[Finished] 196fbe5f ursa-i9-9960x
[Finished] 196fbe5f ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

ArgusLi pushed a commit to Bit-Quill/arrow that referenced this pull request May 15, 2023
@teo-tsirpanis @eerhardt @ArgusLi
apacheGH-25163: [C#] Support half-float arrays. (apache#34618)
c662f09 
### 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@westonpace westonpace westonpace left review comments

@eerhardt eerhardt eerhardt approved these changes

Assignees
No one assigned
Labels
awaiting merge Awaiting merge Component: C# Component: Documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[C#] Support Float16
4 participants
@teo-tsirpanis @eerhardt @westonpace @ursabot