[
https://issues.apache.org/jira/browse/ARROW-2155?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16365698#comment-16365698
]
ASF GitHub Bot commented on ARROW-2155:
---------------------------------------
wesm closed pull request #1608: ARROW-2155: [Python] frombuffer() should
respect mutability of argument
URL: https://github.com/apache/arrow/pull/1608
This is a PR merged from a forked repository.
As GitHub hides the original diff on merge, it is displayed below for
the sake of provenance:
As this is a foreign pull request (from a fork), the diff is supplied
below (as it won't show otherwise due to GitHub magic):
diff --git a/cpp/src/arrow/python/common.cc b/cpp/src/arrow/python/common.cc
index 14a8ae6fd..1ded88071 100644
--- a/cpp/src/arrow/python/common.cc
+++ b/cpp/src/arrow/python/common.cc
@@ -23,6 +23,7 @@
#include "arrow/memory_pool.h"
#include "arrow/status.h"
+#include "arrow/util/logging.h"
namespace arrow {
namespace py {
@@ -47,22 +48,39 @@ MemoryPool* get_memory_pool() {
// ----------------------------------------------------------------------
// PyBuffer
-PyBuffer::PyBuffer(PyObject* obj) : Buffer(nullptr, 0), obj_(nullptr) {
- if (PyObject_CheckBuffer(obj)) {
- obj_ = PyMemoryView_FromObject(obj);
- Py_buffer* buffer = PyMemoryView_GET_BUFFER(obj_);
- data_ = reinterpret_cast<const uint8_t*>(buffer->buf);
- size_ = buffer->len;
- capacity_ = buffer->len;
- is_mutable_ = false;
+PyBuffer::PyBuffer() : Buffer(nullptr, 0) {}
+
+Status PyBuffer::Init(PyObject* obj) {
+ if (!PyObject_GetBuffer(obj, &py_buf_, PyBUF_ANY_CONTIGUOUS)) {
+ data_ = reinterpret_cast<const uint8_t*>(py_buf_.buf);
+ DCHECK(data_ != nullptr);
+ size_ = py_buf_.len;
+ capacity_ = py_buf_.len;
+ is_mutable_ = !py_buf_.readonly;
+ return Status::OK();
+ } else {
+ return Status(StatusCode::PythonError, "");
}
}
+Status PyBuffer::FromPyObject(PyObject* obj, std::shared_ptr<Buffer>* out) {
+ PyBuffer* buf = new PyBuffer();
+ std::shared_ptr<Buffer> res(buf);
+ RETURN_NOT_OK(buf->Init(obj));
+ *out = res;
+ return Status::OK();
+}
+
PyBuffer::~PyBuffer() {
- PyAcquireGIL lock;
- Py_XDECREF(obj_);
+ if (data_ != nullptr) {
+ PyAcquireGIL lock;
+ PyBuffer_Release(&py_buf_);
+ }
}
+// ----------------------------------------------------------------------
+// Python exception -> Status
+
Status CheckPyError(StatusCode code) {
if (PyErr_Occurred()) {
PyObject* exc_type = nullptr;
diff --git a/cpp/src/arrow/python/common.h b/cpp/src/arrow/python/common.h
index b1e0888af..269385c1a 100644
--- a/cpp/src/arrow/python/common.h
+++ b/cpp/src/arrow/python/common.h
@@ -18,6 +18,7 @@
#ifndef ARROW_PYTHON_COMMON_H
#define ARROW_PYTHON_COMMON_H
+#include <memory>
#include <string>
#include "arrow/python/config.h"
@@ -140,15 +141,17 @@ ARROW_EXPORT MemoryPool* get_memory_pool();
class ARROW_EXPORT PyBuffer : public Buffer {
public:
- /// Note that the GIL must be held when calling the PyBuffer constructor.
- ///
- /// While memoryview objects support multi-demensional buffers, PyBuffer
only supports
+ /// While memoryview objects support multi-dimensional buffers, PyBuffer
only supports
/// one-dimensional byte buffers.
- explicit PyBuffer(PyObject* obj);
~PyBuffer();
+ static Status FromPyObject(PyObject* obj, std::shared_ptr<Buffer>* out);
+
private:
- PyObject* obj_;
+ PyBuffer();
+ Status Init(PyObject*);
+
+ Py_buffer py_buf_;
};
} // namespace py
diff --git a/cpp/src/arrow/python/io.cc b/cpp/src/arrow/python/io.cc
index 2cff04608..801a32574 100644
--- a/cpp/src/arrow/python/io.cc
+++ b/cpp/src/arrow/python/io.cc
@@ -149,14 +149,11 @@ Status PyReadableFile::Read(int64_t nbytes, int64_t*
bytes_read, void* out) {
Status PyReadableFile::Read(int64_t nbytes, std::shared_ptr<Buffer>* out) {
PyAcquireGIL lock;
- PyObject* bytes_obj = NULL;
- ARROW_RETURN_NOT_OK(file_->Read(nbytes, &bytes_obj));
- DCHECK(bytes_obj != NULL);
-
- *out = std::make_shared<PyBuffer>(bytes_obj);
- Py_XDECREF(bytes_obj);
+ OwnedRef bytes_obj;
+ ARROW_RETURN_NOT_OK(file_->Read(nbytes, bytes_obj.ref()));
+ DCHECK(bytes_obj.obj() != NULL);
- return Status::OK();
+ return PyBuffer::FromPyObject(bytes_obj.obj(), out);
}
Status PyReadableFile::ReadAt(int64_t position, int64_t nbytes, int64_t*
bytes_read,
@@ -219,13 +216,5 @@ Status PyOutputStream::Write(const void* data, int64_t
nbytes) {
return file_->Write(data, nbytes);
}
-// ----------------------------------------------------------------------
-// A readable file that is backed by a PyBuffer
-
-PyBytesReader::PyBytesReader(PyObject* obj)
- : io::BufferReader(std::make_shared<PyBuffer>(obj)) {}
-
-PyBytesReader::~PyBytesReader() {}
-
} // namespace py
} // namespace arrow
diff --git a/cpp/src/arrow/python/io.h b/cpp/src/arrow/python/io.h
index 0632d28fa..648f6de06 100644
--- a/cpp/src/arrow/python/io.h
+++ b/cpp/src/arrow/python/io.h
@@ -79,13 +79,6 @@ class ARROW_EXPORT PyOutputStream : public io::OutputStream {
int64_t position_;
};
-// A zero-copy reader backed by a PyBuffer object
-class ARROW_EXPORT PyBytesReader : public io::BufferReader {
- public:
- explicit PyBytesReader(PyObject* obj);
- virtual ~PyBytesReader();
-};
-
// TODO(wesm): seekable output files
} // namespace py
diff --git a/cpp/src/arrow/python/python-test.cc
b/cpp/src/arrow/python/python-test.cc
index d9919ee49..bcf89a4f6 100644
--- a/cpp/src/arrow/python/python-test.cc
+++ b/cpp/src/arrow/python/python-test.cc
@@ -33,7 +33,14 @@
namespace arrow {
namespace py {
-TEST(PyBuffer, InvalidInputObject) { PyBuffer buffer(Py_None); }
+TEST(PyBuffer, InvalidInputObject) {
+ std::shared_ptr<Buffer> res;
+ PyObject* input = Py_None;
+ auto old_refcnt = Py_REFCNT(input);
+ ASSERT_RAISES(PythonError, PyBuffer::FromPyObject(input, &res));
+ PyErr_Clear();
+ ASSERT_EQ(old_refcnt, Py_REFCNT(input));
+}
class DecimalTest : public ::testing::Test {
public:
diff --git a/python/pyarrow/includes/libarrow.pxd
b/python/pyarrow/includes/libarrow.pxd
index 2e83f0701..81cc4d276 100644
--- a/python/pyarrow/includes/libarrow.pxd
+++ b/python/pyarrow/includes/libarrow.pxd
@@ -894,7 +894,8 @@ cdef extern from "arrow/python/api.h" namespace "arrow::py"
nogil:
" arrow::py::get_memory_pool"()
cdef cppclass PyBuffer(CBuffer):
- PyBuffer(object o)
+ @staticmethod
+ CStatus FromPyObject(object obj, shared_ptr[CBuffer]* out)
cdef cppclass PyReadableFile(RandomAccessFile):
PyReadableFile(object fo)
@@ -902,9 +903,6 @@ cdef extern from "arrow/python/api.h" namespace "arrow::py"
nogil:
cdef cppclass PyOutputStream(OutputStream):
PyOutputStream(object fo)
- cdef cppclass PyBytesReader(CBufferReader):
- PyBytesReader(object fo)
-
cdef struct PandasOptions:
c_bool strings_to_categorical
c_bool zero_copy_only
diff --git a/python/pyarrow/io.pxi b/python/pyarrow/io.pxi
index bd508cf57..b0996f85e 100644
--- a/python/pyarrow/io.pxi
+++ b/python/pyarrow/io.pxi
@@ -802,12 +802,8 @@ def frombuffer(object obj):
Construct an Arrow buffer from a Python bytes object
"""
cdef shared_ptr[CBuffer] buf
- try:
- memoryview(obj)
- buf.reset(new PyBuffer(obj))
- return pyarrow_wrap_buffer(buf)
- except TypeError:
- raise ValueError('Must pass object that implements buffer protocol')
+ check_status(PyBuffer.FromPyObject(obj, &buf))
+ return pyarrow_wrap_buffer(buf)
cdef get_reader(object source, shared_ptr[RandomAccessFile]* reader):
diff --git a/python/pyarrow/tests/test_io.py b/python/pyarrow/tests/test_io.py
index da26b101d..0947cb7c7 100644
--- a/python/pyarrow/tests/test_io.py
+++ b/python/pyarrow/tests/test_io.py
@@ -104,7 +104,7 @@ def test_bytes_reader():
def test_bytes_reader_non_bytes():
- with pytest.raises(ValueError):
+ with pytest.raises(TypeError):
pa.BufferReader(u('some sample data'))
@@ -132,6 +132,7 @@ def test_buffer_bytes():
buf = pa.frombuffer(val)
assert isinstance(buf, pa.Buffer)
+ assert not buf.is_mutable
result = buf.to_pybytes()
@@ -143,6 +144,7 @@ def test_buffer_memoryview():
buf = pa.frombuffer(val)
assert isinstance(buf, pa.Buffer)
+ assert not buf.is_mutable
result = memoryview(buf)
@@ -154,13 +156,20 @@ def test_buffer_bytearray():
buf = pa.frombuffer(val)
assert isinstance(buf, pa.Buffer)
+ assert buf.is_mutable
result = bytearray(buf)
assert result == val
-def test_buffer_numpy():
+def test_buffer_invalid():
+ with pytest.raises(TypeError,
+ match="(bytes-like object|buffer interface)"):
+ pa.frombuffer(None)
+
+
+def test_buffer_to_numpy():
# Make sure creating a numpy array from an arrow buffer works
byte_array = bytearray(20)
byte_array[0] = 42
@@ -170,6 +179,19 @@ def test_buffer_numpy():
assert array.base == buf
+def test_buffer_from_numpy():
+ # C-contiguous
+ arr = np.arange(12, dtype=np.int8).reshape((3, 4))
+ buf = pa.frombuffer(arr)
+ assert buf.to_pybytes() == arr.tobytes()
+ # F-contiguous; note strides informations is lost
+ buf = pa.frombuffer(arr.T)
+ assert buf.to_pybytes() == arr.tobytes()
+ # Non-contiguous
+ with pytest.raises(ValueError, match="not contiguous"):
+ buf = pa.frombuffer(arr.T[::2])
+
+
def test_allocate_buffer():
buf = pa.allocate_buffer(100)
assert buf.size == 100
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
> [Python] pa.frombuffer(bytearray) returns immutable Buffer
> ----------------------------------------------------------
>
> Key: ARROW-2155
> URL: https://issues.apache.org/jira/browse/ARROW-2155
> Project: Apache Arrow
> Issue Type: Improvement
> Components: Python
> Affects Versions: 0.8.0
> Reporter: Antoine Pitrou
> Assignee: Antoine Pitrou
> Priority: Minor
> Labels: pull-request-available
> Fix For: 0.9.0
>
>
> I'd expect it to return a mutable buffer:
> {code:python}
> >>> pa.frombuffer(bytearray(10)).is_mutable
> False
> {code}
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
