roryqi commented on code in PR #12234:
URL: https://github.com/apache/gravitino/pull/12234#discussion_r3683525344
##########
core/src/main/java/org/apache/gravitino/SupportsRelationOperations.java:
##########
@@ -92,6 +94,55 @@ <E extends Entity & HasIdentifier> List<E>
listEntitiesByRelation(
Type relType, NameIdentifier nameIdentifier, Entity.EntityType
identType, boolean allFields)
throws IOException;
+ /**
+ * List the entities according to a given entity in a specific relation,
optionally filtering by
+ * an exact value carried by the relation row.
+ *
+ * @param <E> the type of entities returned.
+ * @param relType The type of relation.
+ * @param nameIdentifier The given entity identifier.
Review Comment:
Clarified in the Javadocs. This parameter is the anchor entity for the
relation lookup; it can be either endpoint of the relation. `identType` tells
which endpoint it represents, and the returned entities are from the opposite
endpoint.
##########
core/src/main/java/org/apache/gravitino/storage/relational/RelationalEntityStore.java:
##########
@@ -490,6 +491,41 @@ public <E extends Entity & HasIdentifier> List<E>
updateEntityRelations(
return result;
}
+ @Override
+ public <E extends Entity & HasIdentifier> List<E> listEntitiesByRelation(
+ Type relType,
+ NameIdentifier nameIdentifier,
+ Entity.EntityType identType,
+ boolean allFields,
+ String relationValue)
Review Comment:
For the current use case, yes. The relation value maps to the tag assignment
value, which the public API models as a String and the relational storage
stores as a VARCHAR. I clarified the Javadocs to call this an exact string
value carried by the relation edge. If a future relation needs structured
relation metadata, we should introduce a separate typed payload instead of
overloading this field.
##########
core/src/main/java/org/apache/gravitino/storage/relational/RelationalEntityStore.java:
##########
@@ -490,6 +491,41 @@ public <E extends Entity & HasIdentifier> List<E>
updateEntityRelations(
return result;
}
+ @Override
+ public <E extends Entity & HasIdentifier> List<E> listEntitiesByRelation(
+ Type relType,
+ NameIdentifier nameIdentifier,
+ Entity.EntityType identType,
+ boolean allFields,
+ String relationValue)
+ throws IOException {
+ if (relationValue == null) {
+ return listEntitiesByRelation(relType, nameIdentifier, identType,
allFields);
+ }
+
+ return backend.listEntitiesByRelation(
+ relType, nameIdentifier, identType, allFields, relationValue);
+ }
+
+ @Override
+ public <E extends Entity & HasIdentifier> List<E> updateEntityRelations(
+ Type relType,
+ NameIdentifier srcEntityIdent,
+ Entity.EntityType srcEntityType,
+ RelationTarget[] destEntitiesToAdd,
+ RelationTarget[] destEntitiesToRemove)
+ throws IOException, NoSuchEntityException, EntityAlreadyExistsException {
+ List<E> result =
+ backend.updateEntityRelations(
+ relType, srcEntityIdent, srcEntityType, destEntitiesToAdd,
destEntitiesToRemove);
+
+ cache.invalidate(srcEntityIdent, srcEntityType, relType);
+ invalidateRelationTargetCache(relType, destEntitiesToAdd);
+ invalidateRelationTargetCache(relType, destEntitiesToRemove);
Review Comment:
I do not think we need an extra store-level cache lock here. The backend
update owns the transactional write, and this path does not read/populate the
cache. The invalidation calls already take the cache segmented lock internally.
I kept the same post-backend invalidation ordering as the existing update path,
and also fixed the legacy NameIdentifier[] overload to invalidate the
destination relation cache with the actual destination type.
--
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]
