Skip to content

[SPARK-47234][BUILD] Upgrade Scala to 2.13.13#45342

Closed
panbingkun wants to merge 4 commits intoapache:masterfrom
panbingkun:SPARK-47234
Closed

[SPARK-47234][BUILD] Upgrade Scala to 2.13.13#45342
panbingkun wants to merge 4 commits intoapache:masterfrom
panbingkun:SPARK-47234

Conversation

@panbingkun
Copy link
Contributor

@panbingkun panbingkun commented Feb 29, 2024

What changes were proposed in this pull request?

The pr aims to upgrade scala from 2.13.12 to 2.13.13.

Why are the changes needed?

Does this PR introduce any user-facing change?

Yes, The scala version is changed from 2.13.12 to 2.13.13.

How was this patch tested?

  • Pass GA.
  • After the master is upgraded to this version 2.13.13, we need to continue to observe.

Was this patch authored or co-authored using generative AI tooling?

No.

@panbingkun
Copy link
Contributor Author

panbingkun commented Mar 1, 2024

From GA testing, it seems that ammonite has not yet supported scala 2.13.13.
com.lihaoyi:ammonite_2.13.13:2.5.11

A PR has been submitted to the upstream ammonite community to address this issue:
com-lihaoyi/Ammonite#1430

@dongjoon-hyun
Copy link
Member

Thank you, @panbingkun !

Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

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

Actually, according to the release note, we need to upgrade SBT too.

