[GitHub] [parquet-mr] steveloughran commented on a diff in pull request #951: PARQUET-2134: Fix type checking in HadoopStreams.wrap
steveloughran commented on code in PR #951:
URL: https://github.com/apache/parquet-mr/pull/951#discussion_r901856428
##
parquet-hadoop/src/main/java/org/apache/parquet/hadoop/util/HadoopStreams.java:
##
@@ -50,51 +46,45 @@ public class HadoopStreams {
*/
public static SeekableInputStream wrap(FSDataInputStream stream) {
Objects.requireNonNull(stream, "Cannot wrap a null input stream");
-if (byteBufferReadableClass != null && h2SeekableConstructor != null &&
-byteBufferReadableClass.isInstance(stream.getWrappedStream())) {
- try {
-return h2SeekableConstructor.newInstance(stream);
- } catch (InstantiationException | IllegalAccessException e) {
-LOG.warn("Could not instantiate H2SeekableInputStream, falling back to
byte array reads", e);
-return new H1SeekableInputStream(stream);
- } catch (InvocationTargetException e) {
-throw new ParquetDecodingException(
-"Could not instantiate H2SeekableInputStream",
e.getTargetException());
- }
+if (isWrappedStreamByteBufferReadable(stream)) {
+ return new H2SeekableInputStream(stream);
} else {
return new H1SeekableInputStream(stream);
}
}
- private static Class getReadableClass() {
-try {
- return Class.forName("org.apache.hadoop.fs.ByteBufferReadable");
-} catch (ClassNotFoundException | NoClassDefFoundError e) {
- return null;
+ /**
+ * Is the inner stream byte buffer readable?
+ * The test is "the stream is not FSDataInputStream
+ * and implements ByteBufferReadable'
+ *
+ * That is: all streams which implement ByteBufferReadable
+ * other than FSDataInputStream successfuly support read(ByteBuffer).
+ * This is true for all filesytem clients the hadoop codebase.
+ *
+ * In hadoop 3.3.0+, the StreamCapabilities probe can be used to
+ * check this: only those streams which provide the read(ByteBuffer)
+ * semantics MAY return true for the probe "in:readbytebuffer";
+ * FSDataInputStream will pass the probe down to the underlying stream.
+ *
+ * @param stream stream to probe
+ * @return true if it is safe to a H2SeekableInputStream to access the data
+ */
+ private static boolean isWrappedStreamByteBufferReadable(FSDataInputStream
stream) {
+if (stream.hasCapability("in:readbytebuffer")) {
Review Comment:
no, the StreamCapabilities probe has been around since hadoop 2. it is just
in 3.3.0 all streams which implement the api return true for this probe...a
probe which gets passed down the wrapped streams. It avoids looking at the
wrapped streams as you should be able to trust the response (put differently:
if something lied it is in trouble)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@parquet.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [parquet-mr] steveloughran commented on a diff in pull request #951: PARQUET-2134: Fix type checking in HadoopStreams.wrap
steveloughran commented on code in PR #951:
URL: https://github.com/apache/parquet-mr/pull/951#discussion_r890391185
##
parquet-hadoop/src/main/java/org/apache/parquet/hadoop/util/HadoopStreams.java:
##
@@ -66,6 +67,19 @@ public static SeekableInputStream wrap(FSDataInputStream
stream) {
}
}
+ private static boolean isWrappedStreamByteBufferReadable(FSDataInputStream
stream) {
+InputStream wrapped = stream.getWrappedStream();
+if (wrapped == stream) {
+ throw new ParquetDecodingException("Illegal FSDataInputStream as wrapped
itself");
Review Comment:
this can't happen. the inner stream is set in the constructor, so cannot
take the not-yet-constructed class as an argument...no need to worry about
recursion.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@parquet.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [parquet-mr] steveloughran commented on a diff in pull request #951: PARQUET-2134: Fix type checking in HadoopStreams.wrap
steveloughran commented on code in PR #951:
URL: https://github.com/apache/parquet-mr/pull/951#discussion_r890388928
##
parquet-hadoop/src/main/java/org/apache/parquet/hadoop/util/HadoopStreams.java:
##
@@ -51,7 +52,7 @@ public class HadoopStreams {
public static SeekableInputStream wrap(FSDataInputStream stream) {
Objects.requireNonNull(stream, "Cannot wrap a null input stream");
if (byteBufferReadableClass != null && h2SeekableConstructor != null &&
-byteBufferReadableClass.isInstance(stream.getWrappedStream())) {
+isWrappedStreamByteBufferReadable(stream)) {
Review Comment:
this is really going into the internals of the hadoop classes and
potentially tricky if there is any dynamic decision making in the inner class.
The good news there is I don't see anything doing that.
there is a way to ask (hadoop 3.2+) if a stream does support the API before
calling, using the StreamCapabilities interface.
https://issues.apache.org/jira/browse/HDFS-14111
```java
if (stream.hasCapability( "in:readbytebuffer") {
// stream is confident it has the api
) else {
// do the checking of the inner class
}
##
parquet-hadoop/src/main/java/org/apache/parquet/hadoop/util/HadoopStreams.java:
##
@@ -66,6 +67,15 @@ public static SeekableInputStream wrap(FSDataInputStream
stream) {
}
}
+ private static boolean isWrappedStreamByteBufferReadable(FSDataInputStream
stream) {
+InputStream wrapped = stream.getWrappedStream();
+if (wrapped instanceof FSDataInputStream) {
+ return isWrappedStreamByteBufferReadable(((FSDataInputStream) wrapped));
Review Comment:
you can't. the inner stream is set in the constructor, so cannot take the
not-yet-constructed class as an argument.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@parquet.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
