Feat: Support Spark 4.0.0 part1 #1830

huaxingao · 2025-06-02T17:08:23Z

Which issue does this PR close?

Closes #.

Rationale for this change

Adding shim files for Spark 4.0.0 support

What changes are included in this PR?

How are these changes tested?

spark/src/main/spark-3.5/org/apache/spark/sql/comet/shims/ShimCometTPCDSMicroBenchmark.scala

andygrove · 2025-06-04T18:15:04Z

I ran into compilation issues locally when building for Spark 3.4. I think these can be resolved simply by moving the new spark-3.5 shims into the spark-3.x shim folder instead.

Also, some jobs fail with scalastyle errors, which can be fixed by running make format.

andygrove · 2025-06-04T18:30:09Z

common/src/main/java/org/apache/comet/parquet/TypeUtil.java

@@ -74,7 +74,7 @@ public static ColumnDescriptor convertToParquet(StructField field) {
      builder = Types.primitive(PrimitiveType.PrimitiveTypeName.INT64, repetition);
    } else if (type == DataTypes.BinaryType) {
      builder = Types.primitive(PrimitiveType.PrimitiveTypeName.BINARY, repetition);
-    } else if (type == DataTypes.StringType) {
+    } else if (type == DataTypes.StringType || type.sameType(DataTypes.StringType)) {


I'm not familiar with the sameType method. Why is it needed for strings but not for the other types?

This is to support String Collation in Spark 4.0. (e.g. test("Check order by on table with collated string column") )
Without String Collation, it goes to https://github.com/apache/spark/blob/master/sql/api/src/main/scala/org/apache/spark/sql/catalyst/parser/DataTypeAstBuilder.scala#L89 which uses singleton DataTypes.StringType, so type == DataTypes.StringType, But with String Collation, it goes to https://github.com/apache/spark/blob/master/sql/api/src/main/scala/org/apache/spark/sql/catalyst/parser/DataTypeAstBuilder.scala#L94, so type == DataTypes.StringType fails and I added type.sameType(DataTypes.StringType to let String Collation pass. Actually, I think it should be

else if ( type == DataTypes.StringType || (type.sameType(DataTypes.StringType) && isSpark40Plus()) )

codecov-commenter · 2025-06-05T07:43:08Z

Codecov Report

Attention: Patch coverage is 0% with 14 lines in your changes missing coverage. Please review.

Project coverage is 58.70%. Comparing base (f09f8af) to head (f13351b).
Report is 261 commits behind head on main.

Files with missing lines	Patch %	Lines
...c/main/java/org/apache/comet/parquet/TypeUtil.java	0.00%	5 Missing and 2 partials ⚠️
...ffle/comet/CometBoundedShuffleMemoryAllocator.java	0.00%	6 Missing ⚠️
.../scala/org/apache/comet/serde/QueryPlanSerde.scala	0.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #1830      +/-   ##
============================================
+ Coverage     56.12%   58.70%   +2.57%     
- Complexity      976     1139     +163     
============================================
  Files           119      130      +11     
  Lines         11743    12795    +1052     
  Branches       2251     2406     +155     
============================================
+ Hits           6591     7511     +920     
- Misses         4012     4060      +48     
- Partials       1140     1224      +84

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

andygrove · 2025-06-05T18:31:32Z

The Spark version will also need to be updated in .github/workflows/spark_sql_test_ansi.yml. It currently has:

spark-version: [{short: '4.0', full: '4.0.0-preview1'}]

parthchandra

Just some minor comments.

common/src/main/java/org/apache/comet/parquet/TypeUtil.java

spark/src/main/scala/org/apache/comet/serde/QueryPlanSerde.scala

YanivKunda

A few dependency alignment as part of the Spark 4.0.0 release

YanivKunda · 2025-06-08T22:07:41Z

pom.xml

@@ -600,7 +600,7 @@ under the License.
        <!-- Use Scala 2.13 by default -->
        <scala.version>2.13.14</scala.version>


Scala was upgraded in the 4.0.0 release version:

Suggested change

<scala.version>2.13.14</scala.version>

<scala.version>2.13.16</scala.version>

YanivKunda · 2025-06-08T22:07:52Z

pom.xml

@@ -600,7 +600,7 @@ under the License.
        <!-- Use Scala 2.13 by default -->
        <scala.version>2.13.14</scala.version>
        <scala.binary.version>2.13</scala.binary.version>
-        <spark.version>4.0.0-preview1</spark.version>
+        <spark.version>4.0.0</spark.version>
        <spark.version.short>4.0</spark.version.short>
        <parquet.version>1.13.1</parquet.version>


Parquet was upgraded in the 4.0.0 release version:

Suggested change

<parquet.version>1.13.1</parquet.version>

<parquet.version>1.15.2</parquet.version>

YanivKunda · 2025-06-08T22:08:12Z

pom.xml

@@ -600,7 +600,7 @@ under the License.
        <!-- Use Scala 2.13 by default -->
        <scala.version>2.13.14</scala.version>
        <scala.binary.version>2.13</scala.binary.version>
-        <spark.version>4.0.0-preview1</spark.version>
+        <spark.version>4.0.0</spark.version>
        <spark.version.short>4.0</spark.version.short>
        <parquet.version>1.13.1</parquet.version>
        <semanticdb.version>4.9.5</semanticdb.version>


If Scala is upgraded, this probably needs to be upgraded to:

at least 4.12.0: the first version to support scala 2.13.16

4.12.7: the latest 4.12.x version

4.13.6: the latest of the latest

Note: this might need to be also aligned in the scala-2.13 profile!

And also, because I can't comment on the next line -
slf4j was upgraded in the 4.0.0 release version, to version 2.0.16

YanivKunda

Found some files that might have been left over from local operations -
should probably be deleted.

YanivKunda · 2025-06-18T12:58:54Z

dev/.DS_Store

macOS local file

YanivKunda · 2025-06-18T12:59:02Z

dev/diffs/.DS_Store

macOS local file

YanivKunda · 2025-06-18T12:59:43Z

dev/diffs/4.0.0-diff.patch

looks like a temporary diff patch file (I know it sounds like "salsa sauce", but it probably shouldn't be here at all)

YanivKunda · 2025-06-18T13:00:05Z

.DS_Store

macOS local file

Feat: Support Spark 4.0.0 part1

f4b6d72

andygrove reviewed Jun 2, 2025

View reviewed changes

spark/src/main/spark-3.5/org/apache/spark/sql/comet/shims/ShimCometTPCDSMicroBenchmark.scala Outdated Show resolved Hide resolved

remove unnecessary shim

4fdc562

andygrove mentioned this pull request Jun 4, 2025

Update 4.0.0.diff to reflect recent improvements in 3.5.5.diff #1846

Open

andygrove reviewed Jun 4, 2025

View reviewed changes

address comments

b39c6d2

huaxingao force-pushed the 4.0shim branch from e768b43 to b39c6d2 Compare June 5, 2025 07:19

huaxingao added 2 commits June 5, 2025 09:53

fix

a844503

fix

33c7694

huaxingao added 2 commits June 5, 2025 14:10

update spark version in spark_sql_test_ansi.yml

037013a

update diff

cde0688

parthchandra reviewed Jun 6, 2025

View reviewed changes

common/src/main/java/org/apache/comet/parquet/TypeUtil.java Show resolved Hide resolved

spark/src/main/scala/org/apache/comet/serde/QueryPlanSerde.scala Show resolved Hide resolved

fix

5228b4c

huaxingao force-pushed the 4.0shim branch from d5f5ecf to 5228b4c Compare June 7, 2025 18:37

fix

af0445c

YanivKunda suggested changes Jun 8, 2025

View reviewed changes

huaxingao force-pushed the 4.0shim branch from 322ee05 to db3254f Compare June 9, 2025 06:08

address comments

381c953

huaxingao force-pushed the 4.0shim branch from db3254f to 381c953 Compare June 9, 2025 06:18

huaxingao added 4 commits June 16, 2025 13:41

Expected column index is not null for spark4

e128882

update diff to disable a couple of sql tests

83a5988

disable columnarShuffleOnMapTest for spark4.0

5402cd3

fix style

f13351b

YanivKunda suggested changes Jun 18, 2025

View reviewed changes

		@@ -600,7 +600,7 @@ under the License.
		<!-- Use Scala 2.13 by default -->
		<scala.version>2.13.14</scala.version>

	<scala.version>2.13.14</scala.version>
	<scala.version>2.13.16</scala.version>

	<parquet.version>1.13.1</parquet.version>
	<parquet.version>1.15.2</parquet.version>

Feat: Support Spark 4.0.0 part1 #1830

Are you sure you want to change the base?

Feat: Support Spark 4.0.0 part1 #1830

Uh oh!

Conversation

huaxingao commented Jun 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

How are these changes tested?

Uh oh!

Uh oh!

andygrove commented Jun 4, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Jun 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

andygrove commented Jun 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

parthchandra left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

YanivKunda left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

YanivKunda left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

huaxingao commented Jun 2, 2025 •

edited

Loading

codecov-commenter commented Jun 5, 2025 •

edited

Loading

andygrove commented Jun 5, 2025 •

edited

Loading