Skip to content

Finalize Iceberg root and shim class placement [reduced-it] - #15944

Open
gerashegalov wants to merge 12 commits into
NVIDIA:mainfrom
gerashegalov:codex/unshim-iceberg-final-layout
Open

Finalize Iceberg root and shim class placement [reduced-it]#15944
gerashegalov wants to merge 12 commits into
NVIDIA:mainfrom
gerashegalov:codex/unshim-iceberg-final-layout

Conversation

@gerashegalov

@gerashegalov gerashegalov commented Sep 10, 2026

Copy link
Copy Markdown
Collaborator

Related to #15821.

Description

This PR completes the Iceberg class-placement milestone:

  • promote conditional root-safe module classes from the newest selected Spark classifier that provides each class
  • move five conditional Iceberg helpers into iceberg-common
  • retain all remaining Iceberg implementation classes in their Spark-specific directories instead of spark-shared
  • remove all seven remaining Iceberg entries from unshimmed-common-from-single-shim.txt
  • apply the same conditional-root selection to the standard and fast distribution assemblers

After this change, assembled multi-shim distribution JARs contain no Iceberg classes under spark-shared. The shared, root-safe Iceberg helpers are at the JAR root, while Spark- and Iceberg-version-specific implementations remain under sparkXYZ.

There are no user-facing configuration or API changes.

Validation:

  • post-upmerge clean Scala 2.12 multi-shim dist build: 334,344,353,359
  • post-upmerge clean Scala 2.13 multi-shim dist build: 353,359,404,413,420
  • package-private-access audit passed for 11 callers across all selected real Iceberg runtime worlds
  • final JAR inspection found 53 Iceberg class files at root, zero under spark-shared, and the remaining classes under Spark-specific directories
  • verified that conditional root classes have no copies under the selected shim directories
  • verified newest-provider precedence: general helpers came from Spark 4.1.3 and GpuBaseReader came from its newest applicable classifier, Spark 3.5.3
  • Spark 4.1.3 focused Iceberg unit tests: 5 passed
  • Spark 3.5.3/Scala 2.13 GPU integration smoke with Iceberg loaded through extraClassPath: 6 passed
  • fast direct multi-shim assembler completed successfully and produced the same Iceberg placement
  • shell, Python syntax, Scala 2.13 POM synchronization, and whitespace checks passed

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
    (IcebergDeletionVectorSuite and the Iceberg read, append, and no-op-write integration smoke 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

Performance testing is not required because this changes build-time source selection and JAR placement without modifying runtime execution logic.

This change and its validation were prepared with AI assistance and reviewed by a human before publication.

Signed-off-by: Gera Shegalov <gshegalov@nvidia.com>
Signed-off-by: Gera Shegalov <gshegalov@nvidia.com>
…independent-helpers

Signed-off-by: Gera Shegalov <gshegalov@nvidia.com>

# Conflicts:
#	dist/unshimmed-common-from-single-shim.txt
…independent-helpers

Signed-off-by: Gera Shegalov <gshegalov@nvidia.com>
Signed-off-by: Gera Shegalov <gshegalov@nvidia.com>
…independent-helpers

Signed-off-by: Gera Shegalov <gshegalov@nvidia.com>

# Conflicts:
#	dist/unshimmed-common-from-single-shim.txt
…independent-helpers

Signed-off-by: Gera Shegalov <gshegalov@nvidia.com>

# Conflicts:
#	dist/unshimmed-common-from-single-shim.txt
Signed-off-by: Gera Shegalov <gshegalov@nvidia.com>
…independent-helpers

Signed-off-by: Gera Shegalov <gshegalov@nvidia.com>
Signed-off-by: Gera Shegalov <gshegalov@nvidia.com>
Signed-off-by: Gera Shegalov <gshegalov@nvidia.com>
…final-layout

Signed-off-by: Gera Shegalov <gshegalov@nvidia.com>

# Conflicts:
#	dist/unshimmed-common-from-single-shim.txt
@gerashegalov
gerashegalov marked this pull request as ready for review September 10, 2026 15:24
@gerashegalov
gerashegalov requested a review from a team as a code owner September 10, 2026 15:24
@gerashegalov

Copy link
Copy Markdown
Collaborator Author

build

@gerashegalov gerashegalov added the task Work required that improves the product but is not user facing label Sep 10, 2026
@greptile-apps

greptile-apps Bot commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

RetriggerConfidence Score: 4/5

The PR appears safe to merge, with a non-blocking request for automated regression coverage of the new class-placement algorithms.

Summary

  • Applies matching newest-provider selection to the standard and fast assemblers.
  • Moves five conditional Iceberg helpers into iceberg-common and adds their shim annotations.
  • Updates deduplication rules to remove promoted shim copies while preventing remaining Iceberg implementations from entering spark-shared.
  • Keeps the Scala 2.12 and 2.13 iceberg-common POMs synchronized.
  • The main remaining concern is the absence of focused automated coverage for the new packaging-selection behavior.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart TD
  A[Selected Spark classifiers] --> B[Process newest first]
  B --> C[Read root-safe module class members]
  C --> D{Class already promoted?}
  D -- No --> E[Copy aggregator class to JAR root]
  D -- Yes --> F[Keep existing newer root copy]
  E --> G[Record promoted class path]
  F --> H[Extract full artifact under sparkXYZ]
  G --> H
  H --> I[Binary dedupe]
  I --> J[Remove root-promoted shim copies]
  I --> K[Retain version-specific Iceberg classes under sparkXYZ]
  J --> L[Final multi-shim distribution]
  K --> L
Loading

Reviews (1) · Last reviewed commit: "Merge remote-tracking branch 'origin/mai..."

@gerashegalov
gerashegalov requested a review from a team September 10, 2026 15:31
@gerashegalov

Copy link
Copy Markdown
Collaborator Author

build

raise Exception(
"root-safe module classes missing from aggregator: %s" %
", ".join(missing_members))
new_root_safe_members = (

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.

Could we add a small synthetic multi-classifier regression test for this provider-selection path and its fast-assembler counterpart? A 4.1.3/3.5.3 fixture could verify that a shared class comes from the newest classifier that provides it, an older-only class is still promoted, and dedupe leaves promoted helpers only at the root while keeping version-specific Iceberg classes in their shim directories. The current implementation and multi-shim validation look correct; this would just protect those invariants from future regressions.

@firestarman firestarman 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, just one nit.

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

Labels

task Work required that improves the product but is not user facing

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants