Add 3.5.1-SNAPSHOT Shim by razajafri · Pull Request #9962 · NVIDIA/spark-rapids

razajafri · 2023-12-05T19:10:46Z

This PR adds shims for Spark 3.5.1-SNAPSHOT.

Changes Made:

The following Shimplify command was run

mvn generate-sources -Dshimplify=true -Dshimplify.move=true -Dshimplify.overwrite=true -Dshimplify.add.shim=351 -Dshimplify.add.base=350

The only files that were manually changed were pom.xml and ShimServiceProvider.scala to add the SNAPSHOT version to the VERSIONNAMES. Also removed some empty lines as a result of the above Shimplify command

Added a DecimalUtilShims.scala which calls the respective multiplication method depending on the Spark version. In Spark 3.5.1 and other versions, the multiplication doesn't perform an interim cast and as part of spark-rapids-jni PR another method called mul128 was added which skips the interim cast.
Added ComputeSequenceSize.scala to provide a shim for the new method to calculate sequence size and to make sure it's within limit.
Made relevant changes to GpuBatchScanExec to match the changes in Spark

Tests:
All integration tests were run locally

fixes #9258
fixes #9859
fixes #9875
fixes #9743

Signed-off-by: Raza Jafri <rjafri@nvidia.com>

tgravescs · 2023-12-07T16:10:57Z

I'm assuming your decimal multiple is related to #9859... If so pleas emake sure it fixes it all the way or we comment on that issue. the shim is very hard to read, one calls mul128 the other calls multiply128. I haven't went and looked at those but one its hard to even see that diff so you should in the very least add a comment or point to issue and explain.

razajafri · 2023-12-07T16:59:23Z

the shim is very hard to read, one calls mul128 the other calls multiply128. I haven't went and looked at those but one its hard to even see that diff so you should in the very least add a comment or point to issue and explain.

I will go ahead and put in some comments to highlight the change

razajafri · 2023-12-07T18:04:43Z

I'm assuming your decimal multiple is related to #9859... If so pleas emake sure it fixes it all the way or we comment on that issue

Discussed this offline. I missed the division bit of the puzzle. Will verify division and post an update here

sql-plugin/src/main/spark311/scala/com/nvidia/spark/rapids/shims/DecimalUtilShims.scala

razajafri · 2023-12-08T03:50:55Z

I'm assuming your decimal multiple is related to #9859... If so pleas emake sure it fixes it all the way or we comment on that issue

Discussed this offline. I missed the division bit of the puzzle. Will verify division and post an update here

I have verified the Decimal division and we match Spark 3.5.1 output.

It turns out that we were always doing the right thing on the GPU for decimal division. So to match Spark bug for bug we should "fix" the versions Databricks 330+ and Spark versions 340+ by returning the bad answer. I have created an issue for that here

…eflect the method name

razajafri · 2023-12-18T19:38:41Z

build

razajafri · 2023-12-18T22:02:02Z

premerge failing due to an unrelated change

[2023-12-18T20:02:45.179Z] [ERROR] /home/ubuntu/spark-rapids/tests/src/test/scala/org/apache/spark/sql/rapids/metrics/source/MockTaskContext.scala:69: overriding method getKillReason in class TaskContext of type ()Option[String];
[2023-12-18T20:02:45.180Z]  method getKillReason has weaker access privileges; it should be public
[2023-12-18T20:02:45.180Z] [ERROR]   override private[spark] def getKillReason() = None
[2023-12-18T20:02:45.180Z] [ERROR]                               ^
[2023-12-18T20:02:45.180Z] [ERROR] one error found

razajafri · 2023-12-18T22:05:39Z

@NvTimLiu @pxLi why is the premerge picking up databricks 13.3

razajafri · 2023-12-19T00:14:23Z

build

razajafri · 2023-12-19T01:22:47Z

build

razajafri · 2023-12-19T01:23:50Z

@NvTimLiu @pxLi why is the premerge picking up databricks 13.3

Nevermind, I saw that there is a function that checks if there are any changes in the Databricks shims in addition to the word databricks in the title of the PR

razajafri · 2023-12-19T05:35:22Z

build

razajafri · 2023-12-20T16:43:28Z

build

razajafri · 2023-12-20T17:02:48Z

build

This reverts commit 533504f.

razajafri · 2023-12-20T17:23:25Z

I have reverted the tests for versions that we don't support yet. They will be added in other shims

razajafri · 2023-12-20T17:23:29Z

build

razajafri · 2023-12-20T17:25:45Z

@andygrove can you PTAL?

gerashegalov · 2023-12-20T21:29:29Z

sql-plugin/src/main/scala/org/apache/spark/sql/rapids/arithmetic.scala

+          throw RapidsErrorUtils.
+            arithmeticOverflowError("One or more rows overflow for Add operation.")


let us leave formatting-only changes to dedicated PRs

gerashegalov · 2023-12-20T21:40:44Z

sql-plugin/src/main/spark311/scala/com/nvidia/spark/rapids/shims/ComputeSequenceSizes.scala

+      withResource(actualSize) { _ =>
+        val mergedEquals = withResource(start.equalTo(stop)) { equals =>
+          if (step.hasNulls) {
+            // Also set the row to null where step is null.
+            equals.mergeAndSetValidity(BinaryOp.BITWISE_AND, equals, step)
+          } else {
+            equals.incRefCount()
+          }
+        }
+        withResource(mergedEquals) { _ =>
+          mergedEquals.ifElse(one, actualSize)
+        }
+      }
+    }
+
+    withResource(sizeAsLong) { _ =>
+      // check max size
+      withResource(Scalar.fromInt(MAX_ROUNDED_ARRAY_LENGTH)) { maxLen =>
+        withResource(sizeAsLong.lessOrEqualTo(maxLen)) { allValid =>
+          require(isAllValidTrue(allValid),
+            s"Too long sequence found. Should be <= $MAX_ROUNDED_ARRAY_LENGTH")
+        }
+      }
+      // cast to int and return
+      sizeAsLong.castTo(DType.INT32)
+    }
+  }


The bottom portion L85-L111 in 311 and L98-L126 in 351 differ only in the require message let us refactor to minimize shimming

gerashegalov · 2023-12-20T21:47:33Z

.../src/main/spark350/scala/com/nvidia/spark/rapids/shims/api/python/ShimBasePythonRunner.scala

This file should be dropped thanks to #9902

razajafri · 2023-12-21T02:46:05Z

Thanks for taking a look @gerashegalov

PTAL

razajafri · 2023-12-21T02:48:56Z

build

NvTimLiu

LGTM

NvTimLiu · 2023-12-25T02:57:49Z

Will we plan to run nightly integration tests against spark-3.5.1-SNAPSHOT?

razajafri · 2023-12-25T07:18:50Z

Will we plan to run nightly integration tests against spark-3.5.1-SNAPSHOT?

Yes, we do

razajafri added 3 commits December 5, 2023 11:00

Added 351 snapshot shim

0fd8425

Signing off

e6af24f

Signed-off-by: Raza Jafri <rjafri@nvidia.com>

updated 2.13 pom.xml

6b65d39

jlowe previously approved these changes Dec 5, 2023

View reviewed changes

andygrove previously approved these changes Dec 5, 2023

View reviewed changes

razajafri mentioned this pull request Dec 5, 2023

Added Spark-3.4.2 Shims #9967

Merged

sameerz added the feature request New feature or request label Dec 5, 2023

Fixed Decimal 128 Multiplication

175dc8d

razajafri dismissed stale reviews from andygrove and jlowe via 175dc8d December 7, 2023 01:13

added comments to highlight the change in multiply128

51be475

gerashegalov reviewed Dec 7, 2023

View reviewed changes

sql-plugin/src/main/spark311/scala/com/nvidia/spark/rapids/shims/DecimalUtilShims.scala Outdated Show resolved Hide resolved

Removed the import alias

9baf588

razajafri added 4 commits December 11, 2023 20:40

Fixed Sequence size limit check

382db01

changes to use the new multiply128. Changed the name of the shim to r…

99e00f1

…eflect the method name

Handle empty partitions

394e753

upmerged

88f546d

pulled in change from NVIDIA#10070

471bff6

Fixed the overflow check for addition and subtraction

ddd7f8f

razajafri changed the title ~~Added 3.5.1-SNAPSHOT Shim~~ Add 3.5.1-SNAPSHOT Shim Dec 20, 2023

Merge remote-tracking branch 'origin/branch-24.02' into SP-9258-351-shim

08d6a7b

razajafri marked this pull request as ready for review December 20, 2023 16:46

razajafri requested review from GaryShen2008, NvTimLiu, revans2 and tgravescs as code owners December 20, 2023 16:46

Updated test conditions

533504f

Revert "Updated test conditions"

79ed020

This reverts commit 533504f.

razajafri requested review from andygrove and gerashegalov December 20, 2023 18:40

andygrove previously approved these changes Dec 20, 2023

View reviewed changes

gerashegalov requested changes Dec 20, 2023

View reviewed changes

addressed review comments

51cae93

razajafri dismissed andygrove’s stale review via 51cae93 December 21, 2023 02:35

renamed

0afe19e

gerashegalov approved these changes Dec 21, 2023

View reviewed changes

NvTimLiu approved these changes Dec 25, 2023

View reviewed changes

razajafri merged commit 11a91d4 into NVIDIA:branch-24.02 Dec 25, 2023

razajafri deleted the SP-9258-351-shim branch December 25, 2023 07:18

NVnavkumar mentioned this pull request Jan 17, 2024

Add spark350emr shim layer [EMR] #10202

Closed

		throw RapidsErrorUtils.
		arithmeticOverflowError("One or more rows overflow for Add operation.")

Conversation

razajafri commented Dec 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tgravescs commented Dec 7, 2023

Uh oh!

razajafri commented Dec 7, 2023

Uh oh!

razajafri commented Dec 7, 2023

Uh oh!

Uh oh!

razajafri commented Dec 8, 2023

Uh oh!

razajafri commented Dec 18, 2023

Uh oh!

razajafri commented Dec 18, 2023

Uh oh!

razajafri commented Dec 18, 2023

Uh oh!

razajafri commented Dec 19, 2023

Uh oh!

razajafri commented Dec 19, 2023

Uh oh!

razajafri commented Dec 19, 2023

Uh oh!

razajafri commented Dec 19, 2023

Uh oh!

razajafri commented Dec 20, 2023

Uh oh!

razajafri commented Dec 20, 2023

Uh oh!

razajafri commented Dec 20, 2023

Uh oh!

razajafri commented Dec 20, 2023

Uh oh!

razajafri commented Dec 20, 2023

Uh oh!

gerashegalov Dec 20, 2023

Choose a reason for hiding this comment

Uh oh!

gerashegalov Dec 20, 2023

Choose a reason for hiding this comment

Uh oh!

gerashegalov Dec 20, 2023

Choose a reason for hiding this comment

Uh oh!

razajafri commented Dec 21, 2023

Uh oh!

razajafri commented Dec 21, 2023

Uh oh!

NvTimLiu left a comment

Choose a reason for hiding this comment

Uh oh!

NvTimLiu commented Dec 25, 2023

Uh oh!

razajafri commented Dec 25, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

razajafri commented Dec 5, 2023 •

edited

Loading