You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "adamreeve (via GitHub)" <gi...@apache.org> on 2024/04/04 04:12:15 UTC
[PR] GH-36057: [C#] Allow C Data Interface export of ReadOnlyMemory backed buffers [arrow]
adamreeve opened a new pull request, #40992:
URL: https://github.com/apache/arrow/pull/40992
### Rationale for this change
This PR partly addresses #36057, by allowing C Data Interface export of buffers backed by `ReadOnlyMemory` rather than an `IMemoryOwner`. This is useful if you want to export data that has been read from an IPC file or stream for example. Previously you would need to first copy all buffers for this to work.
### What changes are included in this PR?
Updates the `ExportedAllocationOwner` class to allow it to hold pinned `MemoryHandle`s.
### Are these changes tested?
Yes, I've added new unit tests.
### Are there any user-facing changes?
Yes, this is a user-facing change, as export of arrays backed by `ReadOnlyMemory` would previously throw an exception but will now succeed.
### Safety
I can't see a way to do this completely safely, as there's always the possibility that a buffer is backed by a `MemoryManager` that frees or modifies memory after it is exported but before the exported data is disposed and the memory is unpinned.
Users are able to create buffers from arbitrary `ReadOnlyMemory` so there's no limit to how the underlying memory might be managed, but the only situation I'm aware of where `ReadOnlyMemory` is used for buffers created within the C# Arrow library itself is when reading IPC data. This might be read from an arbitrary `ReadOnlyMemory` object, but when reading uncompressed buffers from a `Stream`, the buffer data ends up being backed by slices of memory allocated using a `NativeMemoryAllocator`, and the `RecordBatch` holds a reference to the `IMemoryOwner` that owns this memory so it can all be freed at once.
In order to protect from users disposing a `RecordBatch` that is backed by native memory while it is still in use after export, I've made a change to throw an exception if a `NativeMemoryManager` is disposed while its memory is still pinned.
I'm not sure whether this is really safe enough to merge as is though, given the possibility of buffers using other `MemoryManager` implementations that don't have similar checks. Maybe it would make sense to require using separate export methods to allow export of `ReadOnlyMemory` buffers, eg. `CArrowArrayExporter.ExportArrayUnsafe`? By using this method instead of the normal `ExportArray` method, the user would be aware that they're responsible for verifying that the backing memory won't be disposed before the export client is done with the data.
I'm creating this as a draft PR initially as I think this safety aspect needs some discussion.
--
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
Re: [PR] GH-36057: [C#] Allow C Data Interface export of ReadOnlyMemory backed buffers [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #40992:
URL: https://github.com/apache/arrow/pull/40992#issuecomment-2036151438
:warning: GitHub issue #36057 **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
Re: [PR] GH-36057: [C#] Allow C Data Interface export of ReadOnlyMemory backed buffers [arrow]
Posted by "CurtHagenlocher (via GitHub)" <gi...@apache.org>.
CurtHagenlocher commented on PR #40992:
URL: https://github.com/apache/arrow/pull/40992#issuecomment-2073894296
I'll start thinking about this again shortly as it's blocking reasonable use of the C# Spark ADBC driver from Python.
--
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
Re: [PR] GH-36057: [C#] Allow C Data Interface export of ReadOnlyMemory backed buffers [arrow]
Posted by "adamreeve (via GitHub)" <gi...@apache.org>.
adamreeve commented on PR #40992:
URL: https://github.com/apache/arrow/pull/40992#issuecomment-2036157474
cc @CurtHagenlocher, what are your thoughts on this?
This doesn't address the other problem mentioned in #36057 of being able to continue using an array after export, that might be a separate issue and require reference counted arrays. Although maybe if the `ExportArrayUnsafe` approach is used, that method could also relax this restriction?
The other limitation I can see that would be nice to fix is exporting data that was previously imported.
--
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
Re: [PR] GH-36057: [C#] Allow C Data Interface export of ReadOnlyMemory backed buffers [arrow]
Posted by "CurtHagenlocher (via GitHub)" <gi...@apache.org>.
CurtHagenlocher commented on PR #40992:
URL: https://github.com/apache/arrow/pull/40992#issuecomment-3373355825
Superceded by [https://github.com/apache/arrow-dotnet/pull/112](https://github.com/apache/arrow-dotnet/pull/112).
--
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
Re: [PR] GH-36057: [C#] Allow C Data Interface export of ReadOnlyMemory backed buffers [arrow]
Posted by "CurtHagenlocher (via GitHub)" <gi...@apache.org>.
CurtHagenlocher closed pull request #40992: GH-36057: [C#] Allow C Data Interface export of ReadOnlyMemory backed buffers
URL: https://github.com/apache/arrow/pull/40992
--
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