Core: Add Eager fetch to parquet read path

[varun-lakhyani]

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

• ~39%-42% improvement against default behaviour
  for reference Detailed Datafile eager fetch Benchmark results

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-lakhyani]

Benchmark Setup (JMH)

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

Property	Value
Instance type	r5.4xlarge
vCPUs	16
Memory	128 GB
Network	Up to 10 Gbps
Storage	50 GB gp3 EBS
AMI	Amazon Linux 2023
Region	ap-south-1 (Mumbai)

Benchmark: ManifestBenchmark

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

Results

Parquet v4 (Non-Partitioned)

Number of Columns	Default (s/op)	Eager Fetch (s/op)	Latency Reduction
10	0.142	0.100	29.6%
50	0.134	0.100	25.4%
100	0.138	0.101	26.8%

Parquet v4 (Partitioned)

Number of Columns	Default (s/op)	Eager Fetch (s/op)	Latency Reduction
10	0.161	0.094	41.6%
50	0.170	0.100	41.2%
100	0.134	0.096	28.4%

AVRO v4 (Non-Partitioned)

Number of Columns	Default (s/op)	Eager Fetch (s/op)	Latency Reduction
10	0.086	0.095	-10.5%
50	0.086	0.082	4.7%
100	0.080	0.076	5.0%

AVRO v4 (Partitioned)

Number of Columns	Default (s/op)	Eager Fetch (s/op)	Latency Reduction
10	0.092	0.095	-3.3%
50	0.086	0.086	0.0%
100	0.094	0.088	6.4%

Graphical Comparison

S3 Request Analysis

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

[RussellSpitzer]

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

[varun-lakhyani]

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

[varun-lakhyani]

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]

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.

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.

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]

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]

@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]

@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]

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

[anoopj]

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]

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-lakhyani]

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]

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

[RussellSpitzer]

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

[varun-lakhyani]

fixed

[varun-lakhyani]

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.

[varun-lakhyani]

I believe #16910 is getting close so this PR is ready for review.

[RussellSpitzer]

Probably better that we just pass through the delegate and call getConf on it directly.

[varun-lakhyani]

done updated in latest commit

[RussellSpitzer]

Let's fail if someone calls this if we are leaving it a noop.

[varun-lakhyani]

done updated here and in hadoopinputfile throwing UnsupportedOperationException

[RussellSpitzer]

Not sure we need this spy

[varun-lakhyani]

we are using it for

Mockito.doAnswer(
            invocation -> {
              SeekableInputStream spy =
                  Mockito.spy((SeekableInputStream) invocation.callRealMethod());
              opened.add(spy);
              return spy;
            })
        .when(delegate)
        .newStream();

To get spy on these streams we are using this doAnswer which expects .when(delegate) to be mock/spy.
So probably good to keep it unless there could be minimal setup to mock streams which i might have missed.

[RussellSpitzer]

openedStreams

[varun-lakhyani]

done in latest commit

[RussellSpitzer]

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

[eymetmaria-ops]

Anything I can help there automatic ai query in GitHub [image0.png] [image1.png] [image2.png] Eymet Maria On 24 Aug 2026, at 17:34, Russell Spitzer ***@***.***> wrote:  @RussellSpitzer commented on this pull request.

________________________________ In core/src/main/java/org/apache/iceberg/io/EagerInputFile.java<#17284 (comment)>:

+ private final Configuration conf;

+ + EagerInputFileConfigurable(InputFile delegate, long length, Configuration conf) { + super(delegate, length); + this.conf = conf; + } + + @OverRide + public Configuration getConf() { + return conf; + } + + @OverRide + public void serializeConfWith( + Function<Configuration, SerializableSupplier<Configuration>> confSerializer) { + // no-op: EagerInputFile is not serialized Let's fail if someone calls this if we are leaving it a noop. — Reply to this email directly, view it on GitHub<#17284?email_source=notifications&email_token=CD3ZWNV5LW7CL2G6VQWVBMT5LROARA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTKMBQHE3DOOBVHA32M4TFMFZW63VKON2WE43DOJUWEZLEUVSXMZLOOSWGM33PORSXEX3DNRUWG2Y#pullrequestreview-5009678587>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/CD3ZWNT44QJCT4U2DHOBG535LROARAVCNFSNUABFKJSXA33TNF2G64TZHMYTKOBSGU3DINZZHNEXG43VMU5TIOJRG43TGOBRHAYKC5QC>. You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

[RussellSpitzer]

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

[kevinjqliu]

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:

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

The eager reader then reads exactly that many bytes:

byte[] bytes = new byte[(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.

[RussellSpitzer]

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

[kevinjqliu]

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

[RussellSpitzer]

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

[varun-lakhyani]

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

[kevinjqliu]

LGTM! thanks for working on this.

[kevinjqliu]

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

[anuragmantri]

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

[kevinjqliu]

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]

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

[danielcweeks]

I think we actually have an issue here. We need to test to see if we hit EOF here to ensure that we've read the full content. If there's a concern that the file size is stale, we shouldn't blindly copy the bytes and assume it's correct. We should at lease validate we're at the end of the stream.

I'm not sure what we can do if the file size is smaller than the recorded length because we'll hit the IO exception, but we should at minimum handle the case that it's longer.

[varun-lakhyani]

Done b700403,
smaller than the recorded length already throws from IOUtil.readFully. Eager now also throws when the file is larger by reading one additional byte that must be -1. No new request — it's served from the already open stream.
It also adds a small unit test @kevinjqliu

[varun-lakhyani]

iceberg/core/src/main/java/org/apache/iceberg/io/EagerInputFile.java

Lines 77 to 81 in b17b679

	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");
	}

If this seems good, I would want to replace this with readFullyExpectingEof and add it to IOUtil.
it will look like this in IOUtil:

static void readFullyExpectingEof(InputStream stream, byte[] bytes, int offset, int length)
    throws IOException {
  readFully(stream, bytes, offset, length);
  if (stream.read() != -1) {
    throw new IOException("Did not reach the end of stream after reading " + length + " bytes");
  }
}

IOUtil seems a suitable place for this logic, as it is verifying the EOF after reading which is independent of Eager logic. Would you prefer this, or keep it inline here?