diqiu50 commented on PR #12631: URL: https://github.com/apache/gravitino/pull/12631#issuecomment-5570206883
Looked at why this PR needs 4 separate version-segment modules (479-479, 480-480, 481-481, 482-483) instead of extending the existing range. Diffing the version-specific classes across 478→479→480→481→482, it looks like **478/479/480 are byte-identical** (aside from class/version names) — no actual SPI signature changes land in the per-version classes for this span; the 480 changes (`getComment()`, `SchemaFunctionName`) are already absorbed by the shared `SpiVersionCompat` reflection layer. The only reason 479/480 need their own module is the JDK 25 toolchain + `--add-modules=jdk.incubator.vector` requirement, not a binary-incompatible SPI. **481** and **482/483** are genuinely different — `finishTableExecute` return type change, the credential-aware `createPageSource`, and the `getSplits`/`getNextBatch` rework are real signature breaks that can't share a class file with earlier versions. Given that, would it be worth folding 473-480 into a single module (with per-sourceSet/task JDK toolchain binding for the 479+ subset), and only keeping 481 and 482-483 as separate modules where the SPI actually forks? That would trade some build-script complexity for fewer near-duplicate module directories to keep in sync going forward. Not blocking — just flagging in case a consolidation pass is wanted before this grows further with each new Trino release. -- 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]
