[#8771] feat(spark-connector): Add Spark 4.0 support, drop 3.3 and 3.4 - #12414
[#8771] feat(spark-connector): Add Spark 4.0 support, drop 3.3 and 3.4#12414LuciferYang wants to merge 5 commits into
Conversation
b258afc to
5923cfc
Compare
Code Coverage Report
Files
|
|
Thank you @roryqi |
|
Since we are going to refactor this part, I think the architecture should follow two principles:
The tests should follow the same structure as well. This will keep each Spark version independent and make it much easier to deprecate or remove old versions in the future. |
|
@diqiu50 Thanks for your suggestions. I'll take a look and get back to you later. |
|
Agreed on both principles, and I am happy to take the refactor as a separate PR ahead of this one, then rebase this one on top. It changes how all of 3.x builds, so it deserves review on its own. Below is what I found while sizing it, and a layout to check against before I start. Where the version boundaries actually fall. I tried compiling Three different boundaries means the layout has to be additive
I checked the pieces that make The shared tests are the larger half. All four version modules consume For scale, the shared sources end up compiled once per supported (Spark, Scala) pair after the refactor: 3.3/3.4/3.5 under 2.12 and 2.13, plus 4.0 under 2.13, with the 2.13 ones excluding the Paimon subset. One limit worth stating: the Iceberg catalog subclasses cannot fully follow principle 2. Questions:
|
|
friendly ping @diqiu50 |
|
We could consider supporting Spark 4.1 instead of 4.0. Spark 4.1 is more stable. |
|
No problem, I can adjust it to add support for Spark 4.1. Do you have any feedback on the three questions I raised earlier? Thanks |
|
However, my company’s business are still running on Spark 4.0.x. Is it feasible to support both Spark 4.0 and 4.1 simultaneously? I can submit multiple patches to achieve compatibility with both versions. @diqiu50 also cc @jerryshao |
|
I think we can simplify the layout further. First, we don't need to keep Spark 3.3 support, so there is no need to introduce a Second, I don't think Conceptually: The tests should follow the same model: shared test sources should be compiled separately for each Spark version rather than distributed as a precompiled The Glue download task and similar build logic can be moved out separately. I don't think build-time utilities are a strong enough reason to keep I'm also fine with a reasonable amount of code duplication between version-specific implementations. Keeping clear version boundaries, avoiding cross-version dependencies, and making each supported Spark version independently removable are more important than eliminating every duplicated line of code. |
|
Thanks for your feedback. @diqiu50 |
|
Thanks @LuciferYang |
Alright, let me evaluate it against this rule first. |
|
Hi @LuciferYang , we also discussed offline. I think we can support Spark 4.0 along with 4.1. But I don't want to maintain too many Spark versions. So I want to drop support for Spark 3.4, WDYT? Also cc @diqiu50 |
|
Agreed. Spark's versioning policy keeps 3.5.x on extended LTS through November 2027, while 3.3 and 3.4 are past their maintenance windows: the last releases were 3.3.4 and 3.4.4 (October 2024). This also answers the duplication question @diqiu50 raised. Under the strict one-directory-per-version layout, dropping 3.4 takes duplicated lines from 873 to 285, and the Final matrix: 3.5, 4.0, 4.1. One note: this stops publishing |
db1654b to
1a55481
Compare
LuciferYang
changed the title
|
I suggest we start by supporting two versions. That will give us a clearer picture of the feasibility of this approach and the maintenance cost involved. |
Got it, I'll combine my local PR1 and PR2 into a single submission here, and ping you once the code is ready. |
…he 3.x line Spark 3.3 and 3.4 are past their maintenance windows, so 3.5 is the only 3.x line that will see another release. Dropping them lets the version-specific converter and catalog subclasses collapse back into spark-common. Stops publishing gravitino-spark-connector-runtime-3.3 and -3.4.
…ared source set spark-common stops being a Gradle module. Each version module composes its own source set from spark-common/src/main/java, spark-common/src/main/spark<NN> and its own src/main/java, so the shared code is compiled against every supported Spark version rather than once. The test tree works the same way, replacing the testArtifacts jar. Stops publishing gravitino-spark-common.
1a55481 to
b5f71dd
Compare
Adds v4.0/spark and v4.0/spark-runtime, publishing gravitino-spark-4.0_2.13 and gravitino-spark-connector-runtime-4.0_2.13. Spark 4 is Scala 2.13 only and needs JDK 17. Moves SparkIcebergTable and SparkJdbcTable back to the shared tree: both compile unchanged on 4.0, so the spark35 copies were duplication. Only the authorization parser needs a per-version flavor.
LuciferYang
changed the title
…xclusion lists The spark-connector ITs are meant to run only in the dedicated Spark IT workflow; every other workflow excludes them. v4.0 was added without being added to those lists, so its ITs also ran inside the backend IT job, where -PjdbcBackend=mysql makes two suites drop and recreate the same MYSQL_JDBC_BACKEND database out from under each other. Excluded alongside 3.5 in backend IT and in the two Trino workflows. Also raises the build job cap from 90 to 120 minutes. It has been finishing in 75-84 minutes and :core:test alone has varied from 27 to 45 minutes across runs on identical code, so the margin was already thinner than the runner's own variance.
|
CI passed, Please review it when you have time, and I will revise it if there are any issues. Thanks @diqiu50 |
|
The Spark version is already fixed by the jar on the classpath, so CatalogNameAdaptor reading SPARK_VERSION and looking the catalog classes up again is a second dispatch on the same information — and it costs us 11 hardcoded class names that nothing checks, plus a misleading provider is not supported yet warning when the jar does not match the cluster. Could each version module declare its own catalog classes and pass them to the driver plugin, instead of the shared code looking them up by version? Then the names are checked at compile time, CatalogNameAdaptor and the version package go away, and adding a Spark version stops touching shared code. |
…are its own catalogs CatalogNameAdaptor read SPARK_VERSION at runtime and looked catalog classes up by string, a second dispatch on information the jar on the classpath already fixes. It cost 11 hardcoded class names that nothing checked and a misleading "provider is not supported yet" warning when the jar did not match the cluster. Each version module now declares a SparkCatalogs class at the same FQN, holding compile-time class references, so the driver plugin reads the table from whichever jar is present. Provider to catalog-kind mapping is the same on every version, so that stays shared as SparkCatalogKind. Adding a Spark version no longer touches shared code.
3 participants
What changes were proposed in this pull request?
This revision replaces what this branch previously proposed. The earlier one added Spark 4.0 on top of the 3.3/3.4/3.5 matrix; review here asked for the 3.x matrix to shrink first and, per @diqiu50, for the initial submission to cover two versions so the approach and its maintenance cost can be judged from something concrete. The final matrix is Spark 3.5 and 4.0. Spark 4.1 exists locally as a follow-up and is not part of this PR.
Three commits, each self-contained:
1. Support Spark 3.5 only for the 3.x line. Spark 3.3 and 3.4 are past their maintenance windows; 3.5.x is on extended LTS through November 2027, so it is the only 3.x line that will see another release. With them gone, adaptations that existed only to bridge 3.3/3.4 collapse back into
spark-common: theTimestampNTZTypemapping andUpdateColumnDefaultValueconversion move intoSparkTypeConverterandSparkTableChangeConverter,loadTable(Identifier, Set<TableWritePrivilege>)moves intoBaseCatalog, and four converter subclasses plus the two version modules go away.SparkHiveTableandHiveGravitinoOperationOperatorswitch to the pluralPartitionsAlreadyExistException, becauseSupportsPartitionManagement.createPartitionchanged itsthrowsclause in Spark 3.4.2. Compile
spark-commonas a shared source set.spark-commonstops being a Gradle module. Each version module composes its own source set fromspark-common/src/main/java,spark-common/src/main/spark<NN>and its ownsrc/main/java. The test tree works the same way, replacing thetestArtifactsjar the version modules used to consume.Compiling the shared code once against one Spark version and running it on another is how a version-specific bug hides: the compiler only ever sees the API the pinned version exposes. Per-version compilation means each line's compiler checks the shared code against its own API, and the shared tests run on each line rather than only on the pinned one. Commit 3 shows this paying off immediately.
3. Add Spark 4.0 support. New
v4.0/sparkandv4.0/spark-runtime, publishinggravitino-spark-4.0_2.13andgravitino-spark-connector-runtime-4.0_2.13. Spark 4 is Scala 2.13 only and needs JDK 17, so both modules pin 2.13 rather than reading-PscalaVersion, and they opt out of the repository's JDK 8 target.Only one class needs a Spark 4 flavor: the authorization parser, because Spark 4 added an abstract
parseRoutineParamtoParserInterface.spark35additionally holds the Paimon package, since Paimon publishes nopaimon-spark-4.xartifact at the version this repository pins; the Spark 4 build excludes it and the driver plugin skips the Paimon session extension when the catalog is unavailable, rather than failingSparkSessionconstruction.Iceberg's
loadProceduremoves out of the sharedGravitinoIcebergCataloginto the per-version catalog subclasses. On 3.xProcedureCatalogis a class Iceberg ships andloadProcedurereturns Iceberg'sProcedure; on 4.x Spark ships its own and the return type isUnboundProcedure.implementsis declaration-level, so this one cannot be handled by same-name flavor files.Two fixes to the shared code, both surfaced by compiling the shared tests against Spark 4 for the first time:
SparkTypeConverterandSparkPartitionUtilscheckedinstanceof StringTypebeforeVarcharTypeandCharType. Spark 4 made both extendStringType, so that branch swallowed them and silently dropped the length. Three shared tests caught it. Subtypes are now checked first, which is correct on every supported version.new Column(Literal.create(...)). Spark 4 rebuiltColumnonColumnNodeand dropped that constructor; it now usesfunctions.lit(), available on both.The 4.0 ITs pin Jersey, HK2 and jakarta.validation back to the javax flavor on the test runtime classpath: the embedded Gravitino server serves REST on Jetty 9 with Jersey 2, and Spark 4 brings the jakarta flavor transitively via
spark-hive. Spark's own web UI cannot coexist with that, so the ITs disable it. Both are test-only; the runtime jar bundles neither Jersey nor the server.Why are the changes needed?
Spark 4.0 is GA and the connector topped out at 3.5, leaving Spark 4 users with no supported path. The
VarcharTypebug above is the concrete argument for per-version compilation: it was reachable from any Spark 4 session and invisible to a build that only ever compiled the shared code against 3.x.Fix: #8771
Does this PR introduce any user-facing change?
Yes. Two removals, one addition, and one behavior change on the JDBC PostgreSQL catalog.
gravitino-spark-connector-runtime-3.3and-3.4are no longer published. Spark 3.3 and 3.4 users stay on an earlier Gravitino release.gravitino-spark-commonis no longer published. It was an implementation detail of this repository's own version modules, which now compile its sources directly; nothing outside the repository consumed it.gravitino-spark-4.0_2.13andgravitino-spark-connector-runtime-4.0_2.13are new. On Spark 4 the connector requires Scala 2.13 and JDK 17, and the Paimon catalog is unavailable. Glue resolves a catalog class on 4.0 but is verified on 3.5 only, because the patched Hive JARs it needs are published for Spark 3 alone.GravitinoPostgreSqlCatalogSpark35extends the shared JDBC catalog directly rather than a*Spark34subclass, so it never picked up the three version-specific overrides the other five catalogs had. Folding those into the shared classes gives it all three at once:timestamp_ntzcolumns are accepted instead of rejected;ALTER TABLE … ALTER COLUMN … SET DEFAULTworks instead of failing in the connector (the PostgreSQL backend already supported it); andloadTable(Identifier, Set<TableWritePrivilege>)now routes through the write path, so write operations on a PostgreSQL catalog are checked against write privileges where previously they were not. A user whose grants cover reads but not writes can start seeing denials. This aligns jdbc-postgresql with jdbc-mysql, which has had all three since [#10181] feat(spark-connector): Support TableWritePrivilege for Spark 3.5+ authorization #10194.How was this patch tested?
Unit tests: 3.5 at 118 cases under Scala 2.12 and 115 under 2.13, the difference being the Paimon converter test the build already excluded on 2.13 before this PR; 4.0 at 116 cases. No failures.
Docker ITs on 4.0, four suites: Hive 51, Iceberg on the Hive backend 47, MySQL 34, PostgreSQL 33. 165 cases reported, 126 run and 39 skipped by their own preconditions, 0 failures. The full 3.5 suite as a regression check, ten suites across Hive, Iceberg, MySQL, PostgreSQL, Paimon, authorization and JWKS: 296 cases reported, 167 run, 0 failures. Of the 129 skips, 85 are two suites skipping wholesale for lack of an environment, Glue at 42 and the Iceberg REST backend at 43.
Also both runtime shadow jars, repo-wide
spotlessCheck, and javadoc for both modules. The 4.0 jar was checked to contain the five*Spark40catalog classes and zero Paimon classes; the 3.5 jar still carries its eight.One test worth naming:
TestGravitinoDriverPlugin.testPaimonExtensionFollowsCatalogAvailabilityasserts that the Paimon session extension is registered exactly when the catalog adaptor resolves a Paimon catalog. Because the shared test tree compiles into both modules, the same assertion expects registration on 3.5 and skipping on 4.0, which is what makes it a regression guard rather than a tautology.