Skip to content

[GLUTEN-3475] Add spark35 scala213#3476

Closed
holdenk wants to merge 29 commits intoapache:mainfrom
holdenk:add-spark35-scala213
Closed

[GLUTEN-3475] Add spark35 scala213#3476
holdenk wants to merge 29 commits intoapache:mainfrom
holdenk:add-spark35-scala213

Conversation

@holdenk
Copy link
Copy Markdown
Contributor

@holdenk holdenk commented Oct 20, 2023

What changes were proposed in this pull request?

(Please fill in changes proposed in this fix)

Fixes: #3475

How was this patch tested?

In progress, existing tests and compiling

See https://www.youtube.com/watch?v=UbB7v2f7A6k

@github-actions
Copy link
Copy Markdown

Thanks for opening a pull request!

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

https://github.com/oap-project/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:

@github-actions
Copy link
Copy Markdown

Run Gluten Clickhouse CI

1 similar comment
@github-actions
Copy link
Copy Markdown

Run Gluten Clickhouse CI

@FelixYBW FelixYBW requested a review from JkSelf October 31, 2023 04:27
Copy link
Copy Markdown
Contributor

@LuciferYang LuciferYang left a comment

Choose a reason for hiding this comment

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

Seems that some code has not been uploaded yet? I haven't been able to compile successfully locally yet

mvn clean package -Pbackends-velox -Prss -Pspark-3.3 -DskipTests
mvn clean package -Pbackends-velox -Prss -Pspark-3.4 -DskipTests
mvn clean package -Pbackends-velox -Prss -Pspark-3.5 -DskipTests
# No celbron Scala 2.13 support yet.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Typo: celeborn

<groupId>org.scalacheck</groupId>
<artifactId>scalacheck_${scala.binary.version}</artifactId>
<version>1.13.5</version>
<version>1.14.3</version>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

https://github.com/oap-project/gluten/blob/d843bd93b460d6988c5d654d57978d096b0a835a/backends-clickhouse/pom.xml#L106-L117

There are some related dependencies that use hard coding _ 2.12 in this pom.xml, they should also use ${scala.binary.version}, I suspect there are other similar cases

private var executorEndpoint: GlutenExecutorEndpoint = _
private val taskListeners: Seq[TaskListener] = Array(TaskResources)
private val taskListeners: Seq[TaskListener] =
(immutable.ArraySeq.unsafeWrapArray(Array(TaskResources)))
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

immutable.ArraySeq.unsafeWrapArray is a Scala 2.13 only api, if this performance optimization is necessary, this file may need to be split into Scala 2.12 and Scala 2.13 versions

import java.util.ServiceLoader

import scala.collection.JavaConverters
import scala.jdk.CollectionConverters._
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

