GH-3080: HadoopStreams to support ByteBufferPositionedReadable #3096
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Based of the H2 stream test suite but * parameterized for on/off heap * expect no changes in buffer contents on out of range reads. Still one test failure.
* changing how stream capabilities are set up and queried, makes it easy to generate streams with different declared behaviours. * pull out common assertions * lots of javadoc of what each test case is trying to do. + all the tests are happy.
wgtmac
reviewed
Dec 13, 2024
|
|
||
| if (isByteBufferReadable) { | ||
| if (stream.hasCapability(READBYTEBUFFER)) { |
There was a problem hiding this comment.
We can avoid any reflection here because of the Hadoop version bump?
| * @param stream stream to probe | ||
| * @return A H2SeekableInputStream to access, or H1SeekableInputStream if the stream is not seekable | ||
| */ | ||
| private static Function<FSDataInputStream, SeekableInputStream> unwrapByteBufferReadableLegacy( |
There was a problem hiding this comment.
Is there any behavior change of a wrapped stream after removing unwrapByteBufferReadableLegacy?
| @@ -100,6 +100,10 @@ public void readVectored(List<ParquetFileRange> ranges, ByteBufferAllocator allo | |||
| VectorIoBridge.instance().readVectoredRanges(stream, ranges, allocator); | |||
| } | |||
|
|
|||
| protected Reader getReader() { | |||
There was a problem hiding this comment.
Why adding this but not used elsewhere?
| * @param buf a byte buffer to fill with data from the stream | ||
| * @return number of bytes read. | ||
| * | ||
| * @throws EOFException the buffer length is greater than the file length |
There was a problem hiding this comment.
Add EOFException to the method signature?
| * {@code ByteBufferPositionedReadable.readFully()}. | ||
| * <p>This is implemented by HDFS and possibly other clients, | ||
| */ | ||
| class H3ByteBufferInputStream extends H2SeekableInputStream { |
There was a problem hiding this comment.
I'm fine with the inheritance to reduce code duplication.
|
I'm away until 2025; will reply to comments then. Thanks for the review. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Rationale for this change
If a stream declares in its StreamCapabilities that it supports
ByteBufferPositionedReadable, then use that API forreadFully(ByteBuffer)Adding support for Hadoop
ByteBufferPositionedReadablestreams may improve performanceby pushing retry/recovery logic into the filesystem client library.
This interface is implemented by the HDFS input stream; we are considering adding
it elsewhere.
What changes are included in this PR?
H3ByteBufferInputStreamHadoopStreamsif theFSDataInputStreamis considered suitable.Class
H3ByteBufferInputStreamThe reading is done in a new class,
H3ByteBufferInputStream, which subclassesH2ByteBufferInputStream. This reduces the amount of duplicate code, it just makes it a bit unclean.The purist way to do it would be to create an abstract superclass
HadoopInputStreamto hold all commonality between the the three input streams.I'm happy to do this, just didn't want to doing some larger refactoring without (a) showing the core design worked and (b) getting permission to do this. Should I do this?
HadoopStreamschangesSelection of the new input stream is done if and only if the stream declares the capability
in:preadbytebuffer.There is no equivalent of
isWrappedStreamByteBufferReadable()which recurses througha chain of wrapped streams looking for the API.
If a stream doesn't declare its support for the API, it won't get picked up.
This is done knowing that the sole production implemenation which currently exists,
the HDFS input stream, does declare this capability.
Are these changes tested?
There is new test suite, for new behavior and ensuring that the integration with
HadoopStreams still retains the correct behavior for existing streams.
Suite is parameterized on heap and direct buffers.
Are there any user-facing changes?
No
Closes GH-3080