GH-34737: [C#] C Data interface for schemas and types #34133

wjones127 · 2023-02-10T22:22:44Z

Rationale for this change

This starts the C Data Interface implementation for C# with integration for ArrowSchema. ArrowArray will come in a follow-up PR.

What changes are included in this PR?

Adds classes CArrowSchema and ImportedArrowSchema which allow interacting with the CArrowSchema.
Adds integration tests with PyArrow, inspired by the similar integration tests in arrow-rs

Are these changes tested?

Yes, the PyArrow integration tests validate the functionality.

Are there any user-facing changes?

This only adds new APIs, and doesn't change any existing ones.

github-actions · 2023-02-10T22:23:05Z

Closes: [C#] Implement C Data Interface for C# #33856

github-actions · 2023-02-10T22:23:07Z

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

ci/scripts/csharp_test.sh

csharp/src/Apache.Arrow/C/CArrowSchema.cs

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

wjones127 · 2023-02-27T16:54:57Z

@adamreeve would you be interested in reviewing?

csharp/src/Apache.Arrow/C/CArrowSchema.cs

adamreeve · 2023-02-27T21:41:03Z

csharp/src/Apache.Arrow/C/CArrowSchema.cs

+        public string metadata;
+        public long flags;
+        public long n_children;
+        public IntPtr* children;


Should children just be IntPtr rather than IntPtr*? That would allow the struct itself to not need to be unsafe, although might make dealing with children internally a bit more fiddly.

Yeah looking at this, I don't know how I would do this without unsafe:

Marshal.AllocHGlobal(numFields * sizeof(IntPtr))

And that's called in the constructor. (Compiler says that you can't call sizeof(IntPtr) outside of an unsafe context; not sure why.)

What about IntPtr.Size?

Marshal.AllocHGlobal(numFields * IntPtr.Size);

Yeah I think IntPtr.Size works here, for other struct types you'd generally want to use Marshal.SizeOf in this scenario

Okay I've narrowed it so unsafe only appears when constructing and accessing children. Does that seem like the improvement we are looking for?

👍 yeah I think that is preferable

csharp/src/Apache.Arrow/C/ImportedArrowSchema.cs

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

wjones127 · 2023-02-28T22:10:55Z

Thanks for the feedback @adamreeve. I've realized I could simplify the API by changing the Export* methods into constructors for CArrowSchema. And I've moved the ImportedArrowSchema into an internal class within CArrowSchema so that it can handle the common import and disposal logic. Let me know what you think.

westonpace

Sorry, just barely started looking at this but will have to come back to it tomorrow.

westonpace · 2023-03-01T01:31:53Z

csharp/src/Apache.Arrow/C/CArrowSchema.cs

+using System.Runtime.InteropServices;
+using Apache.Arrow.Types;
+
+[UnmanagedFunctionPointer(CallingConvention.StdCall)]


Why StdCall?

To be honest, I'm not sure on this. It was the default and seemed to work with the integration tests fine.

Ah, I read up some more on this. I don't think StdCall is the default (that would be WinApi which defaults to StdCall on Windows and CDecl on Linux).

That being said, I don't think we need to worry about calling convention anywhere as it is only relevant when working with 32-bit binaries: https://stackoverflow.com/questions/34832679/is-the-callingconvention-ignored-in-64-bit-net-applications

I'm pretty sure Arrow-C++ doesn't even build on 32-bit (#32111) so this is probably a non-issue. With that in mind, perhaps skip the UnamangedFunctionPointer entirely? It seems its only purpose is to specify the calling convention.

@westonpace Arrow C++ does build on 32-bit Linux (we have nightly crossbow tests for that).

westonpace · 2023-03-01T01:40:49Z

csharp/src/Apache.Arrow/C/CArrowSchema.cs

+        public string metadata;
+        public long flags;
+        public long n_children;
+        public IntPtr* children;


What about IntPtr.Size?

Marshal.AllocHGlobal(numFields * IntPtr.Size);

eerhardt

I just had a few more comments that can be addressed post merge.

Thanks for the contribution here, @wjones127!

I'll merge this early this week, unless I see more feedback.

eerhardt · 2023-03-25T16:17:30Z

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

+        }
+    }
+
+    public class CDataSchemaPythonTest


Can we split this into a new file? Having a single class per file makes it easier to navigate the code and find what you are looking for.

eerhardt · 2023-03-25T17:47:55Z

csharp/src/Apache.Arrow/C/CArrowSchema.cs

+        public long n_children;
+        public CArrowSchema** children;
+        public CArrowSchema* dictionary;
+        public delegate* unmanaged[Stdcall]<CArrowSchema*, void> release;


In the future (when we start targeting net6.0+), I think we will want to drop the [Stdcall] here and just use the default:

public delegate* unmanaged<CArrowSchema*, void> release;

But that doesn't work in the older TFMs, so what we have now is fine.

eerhardt · 2023-03-25T17:53:43Z

csharp/src/Apache.Arrow/C/CArrowSchemaExporter.cs

+        {
+            ExportType(field.DataType, schema);
+            schema->name = StringUtil.ToCStringUtf8(field.Name);
+            // TODO: field metadata


Can you log issues for these TODOs?

I've created an umbrella issue to track the remaining C data interface tasks (#33856) and another one to track remaining data types to implement (#34736).

github-actions · 2023-03-27T02:18:20Z

Closes: [C#] Define C Data Interface for schemas/fields/types #34737

github-actions · 2023-03-27T02:18:22Z

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

eerhardt · 2023-03-28T14:44:29Z

Re-running the macOS 12 Python 3 leg which failed. Once it passes, I'll merge this PR.

eerhardt · 2023-03-30T19:04:04Z

@westonpace - I see you still have changes requested. Is that stale? or are you still requesting changes on the current proposal?

The macOS 12 Python 3 leg is still failing. Does anyone know if that is related to these changes? @wjones127, can you merge with main to see if the errors go away?

wjones127 · 2023-03-30T19:25:11Z

@eerhardt I think we can ignore that error for now. The test is flakey and we are still looking into the root cause.

#34743

westonpace

Sorry, my review was stale. I'm happy with where this is now.

ursabot · 2023-04-04T06:03:18Z

Benchmark runs are scheduled for baseline = 7c307b1 and contender = f02d351. f02d351 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.0% ⬆️0.0%] test-mac-arm
[Finished ⬇️0.0% ⬆️0.0%] ursa-i9-9960x
[Failed ⬇️0.0% ⬆️0.0%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] f02d3511 ec2-t3-xlarge-us-east-2
[Failed] f02d3511 test-mac-arm
[Finished] f02d3511 ursa-i9-9960x
[Failed] f02d3511 ursa-thinkcentre-m75q
[Finished] 7c307b14 ec2-t3-xlarge-us-east-2
[Failed] 7c307b14 test-mac-arm
[Finished] 7c307b14 ursa-i9-9960x
[Failed] 7c307b14 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

…4133) ### Rationale for this change This starts the C Data Interface implementation for C# with integration for `ArrowSchema`. `ArrowArray` will come in a follow-up PR. ### What changes are included in this PR? * Adds classes `CArrowSchema` and `ImportedArrowSchema` which allow interacting with the `CArrowSchema`. * Adds integration tests with PyArrow, inspired by the similar integration tests in [arrow-rs](https://github.com/apache/arrow-rs/blob/master/arrow/src/pyarrow.rs) ### Are these changes tested? Yes, the PyArrow integration tests validate the functionality. ### Are there any user-facing changes? This only adds new APIs, and doesn't change any existing ones. * Closes: apache#33856 * Closes: apache#34737 Lead-authored-by: Will Jones <[email protected]> Co-authored-by: Weston Pace <[email protected]> Signed-off-by: Eric Erhardt <[email protected]>

github-actions bot added Component: C# Component: Python labels Feb 10, 2023

wjones127 force-pushed the GH-33856-csharp-c-data branch 2 times, most recently from fce7987 to 59eb7ea Compare February 11, 2023 00:10

github-actions bot removed the Component: Python label Feb 11, 2023

wjones127 force-pushed the GH-33856-csharp-c-data branch 2 times, most recently from 59f34ac to 5198a0f Compare February 18, 2023 00:06

wjones127 force-pushed the GH-33856-csharp-c-data branch 6 times, most recently from 132b21c to 87e734f Compare February 25, 2023 00:05

wjones127 marked this pull request as ready for review February 25, 2023 00:20

wjones127 requested review from assignUser, kou, raulcd and westonpace as code owners February 25, 2023 00:20

