Skip to content
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

[VL] Add Scala 2.13 support #6326

Merged
merged 30 commits into from
Aug 10, 2024
Merged

Conversation

Preetesh2110
Copy link
Contributor

What changes were proposed in this pull request?

Add Scala 2.13 support

(Fixes: #6316)

How was this patch tested?

Build locally

(If this patch involves UI changes, please attach a screenshot; otherwise, remove this)

Copy link

github-actions bot commented Jul 3, 2024

Thanks for opening a pull request!

Could you open an issue for this pull request on Github Issues?

https://github.com/apache/incubator-gluten/issues

Then could you also rename commit message and pull request title in the following format?

[GLUTEN-${ISSUES_ID}][COMPONENT]feat/fix: ${detailed message}

See also:

Copy link

github-actions bot commented Jul 3, 2024

Run Gluten Clickhouse CI

Copy link

github-actions bot commented Jul 3, 2024

Run Gluten Clickhouse CI

@surnaik
Copy link
Contributor

surnaik commented Jul 4, 2024

@zhztheplayer Could you please review this PR? Thanks!

@LuciferYang
Copy link

Prior to Spark 4.0, Scala 2.13 was not the default Scala version, and I personally do not recommend supporting it for Spark 3.x.

@surnaik
Copy link
Contributor

surnaik commented Jul 8, 2024

Thanks @LuciferYang, I think Scala 2.13 support is already added for CH backend, plan was to extend to VL backend as well, since the build fails when you enable -Pbackends-velox -Pscala-2.13, that was the plan, does that make sense.

FYI, the support for Scala 2.13 was added here already - #6311

cc: @zhztheplayer

@surnaik
Copy link
Contributor

surnaik commented Jul 8, 2024

@Preetesh2110 please take a look at failing CI and maybe also add a CI option to build with Scala 2.13

@LuciferYang
Copy link

Thanks @LuciferYang, I think Scala 2.13 support is already added for CH backend, plan was to extend to VL backend as well, since the build fails when you enable -Pbackends-velox -Pscala-2.13, that was the plan, does that make sense.

FYI, the support for Scala 2.13 was added here already - #6311

cc: @zhztheplayer

Got

Copy link

Run Gluten Clickhouse CI

@Preetesh2110
Copy link
Contributor Author

thanks @surnaik. fixed the scala Style issues.

@zhztheplayer
Copy link
Member

Do we need to have a test for Scala 2.13 (with Spark 3.5 probably) ? https://github.com/apache/incubator-gluten/blob/main/.github/workflows/velox_docker.yml

Copy link

Run Gluten Clickhouse CI

@Preetesh2110
Copy link
Contributor Author

Thanks for the feedback. I have added a workflow @zhztheplayer.

@zhztheplayer
Copy link
Member

Run Gluten Clickhouse CI

Comment on lines 1092 to 1097
- name: Build and Run unit test for Spark 3.5.1 with Scala 2.13 (other tests)
run: |
cd $GITHUB_WORKSPACE/
export SPARK_SCALA_VERSION=2.13
$MVN_CMD clean install -Pspark-3.5 -Pscala-2.13 -Pbackends-velox -Pceleborn -Piceberg -Pdelta -Pspark-ut -DargLine="-Dspark.test.home=$GITHUB_WORKSPACE//shims/spark35/spark_home/" -DtagsToExclude=org.apache.spark.tags.ExtendedSQLTest,org.apache.gluten.tags.UDFTest,org.apache.gluten.tags.SkipTestTags && \
$MVN_CMD test -Pspark-3.5 -Pscala-2.13 -Pbackends-velox -Piceberg -Pdelta -DtagsToExclude=None -DtagsToInclude=org.apache.gluten.tags.UDFTest
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could make job run-spark-test-spark35 much slower.

If so, probably we should add a job run-spark-test-spark35-scala213. cc @zhouyuan @PHILO-HE

Copy link
Contributor Author

@Preetesh2110 Preetesh2110 Jul 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Thanks @zhztheplayer

Copy link

Run Gluten Clickhouse CI

Copy link

Run Gluten Clickhouse CI

Copy link

Run Gluten Clickhouse CI

Copy link

Run Gluten Clickhouse CI

Copy link

Run Gluten Clickhouse CI

Copy link

github-actions bot commented Aug 6, 2024

Run Gluten Clickhouse CI

@Preetesh2110
Copy link
Contributor Author

Preetesh2110 commented Aug 6, 2024

@Preetesh2110

Thank you for keeping working on this!

BTW, in case you may wait for CI to be triggered, you can open a PR in your own repo to run CI if you want.

Hey @zhztheplayer thanks for your message, how can I trigger the CIs, I have created a PR in my own repo Preetesh2110#1.

Copy link

github-actions bot commented Aug 6, 2024

Run Gluten Clickhouse CI

Copy link

github-actions bot commented Aug 6, 2024

Run Gluten Clickhouse CI

Copy link

github-actions bot commented Aug 6, 2024

Run Gluten Clickhouse CI

Copy link

github-actions bot commented Aug 6, 2024

Run Gluten Clickhouse CI

Copy link

github-actions bot commented Aug 6, 2024

Run Gluten Clickhouse CI

@zhztheplayer
Copy link
Member

@Preetesh2110
Thank you for keeping working on this!
BTW, in case you may wait for CI to be triggered, you can open a PR in your own repo to run CI if you want.

Hey @zhztheplayer thanks for your message, how can I trigger the CIs, I have created a PR in my own repo Preetesh2110#1.

You can sync gluten's main branch to your own main branch, then open a PR targeting to your own main branch. The CI should be directly runnable.

Anyway you can still update here if it's OK for you to wait for CI to be triggered.

Once you have one PR merged you will not get blocked in your subsequent PRs.

Copy link

github-actions bot commented Aug 7, 2024

Run Gluten Clickhouse CI

Copy link

github-actions bot commented Aug 7, 2024

Run Gluten Clickhouse CI

Copy link

github-actions bot commented Aug 8, 2024

Run Gluten Clickhouse CI

Copy link

github-actions bot commented Aug 8, 2024

Run Gluten Clickhouse CI

@zhouyuan
Copy link
Contributor

zhouyuan commented Aug 9, 2024

@Preetesh2110 FYI, below mvn command can fix the style issues so this will not stop CI
https://github.com/apache/incubator-gluten/blob/main/docs/developers/NewToGluten.md?plain=1#L111

thanks, -yuan

Copy link
Contributor

@surnaik surnaik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - Non binding

@surnaik
Copy link
Contributor

surnaik commented Aug 9, 2024

@zhztheplayer please take a look, thanks!
cc: @weiting-chen

Copy link
Member

@zhztheplayer zhztheplayer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 overall. I haven't try with the feature yet but assume CI should cover the changes.

Thank your for the effort @Preetesh2110

.scalafmt.conf Show resolved Hide resolved
@weiting-chen weiting-chen merged commit 51d0a37 into apache:main Aug 10, 2024
43 checks passed
weiting-chen pushed a commit to weiting-chen/gluten that referenced this pull request Aug 10, 2024
* [VL] Add Scala 2.13 support

* Fix scalaStyle issues

* Fix Scala Style issues

* Add Spark 3.5.1 and Scala 2.13 test in workflow

* Add run-spark-test-spark35-scala213 job

* Add Spark 3.5.1 and Scala 2.13 test in workflow

* Fix tests failures

* Fix tests failures

* ScalaStyle fix

* Fix SoftAffinitySuite

* Fix ArrowUtil error

* Fix backend-velox scala issues

* Fix ColumnarArrowEvalPythonExec issues

* Fix ColumnarArrowEvalPythonExec issues

* Fix TestOperator.scala for style issues

* Fix TestOperator.scala for style issues

* Fix issues in DeltaRewriteTransformerRules.scala

* DeltaRewriteTransformerRules fix

* Fix style issues

* Fix issues

* Fix issues

* Fix issues

* Fix issues

* Fix issues

* Fix issues

* Fix issues

---------

Co-authored-by: Hongze Zhang <[email protected]>
weiting-chen added a commit that referenced this pull request Aug 12, 2024
* [GLUTEN-6612] Fix ParquetFileFormat issue caused by the setting of local property isNativeApplicable (#6627)

* [CORE] Fix schema mismatch between ReadRelNode and LocalFilesNode (#6746)

Co-authored-by: 蒋添 <[email protected]>

* [UT] Test input_file_name, input_file_block_start & input_file_block_length when scan falls back (#6318)

* [VL] Fix E function fallback issue (#6397)

* [VL] Add Scala 2.13 support (#6326)

* [VL] Add Scala 2.13 support

* Fix scalaStyle issues

* Fix Scala Style issues

* Add Spark 3.5.1 and Scala 2.13 test in workflow

* Add run-spark-test-spark35-scala213 job

* Add Spark 3.5.1 and Scala 2.13 test in workflow

* Fix tests failures

* Fix tests failures

* ScalaStyle fix

* Fix SoftAffinitySuite

* Fix ArrowUtil error

* Fix backend-velox scala issues

* Fix ColumnarArrowEvalPythonExec issues

* Fix ColumnarArrowEvalPythonExec issues

* Fix TestOperator.scala for style issues

* Fix TestOperator.scala for style issues

* Fix issues in DeltaRewriteTransformerRules.scala

* DeltaRewriteTransformerRules fix

* Fix style issues

* Fix issues

* Fix issues

* Fix issues

* Fix issues

* Fix issues

* Fix issues

* Fix issues

---------

Co-authored-by: Hongze Zhang <[email protected]>

* [VL] Fix Alinux3 arrow build issue (#6363)

* update velox docker and port PR #6363 for get_velox.sh update

---------

Co-authored-by: PHILO-HE <[email protected]>
Co-authored-by: jiangjiangtian <[email protected]>
Co-authored-by: 蒋添 <[email protected]>
Co-authored-by: 高阳阳 <[email protected]>
Co-authored-by: Preetesh2110 <[email protected]>
Co-authored-by: Hongze Zhang <[email protected]>
Co-authored-by: Joey <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[VL] Add Scala 2.13 support
6 participants