diqiu50 commented on PR #12631:
URL: https://github.com/apache/gravitino/pull/12631#issuecomment-5909831228
Thanks for the update. Dropping SpiVersionCompat and renaming the
single-version modules both look good. My remaining concerns are
the duplication introduced by the new modules, and the reflection that is
still left in the shared code.
Current state. Since Trino 480 a few shared classes no longer compile
against the SPI (ColumnComments, SchemaFunctionNames,
GravitinoPageSinkProvider, GravitinoDataSourceProvider), and 482 breaks
more (GravitinoSplitSource, TypeSignatures, getSplits,
createPageSink/createMergeSink). Each module excludes these from the
shared source and ships its own same-named copy. Comparing the
current head:
- 480 vs 481: about 1.1k lines each and effectively identical.
ColumnComments, SchemaFunctionNames, GravitinoPageSinkProvider,
GravitinoConnectorFactory, GravitinoPlugin,
GravitinoNodePartitioningProvider and GravitinoDataSourceProvider are 100% the
same.
The only real SPI difference is the finishTableExecute return type in
GravitinoMetadata.
- 481 vs 482-483: ColumnComments, SchemaFunctionNames, ConnectorFactory,
Plugin and NodePartitioningProvider are still identical.
Only PageSinkProvider, SplitManager, SystemConnector, DataSourceProvider
and TypeSignatures really differ.
Every new Trino release would add another ~1.1k lines of mostly identical
code, and any fix to these helpers has to be applied in
several places. Also, changing what gets compiled via java.exclude on a
shared directory is fragile and hard to follow.
Suggestion: group the shared code by SPI shape, like spark-common in the
Spark connector. spark-common is not a Gradle module, just a
source directory that each version module adds to its srcDirs, with no
excludes. The same can work here:
trino-connector/common/ version-agnostic code, used by all
modules
trino-connector/common-473-479/ classes with the pre-480 SPI shape
trino-connector/common-480-481/ classes with the 480/481 SPI shape
- The real SPI boundaries are at 480 (getComment() is Optional,
SchemaFunctionName is a record, credential-aware sink/source) and
482, not at 481. 480 and 481 use identical shared classes, so one
directory covers both. 480 cannot share source with 479 without
reflection.
- 482-483 keeps only its own few classes for now, and can be promoted to a
shared directory when 484+ needs the same shapes.
- The pre-480 classes currently in the shared source move into
common-473-479, so each module just lists the directories it needs and
no exclude is required. Existing modules only need one extra srcDirs
line.
- Per-version modules then only contain classes whose SPI really differs
(e.g. GravitinoMetadata for 481).
- The near-identical build.gradle.kts files can also move into a
convention plugin or buildSrc, so each module declares only its
Trino version range and source directories.
Remaining reflection.
- GravitinoConstraint: predicate() and getPredicateColumns() exist up to
481 and were removed in 482, so this is a per-shape
difference, not something to detect at runtime. Keep a plain
GravitinoConstraint with the normal @Override and direct
delegate.predicate() / delegate.getPredicateColumns() calls in
common-473-479 and common-480-481, and a version without these two
methods for 482-483. The getMethod/invoke lookup, the
@SuppressWarnings("unchecked") and the exception unwrapping (about 60 lines)
can then be removed.
- TypeSignatureDeserializer (parseTypeSignature): same idea, move it into
the matching shape directory as a regular compile-time
implementation.
--
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]
