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/05/28 23:15:10 UTC
[GitHub] [arrow] teo-tsirpanis opened a new pull request, #35810: GH-35809: [C#] Improvements to the C interface.
teo-tsirpanis opened a new pull request, #35810:
URL: https://github.com/apache/arrow/pull/35810
### Rationale for this change
This PR fixes issues identified while reading the code of the `Apache.Arrow.C` namespace.
### What changes are included in this PR?
See each commit message for more details.
### Are these changes tested?
Using the existing test suite.
### Are there any user-facing changes?
No.
--
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 #35810: GH-35809: [C#] Improvements to the C Data Interface
Posted by "teo-tsirpanis (via GitHub)" <gi...@apache.org>.
teo-tsirpanis commented on code in PR #35810:
URL: https://github.com/apache/arrow/pull/35810#discussion_r1255008479
##########
csharp/src/Apache.Arrow/C/CArrowArrayStream.cs:
##########
@@ -35,15 +35,23 @@ public unsafe struct CArrowArrayStream
///
/// Return value: 0 if successful, an `errno`-compatible error code otherwise.
///</summary>
- public delegate* unmanaged[Stdcall]<CArrowArrayStream*, CArrowSchema*, int> get_schema;
Review Comment:
They should not; the importer will take care of calling them.
--
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] CurtHagenlocher commented on a diff in pull request #35810: GH-35809: [C#] Improvements to the C interface.
Posted by "CurtHagenlocher (via GitHub)" <gi...@apache.org>.
CurtHagenlocher commented on code in PR #35810:
URL: https://github.com/apache/arrow/pull/35810#discussion_r1210389059
##########
csharp/src/Apache.Arrow/C/CArrowArrayExporter.cs:
##########
@@ -203,6 +207,10 @@ private unsafe static void ReleaseArray(CArrowArray* cArray)
private unsafe static void Dispose(void** ptr)
{
GCHandle gch = GCHandle.FromIntPtr((IntPtr)(*ptr));
+ if (!gch.IsAllocated)
Review Comment:
This is effectively a noop. If the pointer was null, the previous line will throw InvalidOperationException and if it wasn't null then IsAllocated will return true.
The overall change here also means that calling ReleaseArray twice will now throw an exception instead of the second call being a no-op.
##########
csharp/src/Apache.Arrow/C/CArrowArrayStreamExporter.cs:
##########
@@ -140,6 +167,18 @@ sealed unsafe class ExportedArrayStream : IDisposable
return (void*)GCHandle.ToIntPtr(gch);
}
+ public static void Free(void** ptr)
+ {
+ GCHandle gch = GCHandle.FromIntPtr((IntPtr)ptr);
+ if (!gch.IsAllocated)
Review Comment:
(Same comment about IsAllocated.)
##########
csharp/src/Apache.Arrow/C/CArrowArrayExporter.cs:
##########
@@ -184,13 +189,12 @@ private unsafe static void ConvertRecordBatch(ExportedAllocationOwner sharedOwne
cArray->dictionary = null;
}
+#if NET5_0_OR_GREATER
+ [UnmanagedCallersOnly(CallConvs = new[] { typeof(CallConvStdcall) })]
Review Comment:
If there were some reason for the fields to stay public, this one could also probably be defined as a union between a public IntPtr and a private delegate. (I don't know why the fields would need to be public; I'm pretty sure I just followed the pattern that was present for schemas.)
--
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] CurtHagenlocher commented on pull request #35810: GH-35809: [C#] Improvements to the C interface.
Posted by "CurtHagenlocher (via GitHub)" <gi...@apache.org>.
CurtHagenlocher commented on PR #35810:
URL: https://github.com/apache/arrow/pull/35810#issuecomment-1583295707
> It sounds like we might've worked through most of the issues. If I understand it correctly, if someone wants to use 32-bit windows, then they need to use .NET 5 or greater, which seems like a reasonable tradeoff to me.
I'd understood it differently (though perhaps incorrectly). I thought that using the explicit [Stdcall] would work everywhere except 32-bit non-Windows platforms, while removing the [Stdcall] would prevent things from working on .NET < 5.
--
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 merged pull request #35810: GH-35809: [C#] Improvements to the C Data Interface
Posted by "westonpace (via GitHub)" <gi...@apache.org>.
westonpace merged PR #35810:
URL: https://github.com/apache/arrow/pull/35810
--
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] pitrou commented on a diff in pull request #35810: GH-35809: [C#] Improvements to the C interface.
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #35810:
URL: https://github.com/apache/arrow/pull/35810#discussion_r1210659553
##########
csharp/src/Apache.Arrow/C/CArrowArrayExporter.cs:
##########
@@ -184,13 +189,12 @@ private unsafe static void ConvertRecordBatch(ExportedAllocationOwner sharedOwne
cArray->dictionary = null;
}
+#if NET5_0_OR_GREATER
+ [UnmanagedCallersOnly(CallConvs = new[] { typeof(CallConvStdcall) })]
Review Comment:
In any case, keeping the default convention seems more theoretically sound (and forward-looking, perhaps).
--
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 #35810: GH-35809: [C#] Improvements to the C Data Interface
Posted by "eerhardt (via GitHub)" <gi...@apache.org>.
eerhardt commented on code in PR #35810:
URL: https://github.com/apache/arrow/pull/35810#discussion_r1255175664
##########
csharp/src/Apache.Arrow/C/CArrowArrayStream.cs:
##########
@@ -35,15 +35,23 @@ public unsafe struct CArrowArrayStream
///
/// Return value: 0 if successful, an `errno`-compatible error code otherwise.
///</summary>
- public delegate* unmanaged[Stdcall]<CArrowArrayStream*, CArrowSchema*, int> get_schema;
Review Comment:
Another option would be to only publicly expose the function pointer fields on `net5.0+`. Keep them `internal` for netstandard and netfx where they need to be declared `Cdecl` (and honestly function pointers aren't really meant to be used on netfx and netstandard since they came in C# 9 - https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/configure-language-version).
That way users on net5+ can still call the function pointers, if they need to. For netstandard and netfx, they are no worse off - they can't call them with this change anyway.
--
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 #35810: GH-35809: [C#] Improvements to the C interface.
Posted by "westonpace (via GitHub)" <gi...@apache.org>.
westonpace commented on PR #35810:
URL: https://github.com/apache/arrow/pull/35810#issuecomment-1584531095
> I'd understood it differently (though perhaps incorrectly). I thought that using the explicit [Stdcall] would work everywhere except 32-bit non-Windows platforms, while removing the [Stdcall] would prevent things from working on .NET < 5.
Yes, looking at the code now I think you're right. I interpreted the above conversation as:
* [This](https://github.com/apache/arrow/pull/35810#discussion_r1210668239) would be safe but lead to an unstable API surface
* If we make these methods/fields private we don't have to worry about the API surface
However, looking at the code, I don't see that change. Are we still waiting on that change? Or is there some reason this wouldn't work?
--
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 #35810: GH-35809: [C#] Improvements to the C interface.
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #35810:
URL: https://github.com/apache/arrow/pull/35810#issuecomment-1566298476
:warning: GitHub issue #35809 **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] pitrou commented on a diff in pull request #35810: GH-35809: [C#] Improvements to the C interface.
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #35810:
URL: https://github.com/apache/arrow/pull/35810#discussion_r1210678633
##########
csharp/src/Apache.Arrow/C/CArrowArrayExporter.cs:
##########
@@ -184,13 +189,12 @@ private unsafe static void ConvertRecordBatch(ExportedAllocationOwner sharedOwne
cArray->dictionary = null;
}
+#if NET5_0_OR_GREATER
+ [UnmanagedCallersOnly(CallConvs = new[] { typeof(CallConvStdcall) })]
Review Comment:
If you take a look for example at the Arrow C++ implementation, its release functions are entirely private. For example `ReleaseExportedSchema` below is in the anonymous namespace, which doesn't expose the function publicly:
https://github.com/apache/arrow/blob/e628ca50078c749ea7a10b53defe12cbec7d581f/cpp/src/arrow/c/bridge.cc#L107
--
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 #35810: GH-35809: [C#] Improvements to the C Data Interface
Posted by "eerhardt (via GitHub)" <gi...@apache.org>.
eerhardt commented on code in PR #35810:
URL: https://github.com/apache/arrow/pull/35810#discussion_r1255007350
##########
csharp/src/Apache.Arrow/C/CArrowArrayStream.cs:
##########
@@ -35,15 +35,23 @@ public unsafe struct CArrowArrayStream
///
/// Return value: 0 if successful, an `errno`-compatible error code otherwise.
///</summary>
- public delegate* unmanaged[Stdcall]<CArrowArrayStream*, CArrowSchema*, int> get_schema;
Review Comment:
If we make these function pointers `internal`, how are users of these APIs supposed to call them?
--
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] pitrou commented on pull request #35810: GH-35809: [C#] Improvements to the C Data Interface
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #35810:
URL: https://github.com/apache/arrow/pull/35810#issuecomment-1623913721
(and, yes, it would only make a difference on 32-bit x86 machines, and even then, perhaps in some cases the two are equivalent)
--
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] pitrou commented on pull request #35810: GH-35809: [C#] Improvements to the C Data Interface
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #35810:
URL: https://github.com/apache/arrow/pull/35810#issuecomment-1623926036
Feel free to merge :-)
--
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 #35810: GH-35809: [C#] Improvements to the C interface.
Posted by "teo-tsirpanis (via GitHub)" <gi...@apache.org>.
teo-tsirpanis commented on code in PR #35810:
URL: https://github.com/apache/arrow/pull/35810#discussion_r1210668239
##########
csharp/src/Apache.Arrow/C/CArrowArrayExporter.cs:
##########
@@ -184,13 +189,12 @@ private unsafe static void ConvertRecordBatch(ExportedAllocationOwner sharedOwne
cArray->dictionary = null;
}
+#if NET5_0_OR_GREATER
+ [UnmanagedCallersOnly(CallConvs = new[] { typeof(CallConvStdcall) })]
Review Comment:
If I understand correctly, are you saying to change this: https://github.com/apache/arrow/blob/e628ca50078c749ea7a10b53defe12cbec7d581f/csharp/src/Apache.Arrow/C/CArrowArray.cs#L41
into this?
```csharp
#if NET5_0_OR_GREATER
public delegate* unmanaged<CArrowArray*, void> release;
#else
public delegate* unmanaged[Stdcall]<CArrowArray*, void> release;
#endif
```
That would cause an incompatible API surface between the assembly compiled for .NET 6 and that compiled for the earlier frameworks. We have two options:
* Lie and keep the `stdcall` calling convention on the function pointers.
* Use the default unmanaged calling convention but support the C interface only on .NET 6+ (we don't target 5 as it is unsupported).
--
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 #35810: GH-35809: [C#] Improvements to the C Data Interface
Posted by "eerhardt (via GitHub)" <gi...@apache.org>.
eerhardt commented on code in PR #35810:
URL: https://github.com/apache/arrow/pull/35810#discussion_r1255013278
##########
csharp/src/Apache.Arrow/C/CArrowArrayStream.cs:
##########
@@ -35,15 +35,23 @@ public unsafe struct CArrowArrayStream
///
/// Return value: 0 if successful, an `errno`-compatible error code otherwise.
///</summary>
- public delegate* unmanaged[Stdcall]<CArrowArrayStream*, CArrowSchema*, int> get_schema;
Review Comment:
I don't fully follow. If I wanted to take some C arrow data (let's say coming from C++) and natively work with it - not using the "normal" C# ArrowTypes, Schema, RecordBatches, etc. but instead manually calling into the native functions on the CArrowArrayStream, that is no longer possible? Why not?
The reason for interoping at the native layer would be for performance - I wouldn't need to allocate a bunch of managed objects just to interact with the Arrow information coming from some other library.
--
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 #35810: GH-35809: [C#] Improvements to the C Data Interface
Posted by "teo-tsirpanis (via GitHub)" <gi...@apache.org>.
teo-tsirpanis commented on code in PR #35810:
URL: https://github.com/apache/arrow/pull/35810#discussion_r1255014227
##########
csharp/src/Apache.Arrow/C/CArrowArrayStream.cs:
##########
@@ -35,15 +35,23 @@ public unsafe struct CArrowArrayStream
///
/// Return value: 0 if successful, an `errno`-compatible error code otherwise.
///</summary>
- public delegate* unmanaged[Stdcall]<CArrowArrayStream*, CArrowSchema*, int> get_schema;
Review Comment:
See https://github.com/apache/arrow/pull/35810#issuecomment-1584531095 for an explanation on why the function pointer fields are internal.
--
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] conbench-apache-arrow[bot] commented on pull request #35810: GH-35809: [C#] Improvements to the C Data Interface
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #35810:
URL: https://github.com/apache/arrow/pull/35810#issuecomment-1633810879
Conbench analyzed the 6 benchmark runs on commit `88cb5170`.
There were 2 benchmark results indicating a performance regression:
- Commit Run on `ursa-thinkcentre-m75q` at [2023-07-10 08:20:40Z](http://conbench.ursa.dev/compare/runs/9a0b37798e1e4880a32552ec7e3973da...438eab56ace443febcd2f8882004c20e/)
- [params=4096, source=cpp-micro, suite=parquet-encoding-benchmark](http://conbench.ursa.dev/compare/benchmarks/064ab9959c9873a68000d9589c01ab2d...064abbfc758874148000966b590783dd)
- Commit Run on `ursa-i9-9960x` at [2023-07-13 05:36:26Z](http://conbench.ursa.dev/compare/runs/42097f0a73454281a9336e8e1b90e778...b29d36f72e6e42baa1722f9139a9ed21/)
- [engine=arrow, format=native, language=R, memory_map=False, query_id=TPCH-01, scale_factor=1](http://conbench.ursa.dev/compare/benchmarks/064af77c279270c180003042295feb52...064afa58ac5a720a800037f3f7d5adf6)
The [full Conbench report](https://github.com/apache/arrow/runs/15006594462) has more details.
--
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] pitrou commented on a diff in pull request #35810: GH-35809: [C#] Improvements to the C interface.
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #35810:
URL: https://github.com/apache/arrow/pull/35810#discussion_r1210588958
##########
csharp/src/Apache.Arrow/C/CArrowArrayExporter.cs:
##########
@@ -15,16 +15,21 @@
using System;
+using System.Runtime.CompilerServices;
using System.Runtime.InteropServices;
using Apache.Arrow.Memory;
namespace Apache.Arrow.C
{
public static class CArrowArrayExporter
{
+#if NET5_0_OR_GREATER
Review Comment:
Perhaps add a comment explaining why this needs .Net 5.0+? Or will it be obvious to a C# developer?
--
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] pitrou commented on a diff in pull request #35810: GH-35809: [C#] Improvements to the C interface.
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #35810:
URL: https://github.com/apache/arrow/pull/35810#discussion_r1210675397
##########
csharp/src/Apache.Arrow/C/CArrowArrayExporter.cs:
##########
@@ -184,13 +189,12 @@ private unsafe static void ConvertRecordBatch(ExportedAllocationOwner sharedOwne
cArray->dictionary = null;
}
+#if NET5_0_OR_GREATER
+ [UnmanagedCallersOnly(CallConvs = new[] { typeof(CallConvStdcall) })]
Review Comment:
Well, why is this `release` member public here? It will be exposed to C Data Interface consumers as the `release` pointer, but needn't (and probably shouldn't) be part of the Arrow C# API.
Arrow C# API users should only see the high-level import and export methods such as `ImportArray` and `ExportArray`.
(an important thing to understand is that the C Data Interface is a _binary_ interface, not an API)
--
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] pitrou commented on a diff in pull request #35810: GH-35809: [C#] Improvements to the C interface.
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #35810:
URL: https://github.com/apache/arrow/pull/35810#discussion_r1210661570
##########
csharp/src/Apache.Arrow/C/CArrowArrayExporter.cs:
##########
@@ -184,13 +189,12 @@ private unsafe static void ConvertRecordBatch(ExportedAllocationOwner sharedOwne
cArray->dictionary = null;
}
+#if NET5_0_OR_GREATER
+ [UnmanagedCallersOnly(CallConvs = new[] { typeof(CallConvStdcall) })]
Review Comment:
cc @westonpace @lidavidm for opinions.
--
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 #35810: GH-35809: [C#] Improvements to the C Data Interface
Posted by "westonpace (via GitHub)" <gi...@apache.org>.
westonpace commented on PR #35810:
URL: https://github.com/apache/arrow/pull/35810#issuecomment-1624021555
Sorry, didn't see your comment on #36506 but it looks like you got the change in just in time. Fortunately, it seems CI is passing :) I'll keep monitoring it.
--
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 #35810: GH-35809: [C#] Improvements to the C Data Interface
Posted by "westonpace (via GitHub)" <gi...@apache.org>.
westonpace commented on PR #35810:
URL: https://github.com/apache/arrow/pull/35810#issuecomment-1624025339
All tests passed, thanks for your attention and the cleanup @teo-tsirpanis
--
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] pitrou commented on a diff in pull request #35810: GH-35809: [C#] Improvements to the C interface.
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #35810:
URL: https://github.com/apache/arrow/pull/35810#discussion_r1210657569
##########
csharp/src/Apache.Arrow/C/CArrowArrayExporter.cs:
##########
@@ -184,13 +189,12 @@ private unsafe static void ConvertRecordBatch(ExportedAllocationOwner sharedOwne
cArray->dictionary = null;
}
+#if NET5_0_OR_GREATER
+ [UnmanagedCallersOnly(CallConvs = new[] { typeof(CallConvStdcall) })]
Review Comment:
Well, the entire point (mostly) of implementing the C Data Interface is to be compatible with non-.Net producers/consumers. Those are extremely likely to use the platform default. So we should get it right at least when possible, i.e. on .Net >= 5.0.
As for https://stackoverflow.com/questions/34832679/is-the-callingconvention-ignored-in-64-bit-net-applications , does it apply here? It's talking about `DllImport`, which might be different from this?
--
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 #35810: GH-35809: [C#] Improvements to the C interface.
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #35810:
URL: https://github.com/apache/arrow/pull/35810#issuecomment-1566298467
* Closes: #35809
--
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] pitrou commented on a diff in pull request #35810: GH-35809: [C#] Improvements to the C interface.
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #35810:
URL: https://github.com/apache/arrow/pull/35810#discussion_r1210601111
##########
csharp/src/Apache.Arrow/C/CArrowArrayExporter.cs:
##########
@@ -184,13 +189,12 @@ private unsafe static void ConvertRecordBatch(ExportedAllocationOwner sharedOwne
cArray->dictionary = null;
}
+#if NET5_0_OR_GREATER
+ [UnmanagedCallersOnly(CallConvs = new[] { typeof(CallConvStdcall) })]
Review Comment:
Shouldn't the default calling convention be used instead? I'm not sure `stdcall` is ok on non-Windows platforms.
--
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 #35810: GH-35809: [C#] Improvements to the C interface.
Posted by "teo-tsirpanis (via GitHub)" <gi...@apache.org>.
teo-tsirpanis commented on code in PR #35810:
URL: https://github.com/apache/arrow/pull/35810#discussion_r1210593264
##########
csharp/src/Apache.Arrow/C/CArrowArrayExporter.cs:
##########
@@ -15,16 +15,21 @@
using System;
+using System.Runtime.CompilerServices;
using System.Runtime.InteropServices;
using Apache.Arrow.Memory;
namespace Apache.Arrow.C
{
public static class CArrowArrayExporter
{
+#if NET5_0_OR_GREATER
Review Comment:
`UnmanagedCallersOnlyAttribute` was introduced in .NET 5.
--
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 #35810: GH-35809: [C#] Improvements to the C interface.
Posted by "westonpace (via GitHub)" <gi...@apache.org>.
westonpace commented on PR #35810:
URL: https://github.com/apache/arrow/pull/35810#issuecomment-1599221566
I thought the point of making them `internal` was so that we could use something like this:
```
#if NET5_0_OR_GREATER
public delegate* unmanaged<CArrowArray*, void> release;
#else
public delegate* unmanaged[Stdcall]<CArrowArray*, void> release;
#endif
```
However, I don't see that. Also, looks like we will need a rebase.
--
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 #35810: GH-35809: [C#] Improvements to the C interface.
Posted by "teo-tsirpanis (via GitHub)" <gi...@apache.org>.
teo-tsirpanis commented on PR #35810:
URL: https://github.com/apache/arrow/pull/35810#issuecomment-1601793434
Done @westonpace.
--
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] kou commented on pull request #35810: GH-35809: [C#] Improvements to the C interface.
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on PR #35810:
URL: https://github.com/apache/arrow/pull/35810#issuecomment-1566304049
> See each commit message for more details.
Could you write it in the pull request description because we use squash merge and the merge commit only uses the pull request description.
It seems that this pull request has several logical changes. It may be better that you split this pull request and associated issue to multiple small pull requests/issues for easy to review.
--
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 #35810: GH-35809: [C#] Improvements to the C interface.
Posted by "teo-tsirpanis (via GitHub)" <gi...@apache.org>.
teo-tsirpanis commented on code in PR #35810:
URL: https://github.com/apache/arrow/pull/35810#discussion_r1210653495
##########
csharp/src/Apache.Arrow/C/CArrowArrayExporter.cs:
##########
@@ -184,13 +189,12 @@ private unsafe static void ConvertRecordBatch(ExportedAllocationOwner sharedOwne
cArray->dictionary = null;
}
+#if NET5_0_OR_GREATER
+ [UnmanagedCallersOnly(CallConvs = new[] { typeof(CallConvStdcall) })]
Review Comment:
See https://github.com/apache/arrow/pull/34133#discussion_r1122532439. Ideally we would have used the default calling convention, but that would not be suppported on anything earlier than .NET 5. And in 64-bit platforms the calling convention doesn't matter either way.
--
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 #35810: GH-35809: [C#] Improvements to the C interface.
Posted by "teo-tsirpanis (via GitHub)" <gi...@apache.org>.
teo-tsirpanis commented on code in PR #35810:
URL: https://github.com/apache/arrow/pull/35810#discussion_r1210684256
##########
csharp/src/Apache.Arrow/C/CArrowArrayExporter.cs:
##########
@@ -184,13 +189,12 @@ private unsafe static void ConvertRecordBatch(ExportedAllocationOwner sharedOwne
cArray->dictionary = null;
}
+#if NET5_0_OR_GREATER
+ [UnmanagedCallersOnly(CallConvs = new[] { typeof(CallConvStdcall) })]
Review Comment:
So I should make the members of these structs private? That also works.
--
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] pitrou commented on a diff in pull request #35810: GH-35809: [C#] Improvements to the C interface.
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #35810:
URL: https://github.com/apache/arrow/pull/35810#discussion_r1210591496
##########
csharp/src/Apache.Arrow/C/CArrowArrayExporter.cs:
##########
@@ -203,6 +207,10 @@ private unsafe static void ReleaseArray(CArrowArray* cArray)
private unsafe static void Dispose(void** ptr)
{
GCHandle gch = GCHandle.FromIntPtr((IntPtr)(*ptr));
+ if (!gch.IsAllocated)
Review Comment:
Add a comment explaining when this might occur? (perhaps if an exception occurs while exporting the array?)
--
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 #35810: GH-35809: [C#] Improvements to the C interface.
Posted by "teo-tsirpanis (via GitHub)" <gi...@apache.org>.
teo-tsirpanis commented on PR #35810:
URL: https://github.com/apache/arrow/pull/35810#issuecomment-1585716053
I made them internal.
--
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 #35810: GH-35809: [C#] Improvements to the C interface.
Posted by "teo-tsirpanis (via GitHub)" <gi...@apache.org>.
teo-tsirpanis commented on PR #35810:
URL: https://github.com/apache/arrow/pull/35810#issuecomment-1584533431
I will make the fields `internal` soon; I'm pretty busy these days.
--
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] pitrou commented on pull request #35810: GH-35809: [C#] Improvements to the C Data Interface
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #35810:
URL: https://github.com/apache/arrow/pull/35810#issuecomment-1623913169
I will say again that I think the callbacks should be `Cdecl`, not `Stdcall`. `Stdcall` is the calling convention for WinAPI functions and nothing else, AFAICT. `Cdecl` is the default calling convention for any other C/C++ code, such as the callbacks that producers will pass in the `release` member.
See: https://www.codeproject.com/Articles/1388/Calling-Conventions-Demystified
--
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] pitrou commented on pull request #35810: GH-35809: [C#] Improvements to the C interface.
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #35810:
URL: https://github.com/apache/arrow/pull/35810#issuecomment-1621766437
@teo-tsirpanis @westonpace Is there anything left to do here? It would be nice to have this in 13.0.0.
--
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