Skip to content

Commit

Permalink
Try to avoid type-unsafe void*
Browse files Browse the repository at this point in the history
  • Loading branch information
pitrou committed Aug 23, 2023
1 parent 8571b9f commit 4154e4f
Show file tree
Hide file tree
Showing 4 changed files with 39 additions and 19 deletions.
6 changes: 3 additions & 3 deletions python/pyarrow/_flight.pyx
Original file line number Diff line number Diff line change
Expand Up @@ -838,9 +838,9 @@ cdef class FlightInfo(_Weakrefable):
unique_ptr[CFlightInfo] info

@staticmethod
cdef _wrap_unsafe(void* c_info):
cdef wrap(CFlightInfo c_info):
cdef FlightInfo obj = FlightInfo.__new__(FlightInfo)
obj.info.reset(new CFlightInfo(move(deref(<CFlightInfo*> c_info))))
obj.info.reset(new CFlightInfo(move(c_info)))
return obj

def __init__(self, Schema schema, FlightDescriptor descriptor, endpoints,
Expand Down Expand Up @@ -1292,7 +1292,7 @@ cdef class AsyncioFlightClient:
c_future = self._client.client.get().GetFlightInfoAsync(
deref(c_options), c_descriptor)

BindFuture(move(c_future), call.wakeup, FlightInfo._wrap_unsafe)
BindFuture(move(c_future), call.wakeup, FlightInfo.wrap)


cdef class FlightClient(_Weakrefable):
Expand Down
10 changes: 6 additions & 4 deletions python/pyarrow/includes/common.pxd
Original file line number Diff line number Diff line change
Expand Up @@ -154,11 +154,13 @@ cdef extern from "arrow/util/future.h" namespace "arrow" nogil:
CFuture()


ctypedef object PyWrapper(void*)


cdef extern from "arrow/python/async.h" namespace "arrow::py" nogil:
void BindFuture[T](CFuture[T], object cb, PyWrapper wrapper)
# BindFuture's third argument is really a C++ callable with
# the signature `object(T*)`, but Cython does not allow declaring that.
# We use an ellipsis as a workaround.
# Another possibility is to type-erase the argument by making it
# `object(void*)`, but it would lose compile-time C++ type safety.
void BindFuture[T](CFuture[T], object cb, ...)


cdef extern from "arrow/python/common.h" namespace "arrow::py" nogil:
Expand Down
16 changes: 13 additions & 3 deletions python/pyarrow/src/arrow/python/async.h
Original file line number Diff line number Diff line change
Expand Up @@ -24,20 +24,29 @@
#include "arrow/util/future.h"

namespace arrow::py {

/// \brief Bind a Python callback to an arrow::Future.
///
/// If the Future finishes successfully, py_wrapper is called with its
/// result value and should return a PyObject*. If py_wrapper is successful,
/// py_cb is called with its return value.
///
/// If either the Future or py_wrapper fails, py_cb is called with the
/// associated Python exception.
///
/// \param future The future to bind to.
/// \param py_cb The Python callback function. Will be passed the result of
/// py_wrapper, or a Python exception if the future failed or one was
/// raised by py_wrapper.
/// \param py_wrapper A function (likely defined in Cython) to convert the C++
/// result of the future to a Python object.
template <typename T, typename Wrapper = PyObject* (*)(void*)>
void BindFuture(Future<T> future, PyObject* py_cb, Wrapper py_wrapper) {
template <typename T, typename PyWrapper = PyObject* (*)(T)>
void BindFuture(Future<T> future, PyObject* py_cb, PyWrapper py_wrapper) {
Py_INCREF(py_cb);
OwnedRefNoGIL cb_ref(py_cb);

auto future_cb = [cb_ref = std::move(cb_ref), py_wrapper](Result<T> result) {
auto future_cb = [cb_ref = std::move(cb_ref),
py_wrapper = std::move(py_wrapper)](Result<T> result) {
SafeCallIntoPythonVoid([&]() {
OwnedRef py_value_or_exc{WrapResult(std::move(result), std::move(py_wrapper))};
Py_XDECREF(
Expand All @@ -47,4 +56,5 @@ void BindFuture(Future<T> future, PyObject* py_cb, Wrapper py_wrapper) {
};
future.AddCallback(std::move(future_cb));
}

} // namespace arrow::py
26 changes: 17 additions & 9 deletions python/pyarrow/src/arrow/python/common.h
Original file line number Diff line number Diff line change
Expand Up @@ -72,19 +72,27 @@ T GetResultValue(Result<T> result) {
}
}

// Wrap a Result<T> and return the corresponding Python object.
// * If the Result<T> is successful, PyWrapper(&value) is called,
// which should return a PyObject*.
// * If the Result<T> is an error, the corresponding Python exception
// is returned.
template <typename T, typename PyWrapper = PyObject* (*)(void*)>
PyObject* WrapResult(Result<T> result, PyWrapper py_wrapper) {
static_assert(std::is_same_v<PyObject*, decltype(py_wrapper(std::declval<T*>()))>,
/// \brief Wrap a Result and return the corresponding Python object.
///
/// If the Result is successful, py_wrapper is called with its result value
/// and should return a PyObject*. If py_wrapper is successful (returns
/// a non-NULL value), its return value is returned.
///
/// If either the Result or py_wrapper fails, the associated Python exception
/// is raised and NULL is returned.
//
/// \param result The Result whose value to wrap in a Python object.
/// \param py_wrapper A function (likely defined in Cython) to convert the C++
/// value of the Result to a Python object.
/// \return A new Python reference, or NULL if an exception occurred
template <typename T, typename PyWrapper = PyObject* (*)(T)>
PyObject* WrapResult(Result<T> result, PyWrapper&& py_wrapper) {
static_assert(std::is_same_v<PyObject*, decltype(py_wrapper(std::declval<T>()))>,
"PyWrapper argument to WrapResult should return a PyObject* "
"when called with a T*");
Status st = result.status();
if (st.ok()) {
PyObject* py_value = py_wrapper(&result.ValueUnsafe());
PyObject* py_value = py_wrapper(result.MoveValueUnsafe());
st = CheckPyError();
if (st.ok()) {
return py_value;
Expand Down

0 comments on commit 4154e4f

Please sign in to comment.