IIRC, scala.jdk.CollectionConverters also Scala 2.13 only

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ah good point, looks like there is a backwards compact lib ( https://stackoverflow.com/questions/59197883/version-agnostic-way-to-convert-from-java-to-scala-collections-and-back ) I'll try for Scala 2.12 support

@github-actions
Copy link
Copy Markdown

Run Gluten Clickhouse CI

@holdenk
Copy link
Copy Markdown
Contributor Author

holdenk commented Nov 11, 2023

Still WIP but made some more progress

@github-actions
Copy link
Copy Markdown

Run Gluten Clickhouse CI

@holdenk holdenk force-pushed the add-spark35-scala213 branch from 18f96d8 to 32f8cfb Compare November 13, 2023 04:34
@github-actions
Copy link
Copy Markdown

Run Gluten Clickhouse CI

3 similar comments
@github-actions
Copy link
Copy Markdown

Run Gluten Clickhouse CI

@github-actions
Copy link
Copy Markdown

Run Gluten Clickhouse CI

@github-actions
Copy link
Copy Markdown

Run Gluten Clickhouse CI

@holdenk
Copy link
Copy Markdown
Contributor Author

holdenk commented Nov 21, 2023

@ LuciferYang any chance I can get read access to Jenkins?

@holdenk holdenk force-pushed the add-spark35-scala213 branch from 31aa990 to 7e1916c Compare November 21, 2023 02:50
@github-actions
Copy link
Copy Markdown

Run Gluten Clickhouse CI

1 similar comment
@github-actions
Copy link
Copy Markdown

Run Gluten Clickhouse CI

@holdenk holdenk changed the title [WIP][3475] Add spark35 scala213 [VL-3475] Add spark35 scala213 Nov 21, 2023
@holdenk holdenk changed the title [VL-3475] Add spark35 scala213 [3475] Add spark35 scala213 Nov 21, 2023
@holdenk holdenk changed the title [3475] Add spark35 scala213 [GLUTEN-3475] Add spark35 scala213 Nov 21, 2023
@github-actions
Copy link
Copy Markdown

#3475

@holdenk holdenk force-pushed the add-spark35-scala213 branch from 29cc597 to 19c02f8 Compare November 22, 2023 01:36
@github-actions
Copy link
Copy Markdown

Run Gluten Clickhouse CI

@holdenk holdenk force-pushed the add-spark35-scala213 branch from 97e70f1 to 7fa97e9 Compare November 22, 2023 01:37
@github-actions
Copy link
Copy Markdown

Run Gluten Clickhouse CI

1 similar comment
@github-actions
Copy link
Copy Markdown

Run Gluten Clickhouse CI

@holdenk
Copy link
Copy Markdown
Contributor Author

holdenk commented Nov 22, 2023

@LuciferYang any chance I can get read access to Jenkins?

@LuciferYang
Copy link
Copy Markdown
Contributor

@LuciferYang any chance I can get read access to Jenkins?

Sorry, I'm not familiar with this matter.

cc @zhouyuan

@github-actions
Copy link
Copy Markdown

Run Gluten Clickhouse CI

@holdenk holdenk force-pushed the add-spark35-scala213 branch from c359501 to 852487c Compare November 22, 2023 21:26
@holdenk holdenk force-pushed the add-spark35-scala213 branch from f23b456 to 19951a9 Compare December 5, 2023 00:46
@github-actions
Copy link
Copy Markdown

github-actions bot commented Dec 5, 2023

Run Gluten Clickhouse CI

@holdenk
Copy link
Copy Markdown
Contributor Author

holdenk commented Dec 25, 2023

Hey y'all, just following up I'm happy to update the PR on top of main if someone can get me access to CI and has the cycles to review, otherwise I'll just leave it be.

@JkSelf
Copy link
Copy Markdown
Contributor

JkSelf commented Dec 26, 2023

@holdenk Thanks for your great work. I have approved the CI running. The changes in this PR are too extensive, making it difficult to review. I noticed that the Scala version in Spark 3.5 is still 2.12 here. So, can we enable only spark 3.5 in this PR and create a separate PR to upgrade the Scala version to 2.13 if necessary? Also, could you please help list some of the main modifications in the PR description to facilitate our review?

@holdenk
Copy link
Copy Markdown
Contributor Author

holdenk commented Dec 26, 2023

@JkSelf thanks! Can I get a log in so I can see the CI results / logs?

As far as splitting out Scala 2.13 / Spark 3.5 changes certainly possible but given everything so far I'd want to know there was someone with merge permissions with the time and interest to do a review.

@JkSelf
Copy link
Copy Markdown
Contributor

JkSelf commented Dec 26, 2023

@holdenk You can click on the Details to view the CI log. I will follow up with the review later. Thanks.
image

@holdenk
Copy link
Copy Markdown
Contributor Author

holdenk commented Dec 26, 2023

So the Jenkins instance requires a login, without that I'm not sure how to fully debug the issues which come up when I fix the GHA issues.

Also before I make another PR would like to know who to work with on review for Spark 3.5.

@JkSelf
Copy link
Copy Markdown
Contributor

JkSelf commented Dec 26, 2023

So the Jenkins instance requires a login, without that I'm not sure how to fully debug the issues which come up when I fix the GHA issues.

@zhztheplayer Do you have any input? Thanks.

@zhztheplayer
Copy link
Copy Markdown
Member

zhztheplayer commented Dec 26, 2023

The Jenkins CI is only for ClickHouse backend, username/password: gluten/hN2xX3uQ4m , see ref.

@holdenk
Copy link
Copy Markdown
Contributor Author

holdenk commented Dec 26, 2023

Awesome, thanks for the login.

@JkSelf
Copy link
Copy Markdown
Contributor

JkSelf commented Dec 26, 2023

@holdenk It appears that the Velox backends jobs have also failed. Please ensure that all Velox and ClickHouse backends pass successfully. Thanks.

@holdenk
Copy link
Copy Markdown
Contributor Author

holdenk commented Dec 26, 2023

So if you're down to review the change sure I'll make a Scala 2.12 PR once I've got some time (maybe later this week or next), but unless there's a reviewer available I'm not working on this anymore.

To be clear: I totally understand if folks don't have review bandwidth I know how much work that can be from Spark -- but it's the holidays in the US and I only have so much bandwidth.

@JkSelf
Copy link
Copy Markdown
Contributor

JkSelf commented Dec 26, 2023

Yes, Can we merge the Spark 3.5 PR first and then proceed with the Scala 2.13 PR?

@holdenk
Copy link
Copy Markdown
Contributor Author

holdenk commented Dec 26, 2023

Rocking, I'll try to put together a Scala 2.12 Spark 3.5 PR for you to review for the first week of January :)

@github-actions
Copy link
Copy Markdown

This PR is stale because it has been open 45 days with no activity. Remove stale label or comment or this will be closed in 10 days.

@github-actions github-actions bot added the stale stale label Feb 13, 2024
@github-actions
Copy link
Copy Markdown

This PR was auto-closed because it has been stalled for 10 days with no activity. Please feel free to reopen if it is still valid. Thanks.

@github-actions github-actions bot closed this Feb 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

stale stale

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support Spark 3.5 & Scala 2.13

4 participants