Skip to content

Fix Iceberg S3 PerfIO access with split classloaders - #15435

Open
liurenjie1024 wants to merge 6 commits into
NVIDIA:release/26.08from
liurenjie1024:ray/nvbugs-6535700
Open

Fix Iceberg S3 PerfIO access with split classloaders#15435
liurenjie1024 wants to merge 6 commits into
NVIDIA:release/26.08from
liurenjie1024:ray/nvbugs-6535700

Conversation

@liurenjie1024

@liurenjie1024 liurenjie1024 commented Jul 30, 2026

Copy link
Copy Markdown
Collaborator

Fixes NVBUGS-6535700.

Description

#15360 reintroduced an IllegalAccessError when Iceberg runtime jars are loaded through spark.driver.extraClassPath and spark.executor.extraClassPath.

Iceberg’s BaseS3File and S3URI classes are package-private. JVM runtime-package identity includes both the package name and defining classloader, so the shim-loaded IcebergS3InputFile cannot access those classes when the Iceberg runtime is loaded by the app classloader.

This change:

  • Restores the root-loadable IcebergS3InputFileAccess bridge.
  • Keeps all access to BaseS3File and S3URI inside that bridge.
  • Adds comments documenting the classloader requirement and why the access must remain isolated.
  • Restores the accessor to unshimmed-common-from-single-shim.txt.
  • Returns raw S3 bucket and key values from the accessor, preserving Fix double escaping of Iceberg S3 input-file URIs #15360’s fix for URI double escaping.

Non-S3 inputs and unsupported S3 client configurations continue to fall back to the standard Iceberg input path.

Checklists

Documentation

  • Updated for new or modified user-facing features or behaviors
  • No user-facing change

Testing

  • Added or modified tests to cover new code paths
  • Covered by existing tests
    (test_iceberg_parquet_read_from_uri_invalid_s3_path and the Iceberg read/write integration suite.)
  • Not required

Performance

  • Tests ran and results are added in the PR description
  • Issue filed with a link in the PR description
  • Not required

Signed-off-by: Renjie Liu liurenjie2008@gmail.com

@liurenjie1024
liurenjie1024 requested a review from a team as a code owner July 30, 2026 03:39
Signed-off-by: Ray Liu <liurenjie2008@gmail.com>
@greptile-apps

greptile-apps Bot commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

Restores split-classloader-safe access to Iceberg’s package-private S3 APIs.

  • Adds a root-loadable bridge that extracts raw S3 bucket and key values without URI re-encoding.
  • Routes IcebergS3InputFile through the bridge while preserving fallback for non-S3 inputs.
  • Adds a targeted extra-classpath CI invocation and fixes explicit Scala eta expansion at the PerfIO call site.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename Overview
iceberg/common/src/main/java/org/apache/iceberg/aws/s3/IcebergS3InputFileAccess.java Introduces the root-loadable bridge that contains all package-private Iceberg S3 access.
iceberg/common/src/main/java/org/apache/iceberg/aws/s3/IcebergS3InputFile.java Uses the bridge-provided raw bucket and key while retaining existing fallback behavior.
dist/unshimmed-common-from-single-shim.txt Promotes the access bridge into the root-loadable distribution layer.
jenkins/spark-premerge-build.sh Adds an Iceberg runtime extra-classpath integration-test invocation.
sql-plugin/src/main/scala/com/nvidia/spark/rapids/parquet/GpuParquetScan.scala Explicitly eta-expands the Parquet magic verification method at the PerfIO boundary.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart LR
    App[App classloader] --> Iceberg[Iceberg runtime classes]
    Root[Root-loaded bridge] --> Iceberg
    Shim[Shim-loaded IcebergS3InputFile] --> Root
    Root --> Values[Raw S3 bucket and key]
    Values --> PerfIO[S3 PerfIO range reader]
Loading

Reviews (4): Last reviewed commit: "Fix build break" | Re-trigger Greptile

@sameerz sameerz added the bug Something isn't working label Jul 30, 2026

@gerashegalov gerashegalov 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.

This is a good PR to add a test per #14959 to prevent regressions.

@liurenjie1024

Copy link
Copy Markdown
Collaborator Author

build

@liurenjie1024

Copy link
Copy Markdown
Collaborator Author

This is a good PR to add a test per #14959 to prevent regressions.

Fixed

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

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants