yuqi1129 commented on code in PR #12234: URL: https://github.com/apache/gravitino/pull/12234#discussion_r3689674327
########## core/src/main/java/org/apache/gravitino/RelationTarget.java: ########## @@ -0,0 +1,77 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.gravitino; + +import com.google.common.base.Preconditions; +import java.util.Optional; +import javax.annotation.Nullable; + +/** + * Represents a target entity in a relation update, with optional metadata carried by the relation + * row. + */ +public final class RelationTarget { Review Comment: Thanks. I agree that `RelationTarget` should not extend `NameIdentifier`; relation-edge metadata should not become identifier state. However, that only addresses the inheritance option and does not address my main concern about API extensibility. A single optional `relationValue` already doubles the `listEntitiesByRelation` overloads and adds a parallel `updateEntityRelations` overload. The suggestion that a future structured payload would require another type indicates that this pattern will keep expanding `SupportsRelationOperations`. I understand that the existing `NameIdentifier[]` method needs to remain for compatibility. Keeping it as a compatibility adapter is fine, but we should introduce one canonical, extensible abstraction—for example, `RelationQuery` for reads and `RelationUpdate`/`RelationEdgeTarget` with relation attributes for writes—so future relation metadata evolves inside those objects rather than through more overloads. An interface exposing `NameIdentifier` may still help with conversion, but the more important point is to separate endpoint identity from edge attributes and define which method is the canonical SPI for implementers. ########## core/src/main/java/org/apache/gravitino/SupportsRelationOperations.java: ########## @@ -220,4 +283,55 @@ default <E extends Entity & HasIdentifier> List<E> updateEntityRelations( throw new UnsupportedOperationException( "updateEntityRelations is not supported by this implementation"); } + + /** + * Updates the relations for a source entity by adding and removing target endpoints that can + * carry optional relation values on the relation edges. + * + * @param <E> The type of the entity returned in the list, which represents the final state of + * related entities. + * @param relType The type of relation to update. + * @param srcEntityIdent The identifier of the source entity whose relations are being updated. + * @param srcEntityType The source entity type. + * @param destEntitiesToAdd Target endpoints to associate with the source entity. + * @param destEntitiesToRemove Target endpoints to disassociate from the source entity. + * @return A list of entities that are related to the given entity after the update. + * @throws IOException If a storage-related error occurs. + * @throws NoSuchEntityException If any of the specified entities does not exist. + * @throws EntityAlreadyExistsException If a relation to be added already exists. + */ + default <E extends Entity & HasIdentifier> List<E> updateEntityRelations( + Type relType, + NameIdentifier srcEntityIdent, + Entity.EntityType srcEntityType, + RelationTarget[] destEntitiesToAdd, + RelationTarget[] destEntitiesToRemove) + throws IOException, NoSuchEntityException, EntityAlreadyExistsException { + if (hasRelationValues(destEntitiesToAdd) || hasRelationValues(destEntitiesToRemove)) { + throw new UnsupportedOperationException( + "updateEntityRelations with relation values is not supported by this implementation"); + } + + return updateEntityRelations( Review Comment: The compatibility path drops `RelationTarget.entityType()` when converting targets to `NameIdentifier[]`, but `RelationalEntityStore` later uses the original entity type for cache invalidation. Since the target type is not validated here, the backend can update the actual relation while the store invalidates a different cache key, leaving the reverse-side relation cache stale. For example, a `TAG_METADATA_OBJECT_REL` target incorrectly marked as `TABLE` can still be written through the legacy backend method, while `(tagIdent, TABLE, relType)` is invalidated instead of the actual `(tagIdent, TAG, relType)` cache entry. Please validate or derive the destination type before the backend write and use the same validated type for invalidation. -- 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]
