refactor: Improve syntax for implementing ArrowArrayStream
from C++
#336
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
An early commit implemented
nanoarrow::EmptyArrayStream
andnanoarrow::VectorArrayStream
in the nanoarrow.hpp C++ helpers. The intention at the time was to make it "easy"/idiomatic to use a C++ object to back anArrowArrayStream
; however, it was in practice difficult to actually make work (I tried briefly and gave up when writing a dummy ADBC driver for this blog post.This PR deprecates the original syntax and migrates it to the following.
Implementation:
Usage:
I'm open to suggestions on how to make that better! It might be that the
ToArrayStream()
bit is confusing (i.e., just useArrayStreamFactory<>::InitArrayStream(new StreamImpl())
directory) but it also seemed better to keep the lines where a raw pointer was floating around to be entirely contained within theStreamImpl
class.It also fixes an issue with the
XXX_pointer()
functions, which because of the way they were declared, required thatnanoarrow.hpp
be confusingly included afternanoarrow_ipc.hpp
. The correct way to do this (I think) was to declare the template and add implementations (rather than use overloads).