PARQUET-1142: Add alternatives to Hadoop classes in the API by rdblue · Pull Request #429 · apache/parquet-java

rdblue · 2017-10-17T17:19:33Z

This updates the read and write paths to avoid using Hadoop classes where possible.

Adds a generic compression interface, CompressionCodecFactory
Adds OutputFile and PositionOutputStream
Adds classes to help implementations wrap input and output streams: DelegatingSeekableInputStream and DelegatingPositionOutputStream
Adds ParquetReadOptions to avoid passing options with Configuration
Updates the read and write APIs to use new abstractions instead of Hadoop

rdblue · 2017-10-17T17:29:35Z

I recommend reviewing this by commit. It touches a lot of files, but the changes aren't that large.

rdblue · 2017-10-17T17:29:47Z

@zivanfi, can you take a look?

zivanfi

@rdblue, I don't think I'm the best person to review this as I'm not too familiar with the affected code. I read through the PR nevertheless and in general it seemed good to me, but I'm unsure about which classes/methods are considered to be part of the public API, so I raised two questions regarding that.

zivanfi · 2017-10-18T12:00:47Z

parquet-hadoop/src/main/java/org/apache/parquet/hadoop/ParquetFileWriter.java

+   * @param maxPaddingSize the maximum padding
   * @throws IOException if the file can not be created
   */
-  public ParquetFileWriter(Configuration configuration, MessageType schema,


Is removing this public method not a breaking change?

Yes, good catch. I'll add the method back.

zivanfi · 2017-10-18T12:21:28Z

pom.xml

                     <exclude>org/apache/parquet/io/ColumnIOFactory/**</exclude> <!-- removed non-API class and methods-->
-		     <exclude>org/apache/parquet/hadoop/codec/SnappyCompressor</exclude> <!-- added synchronized modifier -->
+                     <exclude>org/apache/parquet/hadoop/codec/SnappyCompressor</exclude> <!-- added synchronized modifier -->
+                     <exclude>org/apache/parquet/bytes/BytesInput</exclude> <!-- moved to parquet-common -->


Are these not breaking changes?

We haven't considered moving classes into Jars further up the dependency chain to be breaking so far because you must have parquet-common to use parquet-encoding, and there is no binary incompatibility introduced.

My bad. I was worried about moving the CompressionCodecName and CompressionCodecNotSupportedException classes, because I hadn't realized that they were only moved to a different artifact while remaining in the same Java package.

rdblue · 2017-10-20T16:16:25Z

@zivanfi, thanks for taking a look. No problem with not being too familiar here, I just wanted you to have a first pass to catch things like binary compatibility that you guys care about the most. Plus, it will make you more familiar with these areas.

zivanfi

LGTM, but the automated tests fail (haven't looked into the reason).

zivanfi · 2017-10-24T09:16:46Z

pom.xml

                     <exclude>org/apache/parquet/io/ColumnIOFactory/**</exclude> <!-- removed non-API class and methods-->
-		     <exclude>org/apache/parquet/hadoop/codec/SnappyCompressor</exclude> <!-- added synchronized modifier -->
+                     <exclude>org/apache/parquet/hadoop/codec/SnappyCompressor</exclude> <!-- added synchronized modifier -->
+                     <exclude>org/apache/parquet/bytes/BytesInput</exclude> <!-- moved to parquet-common -->


My bad. I was worried about moving the CompressionCodecName and CompressionCodecNotSupportedException classes, because I hadn't realized that they were only moved to a different artifact while remaining in the same Java package.

gszadovszky

Only had some small findings. Otherwise, it looks good to me.

gszadovszky · 2017-11-30T13:33:47Z

parquet-hadoop/src/main/java/org/apache/parquet/hadoop/util/HadoopOutputFile.java

+  // for hadoop 1 to present. copying it avoids loading DFSConfigKeys.
+  private static final int DFS_BUFFER_SIZE_DEFAULT = 4096;
+
+  // visible for testing


No, it is not :)

I'll move the comment.

gszadovszky · 2017-11-30T13:36:14Z

parquet-hadoop/src/main/java/org/apache/parquet/hadoop/util/HadoopOutputFile.java

+  }
+
+  public static Set<String> getBlockFileSystems() {
+    return BLOCK_FS_SCHEMES;


Do we really want to expose a mutable set here?

This is what's visible for testing. Tests need to add "file". I'm fine with it being mutable because it is so unlikely that callers will change it; plus they're responsible for what happens if they do. The files will be correct either way, they will just get padding in some cases.

I'm good with having it for testing only.

Thanks for moving the comment.

gszadovszky · 2017-11-30T13:56:53Z

parquet-hadoop/src/test/java/org/apache/parquet/hadoop/util/MockHadoopInputStream.java

 import java.io.IOException;

-class MockInputStream extends ByteArrayInputStream
+class MockHadoopInputStream extends ByteArrayInputStream


If we already have MockInputStream in parquet-common why do we need this one as well?

This one implements Hadoop interfaces, Seekable and PositionedReadable, that are required for backing a FSDataInputStream. Those aren't available to MockInputStream. This could inherit from the other, but that would require moving classes around and exposing the test-jar, which doesn't seem worth it when we're only duplicating ~20 lines that may need to be changed independently in the future anyway.

My bad, didn't realise the interfaces are in parquet-hadoop. I agree that exposing test-jar etc. would not worth it.

This adds CompressionCodecFactory to parquet-common. This addition required moving org.apache.parquet.bytes into parquet-common from parquet-encoding. This also adds HadoopCodecs to parquet-hadoop, which implements the API. Moving classes between artifacts required adding exclusions to semver checks.

These are the write path's equivalent for InputFile and SeekableInputStream. The new classes capture what is needed for Parquet to write data so that Hadoop classes can be removed from the API in 2.0. This commit also adds convenience classes for implementing Parquet compatible streams: DelegatingPositionOutputStream and DelegatingSeekableInputStream.

…reams.

gszadovszky

LGTM, thanks for the changes and for making the modifications clear.

gszadovszky · 2017-12-04T08:38:59Z

parquet-hadoop/src/main/java/org/apache/parquet/hadoop/util/HadoopOutputFile.java

+  }
+
+  public static Set<String> getBlockFileSystems() {
+    return BLOCK_FS_SCHEMES;


Thanks for moving the comment.

rdblue · 2017-12-05T01:07:34Z

@julienledem, would you like to have a look? Otherwise I'll merge this tomorrow.

shanielh · 2018-01-21T09:31:42Z

This is great - Will a new version with this PR be released soon?

rdblue · 2018-01-22T18:40:31Z

@shanielh, yes. We are planning a 1.10 release right now. Hopefully it will be out soon.

rdblue force-pushed the PARQUET-1142-add-hadoop-alternatives branch from 4293a1e to f8697f6 Compare October 18, 2017 19:28

zivanfi reviewed Oct 20, 2017

View reviewed changes

zivanfi reviewed Oct 24, 2017

View reviewed changes

rdblue force-pushed the PARQUET-1142-add-hadoop-alternatives branch from 7e3b061 to 81874ae Compare November 15, 2017 00:26

gszadovszky requested changes Nov 30, 2017

View reviewed changes

rdblue added 5 commits December 1, 2017 12:28

PARQUET-1142: Update the read and write paths to use new files and st…

2e3d693

…reams.

PARQUET-1142: Fix binary incompatibilities.

da391b0

PARQUET-1142: Fix problems from Gabor's review.

35eddd7

rdblue force-pushed the PARQUET-1142-add-hadoop-alternatives branch from 81874ae to 35eddd7 Compare December 1, 2017 20:31

gszadovszky approved these changes Dec 4, 2017

View reviewed changes

PARQUET-1142: Fix NPE when not filtering with new read API.

2150033

asfgit closed this in 8bfd9b4 Dec 13, 2017

pearu mentioned this pull request Apr 25, 2019

ARROW-1983: [C++/Python][WIP] Implement metadata writing support apache/arrow#4166

Closed

2 tasks

asfimport mentioned this pull request Jun 23, 2024

Avoid leaking Hadoop API to downstream libraries #2097

Closed

Conversation

rdblue commented Oct 17, 2017

Uh oh!

rdblue commented Oct 17, 2017

Uh oh!

rdblue commented Oct 17, 2017

Uh oh!

zivanfi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rdblue commented Oct 20, 2017

Uh oh!

zivanfi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gszadovszky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gszadovszky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rdblue commented Dec 5, 2017

Uh oh!

shanielh commented Jan 21, 2018

Uh oh!

rdblue commented Jan 22, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants