Skip to content

Core: Add Eager fetch to parquet read path - #17284

Merged
RussellSpitzer merged 11 commits into
apache:mainfrom
varun-lakhyani:manifest-eagerinputfile
Sep 1, 2026
Merged

Core: Add Eager fetch to parquet read path#17284
RussellSpitzer merged 11 commits into
apache:mainfrom
varun-lakhyani:manifest-eagerinputfile

Conversation

@varun-lakhyani

@varun-lakhyanivarun-lakhyani commented Jul 18, 2026

Copy link
Copy Markdown
Contributor

Summary

Wire Eager Fetch (EagerInputFile/EagerInputStream) into Parquet read path so any Parquet read (Manifest, Datafile etc) ≤ 1 MB are read in a single S3 GET instead of three (footer length → footer → row group).

EagerInputFile Implementation#16729
It trusts the supplied (recorded) file length for paruqet - breaks when it doesn't match the real length.

Behavior change for HadoopFileIO

For Parquet reads through HadoopFileIO, files at or below the 1 MiB eager fetch threshold now trust and use the length reported by InputFile.getLength(), which may have been initialized from Iceberg metadata rather than retrieving the physical file size through HadoopInputFile.getStat().

Previously, Hadoop backed reads could tolerate an incorrect metadata length because the physical file size was returned by getStat(). Other implementations such as S3InputFile already trust the metadata provided length when available. This change makes the eager Hadoop path consistent with all those implementations but an incorrect metadata length can now cause the eager read to fail - Eager checks if reported length is equal to actual, if not fails at EagerInputFile.newStream for both cases( file is longer/shorter than reported length).

Why

Detailed Benchmark setup and results comment below

  • S3 GET requests from 3 → 1 per manifest read
  • Improves manifest read by ~25–41% for Parquet manifests
  • ~55% reduction in read time (Benchmarked independently by @vaquarkhan)

Benchmark methodology, JMH output, raw S3 access logs - https://github.com/varun-lakhyani/iceberg-manifest-eagerpath-benchmark/blob/main/README.md

Benchmark for parquet datafile read

Reproducing

Fork this branch - change only the S3 location/region.
(Integration here is little different than actual as benchmark is using flag and just manifest rather than overall)

Run EagerFetchScalingBenchmark or ManifestBenchmark

@varun-lakhyanivarun-lakhyani changed the title Use EagerInputFile in Manifest reader pathCore: Use EagerInputFile in Manifest reader pathJul 18, 2026
@varun-lakhyani

varun-lakhyani commented Jul 20, 2026

Copy link
Copy Markdown
ContributorAuthor

Benchmark Setup (JMH)

Machine: AWS EC2 (same region as the S3 bucket to minimize network latency)

PropertyValue
Instance typer5.4xlarge
vCPUs16
Memory128 GB
NetworkUp to 10 Gbps
Storage50 GB gp3 EBS
AMIAmazon Linux 2023
Regionap-south-1 (Mumbai)

Benchmark:ManifestBenchmark

  • Warmup iterations: 6
  • Measurement iterations: 10
  • Entry Counts: 37500

Results

Parquet v4 (Non-Partitioned)

Number of ColumnsDefault (s/op)Eager Fetch (s/op)Latency Reduction
100.1420.10029.6%
500.1340.10025.4%
1000.1380.10126.8%

Parquet v4 (Partitioned)

Number of ColumnsDefault (s/op)Eager Fetch (s/op)Latency Reduction
100.1610.09441.6%
500.1700.10041.2%
1000.1340.09628.4%

AVRO v4 (Non-Partitioned)

Number of ColumnsDefault (s/op)Eager Fetch (s/op)Latency Reduction
100.0860.095-10.5%
500.0860.0824.7%
1000.0800.0765.0%

AVRO v4 (Partitioned)

Number of ColumnsDefault (s/op)Eager Fetch (s/op)Latency Reduction
100.0920.095-3.3%
500.0860.0860.0%
1000.0940.0886.4%

Graphical Comparison

manifest_grouped_bars

S3 Request Analysis

S3 access logs confirm 3 GETs/read (default) → 1 GET/read (eager)

@varun-lakhyanivarun-lakhyani changed the title Core: Use EagerInputFile in Manifest reader pathCore: Add EagerInputFile in Manifest reader pathJul 20, 2026
return contentCache(io).tryCache(input);
}

if (eagerFetchEnabled(io)) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Let's get some integration tests on this to make sure that when our parameter is enabled it is actually using eager

@varun-lakhyanivarun-lakhyaniJul 28, 2026

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Added in TestManifestReader - 2 tests

  • verifies it routes to EagerInputFile based on flag and size threshold (required making newInputFile package-private)
  • verifies manifest entries are read back correctly when eager fetch is enabled

*/
public static final String IO_MANIFEST_EAGER_FETCH_ENABLED = "io.manifest.eager-fetch-enabled";

public static final boolean IO_MANIFEST_EAGER_FETCH_ENABLED_DEFAULT = false;

@varun-lakhyanivarun-lakhyaniJul 28, 2026

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I think we can make this default true, It would be great improvement for v4 parquet manifest.
I would love to hear thoughts on this.

@vaquarkhan

vaquarkhan commented Jul 29, 2026

Copy link
Copy Markdown

Thanks @varun-lakhyani for the thorough benchmark, the graph, and especially the S3 GET breakdown (3 -> 1). That
request count is the strongest part of the story since it does not depend on the machine.

I reproduced this on my own account (EC2 r7i.2xlarge, us-east-1, S3, JDK 17, same JMH warmup and measurement settings) and the direction holds cleanly:

  • Parquet v4, numCols=50, non-partitioned: 0.141 -> 0.063 s/op with eager on.
  • Avro v4: 0.060 -> 0.061 s/op, no change.

My baseline matches yours closely (0.141 vs your 0.134). My eager number is lower (0.063 vs 0.100),most likely because of the newer instance: the eager path reads the whole file into memory and serves from RAM, so it gains from faster CPU and memory, while the network-bound default path stays about the same. Different hardware, same conclusion.

image

I also ran one extra benchmark, same read path, adding a local filesystem and a size sweep across the 1 MB threshold:

  1. Local filesystem: eager gives no benefit, and for a small Parquet manifest it looked slightly slower (within noise). No round trip to collapse locally, so this is expected. This is why I would keep it behind the flag rather than always on.

  2. Size sweep: below 1 MB the S3 Parquet win holds (~52-53%); above 1 MB it correctly does not fire.
    The 1 MB cap behaves as intended.

  3. Avro vs Parquet baseline (eager off): on S3, a Parquet manifest read is about 1.9x-2.4x slower than an Avro manifest of the same size (non-overlapping error bars). On local disk there is no such gap. With eager on, the S3 Parquet read comes back down close to Avro. So eager fetch is really closing the object-store Parquet penalty, which lines up with your 3 -> 1 GET result.

image

Net: a solid win for Parquet manifests on object storage under the 1 MB cap, little or nothing for Avro or on local storage. I would keep the default off, or scope any future default to object store plus Parquet under the cap, rather than always on.

One reproducibility note: the benchmark branch did not build/run for me as-is. The iceberg-core jmh source set has no iceberg-aws dependency and the AWS SDK is compile-only, so :iceberg-core:jmh fails to compile and then at runtime with missing AWS classes. I added iceberg-aws plus the SDK to the jmh classpath to get it running; might be worth including so others can reproduce.

I'll share a Google Doc with the full numbers and charts, this weekend

vaquarkhan added a commit to vaquarkhan/iceberg that referenced this pull request Jul 29, 2026
Add a JMH benchmark that extends the manifest EagerInputFile work in PR apache#17284
with two dimensions the existing benchmark does not cover:
- storage: local filesystem (HadoopFileIO) vs S3 (S3FileIO)
- manifest size: a sweep that straddles the fixed 1 MB eager-fetch gate
It uses the real read path (io.manifest.eager-fetch-enabled set on the FileIO,
so ManifestFiles.read wraps the InputFile in EagerInputFile) and reuses
ManifestBenchmarkUtil to build the manifests. The measured manifest byte size
is printed per trial so the size-to-gate mapping is verified, not assumed.
Add iceberg-aws and the AWS SDK to the iceberg-core jmh classpath so the
S3FileIO-based manifest benchmarks compile and run.
@vaquarkhan

Copy link
Copy Markdown

Quick follow-up: @varun-lakhyani I pushed that extra benchmark as a commit here (EagerFetchScalingBenchmark plus the small build.gradle change so the jmh module picks up the AWS SDK). It reuses your ManifestBenchmarkUtil and the same read path.

I should have checked with you first before pushing, apologies if I got ahead of it.

Totally your call: keep it in this PR, or I can pull it out and send it as a separate follow-up PR, whichever you prefer. Full results and charts in a shared doc this weekend.

@RussellSpitzer

Copy link
Copy Markdown
Member

@vaquarkhan Thanks for the review here. I'd like to get more community feedback here but I'm thinking we should just have this default to always on with these kind of results. Right now it only applies for parquet manifests in V4 so we may want to disable it for V3 and below or check whether the file is parquet first? I'm not sure if that complexity really makes a difference though.

@vaquarkhan

Copy link
Copy Markdown

@RussellSpitzer , This makes sense. One observation from the numbers: as written the gate is size-based (<=1 MB),
not format-based, so "always on" would also wrap Avro and local-filesystem reads, where I saw no benefit and it still buffers the whole manifest into memory during planning. So our "check whether it's Parquet first" is the right call and matches the data.

That Parquet check also covers V3 and below on its own, since those are Avro-only, and it handles mixed manifests in a V4 table correctly, so I don't think a separate version check is needed.

Only caution: local filesystem showed no gain even for Parquet, and my numbers are single-shot on one instance, so a slightly broader run would be worth it before flipping the default. Happy to help validate.

varun-lakhyani pushed a commit to varun-lakhyani/iceberg that referenced this pull request Jul 30, 2026
Add a JMH benchmark that extends the manifest EagerInputFile work in PR apache#17284
with two dimensions the existing benchmark does not cover:
- storage: local filesystem (HadoopFileIO) vs S3 (S3FileIO)
- manifest size: a sweep that straddles the fixed 1 MB eager-fetch gate
It uses the real read path (io.manifest.eager-fetch-enabled set on the FileIO,
so ManifestFiles.read wraps the InputFile in EagerInputFile) and reuses
ManifestBenchmarkUtil to build the manifests. The measured manifest byte size
is printed per trial so the size-to-gate mapping is verified, not assumed.
Add iceberg-aws and the AWS SDK to the iceberg-core jmh classpath so the
S3FileIO-based manifest benchmarks compile and run.
@varun-lakhyani

varun-lakhyani commented Jul 30, 2026

Copy link
Copy Markdown
ContributorAuthor

Thanks @vaquarkhan for the review and extensive Benchmark run.
Agreed! numbers are almost similar to my run, although I ran for v1-v4 Avro and v4 Parquet but not for local.
I posted my JMH results and s3 bucket logs which conclusively shows 3 -> 1 GET calls on S3 for reference https://github.com/varun-lakhyani/iceberg-manifest-eagerpath-benchmark/blob/main/README.md

I am hoping the see community's view on keeping it default true.
Avro path showed no benefit as expected and local run also showed no result or little overhead as expected.
Let's see if we need some checks along with flag so it doesn't use this eagerPath in local or for Avro

Also, I pushed your commit to the benchmark branch anyone can fork it and run the benchmark as is, included the aws dependencies so its easy for folks to just run the benchmarks as is. Thanks

@kevinjqliu
kevinjqliu self-requested a review August 3, 2026 16:14
@RussellSpitzerRussellSpitzer added this to the Iceberg 1.12.0 milestone Aug 10, 2026

@anoopjanoopj left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please note that v4 manifest readers don't follow the ManifestFiles path, so the PR description is incorrect. We can add this to the v4 reader when #17541 is merged.

@RussellSpitzer

Copy link
Copy Markdown
Member

Please note that v4 manifest readers don't follow the ManifestFiles path, so the PR description is incorrect. We can add this to the v4 reader when #17541 is merged.

We had a discussion on the mailing list, and we are going to rework this to apply at the Parquet File Reader level so it just applies to all Parquet reads so the title will need a bit more reworking but we'll also cover everything.

