PARQUET-2276: Bring back support for Hadoop 2.7.3 #1084

Fokko · 2023-04-29T17:57:33Z

Make sure you have checked all steps below.

Complements the work of #951. This PR removes the breaking change that caused Parquet to be incompatible with Hadoop <2.9. In this change, we dynamically check if the hasCapabilities method is available, and if this is the case, it will use it. Otherwise, it will fall back to the previous implementation, with the addition that it will also check the wrapped streams.

Jira

My PR addresses the following Parquet Jira issues and references them in the PR title. For example, "PARQUET-1234: My Parquet PR"
- https://issues.apache.org/jira/browse/PARQUET-2276
- In case you are adding a dependency, check if the license complies with the ASF 3rd Party License Policy.

Tests

My PR adds the following unit tests OR does not need testing for this extremely good reason:

Commits

My commits all reference Jira issues in their subject lines. In addition, my commits follow the guidelines from "How to write a good git commit message":
1. Subject is separated from body by a blank line
2. Subject is limited to 50 characters (not including Jira issue reference)
3. Subject does not end with a period
4. Subject uses the imperative mood ("add", not "adding")
5. Body wraps at 72 characters
6. Body explains "what" and "why", not "how"

Documentation

In case of new functionality, my PR adds documentation that describes how to use it.
- All the public functions and the classes in the PR contain Javadoc that explain what it does

rdblue · 2023-05-02T17:50:13Z

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

  private static final Logger LOG = LoggerFactory.getLogger(HadoopStreams.class);

+  private static final Class<?> byteBufferReadableClass = getReadableClass();
+  static final Constructor<SeekableInputStream> h2SeekableConstructor = getH2SeekableConstructor();


Rather than using two static methods, you can use DynConstructors instead to make this one expression and reduce error handling code:

private static final DynConstructors.Ctor<SeekableInputStream> h2streamCtor = DynConstructors.Builder(SeekableInputStream.class) .impl("org.apache.parquet.hadoop.util.H2SeekableInputStream", FSDataInputStream.class) .orNull() .build() ... if (h2streamCtor != null) { return h2streamCtor.newInstance(stream); }

Looks like you'd need to add the orNull handling. I don't see it here: https://github.com/apache/parquet-mr/blob/master/parquet-common/src/main/java/org/apache/parquet/util/DynConstructors.java

Huh, I guess this was how it was before? Nevermind on the refactoring then.

Yes, I copied most from the old code to avoid refactoring. I think we can greatly simplify it because it was still taking Hadoop1 into account. We still have to check if the wrapped stream is ByteBufferReadable: https://github.com/apache/hadoop/blob/release-2.4.1/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FSDataInputStream.java#L142-L148

