ARROW-6720:[JAVA][C++]Support Parquet Read and Write in Java #5522

xuechendi · 2019-09-27T06:38:26Z

Added a new java interface to support parquet read and write from hdfs or local file.

The purpose of this implementation is that when we loading and dumping parquet data in Java, we can only use rowBased put and get methods. Since arrow already has C++ implementation to load and dump parquet, so we wrapped those codes as Java APIs.

After test, we noticed in our workload, performance improved more than 2x comparing with rowBased load and dump. So we want to contribute codes to arrow.

This is a total independent change:
c++ codes are under cpp/jni/parquet, used as jni bridge.
java codes are under java/adapter/parquet, ParquetReader and ParquetWriter is the main interface class. Java UnitTest is also included.

Signed-off-by: Chendi Xue chendi.xue@intel.com

wesm · 2019-09-27T13:18:39Z

I will review the C++ part of this in more detail when I can, but as a high level comment we will need the code itself to follow the Google C++ style guide and also conform to our file naming conventions (lower_with_underscores.h/cc).

emkornfield · 2019-09-27T14:26:30Z

Another high level comment is this seems to duplicate a lot of code from ORC adapter (the thread-safe map popped out as one example). Is it possible to refactor to share common implementations?

xuechendi · 2019-09-27T14:36:27Z

Another high level comment is this seems to duplicate a lot of code from ORC adapter (the thread-safe map popped out as one example). Is it share common implementations?

@emkornfield, yes, currentmap was copied from orc, so as some codes in java side, the reason I did as that way is that I was thinking to limit my modification to existing codes, so I copied it to parquet folder.
Better way is to make the duplicated part as a common lib, but I am thinking will it be ok if I open another PR or commit for the codes refine?

xuechendi · 2019-09-27T14:41:11Z

I will review the C++ part of this in more detail when I can, but as a high level comment we will need the code itself to follow the Google C++ style guide and also conform to our file naming conventions (lower_with_underscores.h/cc).

@wesm , yes, I noticed that, and I am working on to fixing the coding style problem, wish to pass CI firstly. And thanks for the code review.

github-actions · 2019-09-29T18:46:03Z

https://issues.apache.org/jira/browse/ARROW-6720

ghost · 2019-09-27T13:40:41Z

cpp/src/jni/parquet/FileConnector.cpp

