Copilot commented on code in PR #11201:
URL: https://github.com/apache/gravitino/pull/11201#discussion_r3295759217
##########
core/src/main/java/org/apache/gravitino/storage/relational/mapper/provider/base/SchemaMetaBaseSQLProvider.java:
##########
@@ -86,6 +86,22 @@ public String listSchemaPOsBySchemaIds(@Param("schemaIds")
List<Long> schemaIds)
+ "</script>";
}
+ public String listSchemaPOsByCatalogIdAndNamePrefix(
+ @Param("catalogId") Long catalogId,
+ @Param("schemaName") String schemaName,
+ @Param("descendantPrefix") String descendantPrefix) {
+ return "SELECT schema_id as schemaId, schema_name as schemaName,"
+ + " metalake_id as metalakeId, catalog_id as catalogId,"
+ + " schema_comment as schemaComment, properties, audit_info as
auditInfo,"
+ + " current_version as currentVersion, last_version as lastVersion,"
+ + " deleted_at as deletedAt"
+ + " FROM "
+ + TABLE_NAME
+ + " WHERE catalog_id = #{catalogId}"
+ + " AND (schema_name = #{schemaName} OR schema_name LIKE
CONCAT(#{descendantPrefix}, '%'))"
Review Comment:
The prefix match for cascade deletion uses `LIKE` with an unescaped pattern.
Since schema names are validated with patterns that allow `_` (and could allow
other LIKE metacharacters such as `%` depending on catalog capability), `LIKE
CONCAT(#{descendantPrefix}, '%')` can match unintended schemas (e.g., `a_b`
also matches `axb...`), causing incorrect cascade deletes.
Escape LIKE metacharacters in the prefix and add an `ESCAPE` clause so the
match is a literal prefix match.
##########
iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/service/dispatcher/TestIcebergNamespaceHookDispatcher.java:
##########
@@ -268,12 +268,12 @@ public void
testDropNamespaceDeletesTargetAndPhantomAncestors() throws Exception
verify(mockDispatcher, never()).dropNamespace(mockContext, parent);
verify(mockDispatcher, never()).dropNamespace(mockContext, grandparent);
+ // The leaf and both phantom ancestors are stale, so a single cascade
delete of the outermost
+ // empty ancestor (A) removes the whole stale chain in one batched
operation.
ArgumentCaptor<NameIdentifier> captor =
ArgumentCaptor.forClass(NameIdentifier.class);
- verify(mockEntityStore, org.mockito.Mockito.times(3))
- .delete(captor.capture(), eq(Entity.EntityType.SCHEMA));
- List<String> deletedNames =
-
captor.getAllValues().stream().map(NameIdentifier::name).collect(Collectors.toList());
- Assertions.assertEquals(Arrays.asList("A:B:C", "A:B", "A"), deletedNames);
+ verify(mockEntityStore, org.mockito.Mockito.times(1))
+ .delete(captor.capture(), eq(Entity.EntityType.SCHEMA), eq(true));
Review Comment:
This uses a fully-qualified reference to `org.mockito.Mockito.times(...)`
inside the method body even though the file already relies on
`org.mockito.Mockito` static imports for other helpers. To align with the
project's Java import hygiene guideline (avoid FQNs in code), prefer a static
import for `times` (or import `Mockito`) and call it without the package
qualifier.
##########
iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/dispatcher/IcebergNamespaceHookDispatcher.java:
##########
@@ -148,16 +147,22 @@ public void dropNamespace(IcebergRequestContext context,
Namespace namespace) {
() -> {
dispatcher.dropNamespace(context, namespace);
- // TODO: Use cascade mode deletion
- // Current behavior: only the leaf namespace is dropped from the
Iceberg catalog above.
- // We walk ancestors outermost-to-leaf and clean up a Gravitino
entity only while the
- // corresponding Iceberg namespace no longer exists, stopping at the
first ancestor that
- // still exists. As a result, ancestor entities are cleaned up only
when the underlying
- // Iceberg catalog also removes the empty parent namespaces on
leaf-drop, which is
- // catalog-implementation-dependent. For catalogs that keep empty
parents, operators may
- // need to drop empty parent namespaces manually.
+ // Only the leaf namespace is dropped from the Iceberg catalog
above. Walk its
+ // ancestors outermost-to-leaf to find the outermost ancestor whose
Iceberg namespace
+ // no longer exists, stopping at the first ancestor that still
exists. Everything from
+ // that outermost empty namespace down to the leaf is now stale in
Gravitino.
Review Comment:
This comment says the code walks ancestors "outermost-to-leaf", but the
implementation iterates from the innermost ancestor outward (`for (i = size-1;
i >= 0; i--)`) because `HierarchicalSchemaUtil.getAncestorNames` returns
ancestors ordered outermost→innermost. Updating the wording will help readers
reason about why `outermostStale` ends up being the outermost missing ancestor.
--
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]
