Conversation
|
This comment has been minimized.
This comment has been minimized.
|
Want your agent to iterate on Greptile's feedback? Try greploops. |
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical artifact-resolution and Databricks deployment-path issues block approval.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Renames the cuDF Spark Maven artifacts from rapids-4-spark to cudf-spark across Scala 2.12/2.13 modules, packaging, CI, tests, and documentation.
Changes:
- Renamed Maven coordinates and dependencies.
- Updated packaging, Databricks, Jenkins, and integration-test scripts.
- Updated documentation and generated UDF/native-build templates.
Review findings:
- Critical (1 vote):
.github/workflows/blossom-ci.ymluses an incomplete reactor selector without_2.12. - Critical (3 votes):
jenkins/databricks/deploy.shstill uses the legacy helper-JAR prefix. - Critical (1 vote): Native extraction defaults to 26.06.0 but checks only renamed artifacts.
- Critical (1 vote): The generated UDF template requests unpublished renamed coordinates for 26.06.0.
- Moderate (2 votes):
Plugin.scalano longer excludes legacy submodule JAR prefixes. - Nit (2 votes):
docs/dev/shims.mduses renamed coordinates for historical version 26.04.2. - Nit (1 vote):
pom.xmllacks requested performance validation or justification. - Nit (1 vote):
Plugin.scalalacks requested performance validation or justification.
File summaries
| File | Reviewed change |
|---|---|
udf-compiler/README.md |
Updated distribution artifact reference. |
udf-compiler/pom.xml |
Renamed UDF Maven coordinates. |
tools/pom.xml |
Renamed tools coordinates and dependencies. |
tests/pom.xml |
Renamed test coordinates and dependencies. |
sql-plugin/src/main/scala/com/nvidia/spark/rapids/RapidsConfEntries.scala |
Updated artifact documentation. |
sql-plugin/src/main/scala/com/nvidia/spark/rapids/RapidsConf.scala |
Updated example JAR name. |
sql-plugin/src/main/scala/com/nvidia/spark/rapids/Plugin.scala |
Updated artifact discovery names. |
sql-plugin/pom.xml |
Renamed SQL plugin coordinates and dependencies. |
sql-plugin-shims/pom.xml |
Renamed shim coordinates. |
sql-plugin-format/pom.xml |
Renamed format coordinates. |
sql-plugin-fileio/pom.xml |
Renamed file I/O coordinates. |
sql-plugin-columnar/pom.xml |
Renamed columnar coordinates and dependencies. |
sql-plugin-api/src/main/scala/com/nvidia/spark/rapids/ShimLoader.scala |
Updated JAR examples. |
sql-plugin-api/pom.xml |
Renamed API coordinates. |
skills/udf-gen-test/templates/scala/pom.xml |
Updated generated dependencies. |
skills/udf-convert-to-cuda/templates/cuda/native/src/main/cpp/CMakeLists.txt |
Updated JAR documentation. |
skills/udf-convert-to-cuda/templates/cuda/native/scripts/extract-cudf-libs.sh |
Updated JAR lookup paths. |
skills/udf-convert-to-cuda/references/NATIVE_BUILD_ENV.md |
Updated native-build documentation. |
skills/docs/dev/VERSIONS.md |
Updated artifact documentation. |
shuffle-plugin/pom.xml |
Renamed shuffle coordinates and dependencies. |
shim-deps/pom.xml |
Renamed parent and BOM coordinates. |
shim-deps/databricks/pom.xml |
Renamed Databricks BOM. |
shim-deps/cloudera/pom.xml |
Renamed Cloudera BOM. |
scripts/prioritize-commits.sh |
Updated artifact download paths. |
scala2.13/udf-compiler/pom.xml |
Renamed Scala 2.13 UDF coordinates. |
scala2.13/tools/pom.xml |
Renamed Scala 2.13 tools coordinates. |
scala2.13/tests/pom.xml |
Renamed Scala 2.13 test coordinates. |
scala2.13/sql-plugin/pom.xml |
Renamed Scala 2.13 SQL coordinates. |
scala2.13/sql-plugin-shims/pom.xml |
Renamed Scala 2.13 shim coordinates. |
scala2.13/sql-plugin-format/pom.xml |
Renamed Scala 2.13 format coordinates. |
scala2.13/sql-plugin-fileio/pom.xml |
Renamed Scala 2.13 file I/O coordinates. |
scala2.13/sql-plugin-columnar/pom.xml |
Renamed Scala 2.13 columnar coordinates. |
scala2.13/sql-plugin-api/pom.xml |
Renamed Scala 2.13 API coordinates. |
scala2.13/shuffle-plugin/pom.xml |
Renamed Scala 2.13 shuffle coordinates. |
scala2.13/shim-deps/pom.xml |
Renamed Scala 2.13 dependency coordinates. |
scala2.13/shim-deps/databricks/pom.xml |
Renamed Scala 2.13 Databricks BOM. |
scala2.13/shim-deps/cloudera/pom.xml |
Renamed Scala 2.13 Cloudera BOM. |
scala2.13/pom.xml |
Renamed parent and integration properties. |
scala2.13/integration_tests/pom.xml |
Renamed integration-test coordinates. |
scala2.13/iceberg/pom.xml |
Renamed Iceberg coordinates. |
scala2.13/iceberg/iceberg-stub/pom.xml |
Renamed Iceberg stub coordinates. |
scala2.13/iceberg/iceberg-1-9-x/pom.xml |
Renamed Iceberg 1.9 coordinates. |
scala2.13/iceberg/iceberg-1-6-x/pom.xml |
Renamed Iceberg 1.6 coordinates. |
scala2.13/iceberg/iceberg-1-11-x/pom.xml |
Renamed Iceberg 1.11 coordinates. |
scala2.13/iceberg/iceberg-1-10-x/pom.xml |
Renamed Iceberg 1.10 coordinates. |
scala2.13/iceberg-common/pom.xml |
Renamed common Iceberg coordinates. |
scala2.13/dist/pom.xml |
Renamed distribution coordinates. |
scala2.13/delta-lake/delta-stub/pom.xml |
Renamed Delta stub coordinates. |
scala2.13/delta-lake/delta-spark400db173/pom.xml |
Renamed Databricks Delta coordinates. |
scala2.13/delta-lake/delta-spark350db143/pom.xml |
Renamed Databricks Delta coordinates. |
scala2.13/delta-lake/delta-spark332db/pom.xml |
Renamed Databricks Delta coordinates. |
scala2.13/delta-lake/delta-spark330db/pom.xml |
Renamed Databricks Delta coordinates. |
scala2.13/delta-lake/delta-42x/pom.xml |
Renamed Delta 4.2 coordinates. |
scala2.13/delta-lake/delta-41x/pom.xml |
Renamed Delta 4.1 coordinates. |
scala2.13/delta-lake/delta-40x/pom.xml |
Renamed Delta 4.0 coordinates. |
scala2.13/delta-lake/delta-33x/pom.xml |
Renamed Delta 3.3 coordinates. |
scala2.13/delta-lake/delta-24x/pom.xml |
Renamed Delta 2.4 coordinates. |
scala2.13/delta-lake/delta-23x/pom.xml |
Renamed Delta 2.3 coordinates. |
scala2.13/delta-lake/delta-22x/pom.xml |
Renamed Delta 2.2 coordinates. |
scala2.13/delta-lake/delta-21x/pom.xml |
Renamed Delta 2.1 coordinates. |
scala2.13/delta-lake/delta-20x/pom.xml |
Renamed Delta 2.0 coordinates. |
scala2.13/datagen/pom.xml |
Updated parent coordinate. |
scala2.13/api_validation/pom.xml |
Renamed API validation coordinates. |
scala2.13/aggregator/pom.xml |
Renamed aggregator coordinates and dependencies. |
README.md |
Updated Maven artifact examples. |
pom.xml |
Renamed root coordinates and integration properties. |
jenkins/spark-tests.sh |
Updated artifact downloads. |
jenkins/spark-premerge-build.sh |
Updated build artifact paths. |
jenkins/spark-nightly-build.sh |
Updated copied module names. |
jenkins/printJarVersion.sh |
Updated artifact documentation. |
jenkins/databricks/install_deps.py |
Renamed temporary installer artifact. |
jenkins/databricks/deploy.sh |
Updated Databricks publication paths. |
jenkins/databricks/cudf_udf_test.sh |
Updated plugin JAR lookup. |
jenkins/databricks/build.sh |
Updated built JAR name. |
integration_tests/ScaleTest.md |
Updated test JAR example. |
integration_tests/run_pyspark_from_build.sh |
Added renamed and legacy JAR resolution. |
integration_tests/README.md |
Updated test documentation. |
integration_tests/pom.xml |
Renamed integration-test coordinates. |
iceberg/iceberg-stub/pom.xml |
Renamed Iceberg stub coordinates. |
iceberg/iceberg-1-9-x/pom.xml |
Renamed Iceberg 1.9 coordinates. |
iceberg/iceberg-1-6-x/pom.xml |
Renamed Iceberg 1.6 coordinates. |
iceberg/iceberg-1-11-x/pom.xml |
Renamed Iceberg 1.11 coordinates. |
iceberg/iceberg-1-10-x/pom.xml |
Renamed Iceberg 1.10 coordinates. |
iceberg-common/pom.xml |
Renamed common Iceberg coordinates. |
docs/download.md |
Updated artifact naming and release notes. |
docs/dev/shims.md |
Updated artifact analysis examples. |
docs/demo/GCP/mortgage-xgboost4j-gpu-scala.ipynb |
Updated example JAR names. |
docs/configs.md |
Updated JAR examples. |
docs/additional-functionality/advanced_configs.md |
Updated configuration documentation. |
dist/scripts/tests/test_root_safe_provider_selection.py |
Updated packaging test fixtures. |
dist/scripts/tests/test_check_iceberg_package_private_access.py |
Updated Iceberg test fixtures. |
dist/scripts/build-unshim-parallel-world.py |
Updated artifact construction. |
dist/pom.xml |
Renamed distribution coordinates. |
dist/maven-antrun/build-parallel-worlds.xml |
Updated aggregator matching. |
dist/build/package-parallel-worlds.py |
Updated packaging artifact names. |
dist/build/iceberg_runtime.py |
Updated Iceberg artifact discovery. |
delta-lake/delta-stub/pom.xml |
Renamed Delta stub coordinates. |
delta-lake/delta-spark400db173/pom.xml |
Renamed Databricks Delta coordinates. |
delta-lake/delta-spark350db143/pom.xml |
Renamed Databricks Delta coordinates. |
delta-lake/delta-spark332db/pom.xml |
Renamed Databricks Delta coordinates. |
delta-lake/delta-spark330db/pom.xml |
Renamed Databricks Delta coordinates. |
delta-lake/delta-40x/pom.xml |
Renamed Delta 4.0 coordinates. |
delta-lake/delta-33x/pom.xml |
Renamed Delta 3.3 coordinates. |
delta-lake/delta-24x/pom.xml |
Renamed Delta 2.4 coordinates. |
delta-lake/delta-23x/pom.xml |
Renamed Delta 2.3 coordinates. |
delta-lake/delta-22x/pom.xml |
Renamed Delta 2.2 coordinates. |
delta-lake/delta-21x/pom.xml |
Renamed Delta 2.1 coordinates. |
delta-lake/delta-20x/pom.xml |
Renamed Delta 2.0 coordinates. |
datagen/pom.xml |
Updated parent coordinate. |
CONTRIBUTING.md |
Updated build examples. |
build/coverage-report |
Updated coverage JAR paths. |
build/buildall |
Updated reusable artifact names. |
api_validation/README.md |
Updated dependency documentation. |
api_validation/pom.xml |
Renamed API validation coordinates. |
aggregator/pom.xml |
Renamed aggregator coordinates and dependencies. |
.github/workflows/blossom-ci.yml |
Updated Black Duck Maven selection. |
Review details
Suppressed comments (2)
pom.xml:23
- The PR checklist marks
Performance: Not required, but this change also modifies the runtime plugin classpath-discovery path inPlugin.scala. Please add a performance validation/result or state a verifiable reason in the PR description why this runtime path cannot affect performance.
sql-plugin/src/main/scala/com/nvidia/spark/rapids/Plugin.scala:163 - The PR marks performance validation as “Not required”, but this change modifies startup classpath discovery and the packaging/deployment artifact lookups. Please add representative validation or document a verifiable reason these changed artifact-discovery paths cannot affect startup/runtime performance before merging.
- Files reviewed: 105/113 changed files
- Comments generated: 6
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🟡 Changes recommended
One or more issues must be addressed before approval.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (4)
Previously missed (1) — in code that hasn't changed since the last review.
sql-plugin/src/main/scala/com/nvidia/spark/rapids/Plugin.scala:163
- The PR marks performance validation as “Not required”, but this changes production startup classpath discovery. Please add a verifiable rationale that this startup-only path cannot affect runtime performance, or provide a targeted startup-impact check; otherwise the performance checklist is incomplete.
.github/workflows/blossom-ci.yml:115
- The reactor selector no longer names an existing project: the root POM declares
com.nvidia:cudf-spark-parent_2.12(pom.xml:23), but this command usescom.nvidia:cudf-spark-parentwithout the Scala suffix. Black Duck's Maven scan will fail to resolve that-plselector; keep the_2.12suffix as used by the actual root artifact.
echo detect.maven.build.command="-pl=com.nvidia:cudf-spark-parent,com.nvidia:cudf-spark-sql_2.12 -am" >> application.properties
sql-plugin/src/main/scala/com/nvidia/spark/rapids/Plugin.scala:163
- Please retain the legacy prefix in this exclusion. A legacy
rapids-4-spark-aggregator(or other submodule) also containsrapids4spark-version-info.properties, but no longer matches this filter, sodetectMultipleJarcounts old submodules as additional main plugin jars. Withspark.rapids.sql.allowMultipleJars=NEVER, a classpath containing one legacy plugin can now fail; filter both prefixes (or identify only the main artifact) to preserve legacy compatibility.
sql-plugin/src/main/scala/com/nvidia/spark/rapids/Plugin.scala:163 detectMultipleJaris also used forJNI_PROPS_FILENAME, but this prefix check is applied to every resource. Acudf-spark-jnijar's URL necessarily containscudf-spark-, so all JNI version-info resources are now filtered out and multiple-JNI detection becomes a no-op, even thoughspark.rapids.sql.allowMultipleJarsstill documents and guards that case. Make the submodule filter specific to the plugin resource or pass the appropriate prefix per call.
- Files reviewed: 105/113 changed files
- Comments generated: 4
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
Compatibility fallbacks are missing in two runners, native-UDF coordinates remain inconsistent, JNI duplicate filtering is incorrect, and requested startup-scan performance validation is outstanding.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (4)
jenkins/databricks/cudf_udf_test.sh:64
- This Databricks UDF test runner now assumes every plugin file is
cudf-spark_*.jar. With a released 26.08-or-earlierrapids-4-spark_*.jarinLOCAL_JAR_PATH,set -eaborts here beforerun_pyspark_from_build.shcan use its legacy fallback, so the runner is not compatible with both filename schemes as described. Select the new glob first and fall back to the legacy glob.
jenkins/spark-tests.sh:107 - This runner now downloads only the
cudf-sparkfilenames, so settingPROJECT_VER/PROJECT_TEST_VERto a pre-26.10 release (which still publishesrapids-4-sparkcoordinates) makes the firstwgetfail and exits before tests start. Please add the same new-first/legacy-fallback selection used byintegration_tests/run_pyspark_from_build.shfor the plugin and test artifacts.
skills/udf-convert-to-cuda/templates/cuda/native/src/main/cpp/CMakeLists.txt:47 - This changes only the CMake description, but the generated native-UDF Maven profile and
extract-cudf-libs.shstill resolve and search forrapids-4-spark(for example, the template POM uses that artifactId). A native UDF generated for a post-rename release will therefore fail to resolve/find the plugin JAR before this option can use it; update the template coordinates, lookup paths, and related documentation together.
sql-plugin/src/main/scala/com/nvidia/spark/rapids/Plugin.scala:164 - This predicate is applied to all three property files, not only the plugin property. Since every
cudf-spark-jni-...URL containscudf-spark-, all JNI version-info URLs are filtered out, so duplicate JNI jars bypass theSAME_REVISION/NEVERchecks. Make the submodule test depend on the requestedjarNamewhile retaining the legacy plugin-prefix check.
- Files reviewed: 105/113 changed files
- Comments generated: 1
- Review effort level: Lite
Align Maven coordinates and generated artifact names with the repository rename from spark-rapids to cudf-spark. This change: - renames the Scala 2.12 and 2.13 parent, distribution, and module artifact IDs from rapids-4-spark* to cudf-spark* - updates packaging, assembly, dependency lookup, deployment, test, diagnostic, and multiple-JAR detection paths - synchronizes the generated Scala 2.13 POMs and refreshes user documentation, developer documentation, and UDF templates - preserves archived release documentation and the 26.08 Maven download coordinates so existing releases remain valid The dependency contents and Spark execution paths are unchanged. Validation: - ./build/make-scala-version-build-files.sh 2.13 - mvn -q -DskipTests -Dbuildver=330 validate - mvn -q -f scala2.13/pom.xml -DskipTests -Dbuildver=350 validate - python3 -m unittest -v dist/scripts/tests/test_root_safe_provider_selection.py Refs: NVIDIA#15882 Signed-off-by: timl <timl@nvidia.com>
Signed-off-by: timl <timl@nvidia.com>
Signed-off-by: timl <timl@nvidia.com>
Signed-off-by: timl <timl@nvidia.com>
Signed-off-by: timl <timl@nvidia.com>
keep the root-resource check for all three resource types, but apply the current/legacy plugin-submodule exclusions only when checking plugin metadata Co-authored-by: Peixin <pxli@nyu.edu>
1532307 to
5c0c74c
Compare
|
Fixed merge conflicts for docs/additional-functionality/advanced_configs.md |
|
/nvskills-ci |
|
@gerashegalov can you help take a look at this one? thanks |
|
build |
Revert the surviving skills changes from the artifact rename commit series so PR NVIDIA#16053 no longer modifies the skills directory. This addresses the failed require-nvskills-ci job: https://github.com/NVIDIA/cudf-spark/actions/runs/35555804089/job/106202291548?pr=16053 The skills updates will be submitted in a new pull request from a branch hosted in NVIDIA/cudf-spark, not from a forked GitHub repository, so the required skills CI can run in the intended repository context. Signed-off-by: timl <timl@nvidia.com>
|
build |
| <parent> | ||
| <groupId>com.nvidia</groupId> | ||
| <artifactId>rapids-4-spark-parent_2.12</artifactId> | ||
| <artifactId>cudf-spark-parent_2.12</artifactId> |
There was a problem hiding this comment.
Could we implement the compatibility window described in #15882 and #15741 before switching the canonical artifact ID? At the moment jenkins/deploy.sh publishes and validates only project.artifactId, so a 26.10 release would publish cudf-spark_* but nothing under rapids-4-spark_*. That breaks existing Maven/Ivy coordinates and direct JAR URLs instead of supporting both names for several releases. We should publish the legacy relocation POMs, preserve legacy binaries if direct downloads remain supported, and add both coordinate families to the clean-cache dependency checks.
| // cudf-spark_2.12-26.10.0-cuda12.jar!/rapids4spark-version-info.properties | ||
| urlPath.endsWith("!/" + propName) && | ||
| (propName != PLUGIN_PROPS_FILENAME || | ||
| (!urlPath.contains("cudf-spark-") && |
There was a problem hiding this comment.
Could we apply this exclusion to the containing JAR’s filename rather than the complete resource URL? For example, a valid main JAR under /work/cudf-spark-pr-16053/... is excluded because its parent path contains cudf-spark-. revisionMap can then be empty, silently disabling duplicate-plugin detection. The validation in the resolved thread reproduced this case. Please extract the JAR filename, perhaps through JarURLConnection, and add coverage where the parent directory contains `cudf-spark-
Part of #15882, to rename artifactIDs(names) from rapids-4-spark*_2.12/2.13 to cudf-spark*_2.12/2.13 for the cudf-spark project
After this change, the cudf-spark plugin integration tests work with both the rapids-4-spark_2.12/2.13 and cudf-spark_2.12/2.13 JARs. The rapids-4-spark_2.12/2.13 artifacts are backup copies of the corresponding cudf-spark_2.12/2.13 artifacts.
As the old JARs will be deprecated over the next several release cycles, we will update our CI/CD jobs to test against the new cudf-spark_2.12/2.13 JARs.
Summary
cudf-sparkcoordinates.Checklists
Documentation
Testing
Performance
Performance testing is not required because this adds one string check per JAR resource during one-time plugin initialization and does not affect query execution.