roryqi commented on code in PR #11362:
URL: https://github.com/apache/gravitino/pull/11362#discussion_r3354188651
##########
core/src/main/java/org/apache/gravitino/catalog/TableOperationDispatcher.java:
##########
@@ -366,6 +367,17 @@ public boolean dropTable(NameIdentifier ident) {
} catch (Exception e) {
throw new RuntimeException(e);
}
+ if (droppedFromCatalog) {
+ SchemaEntityCleaner.deleteOrphanedSchemaEntities(
+ store,
+ schemaIdentifier,
+ true,
+ schemaIdent ->
+ doWithCatalog(
+ catalogIdent,
+ c -> c.doWithSchemaOps(s ->
s.schemaExists(schemaIdent)),
+ RuntimeException.class));
+ }
Review Comment:
Good catch. Moved the cleanup out of the `if (droppedFromCatalog)` guard so
it now runs unconditionally in `dropTable`, and added a regression test
(`testDropTableCleansUpOrphanSchemasWhenTableAlreadyGone`) for the out-of-band
case where `dropTable` returns false but the orphaned schema entities must
still be removed. This also aligns the core dispatchers with the Iceberg REST
hook path, which already cleans up unconditionally.
##########
core/src/main/java/org/apache/gravitino/catalog/TableOperationDispatcher.java:
##########
@@ -420,6 +432,17 @@ public boolean purgeTable(NameIdentifier ident) throws
UnsupportedOperationExcep
} catch (Exception e) {
throw new RuntimeException(e);
}
+ if (droppedFromCatalog) {
+ SchemaEntityCleaner.deleteOrphanedSchemaEntities(
+ store,
+ schemaIdentifier,
+ true,
+ schemaIdent ->
+ doWithCatalog(
+ catalogIdent,
+ c -> c.doWithSchemaOps(s ->
s.schemaExists(schemaIdent)),
+ RuntimeException.class));
+ }
Review Comment:
Fixed: `purgeTable` now runs the cleanup unconditionally as well. I also
removed the early `return false` in the `NoSuchEntityException` branch, which
previously short-circuited before cleanup even when the catalog purge
succeeded; it now logs and falls through, returning the catalog result
consistently with `dropTable`.
##########
core/src/main/java/org/apache/gravitino/catalog/ViewOperationDispatcher.java:
##########
@@ -292,6 +293,17 @@ public boolean dropView(NameIdentifier ident) {
} catch (Exception e) {
throw new RuntimeException(e);
}
+ if (droppedFromCatalog) {
+ SchemaEntityCleaner.deleteOrphanedSchemaEntities(
+ store,
+ schemaIdentifier,
+ true,
+ schemaIdent ->
+ doWithCatalog(
+ catalogIdent,
+ c -> c.doWithSchemaOps(s ->
s.schemaExists(schemaIdent)),
+ RuntimeException.class));
+ }
Review Comment:
Fixed: `dropView` now runs the cleanup unconditionally, covered by a
regression test (`testDropViewCleansUpOrphanSchemasWhenViewAlreadyGone`) for
the out-of-band case. The shared cleanup invocation is extracted into a
`cleanUpOrphanedSchemaEntities` helper.
--
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]