@varun-lakhyanivarun-lakhyani changed the title Core: Add EagerInputFile in Manifest reader pathCore: Add Eager fetch to parquet read pathAug 13, 2026
@varun-lakhyanivarun-lakhyani changed the title Core: Add Eager fetch to parquet read path[WIP] Core: Add Eager fetch to parquet read pathAug 13, 2026
@varun-lakhyanivarun-lakhyani changed the title [WIP] Core: Add Eager fetch to parquet read pathCore: [WIP] Add Eager fetch to parquet read pathAug 13, 2026
@varun-lakhyani

varun-lakhyani commented Aug 13, 2026

Copy link
Copy Markdown
ContributorAuthor

Yes a bit delay from my end, Updated title and description.
I am working on it, will update as soon I finish and push the changes, mostly 1-2 days.

@RussellSpitzer

Copy link
Copy Markdown
Member

There are some weird failures in https://github.com/apache/iceberg/actions/runs/31809741067/job/94797306211?pr=17284

I'm worried this is a downstream effect of our changes, Can you please check those our @varun-lakhyani

@@ -144,6 +145,12 @@ private Parquet() {}
"parquet.read.support.class",
"parquet.crypto.factory.class");

/*
* Size threshold (bytes) at or below which a Parquet file is fetched eagerly on the first read.
* Fixed at 1 MB - not user configurable

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This line of the comment isn't really needed :)

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

fixed

@varun-lakhyani

Copy link
Copy Markdown
ContributorAuthor

There are some weird failures in https://github.com/apache/iceberg/actions/runs/31809741067/job/94797306211?pr=17284

I'm worried this is a downstream effect of our changes, Can you please check those our @varun-lakhyani

yes working on it.

@RussellSpitzerRussellSpitzer left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Minor nits, let's merge that other pr for fixing rewritePaths then we can merge this.

@eymetmaria-ops

eymetmaria-ops commented Aug 24, 2026 via email

Copy link
Copy Markdown

@RussellSpitzer

Copy link
Copy Markdown
Member

@kevinjqliu I see you self asked for a review, did you still want to take a pass?

private ReadBuilder(InputFile file) {
this.file = file;
long fileLength = file.getLength();
this.file = canEagerFetch(fileLength) ? EagerInputFile.of(file, fileLength) : file;

@kevinjqliukevinjqliuAug 26, 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.

Codex helped me flag this issue. In summary, S3/GCS/ADLS already accepts and use the (potentially staled) length. Hadoop was using getStat() to get the length directly. So this is a new regression for Hadoop.

Details

For small Hadoop-backed Parquet reads, file.getLength() may come from the manifest-list entry and can be stale. Previously, Hadoop reads obtained the physical size through:

HadoopInputFile.fromStatus(hfile.getStat(), hfile.getConf());

See ParquetIO.file().

This PR wraps files up to 1 MiB using the reported length:

longfileLength = file.getLength();
this.file = canEagerFetch(fileLength) ? EagerInputFile.of(file, fileLength) : file;

The eager reader then reads exactly that many bytes:

byte[] bytes = newbyte[(int) length];
IOUtil.readFully(src, bytes, 0, bytes.length);

If the recorded length is smaller than the physical file, the buffer excludes the real Parquet footer and trailing PAR1 magic:

java.lang.RuntimeException: ... is not a Parquet file.
Expected magic number at tail, but found [0, 80, 65, 82]
at org.apache.parquet.hadoop.ParquetFileReader.readFooter(ParquetFileReader.java:622)
at org.apache.iceberg.parquet.ReadConf.newReader(ReadConf.java:194)
at org.apache.iceberg.parquet.ParquetReader.init(ParquetReader.java:74)

If the recorded length is larger, readFully attempts to read past EOF.

#16910 corrects new complete rewrites, but it does not repair existing affected manifest-list entries and still preserves source lengths for manifests carried over by incremental rewrites.

This PR therefore removes Hadoop's existing protection against stale lengths by wrapping HadoopInputFile before ParquetIO can call getStat(). Other input implementations already trusted their reported lengths and are not newly broken.

Please preserve the physical Hadoop FileStatus length before eager wrapping and add a stale-length regression test.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hadoop should not be protecting against this :) Honestly I think this is a misfeature. It exists to cover for bad implementations which store the wrong length, but leads to even weirder situations where HadoopFileIO would work reading a file via S3A that would fail through S3FileIO

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.

Since all the other FileIO implementations already trust the supplied length, I think its reasonable for HadoopFileIO to do this.

I think its worth calling that out, perhaps in the PR description. And perhaps with a unit test

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Also to note, this doesn't apply to manfiest-lists written for any currently valid tables since they are all using Avro for manifest list and manifests which do not break on the wrong length regardless

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

updated PR description to include this change for Hadoop and mentioned the trusting length part

@kevinjqliukevinjqliu left a comment

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.

LGTM! thanks for working on this.

private ReadBuilder(InputFile file) {
this.file = file;
long fileLength = file.getLength();
this.file = canEagerFetch(fileLength) ? EagerInputFile.of(file, fileLength) : file;

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.

Since all the other FileIO implementations already trust the supplied length, I think its reasonable for HadoopFileIO to do this.

I think its worth calling that out, perhaps in the PR description. And perhaps with a unit test

@anuragmantrianuragmantri left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I did a final pass and LGTM. Thanks @varun-lakhyani

@kevinjqliu

Copy link
Copy Markdown
Contributor

nit: how about something like this in the pr description? I want to make sure others (and perhaps myself) reading this PR in the future can quickly understand the behavior change.

Behavior change for HadoopFileIO

This PR changes small Parquet reads through HadoopFileIO to use the length reported by InputFile.getLength()—which may have been initialized from Iceberg metadata—rather than retrieving the physical file size through HadoopInputFile.getStat().

Previously, Hadoop-backed reads could tolerate an incorrect metadata length because getStat() returned the physical file size. Other implementations, such as S3InputFile, already use the metadata-provided length when available.

With eager fetching, files at or below the 1 MiB threshold use the reported length to size the read buffer, including files backed by HadoopInputFile. This makes Hadoop consistent with other InputFile implementations, but an incorrect metadata length that previously succeeded through HadoopFileIO may now cause the read to fail.

@varun-lakhyani

varun-lakhyani commented Aug 27, 2026

Copy link
Copy Markdown
ContributorAuthor

nit: how about something like this in the pr description? I want to make sure others (and perhaps myself) reading this PR in the future can quickly understand the behavior change.

Behavior change for HadoopFileIO

Updated the description - I believe it shows the change clearly now

Comment threadcore/src/main/java/org/apache/iceberg/io/EagerInputFile.java
IOUtil.readFully(src, bytes, 0, bytes.length);
// reads from the already open stream; no additional request
if (src.read() != -1) {
throw new IOException("Did not reach the end of stream after reading " + length + " bytes");

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit on this, the exception is not clear why this is an issue. "Incorrect length provided for file %x, given a length of %d and did not reach the end of stream."

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

done

super(delegate, length);
Preconditions.checkArgument(
delegate instanceof HadoopConfigurable,
"Cannot create a Hadoop configurable eager input file from %s",

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Need to explain why we can't create "Cannot create Hadoop Configurable Eager Input File because %s does not implement HadoopConfigurable" or whatever

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

donw

@RussellSpitzerRussellSpitzer left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Good call, waiting on @danielcweeks to make sure we covered the extra long files. I think it's a safe check that was added so I'm +1

@RussellSpitzer
RussellSpitzer merged commit 86da2dc into apache:mainSep 1, 2026
39 checks passed
@RussellSpitzer

Copy link
Copy Markdown
Member

Merged! Thanks @varun-lakhyani for all your work in this space and for helping us with your GSoC project. Thank you to all the folks who helped review this change, @danielcweeks , @anuragmantri , @kevinjqliu and @vaquarkhan

@varun-lakhyani

Copy link
Copy Markdown
ContributorAuthor

Thanks @RussellSpitzer , @danielcweeks , @anuragmantri , @kevinjqliu , @vaquarkhan for taking part in this change.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants

@varun-lakhyani@vaquarkhan@RussellSpitzer@anuragmantri@eymetmaria-ops@kevinjqliu@anoopj@danielcweeks