Fix frombytes assumption

.gitignore

@@ -29,3 +29,4 @@ cpp/.idea/
 python/.eggs/
 .vscode
 .idea/
+.pytest_cache/

cpp/src/arrow/python/arrow_to_pandas.cc

@@ -265,13 +265,13 @@ class PandasBlock {
   int64_t num_rows_;
   int num_columns_;
 
-  OwnedRef block_arr_;
+  OwnedRefNoGIL block_arr_;
   uint8_t* block_data_;
 
   PandasOptions options_;
 
   // ndarray<int32>
-  OwnedRef placement_arr_;
+  OwnedRefNoGIL placement_arr_;
   int64_t* placement_data_;
 
  private:
@@ -1140,7 +1140,7 @@ class CategoricalBlock : public PandasBlock {
   }
 
   MemoryPool* pool_;
-  OwnedRef dictionary_;
+  OwnedRefNoGIL dictionary_;
   bool ordered_;
   bool needs_copy_;
 };

cpp/src/arrow/python/arrow_to_python.cc

@@ -64,8 +64,8 @@ Status DeserializeDict(PyObject* context, const Array& array, int64_t start_idx,
                        int64_t stop_idx, PyObject* base, const SerializedPyObject& blobs,
                        PyObject** out) {
   const auto& data = static_cast<const StructArray&>(array);
-  ScopedRef keys, vals;
-  ScopedRef result(PyDict_New());
+  OwnedRef keys, vals;
+  OwnedRef result(PyDict_New());
   RETURN_IF_PYERROR();
 
   DCHECK_EQ(2, data.num_fields());
@@ -77,16 +77,16 @@ Status DeserializeDict(PyObject* context, const Array& array, int64_t start_idx,
   for (int64_t i = start_idx; i < stop_idx; ++i) {
     // PyDict_SetItem behaves differently from PyList_SetItem and PyTuple_SetItem.
     // The latter two steal references whereas PyDict_SetItem does not. So we need
-    // to make sure the reference count is decremented by letting the ScopedRef
+    // to make sure the reference count is decremented by letting the OwnedRef
     // go out of scope at the end.
-    PyDict_SetItem(result.get(), PyList_GET_ITEM(keys.get(), i - start_idx),
-                   PyList_GET_ITEM(vals.get(), i - start_idx));
+    PyDict_SetItem(result.obj(), PyList_GET_ITEM(keys.obj(), i - start_idx),
+                   PyList_GET_ITEM(vals.obj(), i - start_idx));
   }
   static PyObject* py_type = PyUnicode_FromString("_pytype_");
-  if (PyDict_Contains(result.get(), py_type)) {
-    RETURN_NOT_OK(CallDeserializeCallback(context, result.get(), out));
+  if (PyDict_Contains(result.obj(), py_type)) {
+    RETURN_NOT_OK(CallDeserializeCallback(context, result.obj(), out));
   } else {
-    *out = result.release();
+    *out = result.detach();
   }
   return Status::OK();
 }
@@ -96,10 +96,10 @@ Status DeserializeArray(const Array& array, int64_t offset, PyObject* base,
   int32_t index = static_cast<const Int32Array&>(array).Value(offset);
   RETURN_NOT_OK(py::TensorToNdarray(*blobs.tensors[index], base, out));
   // Mark the array as immutable
-  ScopedRef flags(PyObject_GetAttrString(*out, "flags"));
-  DCHECK(flags.get() != NULL) << "Could not mark Numpy array immutable";
+  OwnedRef flags(PyObject_GetAttrString(*out, "flags"));
+  DCHECK(flags.obj() != NULL) << "Could not mark Numpy array immutable";
   Py_INCREF(Py_False);
-  int flag_set = PyObject_SetAttrString(flags.get(), "writeable", Py_False);
+  int flag_set = PyObject_SetAttrString(flags.obj(), "writeable", Py_False);
   DCHECK(flag_set == 0) << "Could not mark Numpy array immutable";
   return Status::OK();
 }
@@ -184,23 +184,23 @@ Status GetValue(PyObject* context, const UnionArray& parent, const Array& arr,
 
 #define DESERIALIZE_SEQUENCE(CREATE_FN, SET_ITEM_FN)                                     \
   const auto& data = static_cast<const UnionArray&>(array);                              \
-  ScopedRef result(CREATE_FN(stop_idx - start_idx));                                     \
+  OwnedRef result(CREATE_FN(stop_idx - start_idx));                                      \
   const uint8_t* type_ids = data.raw_type_ids();                                         \
   const int32_t* value_offsets = data.raw_value_offsets();                               \
   for (int64_t i = start_idx; i < stop_idx; ++i) {                                       \
     if (data.IsNull(i)) {                                                                \
       Py_INCREF(Py_None);                                                                \
-      SET_ITEM_FN(result.get(), i - start_idx, Py_None);                                 \
+      SET_ITEM_FN(result.obj(), i - start_idx, Py_None);                                 \
     } else {                                                                             \
       int64_t offset = value_offsets[i];                                                 \
       uint8_t type = type_ids[i];                                                        \
       PyObject* value;                                                                   \
       RETURN_NOT_OK(GetValue(context, data, *data.UnsafeChild(type), offset, type, base, \
                              blobs, &value));                                            \
-      SET_ITEM_FN(result.get(), i - start_idx, value);                                   \
+      SET_ITEM_FN(result.obj(), i - start_idx, value);                                   \
     }                                                                                    \
   }                                                                                      \
-  *out = result.release();                                                               \
+  *out = result.detach();                                                                \
   return Status::OK()
 
 Status DeserializeList(PyObject* context, const Array& array, int64_t start_idx,
@@ -219,13 +219,13 @@ Status DeserializeSet(PyObject* context, const Array& array, int64_t start_idx,
                       int64_t stop_idx, PyObject* base, const SerializedPyObject& blobs,
                       PyObject** out) {
   const auto& data = static_cast<const UnionArray&>(array);
-  ScopedRef result(PySet_New(nullptr));
+  OwnedRef result(PySet_New(nullptr));
   const uint8_t* type_ids = data.raw_type_ids();
   const int32_t* value_offsets = data.raw_value_offsets();
   for (int64_t i = start_idx; i < stop_idx; ++i) {
     if (data.IsNull(i)) {
       Py_INCREF(Py_None);
-      if (PySet_Add(result.get(), Py_None) < 0) {
+      if (PySet_Add(result.obj(), Py_None) < 0) {
         RETURN_IF_PYERROR();
       }
     } else {
@@ -234,12 +234,12 @@ Status DeserializeSet(PyObject* context, const Array& array, int64_t start_idx,
       PyObject* value;
       RETURN_NOT_OK(GetValue(context, data, *data.UnsafeChild(type), offset, type, base,
                              blobs, &value));
-      if (PySet_Add(result.get(), value) < 0) {
+      if (PySet_Add(result.obj(), value) < 0) {
         RETURN_IF_PYERROR();
       }
     }
   }
-  *out = result.release();
+  *out = result.detach();
   return Status::OK();
 }
 

cpp/src/arrow/python/builtin_convert.cc

@@ -931,7 +931,7 @@ static Status ConvertPySequenceReal(PyObject* obj, int64_t size,
   PyAcquireGIL lock;
 
   PyObject* seq;
-  ScopedRef tmp_seq_nanny;
+  OwnedRef tmp_seq_nanny;
 
   std::shared_ptr<DataType> real_type;
 

cpp/src/arrow/python/common.h

@@ -61,66 +61,48 @@ class ARROW_EXPORT PyAcquireGIL {
 
 #define PYARROW_IS_PY2 PY_MAJOR_VERSION <= 2
 
+// A RAII primitive that DECREFs the underlying PyObject* when it
+// goes out of scope.
 class ARROW_EXPORT OwnedRef {
  public:
   OwnedRef() : obj_(NULLPTR) {}
 
   explicit OwnedRef(PyObject* obj) : obj_(obj) {}
 
-  ~OwnedRef() {
-    PyAcquireGIL lock;
-    release();
-  }
+  ~OwnedRef() { reset(); }
 
   void reset(PyObject* obj) {
-    /// TODO(phillipc): Should we acquire the GIL here? It definitely needs to be
-    /// acquired,
-    /// but callers have probably already acquired it
     Py_XDECREF(obj_);
     obj_ = obj;
   }
 
-  void release() {
-    Py_XDECREF(obj_);
-    obj_ = NULLPTR;
-  }
-
-  PyObject* obj() const { return obj_; }
-
- private:
-  PyObject* obj_;
-};
-
-// This is different from OwnedRef in that it assumes that
-// the GIL is held by the caller and doesn't decrement the
-// reference count when release is called.
-class ARROW_EXPORT ScopedRef {
- public:
-  ScopedRef() : obj_(NULLPTR) {}
+  void reset() { reset(NULLPTR); }
 
-  explicit ScopedRef(PyObject* obj) : obj_(obj) {}
-
-  ~ScopedRef() { Py_XDECREF(obj_); }
-
-  void reset(PyObject* obj) {
-    Py_XDECREF(obj_);
-    obj_ = obj;
-  }
-
-  PyObject* release() {
+  PyObject* detach() {
     PyObject* result = obj_;
     obj_ = NULLPTR;
     return result;
   }
 
-  PyObject* get() const { return obj_; }
+  PyObject* obj() const { return obj_; }
 
   PyObject** ref() { return &obj_; }
 
  private:
   PyObject* obj_;
 };
 
+// Same as OwnedRef, but ensures the GIL is taken when it goes out of scope.
+// This is for situations where the GIL is not always known to be held
+// (e.g. if it is released in the middle of a function for performance reasons)
+class ARROW_EXPORT OwnedRefNoGIL : public OwnedRef {
+ public:
+  ~OwnedRefNoGIL() {
+    PyAcquireGIL lock;
+    reset();
+  }
+};
+
 struct ARROW_EXPORT PyObjectStringify {
   OwnedRef tmp_obj;
   const char* bytes;

cpp/src/arrow/python/numpy_to_arrow.cc

@@ -1116,10 +1116,10 @@ Status LoopPySequence(PyObject* sequence, T func) {
       }
     }
   } else if (PyObject_HasAttrString(sequence, "__iter__")) {
-    OwnedRef iter = OwnedRef(PyObject_GetIter(sequence));
+    OwnedRef iter(PyObject_GetIter(sequence));
     PyObject* item;
     while ((item = PyIter_Next(iter.obj()))) {
-      OwnedRef ref = OwnedRef(item);
+      OwnedRef ref(item);
       RETURN_NOT_OK(func(ref.obj()));
     }
   } else {
@@ -1149,11 +1149,11 @@ Status LoopPySequenceWithMasks(PyObject* sequence,
       }
     }
   } else if (PyObject_HasAttrString(sequence, "__iter__")) {
-    OwnedRef iter = OwnedRef(PyObject_GetIter(sequence));
+    OwnedRef iter(PyObject_GetIter(sequence));
     PyObject* item;
     int64_t i = 0;
     while ((item = PyIter_Next(iter.obj()))) {
-      OwnedRef ref = OwnedRef(item);
+      OwnedRef ref(item);
       RETURN_NOT_OK(func(ref.obj(), have_mask && mask_values[i]));
       i++;
     }
@@ -1476,20 +1476,20 @@ Status AppendUTF32(const char* data, int itemsize, int byteorder,
     }
   }
 
-  ScopedRef unicode_obj(PyUnicode_DecodeUTF32(data, actual_length * kNumPyUnicodeSize,
-                                              nullptr, &byteorder));
+  OwnedRef unicode_obj(PyUnicode_DecodeUTF32(data, actual_length * kNumPyUnicodeSize,
+                                             nullptr, &byteorder));
   RETURN_IF_PYERROR();
-  ScopedRef utf8_obj(PyUnicode_AsUTF8String(unicode_obj.get()));
-  if (utf8_obj.get() == NULL) {
+  OwnedRef utf8_obj(PyUnicode_AsUTF8String(unicode_obj.obj()));
+  if (utf8_obj.obj() == NULL) {
     PyErr_Clear();
     return Status::Invalid("failed converting UTF32 to UTF8");
   }
 
-  const int32_t length = static_cast<int32_t>(PyBytes_GET_SIZE(utf8_obj.get()));
+  const int32_t length = static_cast<int32_t>(PyBytes_GET_SIZE(utf8_obj.obj()));
   if (builder->value_data_length() + length > kBinaryMemoryLimit) {
     return Status::Invalid("Encoded string length exceeds maximum size (2GB)");
   }
-  return builder->Append(PyBytes_AS_STRING(utf8_obj.get()), length);
+  return builder->Append(PyBytes_AS_STRING(utf8_obj.obj()), length);
 }
 
 }  // namespace

cpp/src/arrow/python/python_to_arrow.cc

@@ -365,15 +365,15 @@ Status CallCustomCallback(PyObject* context, PyObject* method_name, PyObject* el
   *result = NULL;
   if (context == Py_None) {
     std::stringstream ss;
-    ScopedRef repr(PyObject_Repr(elem));
+    OwnedRef repr(PyObject_Repr(elem));
     RETURN_IF_PYERROR();
 #if PY_MAJOR_VERSION >= 3
-    ScopedRef ascii(PyUnicode_AsASCIIString(repr.get()));
+    OwnedRef ascii(PyUnicode_AsASCIIString(repr.obj()));
     RETURN_IF_PYERROR();
-    ss << "error while calling callback on " << PyBytes_AsString(ascii.get())
+    ss << "error while calling callback on " << PyBytes_AsString(ascii.obj())
        << ": handler not registered";
 #else
-    ss << "error while calling callback on " << PyString_AsString(repr.get())
+    ss << "error while calling callback on " << PyString_AsString(repr.obj())
        << ": handler not registered";
 #endif
     return Status::SerializationError(ss.str());
@@ -386,8 +386,8 @@ Status CallCustomCallback(PyObject* context, PyObject* method_name, PyObject* el
 
 Status CallSerializeCallback(PyObject* context, PyObject* value,
                              PyObject** serialized_object) {
-  ScopedRef method_name(PyUnicode_FromString("_serialize_callback"));
-  RETURN_NOT_OK(CallCustomCallback(context, method_name.get(), value, serialized_object));
+  OwnedRef method_name(PyUnicode_FromString("_serialize_callback"));
+  RETURN_NOT_OK(CallCustomCallback(context, method_name.obj(), value, serialized_object));
   if (!PyDict_Check(*serialized_object)) {
     return Status::TypeError("serialization callback must return a valid dictionary");
   }
@@ -396,8 +396,8 @@ Status CallSerializeCallback(PyObject* context, PyObject* value,
 
 Status CallDeserializeCallback(PyObject* context, PyObject* value,
                                PyObject** deserialized_object) {
-  ScopedRef method_name(PyUnicode_FromString("_deserialize_callback"));
-  return CallCustomCallback(context, method_name.get(), value, deserialized_object);
+  OwnedRef method_name(PyUnicode_FromString("_deserialize_callback"));
+  return CallCustomCallback(context, method_name.obj(), value, deserialized_object);
 }
 
 Status SerializeDict(PyObject* context, std::vector<PyObject*> dicts,
@@ -493,9 +493,9 @@ Status Append(PyObject* context, PyObject* elem, SequenceBuilder* builder,
 #if PY_MAJOR_VERSION >= 3
     char* data = PyUnicode_AsUTF8AndSize(elem, &size);
 #else
-    ScopedRef str(PyUnicode_AsUTF8String(elem));
-    char* data = PyString_AS_STRING(str.get());
-    size = PyString_GET_SIZE(str.get());
+    OwnedRef str(PyUnicode_AsUTF8String(elem));
+    char* data = PyString_AS_STRING(str.obj());
+    size = PyString_GET_SIZE(str.obj());
 #endif
     if (size > std::numeric_limits<int32_t>::max()) {
       return Status::Invalid("Cannot writes bytes over 2GB");
@@ -585,15 +585,15 @@ Status SerializeSequences(PyObject* context, std::vector<PyObject*> sequences,
   SequenceBuilder builder(nullptr);
   std::vector<PyObject*> sublists, subtuples, subdicts, subsets;
   for (const auto& sequence : sequences) {
-    ScopedRef iterator(PyObject_GetIter(sequence));
+    OwnedRef iterator(PyObject_GetIter(sequence));
     RETURN_IF_PYERROR();
-    ScopedRef item;
+    OwnedRef item;
     while (true) {
-      item.reset(PyIter_Next(iterator.get()));
-      if (!item.get()) {
+      item.reset(PyIter_Next(iterator.obj()));
+      if (!item.obj()) {
         break;
       }
-      RETURN_NOT_OK(Append(context, item.get(), &builder, &sublists, &subtuples,
+      RETURN_NOT_OK(Append(context, item.obj(), &builder, &sublists, &subtuples,
                            &subdicts, &subsets, blobs_out));
     }
   }
@@ -739,18 +739,18 @@ Status SerializedPyObject::WriteTo(io::OutputStream* dst) {
 Status SerializedPyObject::GetComponents(MemoryPool* memory_pool, PyObject** out) {
   PyAcquireGIL py_gil;
 
-  ScopedRef result(PyDict_New());
+  OwnedRef result(PyDict_New());
   PyObject* buffers = PyList_New(0);
 
   // TODO(wesm): Not sure how pedantic we need to be about checking the return
   // values of these functions. There are other places where we do not check
   // PyDict_SetItem/SetItemString return value, but these failures would be
   // quite esoteric
-  PyDict_SetItemString(result.get(), "num_tensors",
+  PyDict_SetItemString(result.obj(), "num_tensors",
                        PyLong_FromSize_t(this->tensors.size()));
-  PyDict_SetItemString(result.get(), "num_buffers",
+  PyDict_SetItemString(result.obj(), "num_buffers",
                        PyLong_FromSize_t(this->buffers.size()));
-  PyDict_SetItemString(result.get(), "data", buffers);
+  PyDict_SetItemString(result.obj(), "data", buffers);
   RETURN_IF_PYERROR();
 
   Py_DECREF(buffers);
@@ -792,7 +792,7 @@ Status SerializedPyObject::GetComponents(MemoryPool* memory_pool, PyObject** out
     RETURN_NOT_OK(PushBuffer(buf));
   }
 
-  *out = result.release();
+  *out = result.detach();
   return Status::OK();
 }