ARROW-280: [C++] Refactor IPC / memory map IO to use common arrow_io interfaces. Create arrow_ipc leaf library #138
Conversation
|
cc @emkornfield @xhochy . let me know if you'd like me to put this on gerrit |
wesm
force-pushed
the
ARROW-280
branch
4 times, most recently
from
6576e67
to
48d27fb
Compare
|
@xhochy this will cause some downstream impacts on the arrow_parquet work ongoing. I'm going to make the necessary fixes here and we can port them over to Parquet once PARQUET-712 is done |
|
@wesm feel free to merge PARQUET-712, should be ready now. |
|
@wesm Also note that I pinned the arrow-git-hash in the dependencies for parquet-cpp, so breakages here won't break parquet-cpp's builds. |
* Refactor memory mapped IO interfaces to be in line with other arrow::io classes. * Split arrow_ipc into a leaf library * Refactor pyarrow and arrow_parquet to suit. Move BufferReader to arrow_io. Pyarrow parquet tests currently segfault Change-Id: I9a8eb95dfb6572810adb3e5199bd09cbbd020bab
|
That's great. I got things building again but the pyarrow parquet tests currently segfault, we should investigate separate from this patch |
|
OK, the build is passing. I'm going to move on to implementing the file format on top of this. |
|
I'm going to merge this to be able to start tackling the Parquet code migration in parallel with the file format work. Any feedback I will happily incorporate in a follow up patch. +1 |
wesm
changed the title
| @@ -20,6 +20,7 @@ | |||
|
|
|||
| set(ARROW_IO_LINK_LIBS | |||
| arrow_shared | |||
| dl | |||
There was a problem hiding this comment.
This should not be needed, maybe you mixed somewhere a static and a shared build?
There was a problem hiding this comment.
We are using dlopen in libhdfs_shim.cc, I get a linker error without linking libdl
There was a problem hiding this comment.
Ah, ok. I had the problem with missing dlopen symbols in parquet-cpp as there static and shared test libs where mixed. There it would have been only a workaround to link dl but as we explicitly use it here, it's fine.
| ADD_ARROW_TEST(hdfs-io-test) | ||
| ARROW_TEST_LINK_LIBRARIES(hdfs-io-test | ||
| ADD_ARROW_TEST(io-hdfs-test) | ||
| ARROW_TEST_LINK_LIBRARIES(io-hdfs-test |
There was a problem hiding this comment.
Ah, that's why you need dl. This should probably be conditional on if we build static or shared tests.
There was a problem hiding this comment.
See above, the issue was actually dlopen, dlsym, etc.
| @@ -37,6 +38,7 @@ set(ARROW_IO_TEST_LINK_LIBS | |||
| ${ARROW_IO_PRIVATE_LINK_LIBS}) | |||
There was a problem hiding this comment.
We would need a arrow_io_static here to solve the linking problems in the tests.
| ) | ||
|
|
||
| set(ARROW_IPC_TEST_LINK_LIBS | ||
| arrow_ipc |
There was a problem hiding this comment.
We'll also need a static lib here.
There was a problem hiding this comment.
Roger. This isn't causing immediate problems -- let's create JIRAs to build static libs for the leaf libraries too
Several things here