[GitHub] [arrow-adbc] paleolimbot commented on a diff in pull request #702: feat(python/adbc_driver_manager): experiment with using PyCapsules

["paleolimbot (via GitHub)" <gi...@apache.org>]

paleolimbot commented on code in PR #702:
URL: https://github.com/apache/arrow-adbc/pull/702#discussion_r1227065879


##########
python/adbc_driver_manager/adbc_driver_manager/_lib.pyx:
##########
@@ -1132,3 +1180,85 @@ cdef class AdbcStatement(_AdbcHandle):
             status = AdbcStatementSetSubstraitPlan(
                 &self.statement, c_plan, length, &c_error)
         check_error(status, &c_error)
+
+
+# Implementation of an ArrowArrayStream that keeps a dependent object valid
+
+
+cdef struct ArrowArrayStreamWrapper:
+    cpython.PyObject* parent_statement
+    CArrowArrayStream* parent_array_stream

Review Comment:
   I think you may want this struct to own the memory here rather than just a pointer (i.e., `CArrowArrayStream parent_array_stream`).



##########
python/adbc_driver_manager/adbc_driver_manager/_lib.pyx:
##########
@@ -1132,3 +1180,85 @@ cdef class AdbcStatement(_AdbcHandle):
             status = AdbcStatementSetSubstraitPlan(
                 &self.statement, c_plan, length, &c_error)
         check_error(status, &c_error)
+
+
+# Implementation of an ArrowArrayStream that keeps a dependent object valid
+
+
+cdef struct ArrowArrayStreamWrapper:
+    cpython.PyObject* parent_statement
+    CArrowArrayStream* parent_array_stream
+    bint error_set
+
+
+cdef void wrapper_array_stream_release(CArrowArrayStream* array_stream) nogil noexcept:
+    cdef ArrowArrayStreamWrapper* data
+
+    if array_stream.private_data != NULL:
+        data = <ArrowArrayStreamWrapper*>array_stream.private_data
+        data.parent_array_stream.release(data.parent_array_stream)
+
+        with gil:
+            cpython.Py_DECREF(<AdbcStatement>data.parent_statement)
+
+        free(array_stream.private_data)
+
+    array_stream.release = NULL
+
+
+cdef const char* wrapper_array_stream_get_last_error(CArrowArrayStream* array_stream) nogil noexcept:
+    cdef ArrowArrayStreamWrapper* data = <ArrowArrayStreamWrapper*>array_stream.private_data
+    if data.error_set:
+        return "AdbcStatement already closed"
+    return data.parent_array_stream.get_last_error(data.parent_array_stream)
+
+
+cdef int wrapper_array_stream_get_schema(CArrowArrayStream* array_stream, CArrowSchema* out) nogil noexcept:
+    cdef ArrowArrayStreamWrapper* data = <ArrowArrayStreamWrapper*>array_stream.private_data
+    if (<AdbcStatement>data.parent_statement).closed:
+        data.error_set = True
+        return EIO
+    return data.parent_array_stream.get_schema(data.parent_array_stream, out)
+
+
+cdef int wrapper_array_stream_get_next(CArrowArrayStream* array_stream, CArrowArray* out) nogil noexcept:
+    cdef ArrowArrayStreamWrapper* data = <ArrowArrayStreamWrapper*>(array_stream.private_data)
+    if (<AdbcStatement>data.parent_statement).closed:
+        data.error_set = True

Review Comment:
   I am not sure that you need `error_set`: in theory, the array stream that you are wrapping should be performing that check before doing something that might crash (although I get that right now it might not be)?



-- 
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