Repository navigation
Conversation
dossett
reviewed
Aug 17, 2026
dossett
reviewed
Aug 17, 2026
Member
Author
|
cc @wgtmac can you take a look? |
wgtmac
reviewed
Aug 28, 2026
wgtmac
reviewed
Sep 23, 2026
Track original filesystem allocations, including checksum buffers and buffers backing returned slices, and release them with the row group. Clean up the row group when reading or parsing fails. Include submission and every requested range in a single read deadline. Invalidate failed readers and defer stream cleanup until submission exits, without delaying interruption or recycling buffers still used by IO. Publish available futures after partial submission failures and fall back to ordinary reads when a pre-submission file-length lookup fails. Add regression coverage for buffer ownership, submission failures, timeouts, interruption, and metadata fallback.
sunchao
force-pushed
the
dev/chao/codex/gh-3719-vectored-read-safety
branch
from
September 23, 2026 16:27
f82eaa4 to
5d3d6f3
Compare
wgtmac
reviewed
Sep 24, 2026
Member
|
Do you want to take a look as well? @gszadovszky @Fokko @divjotarora |
Contributor
|
@wgtmac I'd be interested in reviewing, but I'm a bit swamped right now. Can get to it tomorrow if that's OK |
divjotarora
approved these changes
Sep 25, 2026
divjotarora
left a comment
Contributor
There was a problem hiding this comment.
Overall looks good, thanks for the investigation and thorough fix @sunchao!
wgtmac
approved these changes
Oct 8, 2026
wgtmac
reviewed
Oct 8, 2026
This file contains hidden or 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
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.
Why are the changes needed?
The Hadoop vectored-read path can fall back to ordinary reads after filesystem
requests have been submitted or results partially consumed. Replaying reads into
the same chunk builder can duplicate selected page data and return incorrect
filtered rows. Outstanding requests also need to retain ownership of their stream
and buffers during failure cleanup.
The current path requests a single buffer for an entire contiguous projected
range, without splitting it at
parquet.read.allocation.size.What changes were proposed in this PR?
logical page/column plan. Verify dictionary and V1/V2 page checksums across split
buffers without another contiguous checksum allocation.
submission disables vectored IO for that reader and permits ordinary reads.
filesystem rejection; the interface cannot reliably distinguish rejection from
partial submission.
once through the JVM system property
parquet.hadoop.vectored.io.threads.Admission, submission, and range waits share one 300-second deadline. Admission
failure leaves the stream with its reader. Accepted operations retain capacity
until submission exits, the caller transfers or abandons buffer ownership, and
failure cleanup finishes. Cancellation cannot free capacity prematurely.
Idle workers expire after 60 seconds.
withVectoredReadExecutor(new VectoredReadExecutor(executor, maximumConcurrentReads))on read options or the
ParquetReaderbuilder. Reuse the wrapper to share anadmission limit across readers; both base and Hadoop options preserve it when
copied. The existing JVM pool remains the default. Parquet never shuts down a
caller-owned pool. Reject inline execution before filesystem submission and
preserve the reader for retry.
and transfer them to the row-group releaser on success. Preserve automatic
row-group cleanup on sequential reads and reader closure.
readers and defer stream cleanup until submission exits; release buffers only
when completion can be established. Guarantee abort with
finallyeven whendraining sibling reads throws an
Error, and retain the original failure assuppressed on the escaping error. Remove the unused CRC allocator.
The allocation setting bounds requested range lengths, not every filesystem
allocation: backends may merge ranges or align them for checksums and allocate
larger buffers. It does not bound footer/decoder allocations or total memory.
The worker limit does not bound threads created by the filesystem. A backend
that ignores interruption, or a blocked stream close, can retain capacity
indefinitely; when all workers are occupied, further reads time out at admission.
Missing, cancelled, or never-completing result futures can prevent safe buffer
reclamation. Caller-supplied executors must dispatch asynchronously, reject instead
of silently dropping work, and drain accepted tasks during shutdown. Reading
threads must not exhaust that same pool while waiting for their vectored work.
How was this PR tested?
Validated the current implementation on Java 17.0.20.1 with Maven 3.9.16 and
Thrift 0.24.0:
parquet-hadoopverification with default Hadoop 3.3.0: 877 tests,zero failures/errors, 26 Hadoop-capability skips, plus 17 integration tests
with zero failures/errors/skips. Packaging, API compatibility, dependency
analysis, Spotless, and RAT (274 approved licenses) passed.
-Dparquet.hadoop.vectored.io.threads=2:118 tests, zero failures/errors/skips, including the real local-filesystem
vectored and file-range bridge tests.
git diff --checkpassed.The focused run selected
TestParquetFileReaderVectoredIO,TestParquetFileReaderVectoredOwnership,TestVectoredReadBufferAllocator,TestVectoredReadOperation,TestVectoredReadExecutor,TestDataPageChecksums,TestVectorIoBridge,TestFileRangeBridge,TestParquetFileReaderBufferLeak.Coverage includes shared admission across repeated retries, capacity retained
through blocked submission and cleanup, original-buffer ownership, interrupted
admission and reader reuse, queued aborts, and sibling errors with pending reads.
New regressions exercise caller-owned pools through both options builders and
copies, successful reads and error cleanup without pool shutdown, inline and
CallerRunsPolicyrejection, permit recovery, and retrying the same reader afterinline rejection.
The full unit suite used a RAM-backed temporary directory for its large-file tests.
Local setup also used a test-JVM hosts file for the machine hostname and interop
fixtures fetched at the commits pinned in the tests. No live S3 validation or
performance benchmark was performed.
Closes #3719.