Fix double escaping of Iceberg S3 input-file URIs by liurenjie1024 · Pull Request #15360 · NVIDIA/cudf-spark

liurenjie1024 · 2026-07-23T09:07:38Z

Description

Iceberg preserves URL escapes in S3URI.key(). When the RAPIDS plugin converts
an Iceberg S3InputFile to a Java URI, it previously passed that key to a
component-based URI constructor. As a result, % was escaped again: for
example, an object key containing %3A was converted to %253A, and the
plugin attempted to read a different S3 object.

Build the URI from the complete S3 location instead of supplying the already
escaped key as a URI path component. This preserves Iceberg's original escaped
S3 key and allows the RAPIDS Iceberg S3 reader to access the intended object.

The change does not introduce user-facing configuration or behavior beyond
correctly reading existing URL-escaped S3 paths.

The existing Iceberg integration test
test_iceberg_parquet_read_from_url_encoded_path covers URL-escaped partition
paths. The direct-S3 reproduction runs its PERFILE parameterization against
an S3-backed Iceberg catalog, which exercises IcebergS3InputFileAccess.

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
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: Ray Liu <liurenjie2008@gmail.com>

greptile-apps · 2026-07-23T09:11:03Z

Greptile Summary

This PR fixes a double-escaping bug in the RAPIDS Iceberg S3 reader: the old IcebergS3InputFileAccess helper used a component-based java.net.URI constructor that re-encoded an already-escaped S3 key (e.g. %3A → %253A), causing reads to target the wrong S3 object. The fix eliminates the intermediate URI object and passes the raw bucket and key strings from Iceberg's own S3URI directly to the range copier.

Core change (IcebergS3InputFile.java): replaces URI s3Uri field with String s3Bucket / String s3Key extracted via BaseS3File.uri().bucket() / .key(), and removes the now-deleted IcebergS3InputFileAccess helper class entirely.
New integration test (iceberg_test.py): test_iceberg_parquet_read_from_uri_invalid_s3_path creates an Iceberg table whose data path contains a raw space (valid S3 key, invalid URI character) and verifies GPU/CPU result parity; gated on is_iceberg_rest_catalog().
CI (spark-tests.sh): adds PYSP_TEST_spark_rapids_perfio_s3_enabled=true as a startup env var so the new test exercises the PerfIO S3 code path.

Confidence Score: 5/5

Safe to merge: the change is a targeted, mechanical replacement of an intermediate URI object with raw bucket+key strings, removing the only site that caused double-escaping.

The fix is minimal and directly addresses the root cause — the component-based URI constructor that re-encoded already-escaped keys. The deleted class had no other callers, the new code stays in the same package so the package-private BaseS3File.uri() cast remains valid, and inputFile.location() is the correct string to pass to resolveClient. A companion integration test exercises the corrected path with a URI-invalid character in the key. No GPU allocation, retry, or shim logic is touched.

Files Needing Attention: No files require special attention.

Important Files Changed

Filename	Overview
iceberg/common/src/main/java/org/apache/iceberg/aws/s3/IcebergS3InputFile.java	Core fix: replaces URI-based S3 address (which caused double-escaping of %-encoded keys) with direct bucket+key strings extracted from Iceberg's S3URI; logic is correct and the instanceof guard is preserved.
iceberg/common/src/main/java/org/apache/iceberg/aws/s3/IcebergS3InputFileAccess.java	Deleted; was the only source of the double-escaping URI construction; no references remain in the codebase.
integration_tests/src/main/python/iceberg/iceberg_test.py	Adds test_iceberg_parquet_read_from_uri_invalid_s3_path covering S3 keys with raw spaces (URI-invalid chars); gated on is_iceberg_rest_catalog() and uses assert_gpu_and_cpu_are_equal_collect correctly.
jenkins/spark-tests.sh	Adds PYSP_TEST_spark_rapids_perfio_s3_enabled=true so the new S3 path test runs with PerfIO enabled in CI; updates comment to reflect both startup-only configs.
dist/unshimmed-common-from-single-shim.txt	Removes IcebergS3InputFileAccess.class entry, consistent with the class deletion.

Sequence Diagram

