Add catalog SPI modules and refactor coral-common to SPI dispatch#590
Add catalog SPI modules and refactor coral-common to SPI dispatch#590wmoustafa wants to merge 1 commit intolinkedin:masterfrom
Conversation
Introduce a 3-module catalog SPI architecture that decouples Hive and Iceberg implementations from the core classpath: - coral-catalog-spi: Pure interfaces (CoralCatalog, CoralTable, TableType), type system classes, and new SPI contracts (CoralCalciteTableAdapterFactory, CoralCalciteTableAdapterRegistry) with zero Hive/Iceberg dependencies. - coral-catalog-hive: Hive catalog implementations (HiveTable, HiveCalciteTableAdapter, HiveCalciteViewAdapter, TypeConverter) with SPI factory registration via META-INF/services. - coral-catalog-iceberg: Iceberg catalog implementations (IcebergTable, IcebergCalciteTableAdapter, IcebergHiveTableConverter) with SPI factory registration via META-INF/services. coral-common is refactored to use CoralCalciteTableAdapterRegistry for SPI-based dispatch instead of instanceof checks. Original implementation classes are retained as @deprecated wrappers for backward compatibility. Hive/Hadoop/Iceberg dependencies are demoted from api to implementation scope. Known limitation: ToRelConverter.toSqlNode() still couples the core to Hive's Table type (issue linkedin#575).
ruolin59
left a comment
There was a problem hiding this comment.
actually appears to mostly be moving things around and refactoring, but have a few questions about the module rewiring, and also, it would be good to add some new unit tests to cover the new items introduced in this pr, including
CoralCalciteTableAdapterRegistryTest: priority ordering, register() merging with ServiceLoader, UnsupportedOperationException on no-match.- One smoke test per new factory confirming supports() / createAdapter() return the expected adapter class for a representative CoralTable.
- One test per deprecated wrapper confirming new (...) produces an instance assignable to the new type and exposes the same public surface.
| } | ||
|
|
||
| return null; | ||
| return CoralCalciteTableAdapterRegistry.createAdapter(coralTable, |
There was a problem hiding this comment.
this method throws UnsupportedOperationException, I'd just like to confirm that this is the new behavior we want (so no catching an return null, correct?), if so, may be a good idea to at least update the javadoc
| } | ||
|
|
||
| api(deps.'hive'.'hive-metastore') { | ||
| // New catalog SPI and implementation modules |
There was a problem hiding this comment.
coral-catalog-hive and coral-catalog-iceberg appear to still have api dependencies on hive-metastore and iceberg-core respectively, so the consumer would still be pulling these deps transitively right? To clarify,
- Was the intent to demote coral-common's project deps to implementation, or is keeping them as api a deliberate compatibility choice?
- If it's deliberate, which downstream consumers are expected to migrate to depending on
coral-catalog-spidirectly? - Can the redundant implementation block at lines 21-34 be dropped?
What changes are proposed in this pull request, and why are they necessary?
This PR introduces a 3-module catalog SPI architecture that decouples Hive and Iceberg implementations from the core classpath. Currently,
coral-commonbundleshive-metastore,hadoop-common, andiceberg-*asapidependencies, which means every consumer transitively pulls all of them. This refactor isolates catalog-specific code behind an SPI boundary.New modules:
coral-catalog-spi— Pure interfaces (CoralCatalog,CoralTable,TableType), thecom.linkedin.coral.common.typespackage, and new SPI contracts (CoralCalciteTableAdapterFactory,CoralCalciteTableAdapterRegistry). Zero Hive/Iceberg dependencies.coral-catalog-hive— Hive catalog implementations (HiveTable,HiveCalciteTableAdapter,HiveCalciteViewAdapter,HiveToCoralTypeConverter,TypeConverter) withHiveCalciteTableAdapterFactoryregistered viaMETA-INF/services.coral-catalog-iceberg— Iceberg catalog implementations (IcebergTable,IcebergCalciteTableAdapter,IcebergToCoralTypeConverter,IcebergHiveTableConverter) withIcebergCalciteTableAdapterFactoryregistered viaMETA-INF/services.coral-commonchanges:CoralDatabaseSchema.getTable()now usesCoralCalciteTableAdapterRegistryfor SPI-based dispatch instead ofinstanceofchecks.@Deprecatedwrappers delegating to the new modules.apitoimplementationscope.Backward compatibility: All original FQCNs still resolve — interfaces/types kept the same package, and implementation classes have
@Deprecatedwrappers at the old locations. Consumers relying on transitiveapiexposure ofhive-metastoreoriceberg-*fromcoral-commonmay need to add explicit dependencies.Known limitations:
ToRelConverter.toSqlNode(String, Table)still couples the core to Hive'sTabletype (issue Refactor SQL parsing components to Use CoralTable Instead of Hive Table #575).IcebergHiveTableConverterincoral-catalog-icebergretainsimplementationdeps on Hive as a temporary bridge.How was this patch tested?
./gradlew compileJava— all modules compile successfully./gradlew :coral-common:test— existing tests passServiceLoaderdiscovers bothHiveCalciteTableAdapterFactoryandIcebergCalciteTableAdapterFactorycoral-catalog-spihas no Hive/Iceberg on compile classpath