Skip to content

Core: Add EagerInputFile in Manifest reader path#17284

Open
varun-lakhyani wants to merge 2 commits into
apache:mainfrom
varun-lakhyani:manifest-eagerinputfile
Open

Core: Add EagerInputFile in Manifest reader path#17284
varun-lakhyani wants to merge 2 commits into
apache:mainfrom
varun-lakhyani:manifest-eagerinputfile

Conversation

@varun-lakhyani

@varun-lakhyani varun-lakhyani commented Jul 18, 2026

Copy link
Copy Markdown
Contributor

Using EagerInputFile in ManifestFiles.java in newInputFile for it to user EagerInputFile and EagerInputStream.
Using ManifestBenchmark.java to benchmark performance benefit from this changes

@github-actions github-actions Bot added the core label Jul 18, 2026
@varun-lakhyani varun-lakhyani changed the title Use EagerInputFile in Manifest reader path Core: Use EagerInputFile in Manifest reader path Jul 18, 2026
@varun-lakhyani

Copy link
Copy Markdown
Contributor Author

While reviewing this PR, a concern from the discussion on a similar PR: #4518 (comment) should also be revisited here too.
But I think #16729 already does it correctly just a fresh look at this will be worth it.

  • Unbounded byte[] allocation: guarded in two places. ManifestFiles.canEagerFetch only wraps files with 0 < len <= EAGER_FETCH_THRESHOLD_BYTES (currently a fixed 1 MB constant, intentionally not user-configurable), and EagerInputFile additionally validates that 0 <= fileSize <= Integer.MAX_VALUE before allocating the buffer. The maximum allocation is therefore bounded at ~1 MB.

  • Incorrect reported file size (both values come from manifest.length(), so this is about robustness rather than an expected scenario):

    • reported < actual: eager fetching returns a truncated view. For Parquet, this fails the same way a truncated or corrupt file does today, since the footer is no longer where the reported file length says it should be.
    • reported > actual: IOUtil.readFully reaches EOF before filling the buffer, causing the read to fail immediately with an EOFException, which is surfaced as a RuntimeIOException. No partial data is returned.

So I don't think eager fetching introduces any new failure modes here; it either behaves the same as the existing path or fails immediately when the metadata is inconsistent.

Just need a confirmation on here.

@varun-lakhyani

Copy link
Copy Markdown
Contributor Author

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%

Graphical Comparison

manifest_grouped_bars

S3 Request Analysis

The number of S3 GET requests per manifest read is computed as: (Used S3 bucket logs)

Total GET Requests / Total Benchmark Iterations

Configuration GET Requests / Read Request Reduction
Default 3
Eager Fetch 1 67%

@varun-lakhyani varun-lakhyani changed the title Core: Use EagerInputFile in Manifest reader path Core: Add EagerInputFile in Manifest reader path Jul 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant