Copilot commented on code in PR #12098:
URL: https://github.com/apache/gravitino/pull/12098#discussion_r3612083895
##########
core/src/main/java/org/apache/gravitino/catalog/CapabilityHelpers.java:
##########
@@ -343,38 +342,38 @@ private static Expression applyCapabilities(Expression
expression, Capability ca
private static FilesetChange applyCapabilities(
FilesetChange.RenameFileset renameFileset, Capability capabilities) {
+ applyNameSpecification(Capability.Scope.FILESET,
renameFileset.getNewName(), capabilities);
String newName =
applyCaseSensitiveOnName(
Capability.Scope.FILESET, renameFileset.getNewName(),
capabilities);
- applyNameSpecification(Capability.Scope.FILESET, newName, capabilities);
return FilesetChange.rename(newName);
}
private static ViewChange applyCapabilities(
ViewChange.RenameView renameView, Capability capabilities) {
+ applyNameSpecification(Capability.Scope.VIEW, renameView.getNewName(),
capabilities);
String newName =
applyCaseSensitiveOnName(Capability.Scope.VIEW,
renameView.getNewName(), capabilities);
- applyNameSpecification(Capability.Scope.VIEW, newName, capabilities);
return ViewChange.rename(newName);
}
private static TableChange applyCapabilities(
TableChange.RenameTable renameTable, Capability capabilities) {
+ applyNameSpecification(Capability.Scope.TABLE, renameTable.getNewName(),
capabilities);
String newName =
applyCaseSensitiveOnName(Capability.Scope.TABLE,
renameTable.getNewName(), capabilities);
String newSchemaName =
renameTable
.getNewSchemaName()
.map(s -> applyCaseSensitiveOnName(Capability.Scope.SCHEMA, s,
capabilities))
.orElse(null);
Review Comment:
TableChange.RenameTable now validates/normalizes the new table name, but the
optional new schema name is only normalized and never validated against the
SCHEMA name specification. This means an illegal schema name can slip through
(and for quote-aware catalogs, validation should also happen on the original
form before normalization).
##########
core/src/main/java/org/apache/gravitino/catalog/TableNormalizeDispatcher.java:
##########
@@ -52,8 +51,9 @@ public NameIdentifier[] listTables(Namespace namespace)
throws NoSuchSchemaExcep
// The constraints of the name spec may be more strict than underlying
catalog,
// and for compatibility reasons, we only apply case-sensitive
capabilities here.
Namespace caseSensitiveNs = normalizeCaseSensitive(namespace);
- NameIdentifier[] identifiers = dispatcher.listTables(caseSensitiveNs);
- return normalizeCaseSensitive(identifiers);
+ // The catalog guarantees listTables() already returns names in their
canonical, legal
+ // form, so no re-normalization is needed here.
+ return dispatcher.listTables(caseSensitiveNs);
Review Comment:
The new comment says the catalog "guarantees" listTables() returns
canonical/legal names, but that guarantee is not stated in the TableCatalog
contract. Since the code now relies on this assumption (to avoid non-idempotent
normalizeName()), the comment should be softened to avoid overstating the API
contract.
##########
core/src/main/java/org/apache/gravitino/catalog/SchemaNormalizeDispatcher.java:
##########
@@ -57,10 +56,9 @@ public NameIdentifier[] listSchemas(Namespace namespace)
throws NoSuchCatalogExc
NameIdentifier.of(namespace.levels())));
}
- NameIdentifier[] identifiers = dispatcher.listSchemas(namespace);
- // 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 normalizeCaseSensitive(identifiers);
+ // The catalog guarantees listSchemas() already returns names in their
canonical, legal
+ // form, so no re-normalization is needed here.
+ return dispatcher.listSchemas(namespace);
Review Comment:
The new comment claims the catalog "guarantees" listSchemas() returns
canonical/legal names, but this is not stated in the SupportsSchemas contract.
Since the dispatcher behavior now depends on this assumption, reword the
comment to avoid overstating the API contract.
##########
core/src/main/java/org/apache/gravitino/catalog/SchemaNormalizeDispatcher.java:
##########
@@ -57,10 +56,9 @@ public NameIdentifier[] listSchemas(Namespace namespace)
throws NoSuchCatalogExc
NameIdentifier.of(namespace.levels())));
}
- NameIdentifier[] identifiers = dispatcher.listSchemas(namespace);
- // 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 normalizeCaseSensitive(identifiers);
+ // The catalog guarantees listSchemas() already returns names in their
canonical, legal
+ // form, so no re-normalization is needed here.
+ return dispatcher.listSchemas(namespace);
}
Review Comment:
listSchemas() no longer applies case-sensitive normalization to returned
identifiers. There is no unit test that exercises this path with a
quote-aware/non-idempotent normalizeName implementation to ensure we don't
regress back to re-normalizing catalog-returned names.
##########
core/src/main/java/org/apache/gravitino/catalog/TableNormalizeDispatcher.java:
##########
@@ -52,8 +51,9 @@ public NameIdentifier[] listTables(Namespace namespace)
throws NoSuchSchemaExcep
// The constraints of the name spec may be more strict than underlying
catalog,
// and for compatibility reasons, we only apply case-sensitive
capabilities here.
Namespace caseSensitiveNs = normalizeCaseSensitive(namespace);
- NameIdentifier[] identifiers = dispatcher.listTables(caseSensitiveNs);
- return normalizeCaseSensitive(identifiers);
+ // The catalog guarantees listTables() already returns names in their
canonical, legal
+ // form, so no re-normalization is needed here.
+ return dispatcher.listTables(caseSensitiveNs);
Review Comment:
listTables() no longer applies case-sensitive normalization to the returned
identifiers (to avoid non-idempotent normalizeName() behavior). There is no
unit test covering this new behavior with a quote-aware/non-idempotent
normalizeName implementation, so regressions here would be easy to miss.
--
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]
