Skip to content

GH-3719: Fix vectored read allocation limits and fallback safety - #3726

Open
sunchao wants to merge 2 commits into
apache:masterfrom
sunchao:dev/chao/codex/gh-3719-vectored-read-safety
Open

GH-3719: Fix vectored read allocation limits and fallback safety#3726
sunchao wants to merge 2 commits into
apache:masterfrom
sunchao:dev/chao/codex/gh-3719-vectored-read-safety

Conversation

@sunchao

@sunchao sunchao commented Aug 16, 2026

Copy link
Copy Markdown
Member

Why are the changes needed?

Apache Parquet's Hadoop vectored-read path can fall back to ordinary reads after
asynchronous filesystem requests have already been submitted or partially
consumed. Retrying against the same partially populated chunk builder can
duplicate selected page data and return incorrect filtered rows. Outstanding
sibling requests can also continue using a stream while fallback or cleanup
begins.

The same path requests one buffer for an entire contiguous projected range
instead of splitting requests at parquet.read.allocation.size, which can cause
unexpectedly large allocations.

Vectored I/O is enabled by default in Apache Parquet 1.16 and later, so these
issues are not limited to applications that explicitly opt in.

What changes were proposed in this PR?

  • Split contiguous requested filesystem ranges at the configured allocation
    limit while preserving the existing logical page and column plan.
  • Keep ordinary fallback for failures detected while preparing vectored ranges;
    convert failures after the vectored submission call is attempted into
    IOException rather than replaying reads against a partially populated
    builder.
  • Await already-published sibling read futures within the existing shared
    timeout before propagating an asynchronous failure.
  • Verify dictionary pages and V1/V2 data-page checksums incrementally across
    split buffers instead of allocating another oversized contiguous buffer.
  • Add focused vectored-reader and checksum regression coverage.

The current patch still has four known review points that need follow-up:

  • Filesystems that align ranges for checksums can allocate more than the
    requested range size, so splitting requests alone does not guarantee that
    every backend allocation respects the configured cap.
  • A backend that rejects vectored I/O before submitting work should retain safe
    ordinary-read fallback; the current submission boundary cannot distinguish
    that case from a partially submitted failure.
  • Filesystems such as S3A can block while synchronously submitting more ranges
    than their in-flight limit permits, before the existing read timeout applies.
  • A backend can start reads and then fail before publishing their futures to
    the wrapper, leaving those reads invisible to the current cleanup path.

How was this PR tested?

Previously recorded results for this commit on Java 17:

mvn -pl parquet-hadoop \
  -Dtest=TestParquetFileReaderVectoredIO,TestDataPageChecksums \
  -Dsurefire.failIfNoSpecifiedTests=false \
  test

The focused suites reported 37 passing tests, including 19 vectored-reader
tests
and 18 checksum tests.

mvn -pl parquet-hadoop test
mvn -pl parquet-hadoop spotless:check

The complete module reported 748 tests, zero failures, zero errors, and 24
existing Hadoop-capability skips
. Apache RAT approved all 256 scanned
licenses, and Spotless passed. The four review points above are not covered by
those recorded passing tests.

Closes #3719.


final int maximumAllocation = options.getMaxAllocationSize();
Preconditions.checkArgument(maximumAllocation > 0, "Invalid maximum allocation size %s", maximumAllocation);
final long fileLength = file.getLength();

@dossett dossett Aug 17, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding getLength() to every vectored read could be expensive on object stores (unlike local HDFS).

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, addressed in ad623d6. HadoopInputFile already reads its cached FileStatus, but other InputFile implementations need not do so. I now cache the length lazily for vectored-range validation, so it adds at most one length lookup per reader instead of one per row-group read. Ordinary reads, including those with a supplied footer, do not acquire an extra lookup.

Added regression tests for repeated filtered/unfiltered reads and both footer paths. All 40 focused tests pass; the full parquet-hadoop module reports 751 tests, zero failures or errors, and 24 Hadoop-capability skips. Spotless and RAT also pass.

allParts.get(partIndex).readFromVectoredRanges(ranges.subList(firstRange, endRange), builder);
firstRange = endRange;
}
} catch (IllegalArgumentException | UnsupportedOperationException e) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An UnsupportedOperationException will now be rethrown as an IOExeption. If I understand correctly, on master this will now kill the read entirely instead of resulting in a warning and a fallback to non-vectored IO. If so, that seems like a meaningful change.

Maybe I'm missing something, or maybe this just wouldn't happen in practice?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is a deliberate and meaningful behavior change. Even a synchronous UnsupportedOperationException can follow partial submission, and the Hadoop bridge only copies the futures back to Parquet after submission returns. During result consumption, the chunk builder may also already contain data. Falling back in either situation can race outstanding reads or replay chunks and return incorrect rows.

Ordinary I/O still handles disabled/unavailable vectored I/O, and preparation-time IllegalArgumentException/UnsupportedOperationException still permits fallback. I clarified that boundary in ad623d6 and corrected the availability documentation: the Hadoop bridge checks runtime API/allocator support, not per-stream hasCapability. The tests cover ordinary-read selection plus synchronous, asynchronous, and partial-submission failures without fallback.

I do not have an observed production rejection from an otherwise supported backend. A genuinely pre-submission rejection should be recoverable, but the current interface does not reliably distinguish it from partial submission. Restoring broader fallback would need an explicit no-work-submitted guarantee; I have kept the conservative behavior for now.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the thoughtful reply. I'll defer to others more knowledgeable than myself about the tradeoffs here, but I've learned a lot.

@sunchao

sunchao commented Aug 18, 2026

Copy link
Copy Markdown
Member Author

cc @wgtmac can you take a look?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Vectored Parquet reads can fall back unsafely after partial asynchronous reads and exceed allocation limits

2 participants