Fix 330 build error and add 322 shims layer [databricks] #4447

res-life · 2021-12-31T04:06:08Z

Co-authored-by: Firestarman firestarmanllc@gmail.com
Co-authored-by: Kuhu Shukla kuhus@nvidia.com

Compile is broken for builver=330 after https://issues.apache.org/jira/browse/SPARK-37705, And it also impacted the build for Spark 3.2.2 snapshot. So fix the 2 issues togother in one PR.

Signed-off-by: Chong Gao <res_life@163.com>

Signed-off-by: Firestarman <firestarmanllc@gmail.com>

Signed-off-by: Chong Gao <res_life@163.com>

res-life · 2021-12-31T04:35:01Z

Spark 3.2.1 snapshot stoped updating and have no release 3.2.1, currently it changed to 3.2.2 snapshot.
Let's keep 3.2.1 snapshot shims and create 3.2.2 snapshot shims.
Later if confirm 3.2.1 snapshot is useless, then we can remove 3.2.1 snapshot.

Signed-off-by: Firestarman <firestarmanllc@gmail.com>

firestarman · 2021-12-31T07:50:57Z

build

res-life · 2021-12-31T08:01:52Z

This PR is also about it. @kuhushukla Can you help to review?

Signed-off-by: Firestarman <firestarmanllc@gmail.com>

firestarman · 2021-12-31T09:28:05Z

build

kuhushukla · 2021-12-31T09:33:33Z

The changes prima facie look fine. We could probably name the 322+ and 320+ appropriately and more precisely in order to not confuse what they really mean. At this point we have three such folders including 330+.

Signed-off-by: Chong Gao <res_life@163.com>

kuhushukla · 2021-12-31T09:34:06Z

This PR is also about it. @kuhushukla Can you help to review?

It would be nice if you could combine the two PRS and add a coauthor.

res-life · 2021-12-31T09:34:19Z

build

gerashegalov

This looks good to me. I have mostly nits and some ideas to minimizing the patch but it's ok to address it in a follow-on.

gerashegalov · 2021-12-31T18:11:07Z

sql-plugin/src/main/scala/com/nvidia/spark/rapids/SparkShims.scala

@@ -109,7 +108,8 @@ trait SparkShims {
    pushDownStartWith: Boolean,
    pushDownInFilterThreshold: Int,
    caseSensitive: Boolean,
-    datetimeRebaseMode: LegacyBehaviorPolicy.Value): ParquetFilters
+    lookupFileMeta: String => String,
+    modeByConfig: String): ParquetFilters


keep dateTimeRebase prefix: dateTimeRebaseModeByConfig

nit: maybe dateTimeRebaseModeFromConf ?

Yes totally agree these names are too generic. That or we need to add scala docs to explain what they do.

gerashegalov · 2021-12-31T18:34:34Z

sql-plugin/src/main/301db/scala/com/nvidia/spark/rapids/shims/v2/GpuShuffledHashJoinExec.scala


  override def otherCopyArgs: Seq[AnyRef] = cpuLeftKeys :: cpuRightKeys :: Nil
+
+  override def requiredChildDistribution: Seq[Distribution] =
+    HashClusteredDistribution(cpuLeftKeys) :: HashClusteredDistribution(cpuRightKeys) :: Nil


nit:

override def requiredChildDistribution: Seq[Distribution] = Seq(HashClusteredDistribution(cpuLeftKeys), HashClusteredDistribution(cpuRightKeys))

gerashegalov · 2021-12-31T18:44:16Z

...rc/main/311until330-all/scala/com/nvidia/spark/rapids/shims/v2/GpuShuffledHashJoinExec.scala

+    buildSide match {
+      case BuildRight => GpuBuildRight
+      case BuildLeft => GpuBuildLeft
+      case _ => throw new Exception("unknown buildSide Type")


nit:

case unknownBuildSide => throw new IllegalArgumentException(s"unknown buildSide Type: $unknownBuildSide")

gerashegalov · 2021-12-31T18:49:19Z

...rc/main/311until330-all/scala/com/nvidia/spark/rapids/shims/v2/GpuShuffledHashJoinExec.scala

+    condition,
+    isSkewJoin = isSkewJoin) {
+
+  override def otherCopyArgs: Seq[AnyRef] = cpuLeftKeys :: cpuRightKeys :: Nil


nit: cpuLeftKeys ++ cpuRightKeys ?

gerashegalov · 2021-12-31T18:50:00Z

...rc/main/311until330-all/scala/com/nvidia/spark/rapids/shims/v2/GpuShuffledHashJoinExec.scala

+  override def otherCopyArgs: Seq[AnyRef] = cpuLeftKeys :: cpuRightKeys :: Nil
+
+  override def requiredChildDistribution: Seq[Distribution] =
+    HashClusteredDistribution(cpuLeftKeys) :: HashClusteredDistribution(cpuRightKeys) :: Nil


nit: Seq(HashClusteredDistribution(cpuLeftKeys), HashClusteredDistribution(cpuRightKeys)) ?

gerashegalov · 2021-12-31T18:54:48Z

...ugin/src/main/320until322/scala/com/nvidia/spark/rapids/shims/v2/Spark320until322Shims.scala

+    SQLConf.PARQUET_INT96_REBASE_MODE_IN_READ.key
+  override def int96ParquetRebaseWriteKey: String =
+    SQLConf.PARQUET_INT96_REBASE_MODE_IN_WRITE.key
+  override def hasSeparateINT96RebaseConf: Boolean = true


many members of this trait can be factored out into another mix in trait that is common beyond 322

gerashegalov · 2021-12-31T19:01:44Z

sql-plugin/src/main/330+/scala/com/nvidia/spark/rapids/shims/v2/GpuHashPartitioning.scala

+    }
+  }
+
+ // override def createShuffleSpec(distribution: ClusteredDistribution): ShuffleSpec =


remove code commented out

gerashegalov · 2021-12-31T19:02:40Z

sql-plugin/src/main/330+/scala/com/nvidia/spark/rapids/shims/v2/GpuShuffledHashJoinExec.scala


  override def otherCopyArgs: Seq[AnyRef] = cpuLeftKeys :: cpuRightKeys :: Nil
+
+  override def requiredChildDistribution: Seq[Distribution] =
+    ClusteredDistribution(cpuLeftKeys) :: ClusteredDistribution(cpuRightKeys) :: Nil


nit: Seq(ClusteredDistribution(cpuLeftKeys), ClusteredDistribution(cpuRightKeys))

sameerz · 2022-01-01T17:39:37Z

build

revans2

Overall I just have nits. I would like to at least see a follow on issue to address the names of the new parameters for the parquet filter shim layer and something to look into the TODO comment. Even if it ends up being that we just delete the comment.

revans2 · 2022-01-03T17:50:06Z

sql-plugin/src/main/scala/com/nvidia/spark/rapids/SparkShims.scala

@@ -109,7 +108,8 @@ trait SparkShims {
    pushDownStartWith: Boolean,
    pushDownInFilterThreshold: Int,
    caseSensitive: Boolean,
-    datetimeRebaseMode: LegacyBehaviorPolicy.Value): ParquetFilters
+    lookupFileMeta: String => String,
+    modeByConfig: String): ParquetFilters


Yes totally agree these names are too generic. That or we need to add scala docs to explain what they do.

revans2 · 2022-01-03T17:53:29Z

...ugin/src/main/320until322/scala/com/nvidia/spark/rapids/shims/v2/Spark320until322Shims.scala

+  }
+
+  override def getDateFormatter(): DateFormatter = {
+    // TODO verify


Is this TODO done? I see the same thing in the original Spark32XShims.scala so I am fine if this is a follow on issue. We really don't want a TODO like this in our code.

Signed-off-by: Chong Gao <res_life@163.com>

res-life · 2022-01-04T06:22:27Z

build

firestarman · 2022-01-04T07:05:58Z

LGTM

GaryShen2008

Approved for the last commit for some nit changes.
The previous commits have been approved. Let's merge it.

kuhushukla · 2022-01-04T15:50:07Z

@res-life can you add the follow on issues here and assign them to yourself if you are working on them. We can also assign it to someone else if you would prefer.

tgravescs · 2022-01-05T19:43:19Z

sql-plugin/src/main/322+/scala/com/nvidia/spark/rapids/shims/v2/Spark322PlusShims.scala

+import org.apache.spark.unsafe.types.CalendarInterval
+
+/**
+* Shim base class that can be compiled with every supported 3.2.x


I think the comment here is wrong since only 3.2.2+

tgravescs · 2022-01-05T19:47:47Z

sql-plugin/src/main/322+/scala/com/nvidia/spark/rapids/shims/v2/Spark322PlusShims.scala

+/**
+* Shim base class that can be compiled with every supported 3.2.x
+*/
+trait Spark322PlusShims extends SparkShims with RebaseShims with Logging {


this class appears to be a lot of copied code for what is really a single line change:

< .datetimeRebaseMode(lookupFileMeta, dateTimeRebaseModeFromConf) --- > .datetimeRebaseSpec(lookupFileMeta, dateTimeRebaseModeFromConf)

It feels like much of this could be common with Spark320until322Shims. For instance, can't we put all the common code back into Spark32XShims (in 320+) and then just the single function that is different put into Spark320Shims, Spark320Shims, Spark322Shims. Or alternatively just put that single function in the Spark322PlusShims and Spark320until322Shims which extends the Spark32xShims. Let me know if I'm missing something.

Chong Gao and others added 5 commits December 30, 2021 11:53

Add Shims 322

ab73444

Signed-off-by: Chong Gao <res_life@163.com>

Fix the 330 build errors

5664203

Signed-off-by: Firestarman <firestarmanllc@gmail.com>

Add Shims 322

34c2f8b

Signed-off-by: Chong Gao <res_life@163.com>

Update doc

02604fe

Signed-off-by: Chong Gao <res_life@163.com>

Update comments

80e6577

Signed-off-by: Chong Gao <res_life@163.com>

res-life requested review from GaryShen2008, jlowe, NvTimLiu, revans2 and tgravescs as code owners December 31, 2021 04:06

res-life requested a review from firestarman December 31, 2021 04:06

Fix the build errors related to partitioning

c09026f

Signed-off-by: Firestarman <firestarmanllc@gmail.com>

res-life changed the title ~~Add 322 shims layer~~ Fix 330 build error and add 322 shims layer Dec 31, 2021

firestarman linked an issue Dec 31, 2021 that may be closed by this pull request

[BUG] nightly build 3.3.0 failed: HashClusteredDistribution is not a member of org.apache.spark.sql.catalyst.plans.physical #4432

Closed

firestarman changed the title ~~Fix 330 build error and add 322 shims layer~~ Fix 330 build error and add 322 shims layer [databricks] Dec 31, 2021

firestarman mentioned this pull request Dec 31, 2021

Fix the 330 compiling errors related to hash partitioning #4448

Closed

firestarman mentioned this pull request Dec 31, 2021

[WIP] Fix broken build and rework shims after SPARK-37705 for Spark 3.3.0 #4444

Closed

Correct the improt order

3165724

Signed-off-by: Firestarman <firestarmanllc@gmail.com>

add 322 version in the jenkens version def file

57d6d36

Signed-off-by: Chong Gao <res_life@163.com>

gerashegalov previously approved these changes Dec 31, 2021

View reviewed changes

kuhushukla previously approved these changes Dec 31, 2021

View reviewed changes

sameerz added the build Related to CI / CD or cleanly building label Jan 1, 2022

sameerz added this to the Dec 13 - Jan 7 milestone Jan 1, 2022

revans2 previously approved these changes Jan 3, 2022

View reviewed changes

Refactor

ca270e7

Signed-off-by: Chong Gao <res_life@163.com>

res-life dismissed stale reviews from revans2, kuhushukla, and gerashegalov via ca270e7 January 4, 2022 06:22

res-life linked an issue Jan 4, 2022 that may be closed by this pull request

[BUG] SPARK-37705 breaks parquet filters from Spark 3.3.0 and Spark 3.2.2 onwards #4443

Closed

GaryShen2008 approved these changes Jan 4, 2022

View reviewed changes

GaryShen2008 merged commit 9281caa into NVIDIA:branch-22.02 Jan 4, 2022

res-life assigned res-life, firestarman and kuhushukla Jan 5, 2022

tgravescs reviewed Jan 5, 2022

View reviewed changes

res-life mentioned this pull request Jan 6, 2022

Refactor for shims 320+ #4465

Merged

res-life deleted the add-322-shims branch April 16, 2022 00:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix 330 build error and add 322 shims layer [databricks] #4447

Fix 330 build error and add 322 shims layer [databricks] #4447

res-life commented Dec 31, 2021 •

edited

Loading

res-life commented Dec 31, 2021 •

edited

Loading

firestarman commented Dec 31, 2021

res-life commented Dec 31, 2021

firestarman commented Dec 31, 2021

kuhushukla commented Dec 31, 2021

kuhushukla commented Dec 31, 2021

res-life commented Dec 31, 2021

gerashegalov left a comment

gerashegalov Dec 31, 2021

revans2 Jan 3, 2022

gerashegalov Dec 31, 2021

gerashegalov Dec 31, 2021

gerashegalov Dec 31, 2021

gerashegalov Dec 31, 2021

gerashegalov Dec 31, 2021

gerashegalov Dec 31, 2021

gerashegalov Dec 31, 2021

sameerz commented Jan 1, 2022

revans2 left a comment

revans2 Jan 3, 2022

revans2 Jan 3, 2022

res-life commented Jan 4, 2022

firestarman commented Jan 4, 2022

GaryShen2008 left a comment

kuhushukla commented Jan 4, 2022

tgravescs Jan 5, 2022

tgravescs Jan 5, 2022

Fix 330 build error and add 322 shims layer [databricks] #4447

Fix 330 build error and add 322 shims layer [databricks] #4447

Conversation

res-life commented Dec 31, 2021 • edited Loading

res-life commented Dec 31, 2021 • edited Loading

firestarman commented Dec 31, 2021

res-life commented Dec 31, 2021

firestarman commented Dec 31, 2021

kuhushukla commented Dec 31, 2021

kuhushukla commented Dec 31, 2021

res-life commented Dec 31, 2021

gerashegalov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sameerz commented Jan 1, 2022

revans2 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

res-life commented Jan 4, 2022

firestarman commented Jan 4, 2022

GaryShen2008 left a comment

Choose a reason for hiding this comment

kuhushukla commented Jan 4, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

res-life commented Dec 31, 2021 •

edited

Loading

res-life commented Dec 31, 2021 •

edited

Loading