paleolimbot commented on code in PR #509:
URL: https://github.com/apache/arrow-nanoarrow/pull/509#discussion_r1651011204
##########
src/nanoarrow/nanoarrow_device.c:
##########
@@ -175,10 +175,12 @@ ArrowErrorCode ArrowDeviceArrayInitAsync(struct
ArrowDevice* device,
return device->array_init(device, device_array, array, sync_event, stream);
}
- // Handling a sync event or stream is not supported in the default
constructor
- if (sync_event != NULL || stream != NULL) {
- return EINVAL;
- }
+ // TODO: Right now we have to let these events through and ignore them
+ // because we don't have a good way to separate the source and destination
+ // streams.
+ // if (sync_event != NULL || stream != NULL) {
+ // return EINVAL;
+ // }
Review Comment:
I think we're all on the same page! The `stream` argument in
`ArrowDeviceArrayInit()` is "the stream that any operations *were* put on",
which is handy for the likely case where a consumer just built a bunch of
buffers using a stream, and is ready to hand a `ArrowDeviceArray` off to a
consumer.
There is plenty of flexibility to refactor/rename/document exactly how/where
this happens (e.g., perhaps the `ArrowDevice` abstraction is just not needed
since the idioms for Metal mostly don't apply here). For now, I needed to
implement the `stream` -> `event` logic somewhere and was a not unreasonable
place to put 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