sequenceDiagram
    participant IC as IcebergS3InputFile
    participant BS as BaseS3File (Iceberg)
    participant S3U as S3URI
    participant RC as IcebergS3RangeCopier

    Note over IC: Before fix (double-escape path)
    IC->>BS: instanceof BaseS3File cast
    IC->>BS: uri()
    BS-->>IC: S3URI (key already has URL escapes e.g. %3A)
    IC->>IC: new URI(s3, bucket, /+key, null) → %3A re-encoded to %253A
    IC->>RC: resolveClient(s3Uri.toString(), ...)
    IC->>RC: copyToHMB(client, output, s3Uri, ...)

    Note over IC: After fix (no double-escape)
    IC->>BS: instanceof BaseS3File cast
    IC->>BS: uri()
    BS-->>IC: S3URI
    IC->>S3U: bucket() → s3Bucket
    IC->>S3U: key() → s3Key (raw, preserves %3A)
    IC->>RC: resolveClient(inputFile.location(), ...)
    IC->>RC: copyToHMB(client, output, s3Bucket, s3Key, ...)

_{Reviews (8): Last reviewed commit: "Merge remote-tracking branch 'upstream/r..." | Re-trigger Greptile}

liurenjie1024 · 2026-07-23T10:10:02Z

build

gerashegalov

The cited integration test covers existing %HH escapes, not URI-invalid raw keys or fallback behavior.

The PR description also ends with an internal drafting instruction; remove it

liurenjie1024 · 2026-07-24T07:37:17Z

Comment addressed, requires a change in internal repo. cc @res-life @gerashegalov

gerashegalov

LGTM

gerashegalov · 2026-07-26T20:25:09Z

+@pytest.mark.parametrize('reader_type', rapids_reader_types)
+@pytest.mark.skipif(not is_iceberg_rest_catalog(),
+                    reason="S3 path handling is exercised only with the REST catalog")
+def test_iceberg_parquet_read_from_uri_invalid_s3_path(spark_tmp_table_factory, reader_type):


Assert effective PerfIO S3 enablement and preferably verify that no Iceberg fallback was recorded. Otherwise this test does not really exercise the previously-broken call path.

Add a config in test to ensure perf io has been enabled.

nvauto · 2026-07-27T06:53:25Z

NOTE: release/26.08 has been created from main. Please retarget your PR to release/26.08 if it should be included in the release.

liurenjie1024 · 2026-07-27T07:15:17Z

build

res-life · 2026-07-27T08:59:24Z

+    assert_gpu_and_cpu_are_equal_collect(
+        lambda spark: spark.sql(f"SELECT * FROM {table}"),
+        conf={
+            'spark.rapids.perfio.s3.enabled': 'true',


This configuration is startup-only and is cached during executor plugin initialization. If PerfIO is not enabled when Spark starts, the test will use the standard Iceberg
delegate. As a result, the CPU/GPU equality test may still pass even if the newly fixed range-copy code is never executed.

Could this help?

def is_perfio_s3_enabled(spark): return spark._jvm.com.nvidia.spark.rapids.fileio \ .RapidsInputFiles.isS3PerfEnabled()

res-life · 2026-07-27T09:02:05Z

 {"spark": "356"}
 {"spark": "357"}
 {"spark": "358"}
+{"spark": "359"}


#15388 will fix this. Please remove.

gerashegalov

LGTM

liurenjie1024 · 2026-07-27T23:43:22Z

build

liurenjie1024 · 2026-07-28T01:28:48Z

build

res-life

LGTM

Fixes #15323. ### Description This is a follow-up to #15360 that makes the Iceberg S3 path regression test reliable with a REST catalog. The test previously constructed the problematic path by overriding `write.data.path` with the configured warehouse. For a REST catalog, the warehouse value can be a logical identifier rather than a physical S3 location. The updated test follows `test_iceberg_parquet_read_from_url_encoded_path`: - Adds a string containing a raw space as special-case partition data. The value is valid in an S3 object key but invalid in a URI unless encoded. - Creates an Iceberg table partitioned by that column and lets Iceberg construct the data-file path through its normal partition writer. - Continues to run only with the REST catalog, where the optimized S3 reader is exercised. - Verifies the startup-only `spark.rapids.perfio.s3.enabled` setting through SparkConf instead of querying an executor-initialized PerfIO singleton from the driver. Testing performed: - The REST-catalog run completed 184 existing tests successfully; the three new parameterized variants reached table setup and exposed the driver-side PerfIO assertion corrected here. - `python3 -m py_compile integration_tests/src/main/python/iceberg/iceberg_test.py` - `git diff --check` A final REST-catalog pipeline rerun is pending. ### Checklists Documentation - [ ] Updated for new or modified user-facing features or behaviors - [x] No user-facing change Testing - [x] Added or modified tests to cover new code paths - [ ] Covered by existing tests - [ ] Not required Performance - [ ] Tests ran and results are added in the PR description - [ ] Issue filed with a link in the PR description - [x] Not required --------- Signed-off-by: Ray Liu <liurenjie2008@gmail.com>

### 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 #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 - [x] No user-facing change Testing - [x] 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 - [x] Not required Signed-off-by: Renjie Liu <liurenjie2008@gmail.com> --------- Signed-off-by: Ray Liu <liurenjie2008@gmail.com>

liurenjie1024 requested review from gerashegalov and res-life July 23, 2026 09:07

Fix double escaping of Iceberg S3 input-file URIs

9f35abf

Signed-off-by: Ray Liu <liurenjie2008@gmail.com>

liurenjie1024 force-pushed the ray/15323 branch from 93f0294 to 9f35abf Compare July 23, 2026 09:08

res-life reviewed Jul 23, 2026

View reviewed changes

Comment thread iceberg/common/src/main/java/org/apache/iceberg/aws/s3/IcebergS3InputFileAccess.java Outdated

gerashegalov requested changes Jul 23, 2026

View reviewed changes

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

Address comments

8048550

liurenjie1024 requested a review from a team as a code owner July 24, 2026 07:21

Merge branch 'main' into ray/15323

f18a53f

gerashegalov previously approved these changes Jul 26, 2026

View reviewed changes

liurenjie1024 added 2 commits July 27, 2026 09:58

Merge branch 'main' into ray/15323

9809537

Fix build break

42d44d5

liurenjie1024 dismissed gerashegalov’s stale review via 42d44d5 July 27, 2026 03:57

liurenjie1024 added 2 commits July 27, 2026 14:28

Merge remote-tracking branch 'upstream/main' into ray/15323

2de16d9

Add conf to ensure perf io is enabled

bb08b45

liurenjie1024 mentioned this pull request Jul 27, 2026

Enable optimized S3 tail reads for Iceberg Parquet footers #15384

Merged

8 tasks

liurenjie1024 changed the base branch from main to release/26.08 July 27, 2026 07:05

res-life reviewed Jul 27, 2026

View reviewed changes

liurenjie1024 added 2 commits July 27, 2026 17:35

Address comments

8a11bab

Merge remote-tracking branch 'upstream/release/26.08' into ray/15323

f495101

gerashegalov approved these changes Jul 27, 2026

View reviewed changes

res-life approved these changes Jul 28, 2026

View reviewed changes

liurenjie1024 merged commit b701b32 into NVIDIA:release/26.08 Jul 28, 2026
55 checks passed

liurenjie1024 mentioned this pull request Jul 29, 2026

[skip ci] Fix Iceberg REST S3 path regression coverage #15422

Merged

8 tasks

liurenjie1024 mentioned this pull request Jul 30, 2026

Fix Iceberg S3 PerfIO access with split classloaders #15435

Merged

8 tasks

Uh oh!

Conversation

liurenjie1024 commented Jul 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklists

Uh oh!

greptile-apps Bot commented Jul 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Greptile Summary

Confidence Score: 5/5

Important Files Changed

Sequence Diagram

Uh oh!

liurenjie1024 commented Jul 23, 2026

Uh oh!

Uh oh!

gerashegalov left a comment

Choose a reason for hiding this comment

Uh oh!

liurenjie1024 commented Jul 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gerashegalov left a comment

Choose a reason for hiding this comment

Uh oh!

gerashegalov Jul 26, 2026

Choose a reason for hiding this comment

Uh oh!

liurenjie1024 Jul 27, 2026

Choose a reason for hiding this comment

Uh oh!

nvauto commented Jul 27, 2026

Uh oh!

liurenjie1024 commented Jul 27, 2026

Uh oh!

res-life Jul 27, 2026

Choose a reason for hiding this comment

Uh oh!

res-life Jul 27, 2026

Choose a reason for hiding this comment

Uh oh!

liurenjie1024 Jul 27, 2026

Choose a reason for hiding this comment

Uh oh!

res-life Jul 27, 2026

Choose a reason for hiding this comment

Uh oh!

gerashegalov left a comment

Choose a reason for hiding this comment

Uh oh!

liurenjie1024 commented Jul 27, 2026

Uh oh!

liurenjie1024 commented Jul 28, 2026

Uh oh!

res-life left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

liurenjie1024 commented Jul 23, 2026 •

edited

Loading

greptile-apps Bot commented Jul 23, 2026 •

edited

Loading

liurenjie1024 commented Jul 24, 2026 •

edited

Loading