Copilot commented on code in PR #9467:
URL: https://github.com/apache/gravitino/pull/9467#discussion_r2613367435
##########
core/src/main/java/org/apache/gravitino/utils/NameIdentifierUtil.java:
##########
@@ -700,6 +703,130 @@ public static NameIdentifier parentNameIdentifier(
}
}
+ /**
+ * Build a NameIdentifier for the given entity type and name, using the
provided entity context.
+ * This method constructs the identifier hierarchically using the parent
entity type
+ * relationships.
+ *
+ * <p>Examples:
+ *
+ * <pre>
+ * 1. Build a TABLE identifier:
+ * Input:
+ * type = Entity.EntityType.TABLE
+ * name = "users"
+ * entities = {
+ * METALAKE -> "my_metalake",
+ * CATALOG -> "my_catalog",
+ * SCHEMA -> "my_schema"
+ * }
+ * Result: NameIdentifier.of("my_metalake", "my_catalog", "my_schema",
"users")
+ *
+ * 2. Build a SCHEMA identifier:
+ * Input:
+ * type = Entity.EntityType.SCHEMA
+ * name = "my_schema"
+ * entities = {
+ * METALAKE -> "my_metalake",
+ * CATALOG -> "my_catalog"
+ * }
+ * Result: NameIdentifier.of("my_metalake", "my_catalog", "my_schema")
+ *
+ * 3. Build a USER identifier (virtual namespace type):
+ * Input:
+ * type = Entity.EntityType.USER
+ * name = "john_doe"
+ * entities = {
+ * METALAKE -> "my_metalake"
+ * }
+ * Result: NameIdentifier.of(NamespaceUtil.ofUser("my_metalake"),
"john_doe")
+ * which creates a NameIdentifier with namespace ["my_metalake",
"system", "user"]
+ * and name "john_doe"
+ *
+ * 4. Build a COLUMN identifier:
+ * Input:
+ * type = Entity.EntityType.COLUMN
+ * name = "id"
+ * entities = {
+ * METALAKE -> "my_metalake",
+ * CATALOG -> "my_catalog",
+ * SCHEMA -> "my_schema",
+ * TABLE -> "users"
+ * }
+ * Result: NameIdentifier.of("my_metalake", "my_catalog", "my_schema",
"users", "id")
+ * </pre>
+ *
+ * @param type The entity type
+ * @param name The entity name
+ * @param entities Map containing the metalake, catalog, schema names and
other parent entities
+ * @return The created NameIdentifier
+ * @throws IllegalArgumentException if the entity type is not supported or
required parent
+ * entities are missing
+ */
+ public static NameIdentifier buildNameIdentifier(
+ Entity.EntityType type, String name, Map<Entity.EntityType, String>
entities) {
+ String metalake = entities.get(Entity.EntityType.METALAKE);
+
+ // Handle metalake as the root
+ if (type == Entity.EntityType.METALAKE) {
+ return ofMetalake(metalake);
+ }
+
+ // Build the full name path by traversing the hierarchy
+ List<String> namePath = Lists.newArrayList();
+ namePath.add(name);
+
+ Entity.EntityType currentType = type;
+ while (currentType != Entity.EntityType.METALAKE) {
+ Entity.EntityType parentType = parentEntityType(currentType);
+ String parentName = entities.get(parentType);
+
+ if (parentType == Entity.EntityType.METALAKE) {
+ namePath.add(0, metalake);
+ break;
+ } else if (parentName == null) {
+ throw new IllegalArgumentException(
+ "Cannot build NameIdentifier for type "
+ + type
+ + ": missing parent entity of type "
+ + parentType);
+ }
+
+ namePath.add(0, parentName);
+ currentType = parentType;
+ }
+
+ if (SUPPORT_VIRTUAL_NAMESPACE_TYPES.contains(type)) {
+ Namespace namespace = createVirtualNamespace(type, metalake);
+ return NameIdentifier.of(namespace, name);
+ }
Review Comment:
There's a logical issue in the buildNameIdentifier method for virtual
namespace types. When building a NameIdentifier for entity types like USER,
GROUP, ROLE, etc., the method first builds the namePath by traversing parent
entities (lines 775-797), adding the metalake and potentially other parent
names to namePath. However, when the type is a virtual namespace type (checked
at line 799), it ignores the already-built namePath and creates a new
NameIdentifier using createVirtualNamespace at line 800-801.
This means for virtual namespace types, the while loop (lines 780-797)
performs unnecessary work building the namePath that gets discarded. The code
should check if the type is a virtual namespace type earlier, right after
handling METALAKE (after line 773), and directly return the result without
building the namePath.
##########
core/src/main/java/org/apache/gravitino/utils/NameIdentifierUtil.java:
##########
@@ -700,6 +703,130 @@ public static NameIdentifier parentNameIdentifier(
}
}
+ /**
+ * Build a NameIdentifier for the given entity type and name, using the
provided entity context.
+ * This method constructs the identifier hierarchically using the parent
entity type
+ * relationships.
+ *
+ * <p>Examples:
+ *
+ * <pre>
+ * 1. Build a TABLE identifier:
+ * Input:
+ * type = Entity.EntityType.TABLE
+ * name = "users"
+ * entities = {
+ * METALAKE -> "my_metalake",
+ * CATALOG -> "my_catalog",
+ * SCHEMA -> "my_schema"
+ * }
+ * Result: NameIdentifier.of("my_metalake", "my_catalog", "my_schema",
"users")
+ *
+ * 2. Build a SCHEMA identifier:
+ * Input:
+ * type = Entity.EntityType.SCHEMA
+ * name = "my_schema"
+ * entities = {
+ * METALAKE -> "my_metalake",
+ * CATALOG -> "my_catalog"
+ * }
+ * Result: NameIdentifier.of("my_metalake", "my_catalog", "my_schema")
+ *
+ * 3. Build a USER identifier (virtual namespace type):
+ * Input:
+ * type = Entity.EntityType.USER
+ * name = "john_doe"
+ * entities = {
+ * METALAKE -> "my_metalake"
+ * }
+ * Result: NameIdentifier.of(NamespaceUtil.ofUser("my_metalake"),
"john_doe")
+ * which creates a NameIdentifier with namespace ["my_metalake",
"system", "user"]
+ * and name "john_doe"
+ *
+ * 4. Build a COLUMN identifier:
+ * Input:
+ * type = Entity.EntityType.COLUMN
+ * name = "id"
+ * entities = {
+ * METALAKE -> "my_metalake",
+ * CATALOG -> "my_catalog",
+ * SCHEMA -> "my_schema",
+ * TABLE -> "users"
+ * }
+ * Result: NameIdentifier.of("my_metalake", "my_catalog", "my_schema",
"users", "id")
+ * </pre>
+ *
+ * @param type The entity type
+ * @param name The entity name
+ * @param entities Map containing the metalake, catalog, schema names and
other parent entities
+ * @return The created NameIdentifier
+ * @throws IllegalArgumentException if the entity type is not supported or
required parent
+ * entities are missing
+ */
+ public static NameIdentifier buildNameIdentifier(
+ Entity.EntityType type, String name, Map<Entity.EntityType, String>
entities) {
+ String metalake = entities.get(Entity.EntityType.METALAKE);
+
+ // Handle metalake as the root
+ if (type == Entity.EntityType.METALAKE) {
+ return ofMetalake(metalake);
+ }
+
+ // Build the full name path by traversing the hierarchy
+ List<String> namePath = Lists.newArrayList();
+ namePath.add(name);
+
+ Entity.EntityType currentType = type;
+ while (currentType != Entity.EntityType.METALAKE) {
+ Entity.EntityType parentType = parentEntityType(currentType);
+ String parentName = entities.get(parentType);
+
+ if (parentType == Entity.EntityType.METALAKE) {
+ namePath.add(0, metalake);
+ break;
+ } else if (parentName == null) {
+ throw new IllegalArgumentException(
+ "Cannot build NameIdentifier for type "
+ + type
+ + ": missing parent entity of type "
+ + parentType);
+ }
+
+ namePath.add(0, parentName);
+ currentType = parentType;
+ }
+
+ if (SUPPORT_VIRTUAL_NAMESPACE_TYPES.contains(type)) {
+ Namespace namespace = createVirtualNamespace(type, metalake);
+ return NameIdentifier.of(namespace, name);
+ }
+
+ // Create NameIdentifier from the full path
+ return NameIdentifier.of(namePath.toArray(new String[0]));
+ }
+
+ /**
+ * Build a map of NameIdentifiers from entity data.
+ *
+ * @param entities Map containing entity types and their names (e.g.,
METALAKE, CATALOG, SCHEMA,
+ * etc.)
+ * @return Map of entity types to their NameIdentifiers
+ */
+ public static Map<Entity.EntityType, NameIdentifier> buildNameIdentifierMap(
+ Map<Entity.EntityType, String> entities) {
+ Map<Entity.EntityType, NameIdentifier> nameIdentifierMap =
Maps.newHashMap();
+
+ entities.forEach(
+ (type, name) -> {
+ NameIdentifier identifier = buildNameIdentifier(type, name,
entities);
+ if (identifier != null) {
+ nameIdentifierMap.put(type, identifier);
+ }
Review Comment:
The null check on line 822 is unnecessary because buildNameIdentifier never
returns null - it either returns a valid NameIdentifier or throws an
IllegalArgumentException. This check adds no value and could mislead future
developers into thinking that buildNameIdentifier might return null. Consider
removing the if statement and always putting the identifier into the map.
```suggestion
nameIdentifierMap.put(type, identifier);
```
--
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]
