-
Notifications
You must be signed in to change notification settings - Fork 236
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Scala 2.13 Support #8592
Scala 2.13 Support #8592
Conversation
Signed-off-by: Navin Kumar <[email protected]>
Signed-off-by: Navin Kumar <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One question about the goal here, are we planning to do
- multiple profiles to build different shims w/ different scalas, and we will deliver 2 sets of artifacts here (2.12 plugin for all spark shims, 2.13 plugin for 330+ shims)
- Or build the plugin using scala 2.12 for spark shims 330-, and scala 2.13 for 330+ as default. We still just release one plugin artifact including 2.12 330- shims, 2.13 330+ shims, and 2.12/2.13 common parts
also cc @sameerz @GaryShen2008
We will release 2 sets of artifacts, a scala 2.12 plugin for all spark shims, and a scala 2.13 plugin for 3.3.0+ shims. |
Got it, thanks for the confirmation! I will follow up CI setup after this PR along w/ the internal private one |
…n.Seq from Spark 3.4 and higher. This is to avoid unnecessary copying of data.
… of converting when returning Seq and using Seq in _* pattern
53a44aa
to
85c0af9
Compare
…a 2.13 and will cause compilation to fail Signed-off-by: Navin Kumar <[email protected]>
Remove checking scala2.13 folder, .github/workflows/mvn-verify-check.yml will check it Signed-off-by: Tim Liu <[email protected]>
@NVnavkumar Sorry I added dup I noticed you've already add the git diff check for it in the .github/workflows/mvn-verify-check.yml file So I pushed a commit here to revert it: |
build |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
||
That way any new dependencies or other changes will be picked up in the Scala 2.13 build. | ||
|
||
## IDE Integration with IntelliJ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this addressed did we file issue?
sql-plugin/src/main/scala-2.13/com/nvidia/spark/rapids/ArmScalaFixes.scala
Outdated
Show resolved
Hide resolved
sql-plugin/src/main/scala/com/nvidia/spark/rapids/GpuAggregateExec.scala
Outdated
Show resolved
Hide resolved
sql-plugin/src/main/scala-2.12/com/nvidia/spark/rapids/GpuBaseAggregateHelper.scala
Outdated
Show resolved
Hide resolved
sql-plugin/src/main/spark340/scala/com/nvidia/spark/rapids/shuffle/RapidsShuffleIterator.scala
Show resolved
Hide resolved
CONTRIBUTING.md
Outdated
@@ -60,6 +60,9 @@ You can find all available build versions in the top level pom.xml file. If you | |||
for Databricks then you should use the `jenkins/databricks/build.sh` script and modify it for | |||
the version you want. | |||
|
|||
See the [scala2.13](scala2.13) directory for instructions on how to build against |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
might also be nice to state that the code must compile against both versions so when developing keep that in mind. Not sure if its useful to mention a few common things that arise. Also might point to the 2.13/2.12 source subdirectories
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added some documentation updates, please let me know if those make sense.
sql-plugin/src/main/scala-2.12/com/nvidia/spark/rapids/ArmScalaFixes.scala
Outdated
Show resolved
Hide resolved
sql-plugin/src/main/scala-2.13/com/nvidia/spark/rapids/ArmScalaFixes.scala
Outdated
Show resolved
Hide resolved
sql-plugin/src/main/scala-2.13/com/nvidia/spark/rapids/GpuBaseAggregateHelper.scala
Outdated
Show resolved
Hide resolved
sql-plugin/src/main/scala-2.13/com/nvidia/spark/rapids/GpuSorter.scala
Outdated
Show resolved
Hide resolved
…the Stack implementation Signed-off-by: Navin Kumar <[email protected]>
Signed-off-by: Navin Kumar <[email protected]>
build |
Signed-off-by: Navin Kumar <[email protected]>
build |
Signed-off-by: Navin Kumar <[email protected]>
build |
Signed-off-by: Navin Kumar <[email protected]>
Signed-off-by: Navin Kumar <[email protected]>
Signed-off-by: Navin Kumar <[email protected]>
build |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
overall looks fine, please file the followups from the comments
build |
Fixes #1525 and #9331.
This fixes the compilation errors, and provides the updates to the build system to enable spark-rapids to build with Scala 2.13.
You can build it by changing to the
scala2.13
directory, and running maven:cd scala2.13 mvn verify
You can also open the scala2.13 directory directly in IntelliJ IDE and it should work very similarly to 2.12.
See
scala2.13/README.md
for more information.Signed-off-by: Navin Kumar [email protected]