+
+Status FileConnector::openReadable() {
+  Status msg = ReadableFile::Open(filePath, &fileReader);
+  if (!msg.ok()) {


In general, all of these instances where exit is called when a non-OK Status is returned should be changed to just return the Status.

@lihalite , I see, thanks, I will fix that.

@lihalite , I have fixed the issue. Thanks

codecov-io · 2019-10-01T17:14:05Z

Codecov Report

Merging #5522 into master will decrease coverage by 10.86%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           master    #5522       +/-   ##
===========================================
- Coverage   88.82%   77.95%   -10.87%     
===========================================
  Files         983       59      -924     
  Lines      132974     4554   -128420     
  Branches     1501        0     -1501     
===========================================
- Hits       118108     3550   -114558     
+ Misses      14501     1004    -13497     
+ Partials      365        0      -365

Impacted Files	Coverage Δ
python/pyarrow/ipc.pxi
cpp/src/parquet/column_page.h
cpp/src/plasma/test/external_store_tests.cc
cpp/src/arrow/array/builder_decimal.cc
cpp/src/plasma/client.cc
.../thirdparty/flatbuffers/include/flatbuffers/base.h
cpp/src/arrow/flight/internal.cc
cpp/src/arrow/compute/compute_test.cc
cpp/src/arrow/python/io.cc
python/pyarrow/hdfs.py
... and 914 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4db8f7b...b248e64. Read the comment docs.

xuechendi · 2019-10-02T13:07:26Z

@wesm , just passed CI checks, thanks

xuechendi · 2019-10-04T00:45:18Z

I will review the C++ part of this in more detail when I can, but as a high level comment we will need the code itself to follow the Google C++ style guide and also conform to our file naming conventions (lower_with_underscores.h/cc).

@wesm , yes, I noticed that, and I am working on to fixing the coding style problem, wish to pass CI firstly. And thanks for the code review.

@wesm , I have renamed all files and codes passed CI checks, thanks

emkornfield · 2019-10-04T23:48:13Z

cpp/src/jni/parquet/connector.h

+  virtual void teardown() = 0;
+
+ protected:
+  std::string filePath;


naming convention for members is file_path_

@emkornfield , thanks, fixed naming convention issue.

emkornfield · 2019-10-04T23:48:34Z

cpp/src/jni/parquet/connector.h

+ public:
+  Connector() {}
+  std::string getFileName() { return filePath; }
+  virtual ::arrow::Status openReadable(bool useHdfs3) = 0;


naming convention for methods is OpenReadable.

emkornfield

I did a partial review. There are still some style issues and lack of documentaiton (at a minimum classes in header files need docs).

Also I'm a little concerned about the Connector classes, it seems that they are replication filesystem / io interfaces without a clear reason why (documentation could help here). Following the pattern that ORC used here seems better. An ideal solution (would allow passing of Readers/Writers directly).

emkornfield · 2019-10-04T23:50:59Z

cpp/src/jni/parquet/connector.h

+namespace jni {
+namespace parquet {
+
+class Connector {


please provide class documentation. for this class. It looks like this has nothing to do with Parquet, can it be moved someplace else (maybe under filesystem)?

Actually how is this different then FileSystem (and for instance HdfsFileSystem (https://github.com/apache/arrow/blob/master/cpp/src/arrow/io/hdfs.h#L71)

@emkornfield , the reason of keeping connector classes under jni/parquet is that, I want to make the connector interface unified regardless its connection type(hdfs or local), while I can't find an unified class under arrow/io or arrow/filesystem, for example, hdfsFileSystem are derived from Filesystem while openReadable under file.h are derived from RandomAccessFile, and LocalFileSystem under arrow/filesystem/localfs.h does not have OpenReadable interface.
Since I hope to limit my code change to existing module, so I am figuring as what I did now to add a small wrapper firstly to make the ParquetReader and ParquetWriter functioning, then I can open a new PR to move the connector classes to arrow/filesystem and make it as a baseFileSystem class? Do you think that makes sense to you?

emkornfield · 2019-10-04T23:51:18Z

cpp/src/jni/parquet/file_connector.h

+namespace jni {
+namespace parquet {
+
+class FileConnector : public Connector {


needs documentation.

documentation added, thanks

emkornfield · 2019-10-04T23:51:59Z

cpp/src/jni/parquet/connector.h

+
+class Connector {
+ public:
+  Connector() {}


Connector() = default; ?

emkornfield · 2019-10-04T23:52:27Z

cpp/src/jni/parquet/connector.h

+class Connector {
+ public:
+  Connector() {}
+  std::string getFileName() { return filePath; }


I think there should be a virtual destructor?

emkornfield · 2019-10-04T23:57:32Z

cpp/src/jni/parquet/connector.h

+  std::string filePath;
+  std::string dirPath;
+  std::string getPathDir(std::string path) {
+    std::string delimiter = "/";


this doesn't seem portable across OSes (i.e. windows)

@emkornfield , thanks, I have fixed that by using methods under arrow::filesystem

emkornfield · 2019-10-04T23:58:48Z

cpp/src/jni/parquet/jni_wrapper.cpp

+  jni::parquet::ParquetReader* reader = (jni::parquet::ParquetReader*)reader_ptr;
+  arrow::Status status = reader->readNext(&record_batch);
+
+  if (!status.ok() || !record_batch) {


I think a not OK status should be propagated as an exception?

@emkornfield , oh, this is not an error, this was used to capture readNextBatch has reached end of rowGroups. That is why I return nullptr here.

I think this is conflating two cases here. status isn't OK. this indicates some sort of error that should be propagated (or checked specifically to be end of stream) and end of stream (signalled by a null record_batch).

I see, I will split these two cases. Thanks

emkornfield · 2019-10-05T00:01:06Z

cpp/src/jni/parquet/parquet_reader.h

+class ParquetReader {
+ public:
+  explicit ParquetReader(std::string path);
+  ::arrow::Status initialize(std::vector<int>& column_indices,


these should either be const references or passed via pointer if a mutation is expected (and moved to the end of the function call).

@emkornfield , fixed, thanks

1. c++ codes are under cpp/jni/parquet, used to doing jni bridge work. 2. java codes are under java/adapter/parquet, ParquetReader and ParquetWriter is the main interface class. Signed-off-by: Chendi Xue <chendi.xue@intel.com>

last ci check shows a glibc installation issue, try again 2 Signed-off-by: Chendi Xue <chendi.xue@intel.com>

Signed-off-by: Chendi Xue <chendi.xue@intel.com>

1. Fixed name convention for fileNames, methods names and variable names. 2. Added some documentations for classes and interfaces. 3. Use arrow::filesystem to get and create directory Signed-off-by: Chendi Xue <chendi.xue@intel.com>

xuechendi · 2019-10-09T09:25:51Z

@emkornfield , Just passed CI checks, fixed all issues you mentioned including documentation, naming convention and OSes compatibility, for the reason of adding connector classes in Jni/parquet, also replied under your review, the main reason is I don’t want to modify too much of existing codes and there is no proper unified interface to use, and I would like to do that after. Would you mind to give another round review?

xuechendi · 2019-10-09T09:32:45Z

@kszucs, would you review again, sorry for I mis overrode your changes.

xuechendi · 2019-10-12T00:24:14Z

@wesm and @emkornfield , do you mind to take a look and see if anything need to change? @emkornfield suggested to move connector classes to somewhere else, my original plan is to do that after this pr merged(to keep this functioning and limit its modification to existing codes in arrow), but if which blocks this PR being merged, I can commit another PR to add hdfs under arrow/filesystem by wrapping the one under arrow/io, then I can remove connector classed in this PR and use arrow/filesystem/Filesystem class then.
Any comments?

siddharthteotia · 2019-10-12T06:46:42Z

For Java side, is it more advantageous to implement reader as a JNI wrapper over C++ native reader as opposed to implementing a native first class vectorized reader in Java itself? I believe going through the JNI bridge will add some performance overhead

xuechendi · 2019-10-12T07:05:28Z

For Java side, is it more advantageous to implement reader as a JNI wrapper over C++ native reader as opposed to implementing a native first class vectorized reader in Java itself? I believe going through the JNI bridge will add some performance overhead

@siddharthteotia , I am a little confused, are you suggesting to read parquet by using some parquet java package and convert to arrow? So to make it pure java?
For your question of overhead, there is no memory copy between native memory and JVM if that's what concerns you, in my implementation, java codes only hold a reference of the data, and when its ref_cnf turned to 0, it will call nativeRelease to free allocated memory.

ghost · 2019-10-13T17:13:30Z

I can commit another PR to add hdfs under arrow/filesystem by wrapping the one under arrow/io, then I can remove connector classed in this PR and use arrow/filesystem/Filesystem class then.
Any comments?

That actually raises an interesting point: would it make more sense to bind the new, higher-level filesystem functionality alongside the Parquet reader/writer? Presumably, a lot of people using Parquet would want both, and it is something that parquet-mr seems to lack right now.

emkornfield · 2019-10-13T17:48:02Z

@xuechendi thank you for effort here. I think what I would prefer to see this broken multiple smaller PRs for this (@wesm I don't know if you have reviewed yet to see if you have any thoughts). Roughly I think there is:

Factor out common JNI code to one location so code isn't duplicated (i.e. the thread safe map, assembly/disassembly or record batches).
A PR that does the core Parquet Reading from local filesystem.
Code that handles dispatching to multiple filesystems. I think the the way to do this is to either a FileSystemFactory or a MetaFileSystem which would handle taking a path (maybe a URL) and delegating the appropriate filesystem implementation. @pitrou do you have any thoughts on this? @lihalite I think this is probably an easier solution than fully wrapping the filesystem. Thoughts?

It would also be nice to create a MemoryPool that can use Java's allocation facilities instead of the other way around. But at for me I don't think this is a blocker.

pitrou · 2019-10-14T14:17:58Z

At some point we'll need a filesystem URI layer in Arrow C++, so that people don't have to instantiate filesystems explicitly.

jacques-n · 2019-10-14T16:28:57Z

.../adapter/parquet/src/main/java/org/apache/arrow/adapter/parquet/AdaptorReferenceManager.java

+
+  @Override
+  public BufferAllocator getAllocator() {
+    return null;


This shouldn't return null.

This file actually is copied from the one under adapter/orc, the reason of returning null here is we didn't really need BufferAllocator here, memory is allocated by native codes. And we use this ReferenceManager class to do ref count, so when java side holds zero reference of this buffer, we can call close to free native memory.

jacques-n · 2019-10-14T16:29:48Z

.../adapter/parquet/src/main/java/org/apache/arrow/adapter/parquet/AdaptorReferenceManager.java

+
+  @Override
+  public OwnershipTransferResult transferOwnership(ArrowBuf sourceBuffer, BufferAllocator targetAllocator) {
+    throw new UnsupportedOperationException();


I think we need to come with a better approach here then UnsupportedOperationException. Isn't there a noop we've defined elsewhere?

I see, I can check with that, thanks

jacques-n · 2019-10-14T16:30:41Z

java/adapter/parquet/src/main/java/org/apache/arrow/adapter/parquet/ArrowBufBuilder.java

+/**
+ * ArrowBufBuilder.
+ */
+public class ArrowBufBuilder {


what is the purpose of this class? The javadoc is useless. Does it need to be package public? Is there a better name?

this class is used to create java object from jni_wrapper.cc, I followed the way from java/adapater/orc.

xuechendi · 2019-10-15T11:48:45Z

At some point we'll need a filesystem URI layer in Arrow C++, so that people don't have to instantiate filesystems explicitly.

yes, I believe so, I will wrapper hdfs codes under arrow/io and add a new one to arrow/Filesystem, then I can use FileSystem interfaces instead create my own connectors.

emkornfield

see comments, we should break this PR up into smaller components so we don't continue to increase technical debt in the JNI layer.

xuechendi · 2019-10-23T07:22:33Z

see comments, we should break this PR up into smaller components so we don't continue to increase technical debt in the JNI layer.

Hi, @emkornfield , thanks for your review, sorry for my delayed response, engaged in my regular work tasks last week. I just submitted a new PR which is to add an filesystemfactory and hadoopfilesystem under arrow/filesystem, to replace the connector classed in this PR. It would be great if you can spare some time to review the codes. #5717

And I am planning to submit another two PRs later on, one for jni/parquet wrapper c++ part and one java part. Will ping you then.

xuechendi · 2019-10-23T15:12:00Z

see comments, we should break this PR up into smaller components so we don't continue to increase technical debt in the JNI layer.

Hi, @emkornfield , thanks for your review, sorry for my delayed response, engaged in my regular work tasks last week. I just submitted a new PR which is to add an filesystemfactory and hadoopfilesystem under arrow/filesystem, to replace the connector classed in this PR. It would be great if you can spare some time to review the codes. #5717

And I am planning to submit another two PRs later on, one for jni/parquet wrapper c++ part and one java part. Will ping you then.

Also added second PR #5719 to wrap parquet codes and provide an adapter for JNI(submit later)

emkornfield · 2019-12-12T08:30:35Z

@xuechendi, if I understand correctly this PR can be closed in favor of #5719?

xuechendi · 2019-12-12T08:31:46Z

@emkornfield , yes, closed.

xuechendi force-pushed the wip_parquet branch 2 times, most recently from 4b537e0 to 38055e1 Compare September 27, 2019 12:26

xuechendi force-pushed the wip_parquet branch from 38055e1 to c6604aa Compare September 27, 2019 15:47

ghost reviewed Sep 30, 2019

View reviewed changes

xuechendi force-pushed the wip_parquet branch from c6604aa to f56d2d3 Compare October 1, 2019 14:10

xuechendi force-pushed the wip_parquet branch 2 times, most recently from a67c25e to 54620b6 Compare October 2, 2019 09:08

fsaintjacques added the Component: Java label Oct 2, 2019

xuechendi force-pushed the wip_parquet branch 3 times, most recently from 89ea33e to 57970fe Compare October 3, 2019 11:57

emkornfield reviewed Oct 4, 2019

View reviewed changes

emkornfield requested changes Oct 5, 2019

View reviewed changes

kszucs force-pushed the master branch from fc93312 to af097e6 Compare October 5, 2019 09:47

kszucs force-pushed the wip_parquet branch from 57970fe to 3952434 Compare October 5, 2019 10:11

xuechendi added 2 commits October 8, 2019 15:48

Add Parquet Reader and Writer Java Wrapper

b8f385a

1. c++ codes are under cpp/jni/parquet, used to doing jni bridge work. 2. java codes are under java/adapter/parquet, ParquetReader and ParquetWriter is the main interface class. Signed-off-by: Chendi Xue <chendi.xue@intel.com>

Rename files to follow arrow naming style

ea62667

last ci check shows a glibc installation issue, try again 2 Signed-off-by: Chendi Xue <chendi.xue@intel.com>

xuechendi force-pushed the wip_parquet branch 3 times, most recently from f69f7be to e9e4fdd Compare October 9, 2019 03:26

Rebase to master and refined pom and exception

96540fd

Signed-off-by: Chendi Xue <chendi.xue@intel.com>

Naming convention fixing and documentation

b248e64

1. Fixed name convention for fileNames, methods names and variable names. 2. Added some documentations for classes and interfaces. 3. Use arrow::filesystem to get and create directory Signed-off-by: Chendi Xue <chendi.xue@intel.com>

xuechendi force-pushed the wip_parquet branch from e9e4fdd to b248e64 Compare October 9, 2019 04:03

xuechendi requested a review from emkornfield October 10, 2019 01:51

jacques-n reviewed Oct 14, 2019

View reviewed changes

emkornfield requested changes Oct 17, 2019

View reviewed changes

xuechendi mentioned this pull request Oct 23, 2019

ARROW-6720: [C++] Add FileSystemFactory and HadoopFileSystem under arrow::fs #5717

Closed

xuechendi mentioned this pull request Oct 23, 2019

ARROW-6720: [C++] Add Parquet Reader/Writer Adapter for JNI Bridge #5719

Closed

xuechendi closed this Dec 12, 2019

asfimport mentioned this pull request Jul 12, 2022

[JAVA][C++]Support Parquet Read and Write in Java #23063

Open

ARROW-6720:[JAVA][C++]Support Parquet Read and Write in Java #5522

ARROW-6720:[JAVA][C++]Support Parquet Read and Write in Java #5522

Conversation

xuechendi commented Sep 27, 2019

wesm commented Sep 27, 2019

emkornfield commented Sep 27, 2019 • edited

xuechendi commented Sep 27, 2019

xuechendi commented Sep 27, 2019

github-actions bot commented Sep 29, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Oct 1, 2019 • edited

Codecov Report

xuechendi commented Oct 2, 2019

xuechendi commented Oct 4, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emkornfield 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

xuechendi commented Oct 9, 2019

xuechendi commented Oct 9, 2019

xuechendi commented Oct 12, 2019

siddharthteotia commented Oct 12, 2019

xuechendi commented Oct 12, 2019

ghost commented Oct 13, 2019

emkornfield commented Oct 13, 2019

pitrou commented Oct 14, 2019

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

xuechendi commented Oct 15, 2019

emkornfield left a comment

Choose a reason for hiding this comment

xuechendi commented Oct 23, 2019

xuechendi commented Oct 23, 2019

emkornfield commented Dec 12, 2019

xuechendi commented Dec 12, 2019

emkornfield commented Sep 27, 2019 •

edited

codecov-io commented Oct 1, 2019 •

edited