diqiu50 commented on code in PR #14682:
URL: https://github.com/apache/gravitino/pull/14682#discussion_r4490479460
##########
core/src/main/java/org/apache/gravitino/tag/TagManager.java:
##########
@@ -482,6 +482,42 @@ private String[] associateTagValuesForMetadataObject(
}
}
+ private static NameIdentifier checkAndNormalizeMetadataObject(
+ String metalake, MetadataObject metadataObject) {
+ NameIdentifier ident = MetadataObjectUtil.toEntityIdent(metalake,
metadataObject);
+ // Existence checks use normalizing dispatchers and must see the original
request. Applying
+ // normalization first would fold quoted names again after their quotes
have been removed.
+ MetadataObjectUtil.checkMetadataObject(metalake, metadataObject);
+ if
(!SUPPORTED_METADATA_OBJECT_TYPES_FOR_TAGS.contains(metadataObject.type())
+ || metadataObject.type() == MetadataObject.Type.CATALOG) {
+ return ident;
+ }
+
+ CatalogManager catalogManager =
GravitinoEnv.getInstance().catalogManager();
+ Capability capability = CapabilityHelpers.getCapability(ident,
catalogManager);
+ Capability.Scope scope =
Capability.Scope.valueOf(metadataObject.type().name());
+ if (scope == Capability.Scope.COLUMN) {
Review Comment:
This duplicates `MetadataIdConverter.normalizeIdentifier`, and the two
already differ on `COLUMN` parent resolution. Can we move it to
`CapabilityHelpers` and share it?
##########
core/src/test/java/org/apache/gravitino/tag/TestTagManager.java:
##########
@@ -381,6 +421,285 @@ public void cleanUp() {
Arrays.stream(tagManager.listTags(METALAKE)).forEach(n ->
tagManager.deleteTag(METALAKE, n));
}
+ @ParameterizedTest
+ @EnumSource(
+ value = Entity.EntityType.class,
+ names = {"SCHEMA", "TABLE", "COLUMN", "VIEW", "FUNCTION"})
+ void testTagOperationsNormalizeCaseInsensitiveNames(Entity.EntityType type) {
Review Comment:
Please add a Glue-style case (`SCHEMA`/`TABLE` fold, `COLUMN` keeps case)
and a test that resolution failure leaves tag relations unchanged.
##########
core/src/main/java/org/apache/gravitino/tag/TagManager.java:
##########
@@ -482,6 +482,42 @@ private String[] associateTagValuesForMetadataObject(
}
}
+ private static NameIdentifier checkAndNormalizeMetadataObject(
+ String metalake, MetadataObject metadataObject) {
+ NameIdentifier ident = MetadataObjectUtil.toEntityIdent(metalake,
metadataObject);
+ // Existence checks use normalizing dispatchers and must see the original
request. Applying
+ // normalization first would fold quoted names again after their quotes
have been removed.
+ MetadataObjectUtil.checkMetadataObject(metalake, metadataObject);
+ if
(!SUPPORTED_METADATA_OBJECT_TYPES_FOR_TAGS.contains(metadataObject.type())
+ || metadataObject.type() == MetadataObject.Type.CATALOG) {
+ return ident;
+ }
+
+ CatalogManager catalogManager =
GravitinoEnv.getInstance().catalogManager();
+ Capability capability = CapabilityHelpers.getCapability(ident,
catalogManager);
+ Capability.Scope scope =
Capability.Scope.valueOf(metadataObject.type().name());
+ if (scope == Capability.Scope.COLUMN) {
+ NameIdentifier table =
+ CapabilityHelpers.applyCaseSensitive(
+ NameIdentifier.of(ident.namespace().levels()),
Capability.Scope.TABLE, capability);
+ table = CapabilityHelpers.resolvePhysicalTableName(table,
catalogManager);
+ return NameIdentifier.of(
+ Namespace.of(ObjectArrays.concat(table.namespace().levels(),
table.name())),
+ CapabilityHelpers.applyCaseSensitiveOnName(scope, ident.name(),
capability));
Review Comment:
Columns are folded without checking the stored name, and there's no
equivalent of `resolvePhysicalTableName`. With a custom `normalizeName`, an
exact column name that worked before can now fail. Should we match against the
table's stored columns?
--
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]
