LuciferYang commented on PR #12414: URL: https://github.com/apache/gravitino/pull/12414#issuecomment-5254016951
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 `spark-common` against 3.5 and it fails on `SparkHiveTable`: `SupportsPartitionManagement.createPartition` declares the singular `PartitionAlreadyExistsException` in 3.3 and the plural `PartitionsAlreadyExistException` in 3.4, 3.5 and 4.0. So that split is 3.3 versus 3.4+. `ParserInterface` splits differently: 3.3, 3.4 and 3.5 all require the same 8 methods, and 4.0 adds `parseRoutineParam`. And Paimon is a third axis: `paimon-spark-4.0` first appears in Paimon 1.3.0, and we pin 1.2.0 (`libs.versions.toml:75`), so no Spark 4 Paimon artifact exists at the version we build against. Three different boundaries means the layout has to be additive `srcDir` lists rather than one directory per version: | dir | contents | used by | |---|---|---| | `java` | the 35 version-neutral files | all | | `spark3x` | 4 Paimon classes + authz parser (3.x form) | 3.3, 3.4, 3.5 | | `spark33` | `SparkHiveTable`, `HiveGravitinoOperationOperator` (singular exception) | 3.3 | | `spark34plus` | the same 2 with the plural exception, + the 4 `*34` converters | 3.4, 3.5, 4.0 | | `spark40` | authz parser (4.0 form) | 4.0 | I checked the pieces that make `spark34plus` viable, and compiled that combination to be sure: shared sources plus the spark4 `SparkHiveTable` and `HiveGravitinoOperationOperator` plus the four `*34` converters build cleanly against both 3.4.3 and 3.5.3. The `PartitionsAlreadyExistException(String, InternalRow, StructType)` constructor they use exists in both, and the four converters are byte-identical to their `v3.4` originals. Moving them there removes both the copy this PR makes and the `v3.5` to `v3.4` edge, which is the concrete payoff of your principle 2. Today `v3.5/spark` depends on `spark-connector:spark-3.4`: four catalogs extend `*Spark34`, and `GravitinoJdbcCatalogSpark35` uses the `*34` converters. **The shared tests are the larger half.** All four version modules consume `spark-common`'s test classes as a `testArtifacts` jar, ~50 files compiled once against 3.3. Under "tests follow the same structure" that tree needs the same flavor split and per-version compilation, and `testArtifacts` stops being shareable as a binary. That is also what would make the 4.0 ITs exercise 4.0-compiled shared code instead of 3.3 bytecode, which they do not today. The Paimon tests are part of this: three files today, excluded by a Scala-version-keyed rule rather than a Spark-version one, so on the test side that rule does need to change even though the directory split handles the main sources. 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. `implements ProcedureCatalog` resolves to a class Iceberg ships on 3.x and one Spark ships on 4.0, and `implements` is declaration-level, so those stay per-version. The refactor can reach "no cross-version module dependency" but not "no per-version code". Questions: 1. Does the table above look right, and is additive `srcDir` composition acceptable? I would rather build the layout you want than guess at naming. 2. Should `spark-common` stay a Gradle module, or should the version modules compile its sources directly? Two things keep it from being purely sources-only: it owns `extra["glueHiveJarsDir"]` and the `downloadGlueHiveJars` task that all three v3.x modules reach through `evaluationDependsOn`, and it is the only producer of the `testArtifacts` jar the spark-connector modules share. Both are relocatable, but that is a decision rather than a file move. Note also that its `compileJava` is pinned to `--release 8` by a path-prefix rule in the root build while the 4.0 compilation needs 17, which is why `spark4-common` had to be listed in `jdk17OnlyProjectPaths`. 3. If it stays a module, does it keep publishing as `gravitino-spark-common` (currently no Scala or Spark suffix, despite being built against 3.3)? Once the version modules consume sources instead, that jar has no internal consumer. -- 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]
