yuqi1129 commented on code in PR #13481:
URL: https://github.com/apache/gravitino/pull/13481#discussion_r4091640885
##########
core/src/main/java/org/apache/gravitino/catalog/TableOperationDispatcher.java:
##########
@@ -159,26 +160,38 @@ public NameIdentifier[] listTables(Namespace namespace)
throws NoSuchSchemaExcep
*/
@Override
public Table loadTable(NameIdentifier ident) throws NoSuchTableException {
+ // Resolve the physical name (for backends whose normalization is not
reversible) under a READ
+ // lock, so the resolved name and the load that follows act atomically.
The resolved identifier
+ // then drives the catalog call, the entity store key and the per-table
locks below. For the
+ // common catalog that does not implement the resolution capability this
is a no-op returning
+ // ident.
+ NameIdentifier resolvedIdent =
+ TreeLockUtils.doWithTreeLock(ident, LockType.READ, () ->
resolvePhysicalName(ident));
Review Comment:
The comment says resolution and the following load act atomically, but this
READ lock is released before `internalLoadTable(resolvedIdent)` acquires a
separate lock, so they are not atomic. The gap itself is harmless (a concurrent
drop just surfaces as `NoSuchTableException`), but it adds an extra tree-lock
acquire/release plus a catalog lookup on every `loadTable`, which is a hot
path. Suggest resolving without the extra lock and rewording the comment.
##########
core/src/test/java/org/apache/gravitino/connector/TestCatalogOperations.java:
##########
@@ -111,7 +111,8 @@ public class TestCatalogOperations
FilesetCatalog,
TopicCatalog,
ModelCatalog,
- SupportsSchemas {
+ SupportsSchemas,
+ SupportsTableNameResolution {
Review Comment:
`TestCatalogOperations` is shared by many test classes. Making it globally
resolve case-insensitively changes the behavior of every test using it, e.g.
any test expecting `NoSuchTableException` for a differently-cased name. Could
this live in a dedicated subclass or test catalog used only by the new tests?
##########
core/src/main/java/org/apache/gravitino/catalog/TableOperationDispatcher.java:
##########
@@ -292,6 +306,11 @@ public Table alterTable(NameIdentifier ident,
TableChange... changes)
nameIdentifierForLock.equals(ident) ? LockType.READ : LockType.WRITE,
() -> {
NameIdentifier catalogIdent = getCatalogIdentifier(ident);
+ // Resolve the physical name inside the alter lock so resolution,
the catalog alter and
+ // the store update act atomically on the same object. No-op for
catalogs that do not
+ // implement the resolution capability. The rename target name
inside the changes is left
+ // as-is (a create-like new name follows the normal folding).
+ NameIdentifier resolvedIdent = resolvePhysicalName(ident);
Review Comment:
Minor: the lock here is taken on the unresolved `ident` (or its
schema/catalog), not on `resolvedIdent`, so "act atomically on the same object"
overstates it for the non-rename case, where the table-level lock is on a
different tree node than the physical table. Same wording in
`dropTable`/`purgeTable` and in the `resolvePhysicalName` Javadoc.
##########
core/src/test/java/org/apache/gravitino/catalog/TestTableOperationDispatcher.java:
##########
@@ -1673,4 +1673,73 @@ private static Table createTable(NameIdentifier ident) {
public static SchemaOperationDispatcher getSchemaOperationDispatcher() {
return schemaOperationDispatcher;
}
+
+ @Test
+ public void testPhysicalNameResolutionRoundTrip() throws IOException {
+ // A catalog whose ops implements SupportsTableNameResolution
(TestCatalogOperations does) must
+ // let a table created under one case be loaded/altered/dropped by a
differently-cased name that
+ // resolves to it, and the resolved name must drive the entity store key
(proving resolution and
+ // the operation act under the same lock, consistently). This is the
end-to-end round-trip that
+ // motivates the SPI.
+ Namespace tableNs = Namespace.of(metalake, catalog, "schema_resolve");
+ Map<String, String> props = ImmutableMap.of("k1", "v1", "k2", "v2");
+
schemaOperationDispatcher.createSchema(NameIdentifier.of(tableNs.levels()),
"comment", props);
+
+ // Physical stored name is mixed-case "physical_Name".
+ NameIdentifier storedIdent = NameIdentifier.of(tableNs, "physical_Name");
+ Column[] columns =
+ new Column[] {
+ TestColumn.builder()
+ .withName("col1")
+ .withPosition(0)
+ .withType(Types.StringType.get())
+ .build()
+ };
+ tableOperationDispatcher.createTable(storedIdent, columns, "comment",
props, new Transform[0]);
+
+ // A caller passes an all-uppercase folded name that no exact entry
matches; the resolver maps
+ // it
+ // case-insensitively to the stored "physical_Name".
+ NameIdentifier foldedIdent = NameIdentifier.of(tableNs, "PHYSICAL_NAME");
+
+ Table loaded = tableOperationDispatcher.loadTable(foldedIdent);
+ Assertions.assertEquals("physical_Name", loaded.name(), "load by folded
name resolves");
+ Assertions.assertTrue(tableOperationDispatcher.tableExists(foldedIdent));
+
+ // The entity store key is the resolved physical name, not the folded one.
+ Assertions.assertTrue(entityStore.exists(storedIdent, TABLE));
+ Assertions.assertFalse(entityStore.exists(foldedIdent, TABLE));
+
+ // Drop by the folded name removes the real object and its store entity
(no orphan).
+ Assertions.assertTrue(tableOperationDispatcher.dropTable(foldedIdent));
+ Assertions.assertFalse(tableOperationDispatcher.tableExists(foldedIdent));
+ Assertions.assertFalse(entityStore.exists(storedIdent, TABLE));
+ }
+
+ @Test
+ public void
testPhysicalNameResolutionExactMatchWinsOverCaseInsensitiveSibling()
Review Comment:
This case mostly exercises the test double's own `resolveTableName` rather
than production code. Missing coverage that would be more valuable:
- the ambiguous case (several case-insensitive matches, no exact match)
keeps the normalized name;
- `alterTable` (including rename) and `purgeTable` through a resolved name;
- `JdbcCatalogOperations.resolveTableName` delegating to
`TableOperation.resolveTableName` and preserving the namespace.
Nit: the comment wrap `the resolver maps it` / `// case-insensitively ...`
in the round-trip test is broken.
--
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]
