ARROW-302: [C++/Python] Implement C++ IO interfaces for interacting with Python file and bytes objects #152

wesm · 2016-10-02T23:58:50Z

This will enable code (such as arrow IPC or Parquet) that only knows about Arrow's IO subsystem to interact with Python objects in various ways. In other words, when we have in C++:

std::shared_ptr<io::ReadableFileInterface> handle = ...;
handle->Read(nbytes, &out);

then the C++ file handle could be invoking the read method of a Python object. Same goes for arrow::io::OutputStream and write methods. There's data copying in some places overhead because of the rigid memory ownership semantics of the PyBytes type, but this can't be avoided here.

Another nice thing is that if we have some data in a Python bytes object that we want to expose to some other C++ component, we can wrap it in the PyBytesReader which provides zero-copy read access to the underlying data.

Change-Id: I712800f68cd0c9b0de1cca0d511f3f7d297c05b2

Change-Id: I2feee0fd6a6faa5c486ab4699a7cdc8c042d5726

Change-Id: Ie9f6f13d7e1fb55e948da2a7709864978df7306a

Change-Id: If86ff0ce8b25bd28c9028e93624af62ba01f8a60

Change-Id: I9d6e656ea152ef678ac01e22e4e24b3783ccacc0

Change-Id: I131f49d73c73097c5ac250bfc0c60639358c0954

Change-Id: I2ef9faca323f333e7f828d757d8740d20e5e467a

wesm · 2016-10-03T00:04:28Z

@xhochy there's a small API change here, I will patch Parquet and bump the pinned hash

Change-Id: I87b6281d7a42ce7c3d0a9b9ae8744afe530a84d6

Change-Id: Icd397820eb6d7d9aa36d617c3f50900ec7d04030

xhochy

Overall LGTM but I hope you're going to fix the part marked with // XXX:

xhochy · 2016-10-03T06:53:58Z

cpp/src/arrow/io/hdfs.cc

+    RETURN_NOT_OK(ReadAt(position, nbytes, &bytes_read, buffer->mutable_data()));
+
+    // XXX: heuristic
+    if (bytes_read < nbytes / 2) { RETURN_NOT_OK(buffer->Resize(bytes_read)); }


Why don't we resize down to bytes_read? PoolBuffer itself will only increase its allocated memory.

will do, I left this mostly to be controversial since the realloc-copy may not be worth it when bytes_read is close to nbytes

xhochy · 2016-10-03T06:54:22Z

cpp/src/arrow/io/hdfs.cc

+    RETURN_NOT_OK(Read(nbytes, &bytes_read, buffer->mutable_data()));
+
+    // XXX: heuristic
+    if (bytes_read < nbytes / 2) { RETURN_NOT_OK(buffer->Resize(bytes_read)); }


Why don't we resize down to bytes_read? PoolBuffer itself will only increase its allocated memory.

Also how do we communicate the actual bytes read then to the caller?

This means we should definitely always exactly resize so that we know the number of bytes read

xhochy · 2016-10-03T07:04:23Z

cpp/src/arrow/io/interfaces.h

 public:
-  virtual ~FileInterface() {}
+  virtual ~FileInterface() = 0;


This is actually implemented interfaces.cc so it should not be deleted here.

I did this on purpose — I had to do some wrangling with the class hierarchy when I added default virtual implementations (which can be overridden, like they are for HDFS) of ReadAt

xhochy · 2016-10-03T09:56:28Z

python/src/pyarrow/io.h

