Add Spark master shim support[reduced-it][fast-ut] - #15517
Conversation
Signed-off-by: Firestarman <firestarmanllc@gmail.com>
|
Too many files changed for review (226 files, 100 file limit). Bypass the limit by tagging |
Resolve binary-dedupe conflicts for Spark 5 and keep the latest Spark 500 compatibility fixes on top of main. Signed-off-by: Firestarman <firestarmanllc@gmail.com>
Remove stale conflict markers from the merge resolution and keep Spark 5 shim matching on top of the new unshimming flow. Signed-off-by: Firestarman <firestarmanllc@gmail.com>
Include Spark 3.5.9 in the pre-Spark 5 partitioning shim headers and leave Spark 5 to its dedicated shim sources. Signed-off-by: Firestarman <firestarmanllc@gmail.com>
Resolve the SparkShims split by keeping Spark 4.2 support on the new Spark 4.2 shim files and carrying Spark 5.0 on the same baseline. Signed-off-by: Firestarman <firestarmanllc@gmail.com>
|
build |
|
ci failuresa are due to missing the latest JNI jar on Artifactory. |
|
build |
Keep the latest main changes and restore the Delta deletion-vector scan helper that the integration tests import. Signed-off-by: Firestarman <firestarmanllc@gmail.com>
|
build |
res-life
left a comment
There was a problem hiding this comment.
I found four issues in this change.
| 'date_trunc("MICROSECOND", a)', | ||
| 'date_trunc("invalid", a)')) | ||
|
|
||
| _LONG_MIN_TIMESTAMP_MICROS = -9223372036854775808 |
There was a problem hiding this comment.
These two date_trunc Long.MinValue tests cover the two distinct overflow-check paths, including the scalar-timestamp/column-format case that previously caught a shape mismatch. Spark 5 continues to reuse the Spark 4.2 overflow shim, so deleting them leaves that behavior unprotected and is unrelated to the Spark master shim work. Could you restore both tests and the is_spark_420_or_later import?
There was a problem hiding this comment.
Restored both date_trunc Long.MinValue tests and the Spark 4.2+ helper import.
| <artifactId>rapids-4-spark-parent_2.12</artifactId> | ||
| <name>NVIDIA cuDF plugin for Apache Spark Root Project</name> | ||
| <description>The root project of the NVIDIA cuDF plugin for Apache Spark</description> | ||
| <name>RAPIDS Accelerator for Apache Spark Root Project</name> |
There was a problem hiding this comment.
This reverts the completed cuDF Spark rename and publishes the former project name. The SCM block below is likewise changed from the canonical NVIDIA/cudf-spark repository back to the legacy NVIDIA/spark-rapids alias, and the same regression appears in scala2.13/pom.xml. Could you restore the cuDF project name, description, and canonical SCM coordinates in both POMs?
There was a problem hiding this comment.
Restored the cuDF project metadata and canonical NVIDIA/cudf-spark SCM coordinates in both POMs.
| def unregisterShuffle(shuffleId: Int): Boolean = realImpl.unregisterShuffle(shuffleId) | ||
|
|
||
| def shuffleBlockResolver: ShuffleBlockResolver = realImpl.shuffleBlockResolver | ||
| def shuffleBlockResolver: ShuffleBlockResolver = |
There was a problem hiding this comment.
shuffleBlockResolver is accessed repeatedly on the local shuffle-block fetch path, so this now performs getMethod and invoke for every fetched block. The delegated resolver is stable and the real implementation stores it in a lazy val. Could you keep the reflective compatibility lookup but cache its result in a private lazy val?
There was a problem hiding this comment.
Updated to keep the reflective compatibility path but cache the resolved ShuffleBlockResolver in a private lazy val.
| * Returns the executor-resolved PerfIO S3 enablement. This includes opportunistic | ||
| * enablement when the configuration is unset and the required classes are available. | ||
| */ | ||
| public static boolean isS3PerfEnabled() { |
There was a problem hiding this comment.
This method no longer returns the executor-resolved PerfIO state described by its contract. It independently re-evaluates the raw setting and classpath, and its S3Client/S3CrtAsyncClient probes do not match PerfIO's S3AsyncClient and selected HTTP-backend resolution. The two decisions can therefore disagree: this can select S3InputFile while PerfIO is disabled, causing the read to fail instead of falling back to S3A. Could this delegate to com.nvidia.spark.rapids.PerfIO$.MODULE$.isS3PerfEnabled(), matching the existing GCS accessor?
There was a problem hiding this comment.
Updated isS3PerfEnabled to delegate to PerfIO, matching the GCS accessor.
| )) | ||
|
|
||
| @pytest.mark.parametrize('data_gen', double_n_long_gens + _arith_decimal_gens_no_neg_scale + [DecimalGen(30, 15)], ids=idfn) | ||
| @disable_ansi_mode |
There was a problem hiding this comment.
Any reason we are disabling the ansi mode here ? If it's to mask the test failures, how are we planning to track it?
There was a problem hiding this comment.
Narrowed this so ANSI is disabled only for the double special-value floor/ceil cases; long and decimal coverage now keeps the default ANSI path.
There was a problem hiding this comment.
Thanks @firestarman ! So looks like we have to fix the double special-value floor/ceil cases later in Spark-5.0 - apache/spark#57127 ?
I am confused should we let the tests fail and then have it fixed it later? Or how are we planning to track such tasks for Spark-5.0 once we merge this PR.
Signed-off-by: Firestarman <firestarmanllc@gmail.com>
Signed-off-by: Firestarman <firestarmanllc@gmail.com>
Signed-off-by: Firestarman <firestarmanllc@gmail.com>
Signed-off-by: Firestarman <firestarmanllc@gmail.com>
|
build |
Signed-off-by: Firestarman <firestarmanllc@gmail.com>
|
build |
| or is_spark_420_or_later() | ||
| ), | ||
| reason="Requires Spark's partial-clustering correctness fix") | ||
| @pytest.mark.xfail( |
There was a problem hiding this comment.
is there a issue to track? It's better to add a issue link in the "reason", e.g.:
reason="issue=https://xxx: Spark 5 requires KeyedPartitionings in a PartitioningCollection to share keys")
There was a problem hiding this comment.
Added a dedicated tracking issue and updated the xfail reason to include #15688.
| ) | ||
|
|
||
| @pytest.mark.skipif(is_spark_500_or_later(), | ||
| reason='Spark 5 exact percentile coverage uses the Spark 500 variant') |
There was a problem hiding this comment.
Add an issue link in the reason to track this case better.
Then it will be easy to use a script tool to gather blocking issues in skipif and xfail.
I remember Sameer have a script to search reason lines and then extract issues.
There was a problem hiding this comment.
Updated the Spark 5 exact percentile skip reason to include #15516.
| lambda spark: exact_percentile_reduction(gen_df(spark, data_gen))) | ||
|
|
||
| @pytest.mark.skipif(not is_spark_500_or_later(), | ||
| reason='Spark 5 exact percentile result tolerance') |
There was a problem hiding this comment.
Add an issue link in the reason to track this case better.
Please check all the skipif and xfail to add corresponding issues.
There was a problem hiding this comment.
Checked the related Spark 5 exact percentile skip/xfail reasons and routed the shared skip reasons through constants that include #15516.
|
|
||
| def shuffleBlockResolver: ShuffleBlockResolver = realImpl.shuffleBlockResolver | ||
| private lazy val realShuffleBlockResolver: ShuffleBlockResolver = | ||
| realImpl.getClass.getMethod("shuffleBlockResolver") |
There was a problem hiding this comment.
We can introduce a shim to do the different behavior between Spark versions, it's better to remove the reflection, we should avoid using reflection as we can.
There was a problem hiding this comment.
Removed the reflection path. The proxy now accesses the stable resolver through the version-independent shuffle delegate interface.
| def readFooterBufferFromInputFile( | ||
| inputFile: RapidsInputFile, | ||
| filePath: Path): HostMemoryBuffer = { | ||
| val fileLen = inputFile.getLength |
There was a problem hiding this comment.
Ray (Renjie Liu)'s PR #15384 already optimized this function, ParquetFooterUtils invokes PerfIO.readParquetFooterBuffer(...)
Seems this change in here is unnecessary? it's here because of merge conflict?
There was a problem hiding this comment.
Reverted this back to PerfIO.readParquetFooterBuffer(...); this change was not needed for Spark 500.
|
And we need to consider the Iceberg and Delta supports. |
|
Updated #14547 with Iceberg and Delta follow-up sub-issues. |
Signed-off-by: Firestarman <firestarmanllc@gmail.com>
|
build |
|
|
res-life
left a comment
There was a problem hiding this comment.
LGTM, but please test all the UTs and ITs
Alreayd done, you can find the results in PR description. |
NvTimLiu
left a comment
There was a problem hiding this comment.
LGTM from CICD point of view, +1
Fixes #14547.
Description
sparkMasterprofile for Spark 500/master so CI can explicitly track Spark master without depending on a release-profile name.SPARK_HOME=<Spark 5.0.0-SNAPSHOT binary> INTEGRATION_TEST_VERSION_OVERRIDE=spark500 ./run_pyspark_from_build.sh --test_oom_injection_mode=neverpassed with 39734 testcases, 0 failures, and 0 errors.SPARK_HOME=<Spark 5.0.0-SNAPSHOT binary> INTEGRATION_TEST_VERSION_OVERRIDE=spark500 mvn -f scala2.13/pom.xml -PsparkMaster -Dcuda.version=cuda13 clean verifypassed.Checklists
Documentation
Testing
(Please provide the names of the existing tests in the PR description.)
Performance