Copilot commented on code in PR #12234:
URL: https://github.com/apache/gravitino/pull/12234#discussion_r3671970828
##########
scripts/postgresql/schema-2.0.0-postgresql.sql:
##########
@@ -523,21 +525,25 @@ CREATE TABLE IF NOT EXISTS tag_relation_meta (
tag_id BIGINT NOT NULL,
metadata_object_id BIGINT NOT NULL,
metadata_object_type VARCHAR(64) NOT NULL,
+ tag_value VARCHAR(256) DEFAULT NULL,
+ value_order INT NOT NULL DEFAULT 0,
audit_info TEXT NOT NULL,
current_version INT NOT NULL DEFAULT 1,
last_version INT NOT NULL DEFAULT 1,
deleted_at BIGINT NOT NULL DEFAULT 0,
- PRIMARY KEY (id),
- UNIQUE (tag_id, metadata_object_id, metadata_object_type, deleted_at)
+ PRIMARY KEY (id)
);
Review Comment:
The previous uniqueness constraint preventing duplicate active tag-object
relations was removed, but no replacement constraint/index was added for the
new multi-valued model. This can allow duplicate active rows for the same
(tag_id, metadata_object_id, metadata_object_type, tag_value), and even
multiple NULL tag_value rows (valueless assignment), especially under
concurrent updates—leading to inconsistent reads and unbounded relation growth.
Consider adding a uniqueness guarantee such as: (1) a unique constraint/index
on (tag_id, metadata_object_id, metadata_object_type, tag_value, deleted_at)
for valued rows, and (2) a separate mechanism to ensure at most one active NULL
tag_value row per tag/object (e.g., partial unique index in PostgreSQL, or a
generated/coalesced key / sentinel approach in MySQL/H2). The same issue
appears in the MySQL and H2 schema diffs where the prior UNIQUE KEY was removed.
##########
core/src/main/java/org/apache/gravitino/storage/relational/mapper/provider/base/TagMetadataObjectRelBaseSQLProvider.java:
##########
@@ -130,6 +160,30 @@ public String
batchDeleteTagMetadataObjectRelsByTagIdsAndMetadataObject(
+ "</script>";
}
+ public String
batchDeleteTagMetadataObjectRelsByTagIdsAndValuesAndMetadataObject(
+ @Param("metadataObjectId") Long metadataObjectId,
+ @Param("metadataObjectType") String metadataObjectType,
+ @Param("tagRels") List<TagMetadataObjectRelPO> tagRelPOs) {
+ return "<script>"
+ + "UPDATE "
+ + TagMetadataObjectRelMapper.TAG_METADATA_OBJECT_RELATION_TABLE_NAME
+ + " SET deleted_at = (UNIX_TIMESTAMP() * 1000.0)"
+ + " + EXTRACT(MICROSECOND FROM CURRENT_TIMESTAMP(3)) / 1000"
+ + " WHERE metadata_object_id = #{metadataObjectId}"
+ + " AND metadata_object_type = #{metadataObjectType} AND deleted_at =
0"
+ + " AND ("
+ + "<foreach item='item' collection='tagRels' separator=' OR '>"
+ + "(tag_id = #{item.tagId}"
+ + "<choose>"
+ + "<when test='item.tagValue == null'> AND tag_value IS NULL</when>"
+ + "<otherwise> AND tag_value = #{item.tagValue}</otherwise>"
+ + "</choose>"
Review Comment:
Building the WHERE clause as a long 'OR' chain can become a query-planning
and execution bottleneck as the number of removals grows (and can hit SQL
length limits). A more scalable approach is to use a set-based form: e.g.,
`(tag_id, tag_value) IN (...)` for non-null values plus a separate `tag_value
IS NULL` branch, or (DB-specific) joining against a derived table/VALUES list
of (tag_id, tag_value) pairs. This typically produces better plans and avoids
very large OR expressions.
##########
core/src/main/java/org/apache/gravitino/storage/relational/service/TagMetaService.java:
##########
@@ -291,80 +311,188 @@ public List<GenericEntity>
listAssociatedMetadataObjectsForTag(NameIdentifier ta
@Monitored(
metricsSource = GRAVITINO_RELATIONAL_STORE_METRIC_NAME,
baseMetricName = "associateTagsWithMetadataObject")
- public List<TagEntity> associateTagsWithMetadataObject(
+ public List<AssignedTagEntity> associateTagsWithMetadataObject(
NameIdentifier objectIdent,
Entity.EntityType objectType,
NameIdentifier[] tagsToAdd,
NameIdentifier[] tagsToRemove)
throws NoSuchEntityException, EntityAlreadyExistsException, IOException {
+ return associateTagValuesWithMetadataObject(
+ objectIdent,
+ objectType,
+ toValuelessTagValues(tagsToAdd),
+ toValuelessTagValues(tagsToRemove),
+ true /* failOnDuplicateValuelessAssignment */);
+ }
+
+ @Monitored(
+ metricsSource = GRAVITINO_RELATIONAL_STORE_METRIC_NAME,
+ baseMetricName = "associateTagValuesWithMetadataObject")
+ public List<AssignedTagEntity> associateTagValuesWithMetadataObject(
+ NameIdentifier objectIdent,
+ Entity.EntityType objectType,
+ TagValue[] tagsToAdd,
+ TagValue[] tagsToRemove)
+ throws NoSuchEntityException, EntityAlreadyExistsException, IOException {
+ return associateTagValuesWithMetadataObject(
+ objectIdent,
+ objectType,
+ tagsToAdd,
+ tagsToRemove,
+ false /* failOnDuplicateValuelessAssignment */);
+ }
+
+ private List<AssignedTagEntity> associateTagValuesWithMetadataObject(
+ NameIdentifier objectIdent,
+ Entity.EntityType objectType,
+ TagValue[] tagsToAdd,
+ TagValue[] tagsToRemove,
+ boolean failOnDuplicateValuelessAssignment)
+ throws NoSuchEntityException, EntityAlreadyExistsException, IOException {
MetadataObject metadataObject =
NameIdentifierUtil.toMetadataObject(objectIdent, objectType);
String metalake = objectIdent.namespace().level(0);
try {
Long metadataObjectId = EntityIdService.getEntityId(objectIdent,
objectType);
-
- // Fetch all the tags need to associate with the metadata object.
- List<String> tagNamesToAdd =
-
Arrays.stream(tagsToAdd).map(NameIdentifier::name).collect(Collectors.toList());
- List<TagPO> tagPOsToAdd =
- tagNamesToAdd.isEmpty()
+ List<TagValue> tagValuesToAdd = new
ArrayList<>(Arrays.asList(nullToEmpty(tagsToAdd)));
+ List<TagValue> tagValuesToRemove = new
ArrayList<>(Arrays.asList(nullToEmpty(tagsToRemove)));
+ Set<TagValue> commonTagValues = new LinkedHashSet<>(tagValuesToAdd);
+ commonTagValues.retainAll(tagValuesToRemove);
+ tagValuesToAdd.removeAll(commonTagValues);
+ tagValuesToRemove.removeAll(commonTagValues);
+
+ List<String> tagNamesToUpdate = tagNamesToUpdate(tagValuesToAdd,
tagValuesToRemove);
+ List<TagPO> tagPOsToUpdate =
+ tagNamesToUpdate.isEmpty()
? Collections.emptyList()
- : getTagPOsByMetalakeAndNames(metalake, tagNamesToAdd);
+ : getTagPOsByMetalakeAndNames(metalake, tagNamesToUpdate);
+ Map<String, TagPO> tagPOsByName = tagPOsByName(tagPOsToUpdate);
- // Fetch all the tags need to remove from the metadata object.
- List<String> tagNamesToRemove =
-
Arrays.stream(tagsToRemove).map(NameIdentifier::name).collect(Collectors.toList());
- List<TagPO> tagPOsToRemove =
- tagNamesToRemove.isEmpty()
- ? Collections.emptyList()
- : getTagPOsByMetalakeAndNames(metalake, tagNamesToRemove);
+ List<TagPO> currentTagPOs =
+ SessionUtils.getWithoutCommit(
+ TagMetadataObjectRelMapper.class,
+ mapper ->
+ mapper.listTagPOsByMetadataObjectIdAndType(
+ metadataObjectId, metadataObject.type().toString()));
+ Map<Long, Set<Optional<String>>> activeValuesByTagId = new
LinkedHashMap<>();
+ Map<Long, Integer> maxValueOrderByTagId = new LinkedHashMap<>();
+ for (TagPO currentTagPO : currentTagPOs) {
+ trackExistingAssignment(currentTagPO, activeValuesByTagId,
maxValueOrderByTagId);
+ }
+
+ List<Long> tagIdsToRemove = new ArrayList<>();
+ List<TagMetadataObjectRelPO> tagRelsToRemove = new ArrayList<>();
+ for (TagValue tagValueToRemove : tagValuesToRemove) {
+ TagPO tagPO = tagPOsByName.get(tagValueToRemove.name());
+ if (tagPO == null) {
+ continue;
+ }
+
+ if (tagValueToRemove.value().isPresent()) {
+ activeValuesByTagId
+ .computeIfAbsent(tagPO.getTagId(), ignored -> new
LinkedHashSet<>())
+ .remove(tagValueToRemove.value());
+ tagRelsToRemove.add(
+ tagRelForValue(tagPO, metadataObjectId, metadataObject,
tagValueToRemove));
+ } else {
+ activeValuesByTagId.remove(tagPO.getTagId());
+ maxValueOrderByTagId.remove(tagPO.getTagId());
+ tagIdsToRemove.add(tagPO.getTagId());
+ }
+ }
+
+ List<TagMetadataObjectRelPO> tagRelsToAdd = new ArrayList<>();
+ for (TagValue tagValueToAdd : tagValuesToAdd) {
+ TagPO tagPO = tagPOsByName.get(tagValueToAdd.name());
+ if (tagPO == null) {
+ continue;
+ }
+
+ validateAllowedValue(tagPO, tagValueToAdd);
+ Set<Optional<String>> activeValues =
+ activeValuesByTagId.computeIfAbsent(tagPO.getTagId(), ignored ->
new LinkedHashSet<>());
+ Optional<String> value = tagValueToAdd.value();
+ if (activeValues.contains(value)) {
+ if (failOnDuplicateValuelessAssignment && !value.isPresent()) {
+ throw new EntityAlreadyExistsException(
+ "Tag %s is already associated to metadata object %s",
+ tagValueToAdd.name(), metadataObject);
+ }
+ continue;
+ }
+
+ if (value.isPresent()) {
+ if (activeValues.remove(Optional.empty())) {
+ tagRelsToRemove.add(
+ tagRelForValue(
+ tagPO,
+ metadataObjectId,
+ metadataObject,
+ TagValue.noValue(tagValueToAdd.name())));
+ }
+ } else {
+ Preconditions.checkArgument(
+ activeValues.isEmpty(),
+ "Cannot add tag with no value %s while valued assignments are
active",
+ tagValueToAdd.name());
Review Comment:
This error message is hard to action because it doesn’t identify which
metadata object is being updated and the phrasing is a bit ambiguous. Consider
including the metadata object identifier/type (or `metadataObject`) and
clarifying that the tag already has valued assignments, e.g., 'Cannot add a
valueless assignment for tag X on metadata object Y because valued assignments
exist'.
##########
core/src/main/java/org/apache/gravitino/meta/AssignedTagEntity.java:
##########
@@ -0,0 +1,133 @@
+/*
+ * 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.meta;
+
+import com.google.common.base.Preconditions;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Optional;
+import org.apache.gravitino.Audit;
+import org.apache.gravitino.Auditable;
+import org.apache.gravitino.Entity;
+import org.apache.gravitino.Field;
+import org.apache.gravitino.HasIdentifier;
+import org.apache.gravitino.Namespace;
+import org.apache.gravitino.tag.Tag;
+import org.apache.gravitino.tag.TagAssignment;
+import org.apache.gravitino.tag.TagValueConstraint;
+
+/** A tag entity in a metadata-object relation context, with assignment values
from the relation. */
+public final class AssignedTagEntity implements Tag, Entity, Auditable,
HasIdentifier {
+
+ private final TagEntity tagEntity;
+ private final TagAssignment assignment;
+
+ private AssignedTagEntity(TagEntity tagEntity, TagAssignment assignment) {
+ this.tagEntity = tagEntity;
+ this.assignment = assignment;
+ }
+
+ /**
+ * Creates an assigned tag entity.
+ *
+ * @param tagEntity The tag definition entity.
+ * @param assignmentValues The assignment values from the relation row.
Empty means no value.
+ * @return The assigned tag entity.
+ */
+ public static AssignedTagEntity of(TagEntity tagEntity, String[]
assignmentValues) {
+ Preconditions.checkArgument(tagEntity != null, "tagEntity must not be
null");
+ String[] values = assignmentValues == null ? new String[0] :
assignmentValues.clone();
+ TagAssignment assignment =
+ values.length == 0 ? TagAssignment.noValue() :
TagAssignment.ofValues(values);
+ return new AssignedTagEntity(tagEntity, assignment);
+ }
+
+ @Override
+ public Map<Field, Object> fields() {
+ return tagEntity.fields();
+ }
Review Comment:
`AssignedTagEntity` is introduced explicitly to carry relation-context
assignment values, but `fields()` currently delegates to `TagEntity.fields()`
and does not expose assignment values through the `Entity` field map. If any
generic serialization / projection / logging relies on `Entity.fields()`
(rather than `Tag.assignment()`), assignment values may be silently dropped.
Consider adding an `ASSIGNMENT` (or similar) field to
`AssignedTagEntity.fields()` that reflects the assignment state/values, or
clearly documenting that assignment values are only accessible via
`Tag.assignment()` and not via `Entity.fields()`.
--
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]
