pitrou commented on code in PR #39455:
URL: https://github.com/apache/arrow/pull/39455#discussion_r1446406542
##########
cpp/src/arrow/c/bridge.cc:
##########
@@ -1979,13 +1979,61 @@ Result<std::shared_ptr<RecordBatch>>
ImportDeviceRecordBatch(
namespace {
+template <typename T>
+static inline Status ExportStreamSchema(const std::shared_ptr<T>& src,
+ struct ArrowSchema* out_schema);
+
+template <>
+inline Status ExportStreamSchema(const std::shared_ptr<RecordBatchReader>& src,
+ struct ArrowSchema* out_schema) {
+ return ExportSchema(*src->schema(), out_schema);
+}
+
+template <>
+inline Status ExportStreamSchema(const std::shared_ptr<ChunkedArray>& src,
+ struct ArrowSchema* out_schema) {
+ return ExportType(*src->type(), out_schema);
+}
+
+template <typename T>
Review Comment:
Same here.
##########
cpp/src/arrow/c/bridge.cc:
##########
@@ -2152,6 +2211,27 @@ class ArrayStreamBatchReader : public RecordBatchReader {
}
private:
+ template <typename BatchT>
Review Comment:
Stuffing everything in one class makes for complicated code. Why not have a
base class with common helpers and then two distinct derived classes?
##########
cpp/src/arrow/c/bridge_test.cc:
##########
@@ -4506,6 +4554,29 @@ class TestArrayStreamRoundtrip : public
BaseArrayStreamTest {
ASSERT_TRUE(weak_reader.expired());
}
+ void Roundtrip(std::shared_ptr<ChunkedArray> src,
+ std::function<void(const std::shared_ptr<ChunkedArray>&)>
check_func) {
+ ArrowArrayStream c_stream;
+
+ // One original copy which to compare the result, one copy held by the
stream
+ std::weak_ptr<ChunkedArray> weak_src(src);
+ ASSERT_EQ(weak_src.use_count(), 2);
+
+ ASSERT_OK(ExportChunkedArray(std::move(src), &c_stream));
+ ASSERT_FALSE(ArrowArrayStreamIsReleased(&c_stream));
+
+ {
+ ASSERT_OK_AND_ASSIGN(auto dst, ImportChunkedArray(&c_stream));
+ // Stream was moved, consumed, and released
+ ASSERT_TRUE(ArrowArrayStreamIsReleased(&c_stream));
+
+ // Stream was released by ImportChunkedArray but original copy remains
+ ASSERT_EQ(weak_src.use_count(), 1);
Review Comment:
I'm not sure I understand what the goal of this test is. In any case,
`MemoryPool::bytes_allocated` would probably be more reliable (the data could
be held alive in another way than through ChunkedArray).
##########
cpp/src/arrow/c/bridge.cc:
##########
@@ -2047,6 +2087,15 @@ class ExportedArrayStream {
return ExportedArrayStream{stream}.GetLastError();
}
+ static Status Make(const std::shared_ptr<T> reader, struct ArrowArrayStream*
out) {
Review Comment:
```suggestion
static Status Make(std::shared_ptr<T> reader, struct ArrowArrayStream*
out) {
```
##########
cpp/src/arrow/c/bridge_test.cc:
##########
@@ -4506,6 +4554,29 @@ class TestArrayStreamRoundtrip : public
BaseArrayStreamTest {
ASSERT_TRUE(weak_reader.expired());
}
+ void Roundtrip(std::shared_ptr<ChunkedArray> src,
+ std::function<void(const std::shared_ptr<ChunkedArray>&)>
check_func) {
+ ArrowArrayStream c_stream;
+
+ // One original copy which to compare the result, one copy held by the
stream
+ std::weak_ptr<ChunkedArray> weak_src(src);
+ ASSERT_EQ(weak_src.use_count(), 2);
Review Comment:
Instead of hardcoding the expected ref count, why not just remember it and
then compare it later?
--
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]