kou changed the title ~~GH-33856: [C#] c data interface for schemas and types~~ GH-33856: [C#] C Data interface for schemas and types Feb 25, 2023

kou requested a review from eerhardt February 25, 2023 21:41

kou reviewed Feb 26, 2023

View reviewed changes

adamreeve reviewed Feb 27, 2023

View reviewed changes

github-actions bot added the awaiting committer review Awaiting committer review label Feb 28, 2023

wjones127 force-pushed the GH-33856-csharp-c-data branch 2 times, most recently from d4aff85 to 293db1d Compare February 28, 2023 22:08

westonpace reviewed Mar 1, 2023

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Mar 16, 2023

eerhardt mentioned this pull request Mar 25, 2023

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

Merged

eerhardt approved these changes Mar 25, 2023

View reviewed changes

github-actions bot added awaiting merge Awaiting merge awaiting changes Awaiting changes and removed awaiting changes Awaiting changes awaiting merge Awaiting merge labels Mar 25, 2023

wjones127 changed the title ~~GH-33856: [C#] C Data interface for schemas and types~~ GH-34737: [C#] C Data interface for schemas and types Mar 27, 2023

refactor: split classes to new files

02ccfd3

github-actions bot added awaiting change review Awaiting change review awaiting changes Awaiting changes and removed awaiting changes Awaiting changes awaiting change review Awaiting change review labels Mar 27, 2023

wjones127 mentioned this pull request Mar 28, 2023

[C#] Implement C Stream Interface in C# #33857

Closed

westonpace approved these changes Mar 31, 2023

View reviewed changes

github-actions bot added awaiting merge Awaiting merge and removed awaiting changes Awaiting changes labels Mar 31, 2023

eerhardt merged commit f02d351 into apache:main Apr 4, 2023

raulcd mentioned this pull request Apr 11, 2023

[C#][CI] Nightly nuget build fails due to missing python #35029

Closed

eerhardt mentioned this pull request May 22, 2023

feat(csharp): adding C# functionality apache/arrow-adbc#697

Merged

teo-tsirpanis mentioned this pull request May 30, 2023

GH-35809: [C#] Improvements to the C Data Interface #35810

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-34737: [C#] C Data interface for schemas and types #34133

GH-34737: [C#] C Data interface for schemas and types #34133

wjones127 commented Feb 10, 2023 •

edited by github-actions bot

Loading

github-actions bot commented Feb 10, 2023

github-actions bot commented Feb 10, 2023

wjones127 commented Feb 27, 2023

adamreeve Feb 27, 2023

wjones127 Feb 28, 2023

westonpace Mar 1, 2023

adamreeve Mar 1, 2023

wjones127 Mar 1, 2023

adamreeve Mar 2, 2023

wjones127 commented Feb 28, 2023

westonpace left a comment

westonpace Mar 1, 2023

wjones127 Mar 1, 2023

westonpace Mar 2, 2023

pitrou May 30, 2023

westonpace Mar 1, 2023

eerhardt left a comment

eerhardt Mar 25, 2023

wjones127 Mar 27, 2023

eerhardt Mar 25, 2023

eerhardt Mar 25, 2023

wjones127 Mar 27, 2023

github-actions bot commented Mar 27, 2023

github-actions bot commented Mar 27, 2023

eerhardt commented Mar 28, 2023

eerhardt commented Mar 30, 2023

wjones127 commented Mar 30, 2023

westonpace left a comment

ursabot commented Apr 4, 2023

GH-34737: [C#] C Data interface for schemas and types #34133

GH-34737: [C#] C Data interface for schemas and types #34133

Conversation

wjones127 commented Feb 10, 2023 • edited by github-actions bot Loading

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

github-actions bot commented Feb 10, 2023

github-actions bot commented Feb 10, 2023

wjones127 commented Feb 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wjones127 commented Feb 28, 2023

westonpace left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eerhardt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Mar 27, 2023

github-actions bot commented Mar 27, 2023

eerhardt commented Mar 28, 2023

eerhardt commented Mar 30, 2023

wjones127 commented Mar 30, 2023

westonpace left a comment

Choose a reason for hiding this comment

ursabot commented Apr 4, 2023

wjones127 commented Feb 10, 2023 •

edited by github-actions bot

Loading