The `hasCapabilities does the same but in a more elegant way.

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

rdblue · 2023-05-02T21:16:13Z

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

+    InputStream wrapped = stream.getWrappedStream();
+    if (wrapped instanceof FSDataInputStream) {
+      LOG.debug("Checking on wrapped stream {} of {} whether is ByteBufferReadable", wrapped, stream);
+      return isWrappedStreamByteBufferReadableLegacy(((FSDataInputStream) wrapped));


Why would a FSDataInputStream have another one inside?

This came from the issue from Presto: prestodb/presto#17435

you can if you try hard, it's just really unusual

you can never wrap an instance by itself.

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

rdblue

I think this should work after comparing it with older code, but it seems like there are some easy improvements to me.

Fokko · 2023-05-02T21:50:56Z

I think this should work after comparing it with older code, but it seems like there are some easy improvements to me.

@rdblue I agree, I did some cleaning up. Let me know what you think.

Fokko · 2023-05-03T16:15:31Z

FWIW, I also ran the Iceberg tests and it ran fine (except the bloom filter ones, more details here).

wgtmac

LGTM. Thanks @Fokko and @rdblue

BTW, is it possible to warn the users somewhere that they are running on a low version of Hadoop and it is time to upgrade?

steveloughran · 2023-05-04T12:24:17Z

I repeat my stance on this: to claim hadoop 2.7 runtime compatibility you should be building against java7. if you don't, well, make clear its a fairly qualified support "hadoop 2.7.3 on java8 only" and not worry about the bits of hadoop which break if they try to do that (kerberos, s3a, anything with joda-time, ...)

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

rdblue · 2023-05-05T16:17:10Z

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

+   * @param stream stream to probe
+   * @return A H2SeekableInputStream to access, or H1SeekableInputStream if the stream is not seekable
+   */
+  private static SeekableInputStream isWrappedStreamByteBufferReadableLegacy(FSDataInputStream stream) {


Usually, isSomething methods return a boolean. This is unwrapping, so I'd prefer naming it unwrapByteBufferReadableLegacy or something to be more clear.

rdblue · 2023-05-05T16:17:49Z

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

+      LOG.debug("Checking on wrapped stream {} of {} whether is ByteBufferReadable", wrapped, stream);
+      return isWrappedStreamByteBufferReadableLegacy(((FSDataInputStream) wrapped));
+    }
+    if (stream.getWrappedStream() instanceof ByteBufferReadable) {


I prefer using the same whitespace conventions as in Iceberg, although that's a bit more relaxed over here.

rdblue · 2023-05-05T16:18:50Z

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

+      return null;
+    }
+
+    Boolean hasCapabilities = hasCapabilitiesMethod.invoke(stream, "in:readbytebuffer");


Is this a boxed boolean? If so, should we update the check to handle null?

I was assuming that it needed to be an object, but a primitive works as well, so changed that. Thanks for catching this!

rdblue · 2023-05-05T16:19:25Z

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

+
+    Boolean hasCapabilities = hasCapabilitiesMethod.invoke(stream, "in:readbytebuffer");
+
+    if (hasCapabilities) {


This variable would be more clear if it were called isByteBufferReadable since that's the capability we are checking for.

rdblue

Merge when you're ready. I think this is correct.

Fokko · 2023-05-05T21:43:33Z

Thanks @shangxinli, @wgtmac, @rdblue, and @steveloughran for the review.

Steve, I'm aware of your stance and I also respect it. Unfortunately, a lot of companies are still using (internally heavy patched versions) of Hadoop, and to get traction in downstream projects we still have to maintain compatibility.

* Bring back support for Hadoop 2.7.3 * Simplify the code * Fix the naming * Comments

Bring back support for Hadoop 2.7.3

e6d948f

Fokko force-pushed the fd-support-hadoop-2-7-3 branch from c30a45a to e6d948f Compare April 29, 2023 18:54

Fokko changed the title ~~Bring back support for Hadoop 2.7.3~~ PARQUET-2276: Bring back support for Hadoop 2.7.3 Apr 29, 2023

Fokko mentioned this pull request Apr 29, 2023

Parquet: Update parquet to 1.13.1 apache/iceberg#7301

Merged

rdblue reviewed May 2, 2023

View reviewed changes

parquet-hadoop/src/main/java/org/apache/parquet/hadoop/util/HadoopStreams.java Outdated Show resolved Hide resolved

rdblue reviewed May 2, 2023

View reviewed changes

parquet-hadoop/src/main/java/org/apache/parquet/hadoop/util/HadoopStreams.java Outdated Show resolved Hide resolved

rdblue approved these changes May 2, 2023

View reviewed changes

Fokko force-pushed the fd-support-hadoop-2-7-3 branch from 2d8b57c to 7822034 Compare May 2, 2023 21:42

Simplify the code

cef627a

Fokko force-pushed the fd-support-hadoop-2-7-3 branch from 7822034 to cef627a Compare May 2, 2023 21:49

wgtmac approved these changes May 4, 2023

View reviewed changes

shangxinli reviewed May 4, 2023

View reviewed changes

parquet-hadoop/src/main/java/org/apache/parquet/hadoop/util/HadoopStreams.java Show resolved Hide resolved

shangxinli reviewed May 4, 2023

View reviewed changes

parquet-hadoop/src/main/java/org/apache/parquet/hadoop/util/HadoopStreams.java Outdated Show resolved Hide resolved

shangxinli approved these changes May 4, 2023

View reviewed changes

Fix the naming

350167f

rdblue reviewed May 5, 2023

View reviewed changes

rdblue approved these changes May 5, 2023

View reviewed changes

Comments

9552742

Fokko merged commit dededb6 into apache:master May 5, 2023

Fokko deleted the fd-support-hadoop-2-7-3 branch May 5, 2023 22:14

Fokko added a commit to Fokko/parquet-mr that referenced this pull request May 5, 2023

PARQUET-2276: Bring back support for Hadoop 2.7.3 (apache#1084)

26e5060

* Bring back support for Hadoop 2.7.3 * Simplify the code * Fix the naming * Comments

Fokko added a commit that referenced this pull request May 9, 2023

PARQUET-2276: Bring back support for Hadoop 2.7.3 (#1084) (#1090)

e2c2499

* Bring back support for Hadoop 2.7.3 * Simplify the code * Fix the naming * Comments


		Boolean hasCapabilities = hasCapabilitiesMethod.invoke(stream, "in:readbytebuffer");

		if (hasCapabilities) {

PARQUET-2276: Bring back support for Hadoop 2.7.3 #1084

PARQUET-2276: Bring back support for Hadoop 2.7.3 #1084

Uh oh!

Conversation

Fokko commented Apr 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Jira

Tests

Commits

Documentation

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!

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!

Uh oh!

rdblue left a comment

Choose a reason for hiding this comment

Uh oh!

Fokko commented May 2, 2023

Uh oh!

Fokko commented May 3, 2023

Uh oh!

wgtmac left a comment

Choose a reason for hiding this comment

Uh oh!

steveloughran commented May 4, 2023

Uh oh!

Uh oh!

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 left a comment

Choose a reason for hiding this comment

Uh oh!

Fokko commented May 5, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Fokko commented Apr 29, 2023 •

edited

Loading