Skip to content

Conversation

vrozov
Copy link
Member

@vrozov vrozov commented Oct 2, 2025

What changes were proposed in this pull request?

Upgrade to scala 2.13.17

Why are the changes needed?

To bring the latest bug fixes and improvements like JDK 25 support. Note that Scala community announces two breaking changes due to the bug fixes.

Breaking changes

  • Mix in the productPrefix hash statically in case class hashCode
  • Improve scala.util.Using suppression order

Does this PR introduce any user-facing change?

No

How was this patch tested?

local and github builds

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

No

def height: Int = _height()

private val _hashCode = new BestEffortLazyVal[Integer](() => MurmurHash3.productHash(this))
private val _hashCode = new BestEffortLazyVal[Integer](() => MurmurHash3.caseClassHash(this))
Copy link
Member Author

Choose a reason for hiding this comment

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

productHash is deprecated in 2.13.17

<dependency>
<groupId>com.lihaoyi</groupId>
<artifactId>ammonite_${scala.version}</artifactId>
<artifactId>ammonite_2.13.16</artifactId>
Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Nice. Thank you, @vrozov .

@dongjoon-hyun
Copy link
Member

Is there any update, @vrozov ?

@vrozov
Copy link
Member Author

vrozov commented Oct 7, 2025

@dongjoon-hyun No, still waiting for ammonite_2.13.17 to be published.

@github-actions github-actions bot removed the CONNECT label Oct 12, 2025
@vrozov vrozov marked this pull request as ready for review October 12, 2025 23:46
@vrozov
Copy link
Member Author

vrozov commented Oct 12, 2025

@dongjoon-hyun upgraded ammonite to 3.0.3, please review.

@dongjoon-hyun
Copy link
Member

Thank you. Please rebase to the master branch once more because Ammonite was merged already.

def height: Int = _height()

private val _hashCode = new BestEffortLazyVal[Integer](() => MurmurHash3.productHash(this))
private val _hashCode = new BestEffortLazyVal[Integer](() => MurmurHash3.caseClassHash(this))
Copy link
Member

Choose a reason for hiding this comment

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

And, please spin off this as a new PR if productHash still exists, @vrozov .

Copy link
Member Author

Choose a reason for hiding this comment

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

@dongjoon-hyun productHash() is deprecated:

[ERROR] [Error] /local/home/vrozov/ws/spark-sbt/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/trees/TreeNode.scala:226: method productHash in object MurmurHash3 is deprecated (since 2.13.17): use `caseClassHash` instead
Applicable -Wconf / @nowarn filters for this fatal warning: msg=<part of the message>, cat=deprecation, site=org.apache.spark.sql.catalyst.trees.TreeNode._hashCode.$anonfun, origin=scala.util.hashing.MurmurHash3.productHash, version=2.13.17

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, @vrozov

For the other reviewers, initially, I proposed to use the existing method, MurmurHash3.productHash, via revising our compilation rules.

spark/pom.xml

Lines 2763 to 2785 in 00d2a54

<arg>-Wconf:msg=^(?=.*?method|value|type|object|trait|inheritance)(?=.*?deprecated)(?=.*?since 2.13).+$:e</arg>
<arg>-Wconf:msg=^(?=.*?Widening conversion from)(?=.*?is deprecated because it loses precision).+$:e</arg>
<!-- SPARK-45610 Convert "Auto-application to `()` is deprecated" to compile error, as it will become a compile error in Scala 3. -->
<arg>-Wconf:cat=deprecation&amp;msg=Auto-application to \`\(\)\` is deprecated:e</arg>
<!--
SPARK-35574 Prevent the recurrence of compilation warnings related to
`procedure syntax is deprecated`
-->
<arg>-Wconf:cat=deprecation&amp;msg=procedure syntax is deprecated:e</arg>
<!--
SPARK-45627 Symbol literals are deprecated in Scala 2.13 and it's a compile error in Scala 3.
-->
<arg>-Wconf:cat=deprecation&amp;msg=symbol literal is deprecated:e</arg>
<!--
SPARK-45627 `enum`, `export` and `given` will become keywords in Scala 3,
so they are prohibited from being used as variable names in Scala 2.13 to
reduce the cost of migration in subsequent versions.
-->
<arg>-Wconf:cat=deprecation&amp;msg=it will become a keyword in Scala 3:e</arg>
<!--
SPARK-49937 ban call the method `SparkThrowable#getErrorClass`
-->
<arg>-Wconf:cat=deprecation&amp;msg=method getErrorClass in trait SparkThrowable is deprecated:e</arg>

However, after digging the original Scala bug and fixes, it turns out that there is no way to keep the previous method because both old and new APIs are changed together.

So, I agree this AS-IS PR and @vrozov .

@dongjoon-hyun
Copy link
Member

dongjoon-hyun commented Oct 13, 2025

BTW, I updated the PR description (for you) because it will become our commit log, @vrozov .

@dongjoon-hyun
Copy link
Member

All Scala-related tests passed.

Merged to master for Apache Spark 4.1.0-preview3.

@dongjoon-hyun
Copy link
Member

Thank you again, @vrozov .

@vrozov
Copy link
Member Author

vrozov commented Oct 13, 2025

All tests passed, looks like github issue in updating status of the check on the PR.

dongjoon-hyun added a commit that referenced this pull request Oct 14, 2025
…Scala 2.13.17

### What changes were proposed in this pull request?

This PR aims to regenerate benchmark results after upgrading to Scala 2.13.17.

### Why are the changes needed?

Since last update, we change important libraries, not only Scala, but also Hadoop, ORC, ZSTD libraries. This PR aims to make the benchmark result up-to-date as a way to detect any performance regression.

- #52509
- #51127
- #52478
- #52591

### Does this PR introduce _any_ user-facing change?

No.

### How was this patch tested?

Manual review.

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

No.

Closes #52600 from dongjoon-hyun/SPARK-53893.

Lead-authored-by: Dongjoon Hyun <[email protected]>
Co-authored-by: dongjoon-hyun <[email protected]>
Signed-off-by: Dongjoon Hyun <[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.

2 participants