Skip to content

Enhance IcebergProbeImpl with jar source logging and version fallback - #14488

Merged
gerashegalov merged 6 commits into
NVIDIA:release/26.04from
gerashegalov:iceberg-probe-version-detection
Apr 1, 2026
Merged

Enhance IcebergProbeImpl with jar source logging and version fallback#14488
gerashegalov merged 6 commits into
NVIDIA:release/26.04from
gerashegalov:iceberg-probe-version-detection

Conversation

@gerashegalov

@gerashegalov gerashegalov commented Mar 30, 2026

Copy link
Copy Markdown
Collaborator

Fixes #14487

Requires #14494

Description

Fallback on the version in the jar file name

Checklists

  • This PR has added documentation for new or modified features or behaviors.
  • This PR has added new tests or modified existing tests to cover new code paths.
    (Please explain in the PR description how the new code paths are tested, such as names of the new/existing tests that cover them.)
  • Performance testing has been performed and its results are added in the PR description. Or, an issue has been filed with a link in the PR description.

@gerashegalov gerashegalov self-assigned this Mar 30, 2026
@greptile-apps

greptile-apps Bot commented Mar 30, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR enhances IcebergProbeImpl with a fallback mechanism that extracts the Iceberg version from the jar file name when a git commit ID is not found in the hard-coded commitToVersion map, and adds Logging to emit a warning when the fallback is used.

Key changes:

  • Adds with Logging to IcebergProbeImpl so Spark's standard log infrastructure is available.
  • Adds jarVersionPattern (a lazy Regex with .unanchored) matching the canonical iceberg-spark-runtime jar naming scheme (e.g. iceberg-spark-runtime-3.5_2.12-1.10.0-...jar).
  • Adds extractVersionFromJarPath(commitId), which defensively chains Option(getProtectionDomain).flatMap(…getCodeSource).flatMap(…getLocation) to avoid NPEs documented for all three calls, then uses collectFirst with the pattern to extract the version string.
  • getDetectedVersion now calls extractVersionFromJarPath as the second-level fallback when the commit map lookup misses; the ultimate fallback is still the raw commit ID, preserving the previous UnsupportedOperationException behaviour in shimPackage.
  • The previous review concerns about null safety (getProtectionDomain, getCodeSource, getLocation) and the missing isSupportedSparkVersion in the trait are fully addressed in this revision.

Confidence Score: 4/5

Safe to merge after considering a minor duplicate-warning issue; no correctness or data-loss risks.

All previously flagged P0/P1 concerns (null safety, trait mismatch) are resolved. The one remaining finding is a P2 style issue: the fallback warning can fire twice due to independent lazy val computations in IcebergProvider. This is cosmetic and does not affect correctness, but keeping the score at 4 to prompt the author to consider caching getDetectedVersion.

iceberg/common/src/main/scala/com/nvidia/spark/rapids/iceberg/IcebergProbeImpl.scala — duplicate warning log in fallback path.

Important Files Changed

Filename Overview
iceberg/common/src/main/scala/com/nvidia/spark/rapids/iceberg/IcebergProbeImpl.scala Adds jar-path-based version fallback with proper null-safety for getProtectionDomain/getCodeSource/getLocation, and mixes in Spark's Logging trait; the warning log can fire twice when both IcebergProvider.detectedVersion and IcebergProvider.shimPackage are accessed.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[getDetectedVersion] --> B{commitId in commitToVersion?}
    B -- yes --> C[return version string]
    B -- no --> D[extractVersionFromJarPath]
    D --> E{getProtectionDomain != null?}
    E -- no --> F[return commitId as-is]
    E -- yes --> G{getCodeSource != null?}
    G -- no --> F
    G -- yes --> H{getLocation != null?}
    H -- no --> F
    H -- yes --> I[url.getPath]
    I --> J{jarVersionPattern matches?}
    J -- no --> F
    J -- yes --> K[logWarning + return version]
Loading

Reviews (7): Last reviewed commit: "Refactor version extraction logic in Ice..." | Re-trigger Greptile

@sameerz sameerz added the bug Something isn't working label Mar 30, 2026
@gerashegalov
gerashegalov changed the base branch from main to release/26.04 March 30, 2026 22:54
@gerashegalov
gerashegalov requested a review from a team as a code owner March 30, 2026 22:54
Signed-off-by: Gera Shegalov <gshegalov@nvidia.com>
Signed-off-by: Gera Shegalov <gshegalov@nvidia.com>
Signed-off-by: Gera Shegalov <gshegalov@nvidia.com>
@gerashegalov
gerashegalov force-pushed the iceberg-probe-version-detection branch from 186de32 to e48ecea Compare March 30, 2026 23:34
Updated the jar version pattern to be more specific to the iceberg-spark-runtime format. Introduced a new private method to extract the version from the jar path, improving the clarity and maintainability of the version detection logic. The getDetectedVersion method now utilizes this new method for better handling of unknown commit IDs.
liurenjie1024
liurenjie1024 previously approved these changes Apr 1, 2026
Updated the method for extracting the version from the jar path to improve clarity and reliability. The change ensures that the protection domain is properly accessed before retrieving the code source, enhancing the robustness of the version detection process.
@gerashegalov

Copy link
Copy Markdown
Collaborator Author

build

@gerashegalov
gerashegalov merged commit ee6e381 into NVIDIA:release/26.04 Apr 1, 2026
47 checks passed
@gerashegalov
gerashegalov deleted the iceberg-probe-version-detection branch April 1, 2026 15:28
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.

[BUG] Iceberg version detection may fail if a vendor build does not include properties behind IcebergBuild

5 participants