Skip to content

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

Merged
liurenjie1024 merged 13 commits into
NVIDIA:release/26.08from
liurenjie1024:ray/15323
Jul 30, 2026
Merged

[skip ci] Fix Iceberg REST S3 path regression coverage#15422
liurenjie1024 merged 13 commits into
NVIDIA:release/26.08from
liurenjie1024:ray/15323

Conversation

@liurenjie1024

@liurenjie1024 liurenjie1024 commented Jul 29, 2026

Copy link
Copy Markdown
Collaborator

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
  • 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

@liurenjie1024
liurenjie1024 requested a review from a team as a code owner July 29, 2026 11:33
@liurenjie1024
liurenjie1024 requested review from gerashegalov and res-life and removed request for a team July 29, 2026 11:35
Signed-off-by: Ray Liu <liurenjie2008@gmail.com>
@liurenjie1024 liurenjie1024 changed the title Fix Iceberg REST S3 path regression coverage [fast-ut] [reduced-it] Fix Iceberg REST S3 path regression coverage Jul 29, 2026
@greptile-apps

greptile-apps Bot commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

Updates the Iceberg REST S3 regression test to create a space-containing partition path through Iceberg’s normal writer and verifies that PerfIO S3 was explicitly enabled in the startup Spark configuration.

  • Replaces the warehouse-derived write.data.path override with a partitioned Iceberg CTAS.
  • Adds a heavily weighted partition value containing spaces.
  • Checks the startup SparkConf before comparing GPU and CPU reads.

Confidence Score: 5/5

The PR appears safe to merge with no actionable correctness or test-reliability issues identified.

The partitioned CTAS matches an existing Iceberg test pattern, the weighted data generator reliably produces the target value, and the REST test launcher explicitly supplies the startup configuration checked by the new assertion.

Important Files Changed

Filename Overview
integration_tests/src/main/python/iceberg/iceberg_test.py The revised deterministic test setup follows an established sibling pattern, exercises all configured reader variants, and checks the REST CI startup configuration without introducing an actionable defect.

Reviews (1): Last reviewed commit: "Revert unnecessary chagnes" | Re-trigger Greptile

@liurenjie1024 liurenjie1024 changed the title [fast-ut] [reduced-it] Fix Iceberg REST S3 path regression coverage [skip ci] Fix Iceberg REST S3 path regression coverage Jul 29, 2026
@liurenjie1024

Copy link
Copy Markdown
Collaborator Author

We could skip ci since the changed test in this pr will not run in premerge.

@liurenjie1024

Copy link
Copy Markdown
Collaborator Author

build

@res-life res-life 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.

LGTM

@liurenjie1024
liurenjie1024 merged commit 1dbb5ce into NVIDIA:release/26.08 Jul 30, 2026
57 of 58 checks passed
nvliyuan added a commit that referenced this pull request Jul 30, 2026
Resolves the latest merge conflicts in #15412 after #15423 merged.

### Description

Merge the current `release/26.08` head into the current `main` head.

The initial conflicts were the root and Scala 2.13 project versions.
Both were resolved by retaining main's `26.10.0-SNAPSHOT` version
instead of the release branch's `26.08.0-SNAPSHOT` version.

The branch was refreshed again after additional PRs landed on
`release/26.08`. The current diff includes all release updates added
after #15423, including:
- #15413 — preserve Spark 4.2 BroadcastHashJoin `isSkewJoin`
- #15422 — fix Iceberg REST S3 path regression coverage
- #15368 — OSS Delta deletion-vector updates
- #15411 — fix OSS Delta RTAS on Spark 4.x+
- #15416 — match Spark 4.2 `date_trunc` overflow behavior

### Checklists

Documentation
- [ ] Updated for new or modified user-facing features or behaviors
- [x] No user-facing change

Testing
- [ ] Added or modified tests to cover new code paths
- [x] Covered by existing tests
      (The included release commits retain their original 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

### Validation

- `git diff --check`
- Parsed both initially resolved POM files as XML
- `python3 -m py_compile` for the modified Iceberg, Delta, and date-time
integration tests

IMPORTANT: Merge this PR using **Create a merge commit** so the release
commit ancestry is preserved and #15412 can close automatically.

---------

Signed-off-by: Sameer Raheja <sraheja@.nvidia.com>
Signed-off-by: Rahul Prabhu <raprabhu@nvidia.com>
Signed-off-by: Chong Gao <chongg@nvidia.com>
Signed-off-by: Firestarman <firestarmanllc@gmail.com>
Signed-off-by: Ray Liu <liurenjie2008@gmail.com>
Signed-off-by: liyuan <yuali@nvidia.com>
Signed-off-by: Jihoon Son <ghoonson@gmail.com>
Signed-off-by: Niranjan Artal <nartal@nvidia.com>
Co-authored-by: Sameer Raheja <sameerz@users.noreply.github.com>
Co-authored-by: Sameer Raheja <sraheja@.nvidia.com>
Co-authored-by: Gary Shen <gashen@nvidia.com>
Co-authored-by: Rahul Prabhu <100436830+sdrp713@users.noreply.github.com>
Co-authored-by: Chong Gao <chongg@nvidia.com>
Co-authored-by: Chong Gao <res_life@163.com>
Co-authored-by: Liangcai Li <firestarmanllc@gmail.com>
Co-authored-by: Renjie Liu <liurenjie2008@gmail.com>
Co-authored-by: Jihoon Son <ghoonson@gmail.com>
Co-authored-by: Niranjan Artal <50492963+nartal1@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants