LuciferYang opened a new pull request, #12414:
URL: https://github.com/apache/gravitino/pull/12414

   ### What changes were proposed in this pull request?
   
   Adds Spark 4.0 support to the Spark connector, alongside the existing 
3.3/3.4/3.5 line.
   
   Spark 4.0 is source-incompatible with 3.x in a handful of places the 
connector touches, so `spark-common` can no longer be compiled once and reused 
by both lines. This PR splits its sources by Spark line and compiles them twice:
   
   - `spark-common/src/main/java` — code that compiles unchanged on both lines 
(35 files).
   - `spark-common/src/main/spark3` — the Spark 3.x flavor of the incompatible 
classes.
   - `spark-common/src/main/spark4` — the Spark 4.0 flavor of the same classes.
   
   The existing `spark-common` module adds `src/main/spark3` as an extra source 
dir and keeps building against Spark 3.3 exactly as before. A new 
`spark4-common` module compiles `src/main/java` + `src/main/spark4` against 
Spark 4.0.3 / Scala 2.13. The two flavors declare the same fully qualified 
names, so `GravitinoDriverPlugin`'s registration by class-name string needs no 
version dispatch: whichever flavor is on the classpath is the one that gets 
used. Sharing one `src` tree between two modules is what Paimon does for 
`paimon-spark4-common`.
   
   Three classes need a per-line flavor: the authorization `ParserInterface` 
implementation (Spark 4.0 added an abstract `parseRoutineParam`), the Hive 
partition operator, and `SparkHiveTable` (Spark 4.0 removed the singular 
`PartitionAlreadyExistsException` along with its single-String constructor).
   
   A fourth incompatibility could not be handled that way. Spark 4.0 took over 
the Iceberg stored-procedure API: `ProcedureCatalog` moved from Iceberg's 
package into Spark's own, `loadProcedure` returns `UnboundProcedure`, and 
`NoSuchProcedureException` is gone. Both the `implements` clause and the method 
signature differ, which two same-named bodies cannot paper over, so 
`loadProcedure` moves out of the shared `GravitinoIcebergCatalog` and into the 
per-version subclasses (`GravitinoIcebergCatalogSpark33/34/40`; 3.5 inherits 
from the 3.4 subclass).
   
   On top of that: `spark-connector:spark-4.0` holds the five per-version 
catalog subclasses (Hive, Iceberg, JDBC, PostgreSQL, Glue), 
`spark-connector:spark-runtime-4.0` produces the shaded 
`gravitino-spark-connector-runtime-4.0_2.13`, and `CatalogNameAdaptor` 
registers the 4.0 catalog class names.
   
   Two shared integration tests needed adjusting for Spark 4.0 API removals, 
both in a way that keeps one form working on every line:
   
   - `SparkIcebergCatalogIT` built a literal column with `new 
Column(Literal.create(...))`. Spark 4.0 rebuilt `Column` on top of `ColumnNode` 
and dropped the constructor taking a catalyst `Expression`. Switched to 
`functions.lit()`, present from 3.3 through 4.0.
   - `SparkHiveCatalogIT` asserted on `"Partition already exists"`. With the 
single-String constructor gone, that message now comes from Spark's 
`PARTITIONS_ALREADY_EXIST` error class instead of being passed through from 
Gravitino, so the expected fragment moved behind an overridable method.
   
   Paimon is deliberately not wired for 4.0. `paimon-spark-4.0` only ships in 
Paimon 1.4.x and Gravitino is on 1.2.0, so it needs a dependency upgrade first. 
The Paimon classes therefore live in `src/main/spark3` only, and 
`lakehouse-paimon-4.0` is not registered, so `GravitinoDriverPlugin` logs "not 
supported yet" and skips it, which is its existing behavior for an unmapped 
provider.
   
   ### Why are the changes needed?
   
   Spark 4.x has been GA for a while (4.0 / 4.1 / 4.2) and is Scala 2.13-only 
on JDK 17+. Gravitino's Spark connector tops out at 3.5, so users on Spark 4 
have no supported path. #8771 asks for v4 support and has had no implementation.
   
   Fix: #8771
   
   ### Does this PR introduce _any_ user-facing change?
   
   Yes, additive only:
   
   - New published artifacts: `gravitino-spark-4.0_2.13`, 
`gravitino-spark4-common_2.13`, and the runtime jar 
`gravitino-spark-connector-runtime-4.0_2.13`.
   - `docs/spark-connector/spark-connector.md` now lists Spark 4.0 in the 
requirement matrix, with its Scala 2.13-only and JDK 17 constraints and a note 
that Paimon catalogs are not supported on 4.0 yet. `docs/how-to-build.md` 
documents the 4.0 build command.
   - No configuration property was added, renamed or removed, and nothing about 
the 3.x artifacts or their behavior changes.
   
   ### How was this patch tested?
   
   **Spark 4.0, real Spark session (docker ITs, `-PskipDockerTests=false`)** — 
the shared ITs run against Spark 4.0.3 via new 4.0 subclasses:
   
   | suite | tests | failures | skipped |
   |---|---|---|---|
   | `SparkHiveCatalogIT40` | 51 | 0 | 5 |
   | `SparkIcebergCatalogHiveBackendIT40` | 46 | 0 | 2 |
   | `SparkJdbcMysqlCatalogIT40` | 33 | 0 | 15 |
   
   The skips are the per-catalog capability switches that the 3.x runs skip 
too: delete, schema evolution, complex types, bucketing.
   
   **3.x zero regression** — unit tests green on both Scala versions (2.12: 28 
suites / 151 tests; 2.13: 27 suites / 148 tests). The 3.5 docker ITs were 
re-run as well, because this PR touches shared IT code: `SparkHiveCatalogIT35` 
51 tests and `SparkIcebergCatalogHiveBackendIT35` 47 tests, no failures.
   
   **New unit test** — `TestSparkTypeConverter40` covers the Spark 4 
compilation of the shared type converters. It was verified in both directions: 
it fails without the fix it guards, and passes with it.
   
   **Also run** — `spotlessCheck` across the repo, `javadoc` for the new 
modules (the build runs it with `-Werror`), and the 4.0 runtime shadow jar.
   
   Two things reviewers should know about the local IT setup, both test-only:
   
   The ITs boot an embedded Gravitino server on Jetty 9 (`javax.servlet`) with 
Jersey 2, and Spark 4.0 brings the jakarta flavor transitively via 
`spark-hive`. Conflict resolution then upgrades the server's copies: Jersey 
3.0.18, whose `ServletContainer` Jetty 9 rejects outright; HK2 3.0.6, which 
reads `jakarta.inject`, so Jersey 2's `javax.inject`-annotated `RequestContext` 
stops being a singleton; and `jakarta.validation-api` 3.0.2, which drops the 
`javax.validation` packages Jersey 2 looks up. The 4.0 module pins all three 
back to the javax flavor on `testRuntimeClasspath`. The connector runtime jar 
bundles neither Jersey nor the server, so none of this reaches users.
   
   Separately, Spark 4.0's web UI serves its REST API with Jersey 3 on its 
shaded Jetty, which cannot coexist with the Jersey 2 pinned above, so the 4.0 
ITs set `spark.ui.enabled=false`. They never exercise the UI.
   
   ### Known gaps, stated plainly
   
   **`src/main/spark4` carries copies of the four `*34` converters.** They live 
in the `v3.4` module today because `spark-common` compiles against Spark 3.3, 
which lacks `TimestampNTZType`; the 4.0 line needs them but must not depend on 
a 3.x version module. Deduplicating means raising `spark-common`'s baseline off 
3.3, which is a separate decision. This is the only duplication beyond the ~200 
lines of genuinely per-line code in `spark3`/`spark4`; the shared code is not 
copied.
   
   **Spark 4.0's collation information is silently dropped** in type 
conversion. Spark 4.0's `StringType` carries a collation, Gravitino's type 
system has no such concept, so `STRING COLLATE UTF8_LCASE` round-trips as a 
plain string with no warning. Fixing it properly means either rejecting 
non-default collations or documenting the limitation, and that needs a product 
decision, so it is filed as follow-up rather than guessed at here.
   
   **Not every Spark-4-only class has a unit test yet.** 
`TestSparkTypeConverter40` covers the two shared converters where a real bug 
turned up. `SparkHiveTypeConverter34` / `SparkJdbcTypeConverter34` and the 
TimestampNTZ branches under `src/main/spark4` do not have 4.0-side unit tests.
   
   **Glue and authorization have no 4.0 IT.** `GravitinoGlueCatalogSpark40` 
compiles and is registered but is untested on 4.0, since the Glue IT needs real 
AWS credentials and is skipped by default. There is no `SparkAuthorizationIT40` 
counterpart either, even though the authorization parser has a new spark4 
flavor.
   
   **Spark 4.1 / 4.2 are out of scope.** Once 4.0 lands, thin `v4.1`/`v4.2` 
modules can reuse `src/main/spark4` if those releases stay source-compatible 
with 4.0. That needs rechecking per version rather than assuming.
   
   ### CI
   
   `.github/workflows/spark-integration-test-action.yml` gains a step that runs 
`:spark-connector:spark-4.0:test`, gated on the JDK 17 / Scala 2.12 leg and 
without `-PscalaVersion`, since the 4.0 modules hardcode 2.13. The backend-IT 
and both trino workflows exclude the three new modules, matching how they 
already exclude the 3.x ones; without that the 4.0 ITs would run inside the 
backend-IT job, and the trino jobs would build Spark 4.0 for nothing. Log 
upload paths gain `spark-4.0-integration-test.log`.
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Reply via email to