You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "teo-tsirpanis (via GitHub)" <gi...@apache.org> on 2023/03/17 20:42:57 UTC
[GitHub] [arrow] teo-tsirpanis opened a new pull request, #34618: GH-25163: [csharp] Support half-float arrays.
teo-tsirpanis opened a new pull request, #34618:
URL: https://github.com/apache/arrow/pull/34618
### 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.
--
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 #34618: GH-25163: [csharp] Support half-float arrays.
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #34618:
URL: https://github.com/apache/arrow/pull/34618#issuecomment-1474385095
* Closes: #25163
--
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] teo-tsirpanis commented on pull request #34618: GH-25163: [C#] Support half-float arrays.
Posted by "teo-tsirpanis (via GitHub)" <gi...@apache.org>.
teo-tsirpanis commented on PR #34618:
URL: https://github.com/apache/arrow/pull/34618#issuecomment-1480062545
Thanks for the review @westonpace. I addressed your comments.
> Do you know if C# runs the integration tests […]?
I have no idea.
--
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] teo-tsirpanis commented on a diff in pull request #34618: GH-25163: [C#] Support half-float arrays.
Posted by "teo-tsirpanis (via GitHub)" <gi...@apache.org>.
teo-tsirpanis commented on code in PR #34618:
URL: https://github.com/apache/arrow/pull/34618#discussion_r1140739008
##########
csharp/src/Apache.Arrow/Apache.Arrow.csproj:
##########
@@ -1,7 +1,7 @@
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
- <TargetFrameworks>netstandard1.3;netstandard2.0;netcoreapp3.1</TargetFrameworks>
+ <TargetFrameworks>netstandard1.3;netstandard2.0;netcoreapp3.1;net6.0</TargetFrameworks>
Review Comment:
How about I remove .NET Standard 1.3 and .NET Core 3.1?
--
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] westonpace commented on a diff in pull request #34618: GH-25163: [C#] Support half-float arrays.
Posted by "westonpace (via GitHub)" <gi...@apache.org>.
westonpace commented on code in PR #34618:
URL: https://github.com/apache/arrow/pull/34618#discussion_r1144856468
##########
csharp/README.md:
##########
@@ -126,12 +126,10 @@ for currently available features.
- Dictionary Encoding
- Types
- Tensor
- - Table
Review Comment:
Are you removing this because `Table` is not a type or because `Table` is supported?
--
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 #34618: GH-25163: [C#] Support half-float arrays.
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #34618:
URL: https://github.com/apache/arrow/pull/34618#issuecomment-1474392265
:warning: GitHub issue #25163 **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
[GitHub] [arrow] westonpace commented on a diff in pull request #34618: GH-25163: [C#] Support half-float arrays.
Posted by "westonpace (via GitHub)" <gi...@apache.org>.
westonpace commented on code in PR #34618:
URL: https://github.com/apache/arrow/pull/34618#discussion_r1147558852
##########
csharp/src/Apache.Arrow/Apache.Arrow.csproj:
##########
@@ -1,7 +1,7 @@
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
- <TargetFrameworks>netstandard1.3;netstandard2.0;netcoreapp3.1</TargetFrameworks>
+ <TargetFrameworks>netstandard1.3;netstandard2.0;netcoreapp3.1;net6.0</TargetFrameworks>
Review Comment:
Perhaps @eerhardt may have some idea of what would be best for long term maintenance. Not sure when it is normal to drop support for the older .NET standards.
--
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] westonpace commented on pull request #34618: GH-25163: [C#] Support half-float arrays.
Posted by "westonpace (via GitHub)" <gi...@apache.org>.
westonpace commented on PR #34618:
URL: https://github.com/apache/arrow/pull/34618#issuecomment-1485600695
> 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.
--
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] eerhardt commented on pull request #34618: GH-25163: [C#] Support half-float arrays.
Posted by "eerhardt (via GitHub)" <gi...@apache.org>.
eerhardt commented on PR #34618:
URL: https://github.com/apache/arrow/pull/34618#issuecomment-1483679314
> 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?
--
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] teo-tsirpanis commented on a diff in pull request #34618: GH-25163: [C#] Support half-float arrays.
Posted by "teo-tsirpanis (via GitHub)" <gi...@apache.org>.
teo-tsirpanis commented on code in PR #34618:
URL: https://github.com/apache/arrow/pull/34618#discussion_r1145063140
##########
csharp/README.md:
##########
@@ -126,12 +126,10 @@ for currently available features.
- Dictionary Encoding
- Types
- Tensor
- - Table
Review Comment:
Because `Table` is supported. It got added in #3806, which is closed instead of merged for some reason.
--
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 #34618: GH-25163: [csharp] Support half-float arrays.
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #34618:
URL: https://github.com/apache/arrow/pull/34618#issuecomment-1474385121
:warning: GitHub issue #25163 **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
[GitHub] [arrow] eerhardt commented on a diff in pull request #34618: GH-25163: [C#] Support half-float arrays.
Posted by "eerhardt (via GitHub)" <gi...@apache.org>.
eerhardt commented on code in PR #34618:
URL: https://github.com/apache/arrow/pull/34618#discussion_r1148205436
##########
csharp/src/Apache.Arrow/Apache.Arrow.csproj:
##########
@@ -1,7 +1,7 @@
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
- <TargetFrameworks>netstandard1.3;netstandard2.0;netcoreapp3.1</TargetFrameworks>
+ <TargetFrameworks>netstandard1.3;netstandard2.0;netcoreapp3.1;net6.0</TargetFrameworks>
Review Comment:
The current guidance is to avoid targeting netstandard1.x: https://learn.microsoft.com/en-us/dotnet/standard/library-guidance/cross-platform-targeting
But let's not do it in this PR. Instead, I think we should open a new issue for it. We may want to wait on netcoreapp3.1 though. It was just EOL'd a couple months ago. It sometimes takes people a bit longer to move off these EOL frameworks. It doesn't cost much to keep supporting it for a few months longer.
--
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] teo-tsirpanis commented on a diff in pull request #34618: GH-25163: [C#] Support half-float arrays.
Posted by "teo-tsirpanis (via GitHub)" <gi...@apache.org>.
teo-tsirpanis commented on code in PR #34618:
URL: https://github.com/apache/arrow/pull/34618#discussion_r1140739453
##########
csharp/src/Apache.Arrow/Apache.Arrow.csproj:
##########
@@ -1,7 +1,7 @@
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
- <TargetFrameworks>netstandard1.3;netstandard2.0;netcoreapp3.1</TargetFrameworks>
+ <TargetFrameworks>netstandard1.3;netstandard2.0;netcoreapp3.1;net6.0</TargetFrameworks>
<AllowUnsafeBlocks>true</AllowUnsafeBlocks>
<DefineConstants>$(DefineConstants);UNSAFE_BYTEBUFFER;BYTEBUFFER_NO_BOUNDS_CHECK;ENABLE_SPAN_T</DefineConstants>
Review Comment:
How about I remove `ENABLE_SPAN_T`? Is there any value in keeping it? Some stuff has two implementations and one is untested.
--
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] eerhardt commented on a diff in pull request #34618: GH-25163: [C#] Support half-float arrays.
Posted by "eerhardt (via GitHub)" <gi...@apache.org>.
eerhardt commented on code in PR #34618:
URL: https://github.com/apache/arrow/pull/34618#discussion_r1148206830
##########
csharp/src/Apache.Arrow/Apache.Arrow.csproj:
##########
@@ -1,7 +1,7 @@
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
- <TargetFrameworks>netstandard1.3;netstandard2.0;netcoreapp3.1</TargetFrameworks>
+ <TargetFrameworks>netstandard1.3;netstandard2.0;netcoreapp3.1;net6.0</TargetFrameworks>
<AllowUnsafeBlocks>true</AllowUnsafeBlocks>
<DefineConstants>$(DefineConstants);UNSAFE_BYTEBUFFER;BYTEBUFFER_NO_BOUNDS_CHECK;ENABLE_SPAN_T</DefineConstants>
Review Comment:
You don't want to remove that define. It makes the flatbuffer code work better.
--
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] eerhardt merged pull request #34618: GH-25163: [C#] Support half-float arrays.
Posted by "eerhardt (via GitHub)" <gi...@apache.org>.
eerhardt merged PR #34618:
URL: https://github.com/apache/arrow/pull/34618
--
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] eerhardt commented on a diff in pull request #34618: GH-25163: [C#] Support half-float arrays.
Posted by "eerhardt (via GitHub)" <gi...@apache.org>.
eerhardt commented on code in PR #34618:
URL: https://github.com/apache/arrow/pull/34618#discussion_r1148226643
##########
csharp/test/Directory.Build.props:
##########
@@ -21,6 +21,7 @@
<PropertyGroup>
<IsPackable>false</IsPackable>
+ <LangVersion>default</LangVersion>
Review Comment:
In https://github.com/apache/arrow/pull/34133 we are moving all the code to `LangVersion=latest`. Maybe we should just make that change here too, instead of just changing the test props.
##########
csharp/README.md:
##########
@@ -79,7 +79,7 @@ for currently available features.
- Int8, Int16, Int32, Int64
- UInt8, UInt16, UInt32, UInt64
-- Float, Double
+- Float, Double, Half-float (.NET 6+)
Review Comment:
Can you update https://github.com/apache/arrow/blob/main/docs/source/status.rst as well?
##########
csharp/src/Apache.Arrow/Arrays/HalfFloatArray.cs:
##########
@@ -0,0 +1,46 @@
+// Licensed to the Apache Software Foundation (ASF) under one or more
+// contributor license agreements. See the NOTICE file distributed with
+// this work for additional information regarding copyright ownership.
+// The ASF licenses this file to You under the Apache License, Version 2.0
+// (the "License"); you may not use this file except in compliance with
+// the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+using Apache.Arrow.Types;
+using System;
+
+namespace Apache.Arrow
+{
+ public class HalfFloatArray : PrimitiveArray<Half>
Review Comment:
Do we have `///` doc comments on the other Array types? Can we add a summary here?
--
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] teo-tsirpanis commented on a diff in pull request #34618: GH-25163: [C#] Support half-float arrays.
Posted by "teo-tsirpanis (via GitHub)" <gi...@apache.org>.
teo-tsirpanis commented on code in PR #34618:
URL: https://github.com/apache/arrow/pull/34618#discussion_r1148358613
##########
csharp/src/Apache.Arrow/Arrays/HalfFloatArray.cs:
##########
@@ -0,0 +1,46 @@
+// Licensed to the Apache Software Foundation (ASF) under one or more
+// contributor license agreements. See the NOTICE file distributed with
+// this work for additional information regarding copyright ownership.
+// The ASF licenses this file to You under the Apache License, Version 2.0
+// (the "License"); you may not use this file except in compliance with
+// the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+using Apache.Arrow.Types;
+using System;
+
+namespace Apache.Arrow
+{
+ public class HalfFloatArray : PrimitiveArray<Half>
Review Comment:
None of the other array types are documented. 😅
--
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] eerhardt commented on a diff in pull request #34618: GH-25163: [C#] Support half-float arrays.
Posted by "eerhardt (via GitHub)" <gi...@apache.org>.
eerhardt commented on code in PR #34618:
URL: https://github.com/apache/arrow/pull/34618#discussion_r1148363019
##########
docs/source/status.rst:
##########
@@ -101,7 +101,8 @@ Data Types
Notes:
-* \(1) Nested dictionaries not supported
+* \(1) FLoat16 support in C# is only available when targeting .NET 6+.
Review Comment:
```suggestion
* \(1) Float16 support in C# is only available when targeting .NET 6+.
```
--
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] ursabot commented on pull request #34618: GH-25163: [C#] Support half-float arrays.
Posted by "ursabot (via GitHub)" <gi...@apache.org>.
ursabot commented on PR #34618:
URL: https://github.com/apache/arrow/pull/34618#issuecomment-1486119822
Benchmark runs are scheduled for baseline = 196fbe5f5e0669b1f8bf71005c8a4c6a4e0fb2da and contender = 5e7e76425185d2408008c7f2bf2470ac06a81d2b. 5e7e76425185d2408008c7f2bf2470ac06a81d2b is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished :arrow_down:0.0% :arrow_up:0.0%] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/28961df07e034a3da70ad1904a218184...4cb49b325ad44c1a82b2e4489c6e6efc/)
[Failed :arrow_down:0.44% :arrow_up:0.03%] [test-mac-arm](https://conbench.ursa.dev/compare/runs/71883c62682e4230a119b36133c50da0...9f90bb6d64814d65b3897ed8d9be6b39/)
[Finished :arrow_down:0.0% :arrow_up:0.0%] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/f1abaff0cf41447f8e1e356b8739b8e0...827875b73ae342b09fca65e6c4afda3b/)
[Finished :arrow_down:0.92% :arrow_up:0.0%] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/c38bd549ed1342e197238b85123ba1ee...c6cbbd7a4b994ad28869606007f60fe7/)
Buildkite builds:
[Finished] [`5e7e7642` ec2-t3-xlarge-us-east-2](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/2585)
[Failed] [`5e7e7642` test-mac-arm](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/2615)
[Finished] [`5e7e7642` ursa-i9-9960x](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/2583)
[Finished] [`5e7e7642` ursa-thinkcentre-m75q](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/2606)
[Finished] [`196fbe5f` ec2-t3-xlarge-us-east-2](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/2584)
[Failed] [`196fbe5f` test-mac-arm](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/2614)
[Finished] [`196fbe5f` ursa-i9-9960x](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/2582)
[Finished] [`196fbe5f` ursa-thinkcentre-m75q](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/2605)
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
--
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