yuqi1129 commented on code in PR #13481:
URL: https://github.com/apache/gravitino/pull/13481#discussion_r4089311510
##########
api/src/main/java/org/apache/gravitino/rel/TableCatalog.java:
##########
@@ -319,4 +319,28 @@ Table alterTable(NameIdentifier ident, TableChange...
changes)
default boolean purgeTable(NameIdentifier ident) throws
UnsupportedOperationException {
throw new UnsupportedOperationException("purgeTable not supported.");
}
+
+ /**
+ * Resolves a normalized table identifier to the identifier under which the
table is physically
+ * stored by the underlying source, when the two can differ.
+ *
+ * <p>Most catalogs store an object under exactly the name Gravitino
normalized it to, so the
+ * default implementation returns {@code ident} unchanged. A catalog whose
name normalization is
+ * not reversible — for example one that folds an unquoted name to a fixed
case while the source
+ * also preserves case-sensitive names created with a different case — may
override this to map
+ * the normalized name back to the actual stored name, so that a name
returned by {@link
+ * #listTables(Namespace)} can be loaded, altered, and dropped as given.
+ *
+ * <p>Implementations must be side-effect free and must not open new
connections beyond what the
+ * catalog already holds; they are invoked on the load/alter/drop path
before the operation runs.
+ * The returned identifier is used both for the underlying source call and
as the Gravitino entity
+ * store key, so it must stay consistent across both.
+ *
+ * @param ident A normalized table identifier.
+ * @return The identifier under which the table is physically stored; {@code
ident} unchanged when
+ * no mapping is needed.
+ */
+ default NameIdentifier resolveTableName(NameIdentifier ident) {
Review Comment:
This is user-facing API (also implemented by the Java client
`RelationalCatalog`), but the method only makes sense server-side. Could this
be a connector-side SPI instead of a default method on `TableCatalog`?
##########
core/src/main/java/org/apache/gravitino/catalog/TableNormalizeDispatcher.java:
##########
@@ -62,7 +64,7 @@ public NameIdentifier[] listTables(Namespace namespace)
throws NoSuchSchemaExcep
public Table loadTable(NameIdentifier ident) throws NoSuchTableException {
// The constraints of the name spec may be more strict than underlying
catalog,
// and for compatibility reasons, we only apply case-sensitive
capabilities here.
- return dispatcher.loadTable(normalizeCaseSensitive(ident));
+ return
dispatcher.loadTable(resolvePhysicalName(normalizeCaseSensitive(ident)));
Review Comment:
By the time the hook runs, the requested name has already been normalized.
If the backend holds both `FOO` and `foo`, the resolver can't tell which one
the user asked for. Could we pass the original `ident` as well?
##########
core/src/main/java/org/apache/gravitino/catalog/TableNormalizeDispatcher.java:
##########
@@ -129,4 +131,32 @@ private NameIdentifier
normalizeNameIdentifier(NameIdentifier tableIdent) {
Capability capability = getCapability(tableIdent, catalogManager);
return applyCapabilities(tableIdent, Capability.Scope.TABLE, capability);
}
+
+ /**
+ * Maps a normalized table identifier to the identifier under which the
table is physically stored
+ * by the catalog's backend, via {@link
org.apache.gravitino.rel.TableCatalog#resolveTableName}.
+ *
+ * <p>For catalogs that do not override that hook (the default), this
returns {@code ident}
+ * unchanged without touching the backend. Resolving here — before the
identifier is handed to the
+ * downstream dispatcher — ensures the same physical identifier drives both
the underlying catalog
+ * operation and the Gravitino entity store key, so the two never diverge.
+ */
+ private NameIdentifier resolvePhysicalName(NameIdentifier ident) {
Review Comment:
Two things here:
- This runs outside the tree lock that `TableOperationDispatcher` takes on
the resolved ident, so the table can be renamed or recreated between resolving
and acting on it.
- Any resolver RuntimeException now fails `tableExists`/`dropTable`
outright. Please define in the contract what a resolver should do when the
table is missing or the lookup fails (return the input unchanged vs. throw).
##########
catalogs/catalog-jdbc-common/src/main/java/org/apache/gravitino/catalog/jdbc/operation/TableOperation.java:
##########
@@ -148,4 +148,26 @@ void alterTable(String databaseName, String tableName,
TableChange... changes)
default JdbcTablePartitionOperations
createJdbcTablePartitionOperations(JdbcTable loadedTable) {
throw new UnsupportedOperationException("Table partition operation is not
supported yet");
}
+
+ /**
+ * Maps a normalized table name to the name under which the table is
physically stored, when the
+ * two can differ for this backend.
+ *
+ * <p>The default returns {@code tableName} unchanged: most backends store a
table under exactly
+ * the normalized name. A backend whose name normalization is not reversible
(for example one that
+ * folds unquoted names to a fixed case while also preserving case-sensitive
names) may override
+ * this to look the real stored name up from its catalog, so a name returned
by {@link
+ * #listTables(String)} round-trips through load/alter/drop.
+ *
+ * <p>Implementations must be side-effect free and reuse the operation's
existing data source
+ * rather than opening new connections.
+ *
+ * @param databaseName The name of the database (schema).
+ * @param tableName The normalized table name.
+ * @return The physically stored table name; {@code tableName} unchanged
when no mapping is
+ * needed.
+ */
+ default String resolveTableName(String databaseName, String tableName) {
Review Comment:
No backend overrides this in the PR, so behavior is unchanged for every
catalog. Could we include the concrete override (and an IT for the list →
load/drop round-trip) that motivates this?
##########
catalogs/catalog-jdbc-common/src/main/java/org/apache/gravitino/catalog/jdbc/JdbcCatalogOperations.java:
##########
@@ -364,6 +364,15 @@ public NameIdentifier[] listTables(Namespace namespace)
throws NoSuchSchemaExcep
* @return The loaded JdbcTable instance representing the table.
* @throws NoSuchTableException If the specified table does not exist in the
Jdbc.
*/
+ @Override
+ public NameIdentifier resolveTableName(NameIdentifier tableIdent) {
Review Comment:
This method was inserted between `loadTable`'s Javadoc and `loadTable`, so
the "Loads a table from the Jdbc..." Javadoc now attaches to `resolveTableName`
and `loadTable` has none. Please move it above that Javadoc block.
##########
core/src/test/java/org/apache/gravitino/catalog/CatalogTestUtils.java:
##########
@@ -65,5 +66,34 @@ public static void mockDoWithCatalog(CatalogManager
catalogManager, BaseCatalog<
})
.when(catalogManager)
.doWithCatalog(Mockito.any(), Mockito.any());
+
+ // TableNormalizeDispatcher resolves the physical table name through
+ // doWithCatalogWrapper(...).doWithTableOps(tableCatalog ->
+ // tableCatalog.resolveTableName(ident)).
+ // Stub the wrapper so that path runs with the default identity
resolveTableName, matching every
+ // catalog that does not override the hook, instead of returning null.
+ try {
+ CatalogManager.CatalogWrapper wrapper =
Mockito.mock(CatalogManager.CatalogWrapper.class);
+ TableCatalog identityTableOps = Mockito.mock(TableCatalog.class);
+ Mockito.when(identityTableOps.resolveTableName(Mockito.any()))
+ .thenAnswer(invocation -> invocation.getArgument(0));
+ Mockito.doAnswer(
+ invocation -> {
+ ThrowableFunction<TableCatalog, Object> fn =
invocation.getArgument(0);
+ return fn.apply(identityTableOps);
+ })
+ .when(wrapper)
+ .doWithTableOps(Mockito.any());
+ Mockito.doAnswer(
+ invocation -> {
+ ThrowableFunction<CatalogManager.CatalogWrapper, Object> fn =
+ invocation.getArgument(1);
+ return fn.apply(wrapper);
+ })
+ .when(catalogManager)
+ .doWithCatalogWrapper(Mockito.any(), Mockito.any());
Review Comment:
This shared helper is used by ~18 test classes and now globally stubs
`doWithCatalogWrapper` with a wrapper where only `doWithTableOps` is stubbed.
Other `doWithXxxOps` calls silently return `null`, which can mask failures in
future tests. Could this stub live only in `TestTableNormalizeDispatcher`?
##########
core/src/test/java/org/apache/gravitino/catalog/TestTableNormalizeDispatcher.java:
##########
@@ -275,6 +278,65 @@ public void testCreateTableListTablesLoadTableRoundTrip()
throws Exception {
Assertions.assertEquals("My Table", loadedIdentCaptor.getValue().name());
}
+ @Test
+ public void testResolvePhysicalNameDrivesDownstreamIdentifier() throws
Exception {
Review Comment:
The comment says load/alter/drop/purge/exists, but only load and drop are
asserted. Please also cover alter/purge/exists and the error paths
(`NoSuchCatalogException` passes through unchanged, checked exceptions get
wrapped, resolver throws).
--
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]