Users of sbt must upgrade to sbt 1.9.9 to use the console task to access the 2.13.13 REPL (sbt/sbt#7502); error message is NoSuchMethodError: org.jline.utils.AttributedString.fromAnsi

I also saw your SBT 1.9.9 test PR and its failure.

In the worst case, we cannot upgrade to Scala 2.13.13.

@panbingkun
Copy link
Contributor Author

Actually, according to the release note, we need to upgrade SBT too.

Users of sbt must upgrade to sbt 1.9.9 to use the console task to access the 2.13.13 REPL (sbt/sbt#7502); error message is NoSuchMethodError: org.jline.utils.AttributedString.fromAnsi

I also saw your SBT 1.9.9 test PR and its failure.

In the worst case, we cannot upgrade to Scala 2.13.13.

Yes, but unfortunately, from the test results of another PR, it seems that sbt 1.9.9 still has issues starting from version sbt 1.9.4. (When there is a corrupted jar in Maven's local repo, it will fail)

@dongjoon-hyun
Copy link
Member

Yes, that's what I meant exactly. Scala 2.13.13 requires SBT 1.9.9 but SBT 1.9.9 is not working properly. In short, there is no working combination in Scala 2.13.13 and this is the worst case where we are currently standing. I don't know if SBT 1.9.10 might solve every issues, but it's also unlikely.

Yes, but unfortunately, from the test results of another PR, it seems that sbt 1.9.9 still has issues starting from version sbt 1.9.4. (When there is a corrupted jar in Maven's local repo, it will fail)

@panbingkun
Copy link
Contributor Author

panbingkun commented Mar 1, 2024

Yes, that's what I meant exactly. Scala 2.13.13 requires SBT 1.9.9 but SBT 1.9.9 is not working properly. In short, there is no working combination in Scala 2.13.13 and this is the worst case where we are currently standing. I don't know if SBT 1.9.10 might solve every issues, but it's also unlikely.

Yes, but unfortunately, from the test results of another PR, it seems that sbt 1.9.9 still has issues starting from version sbt 1.9.4. (When there is a corrupted jar in Maven's local repo, it will fail)

I have already submitted a bug report to SBT: sbt/sbt#7506
image

But it seems to be a problem with the coursier .

cc @LuciferYang

@LuciferYang
Copy link
Contributor

From GA testing, it seems that ammonite has not yet supported scala 2.13.13. com.lihaoyi:ammonite_2.13.13:2.5.11

A PR has been submitted to the upstream ammonite community to address this issue: com-lihaoyi/Ammonite#1430

In fact, com-lihaoyi/Ammonite#1421 is already in the process of upgrading, there are probably some tests that have not passed yet

@dongjoon-hyun
Copy link
Member

Thank you for updates for both.

@dongjoon-hyun
Copy link
Member

It seems that 3.0.0-M1 is released last week.

Could you rebase the PR? I guess the only remaining issue is SBT?

@panbingkun
Copy link
Contributor Author

panbingkun commented Mar 15, 2024

It seems that 3.0.0-M1 is released last week.

Could you rebase the PR? I guess the only remaining issue is SBT?

Okay, let me try it. Thanks for reminding me. ❤️

-->
<arrow.version>15.0.0</arrow.version>
<ammonite.version>2.5.11</ammonite.version>
<ammonite.version>3.0.0-M1</ammonite.version>
Copy link
Member

Choose a reason for hiding this comment

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

Could you update the dependency list? The other tests seem to pass.

+++ b/dev/pr-deps/spark-deps-hadoop-3-hive-2.3
@@ -139,7 +139,7 @@ jettison/1.5.4//jettison-1.5.4.jar
 jetty-util-ajax/11.0.20//jetty-util-ajax-11.0.20.jar
 jetty-util/11.0.20//jetty-util-11.0.20.jar
 jline/2.14.6//jline-2.14.6.jar
-jline/3.22.0//jline-3.22.0.jar
+jline/3.24.1//jline-3.24.1.jar

Copy link
Member

Choose a reason for hiding this comment

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

BTW, it seems that we have two jline libraries at the same time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me analyze it.

Copy link
Contributor

Choose a reason for hiding this comment

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

No problem, it has been checked before that their package names are different, so there will be no conflict

jline 2

image

jline 3

image

Copy link
Contributor Author

@panbingkun panbingkun Mar 15, 2024

Choose a reason for hiding this comment

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

  • jline-2.14.6.jar
    <groupId>jline</groupId>
    <artifactId>jline</artifactId>
    <version>2.14.6</version>
image image
  • jline-3.22.0.jar
    <groupId>org.jline</groupId>
    <artifactId>jline</artifactId>
    <version>3.22.0</version>
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll see what depends on them. If they can be aligned, it would be great if only one is left.
Of course, that's another issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://github.com/apache/hive/blob/36b32612ba0a20d3d305c77a813aca62d7129ed0/cli/src/java/org/apache/hadoop/hive/cli/CliDriver.java#L41-L49
image
From the hive 2.3 version code, it seems that we cannot align here because its import of jline starts with jline....

Copy link
Member

@dongjoon-hyun dongjoon-hyun Mar 15, 2024

Choose a reason for hiding this comment

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

Let me try in SPARK-47405 independently in the scope of upgrading spark-sql shell to use jline3 . If it's difficult, we can live with this as you confirmed that they are different packages now.

@dongjoon-hyun
Copy link
Member

If CI passed, shall we land this to master first to investigate SBT issues further, @panbingkun and @LuciferYang ? It would be must faster to find any potential issues if this patch is in master branch because all people and Daily CIs will verify this.

@dongjoon-hyun dongjoon-hyun changed the title [WIP][SPARK-47234][BUILD] Upgrade Scala to 2.13.13 [SPARK-47234][BUILD] Upgrade Scala to 2.13.13 Mar 15, 2024
@dongjoon-hyun dongjoon-hyun marked this pull request as ready for review March 15, 2024 05:11
@panbingkun
Copy link
Contributor Author

If CI passed, shall we land this to master first to investigate SBT issues further, @panbingkun and @LuciferYang ? It would be must faster to find any potential issues if this patch is in master branch because all people and Daily CIs will verify this.

I agree with you!
Let me add to the description of this PR.

Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

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

+1, LGTM. Thank you, @panbingkun and @LuciferYang .
Merged to master for Apache Spark 4.0.0.

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.

3 participants

Comments