Skip to content

Read JNI metadata from the renamed cuDF Spark artifact - #645

Open
NvTimLiu wants to merge 1 commit into
NVIDIA:mainfrom
NvTimLiu:jni-artifacatId-rename
Open

Read JNI metadata from the renamed cuDF Spark artifact#645
NvTimLiu wants to merge 1 commit into
NVIDIA:mainfrom
NvTimLiu:jni-artifacatId-rename

Conversation

@NvTimLiu

@NvTimLiu NvTimLiu commented Sep 10, 2026

Copy link
Copy Markdown
Collaborator

Summary

Read JNI metadata from the renamed cuDF Spark artifact

Update the accelerated UDF native dependency resolver to consume cudf-spark-jni-version-info.properties after the upstream JNI artifact rename.

Expose the embedded JNI revision as jar.cudf.spark.jni.revision and update resolver errors, status output, Maven comments, and user documentation to use the cudf-spark-jni name. Do not retain the legacy spark-rapids-jni metadata filename or property alias.

Validation: ran the native library extraction unit tests, checked resolver shell syntax, parsed the Maven POM, verified whitespace, and confirmed the changed files contain no legacy JNI references.

Reference: https://github.com/NVIDIA/cudf-spark-jni/pull/5093
Signed-off-by: Tim Liu <timl@nvidia.com>

@greptile-apps

greptile-apps Bot commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

RetriggerConfidence Score: 4/5

The PR is not safe to merge until native pin resolution continues to support released JARs using the legacy JNI metadata filename.

Findings

  1. P1 Legacy Metadata Support Removed
  2. P2 Tests Missing From CI
  3. P2 Metadata Precedence Untested

Summary

  • Reads the renamed JNI metadata filename when resolving native dependency pins.
  • Emits the renamed JNI revision property.
  • Updates Maven comments and native-build documentation.
  • Currently drops compatibility with released JARs carrying the legacy metadata filename.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart TD
  J[RAPIDS JAR] --> M{Metadata filename}
  M -->|cudf-spark-jni-version-info.properties| R[Resolve JNI revision and URL]
  M -->|spark-rapids-jni-version-info.properties| E[Resolver exits with error]
  R --> P[Download matching cuDF pins]
  P --> B[Build native UDF]
  E --> F[Maven native build fails]
Loading

Reviews (2) · Last reviewed commit: "Read JNI metadata from the renamed cuDF ..."

Comment thread examples/UDF-Examples/RAPIDS-accelerated-UDFs/test_resolve_jni_cudf_pins.py Outdated
Comment thread examples/UDF-Examples/RAPIDS-accelerated-UDFs/test_resolve_jni_cudf_pins.py Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The functional changes are small and well-covered by new unit tests; remaining feedback is limited to documentation/diagnostic wording clarity.

Pull request overview

This PR updates the RAPIDS-accelerated UDF native dependency resolver to handle the upstream rename of the JNI artifact metadata, while keeping compatibility with previously released jars that still carry the legacy metadata layout.

Changes:

  • Prefer cudf-spark-jni-version-info.properties and fall back to legacy spark-rapids-jni-version-info.properties when resolving JNI revision/URL.
  • Emit jar.cudf.spark.jni.revision while retaining jar.spark.rapids.jni.revision as a compatibility alias.
  • Update UDF documentation and add unit coverage for both the renamed and legacy metadata layouts.
File summaries
File Description
examples/UDF-Examples/RAPIDS-accelerated-UDFs/resolve-jni-cudf-pins.sh Adds dual-metadata probing and writes the new JNI revision property while keeping a legacy alias.
examples/UDF-Examples/RAPIDS-accelerated-UDFs/test_resolve_jni_cudf_pins.py New unit tests covering both renamed and legacy jar metadata layouts via a controlled curl stub.
examples/UDF-Examples/RAPIDS-accelerated-UDFs/README.md Documentation updates to reference the renamed artifact and resolver behavior.
examples/UDF-Examples/RAPIDS-accelerated-UDFs/pom.xml Updates Maven diagnostics/comments to reflect the renamed JNI artifact.
Review details

Suppressed comments (1)

examples/UDF-Examples/RAPIDS-accelerated-UDFs/README.md:170

  • This sentence states pins are derived from the "cudf-spark-jni" revision, but for older jars the revision comes from spark-rapids-jni. Consider mentioning the legacy name here too so the documentation matches the resolver behavior.
When changing the rapids-4-spark jar version, rebuild the native UDFs with
matching cuDF/RMM/CCCL headers and libraries. In prebuilt mode, the Maven build
derives those native dependency pins from the `cudf-spark-jni` revision
recorded in the jar. After changing jar versions, remove `target/native-deps`,
  • Files reviewed: 4/4 changed files
  • Comments generated: 3
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread examples/UDF-Examples/RAPIDS-accelerated-UDFs/README.md
Comment thread examples/UDF-Examples/RAPIDS-accelerated-UDFs/pom.xml
Update the accelerated UDF native dependency resolver to consume cudf-spark-jni-version-info.properties after the upstream JNI artifact rename.

Expose the embedded JNI revision as jar.cudf.spark.jni.revision and update resolver errors, status output, Maven comments, and user documentation to use the cudf-spark-jni name. Do not retain the legacy spark-rapids-jni metadata filename or property alias.

Validation: ran the native library extraction unit tests, checked resolver shell syntax, parsed the Maven POM, verified whitespace, and confirmed the changed files contain no legacy JNI references.

Reference: NVIDIA/cudf-spark-jni#5093
Signed-off-by: Tim Liu <timl@nvidia.com>
@NvTimLiu
NvTimLiu force-pushed the jni-artifacatId-rename branch from 37d4317 to 3dfd72f Compare September 10, 2026 09:08
@NvTimLiu

NvTimLiu commented Sep 10, 2026

Copy link
Copy Markdown
Collaborator Author

Build PASS:
aa3a74d8-7323-48ae-a703-9edb8e6fddb4

@NvTimLiu NvTimLiu self-assigned this Sep 10, 2026
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