+class PythonFile {
+ public:
+  PythonFile(PyObject* file);
+  ~PythonFile();


Destructors should be virtual.

Not here, this class has no subclasses

xhochy · 2016-10-03T09:56:57Z

python/src/pyarrow/io.h

+class PYARROW_EXPORT PyReadableFile : public arrow::io::ReadableFileInterface {
+ public:
+  explicit PyReadableFile(PyObject* file);
+  ~PyReadableFile();


Destructors should be virtual.

Implicitly virtual because of the abstract base class. I think this can be marked override

A quick Google seems to confirm this, but it would be nice to find a more authoritative reference: http://stackoverflow.com/questions/5610135/with-virtual-destructors-do-i-need-to-explicitly-declare-a-virtual-destructor-f

Actually override doesn't make sense with virtual destructors because they all get invoked, not overridden

xhochy · 2016-10-03T09:57:07Z

python/src/pyarrow/io.h

+class PYARROW_EXPORT PyOutputStream : public arrow::io::OutputStream {
+ public:
+  explicit PyOutputStream(PyObject* file);
+  ~PyOutputStream();


Destructors should be virtual.

xhochy · 2016-10-03T09:57:14Z

python/src/pyarrow/io.h

+class PYARROW_EXPORT PyBytesReader : public arrow::io::BufferReader {
+ public:
+  explicit PyBytesReader(PyObject* obj);
+  ~PyBytesReader();


Destructors should be virtual.

Change-Id: Ib67ce254d8a2aeff3df12e8d57525f3383a3c344

Change-Id: I498fb6e26642065c32c8425e5e6564bfb7b75c78

wesm · 2016-10-04T03:14:02Z

+1

xhochy · 2016-10-04T10:56:04Z

I hope to get some time to setup clang-static-analyzer for arrow in the future. That should help us in the question about where you need virtual for destructors and also notify us of virtual method calls in constructors.

…alues Closes apache#145 (this bug was fixed as part of other recent patches) Author: Wes McKinney <wes.mckinney@twosigma.com> Closes apache#152 from wesm/PARQUET-703 and squashes the following commits: 232d2f1 [Wes McKinney] Validate that ColumnChunk metadata has the number of values set including nulls

…alues Closes apache#145 (this bug was fixed as part of other recent patches) Author: Wes McKinney <wes.mckinney@twosigma.com> Closes apache#152 from wesm/PARQUET-703 and squashes the following commits: 232d2f1 [Wes McKinney] Validate that ColumnChunk metadata has the number of values set including nulls Change-Id: I6f7ac294fe19fb311a4f7457b1216887b4326034

* support invalid charactor in url * check space charactor in url queries * change string to string_view * change the way to check invalid charactors

wesm added 7 commits October 1, 2016 16:53

Draft API for Arrow IO wrappers for Python files

20a3f28

Change-Id: I712800f68cd0c9b0de1cca0d511f3f7d297c05b2

Draft PyReadableFile implementation, not yet tested

8be433f

Change-Id: I2feee0fd6a6faa5c486ab4699a7cdc8c042d5726

Refactoring, more code sharing with native file interfaces

737a8db

Change-Id: Ie9f6f13d7e1fb55e948da2a7709864978df7306a

Share default implementations of ReadAt, add Buffer-based Read API

d470133

Change-Id: If86ff0ce8b25bd28c9028e93624af62ba01f8a60

Get basic Python file read/write working

7e357eb

Change-Id: I9d6e656ea152ef678ac01e22e4e24b3783ccacc0

Add a zero-copy reader for PyBytes

6481e91

Change-Id: I131f49d73c73097c5ac250bfc0c60639358c0954

Test the size() method and fix bug with missing whence

e9b8c60

Change-Id: I2ef9faca323f333e7f828d757d8740d20e5e467a

wesm added 2 commits October 2, 2016 20:08

cpplint

0fc4cf1

Change-Id: I87b6281d7a42ce7c3d0a9b9ae8744afe530a84d6

Python 2.7 fix

e791893

Change-Id: Icd397820eb6d7d9aa36d617c3f50900ec7d04030

xhochy requested changes Oct 3, 2016

View reviewed changes

wesm added 2 commits October 3, 2016 16:32

Code review comments

316b845

Change-Id: Ib67ce254d8a2aeff3df12e8d57525f3383a3c344

Fix compiler warning / bug from OS X

2de9f97

Change-Id: I498fb6e26642065c32c8425e5e6564bfb7b75c78

asfgit closed this in c7e6a07 Oct 4, 2016

wesm deleted the ARROW-302 branch October 4, 2016 03:15

paleolimbot mentioned this pull request Jan 28, 2023

[R] Crash on MacOS (x86) when running tests with homebrew apache-arrow also installed #33903

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ARROW-302: [C++/Python] Implement C++ IO interfaces for interacting with Python file and bytes objects #152

ARROW-302: [C++/Python] Implement C++ IO interfaces for interacting with Python file and bytes objects #152

wesm commented Oct 2, 2016 •

edited

Loading

wesm commented Oct 3, 2016

xhochy left a comment

xhochy Oct 3, 2016

wesm Oct 3, 2016

xhochy Oct 3, 2016

xhochy Oct 3, 2016

wesm Oct 3, 2016

xhochy Oct 3, 2016

wesm Oct 3, 2016

xhochy Oct 3, 2016

wesm Oct 3, 2016

xhochy Oct 3, 2016

wesm Oct 3, 2016

wesm Oct 3, 2016

wesm Oct 3, 2016

xhochy Oct 3, 2016

wesm Oct 3, 2016

xhochy Oct 3, 2016

wesm Oct 3, 2016

wesm commented Oct 4, 2016

xhochy commented Oct 4, 2016

ARROW-302: [C++/Python] Implement C++ IO interfaces for interacting with Python file and bytes objects #152

ARROW-302: [C++/Python] Implement C++ IO interfaces for interacting with Python file and bytes objects #152

Conversation

wesm commented Oct 2, 2016 • edited Loading

wesm commented Oct 3, 2016

xhochy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wesm commented Oct 4, 2016

xhochy commented Oct 4, 2016

wesm commented Oct 2, 2016 •

edited

Loading