Copilot commented on code in PR #12354:
URL: https://github.com/apache/gravitino/pull/12354#discussion_r3712227387
##########
scripts/postgresql/upgrade-1.3.0-to-2.0.0-postgresql.sql:
##########
@@ -31,3 +31,13 @@ CREATE UNIQUE INDEX IF NOT EXISTS uk_mid_geid_del ON
group_meta (metalake_id, ex
ALTER TABLE table_column_version_info
ALTER COLUMN column_comment TYPE VARCHAR(4096);
+
+ALTER TABLE tag_meta ADD COLUMN IF NOT EXISTS allowed_values TEXT DEFAULT NULL;
+COMMENT ON COLUMN tag_meta.allowed_values IS 'tag allowed values as a JSON
string array, NULL allows any value, [] allows no value';
+
+ALTER TABLE tag_relation_meta ADD COLUMN IF NOT EXISTS tag_value VARCHAR(256)
DEFAULT NULL;
+COMMENT ON COLUMN tag_relation_meta.tag_value IS 'tag assignment value';
+
+ALTER TABLE tag_relation_meta DROP CONSTRAINT IF EXISTS
tag_relation_meta_tag_id_metadata_object_id_metadata_object_key;
Review Comment:
The constraint name being dropped looks inconsistent with Postgres’ default
auto-generated name for `UNIQUE (tag_id, metadata_object_id,
metadata_object_type, deleted_at)` (it typically includes all column names,
e.g., `..._metadata_object_type_deleted_at_key`). If this `DROP CONSTRAINT` is
a no-op, the old uniqueness will remain and will prevent storing multiple
assignment values per tag/object (the core feature of this PR). Update the
upgrade script to drop the correct existing constraint name from v1.3.0, or use
a Postgres `DO $$ ... $$` block to locate and drop the constraint by definition.
##########
core/src/main/java/org/apache/gravitino/storage/relational/service/TagMetaService.java:
##########
@@ -394,6 +514,104 @@ public int deleteTagMetasByLegacyTimeline(long
legacyTimeline, int limit) {
return tagDeletedCount[0] + tagMetadataObjectRelDeletedCount[0];
}
+ private static List<TagEntity> tagPOsToTagEntities(List<TagPO> tagPOs,
Namespace namespace) {
+ Map<Long, List<TagPO>> tagPOsByTagId = new LinkedHashMap<>();
+ for (TagPO tagPO : tagPOs) {
+ tagPOsByTagId.computeIfAbsent(tagPO.getTagId(), ignored -> new
ArrayList<>()).add(tagPO);
+ }
+
+ return tagPOsByTagId.values().stream()
+ .map(tagPOGroup -> tagPOsToTagEntity(tagPOGroup, namespace))
+ .collect(Collectors.toList());
+ }
+
+ private static TagEntity tagPOsToTagEntity(List<TagPO> tagPOGroup, Namespace
namespace) {
+ TagPO firstTagPO = tagPOGroup.get(0);
+ List<String> assignmentValues =
+ tagPOGroup.stream()
+ .map(TagPO::getAssignmentValue)
+ .filter(Objects::nonNull)
+ .distinct()
+ .collect(Collectors.toList());
+
+ TagAssignment assignment =
+ assignmentValues.isEmpty()
+ ? TagAssignment.noValue()
+ : TagAssignment.ofValues(assignmentValues.toArray(new String[0]));
+ return POConverters.fromTagPO(firstTagPO,
namespace).copyWithAssignment(assignment);
+ }
+
+ private static TagValue[] toValuelessTagValues(NameIdentifier[] tags) {
+ if (tags == null) {
+ return null;
+ }
+
+ return Arrays.stream(tags).map(tag ->
TagValue.noValue(tag.name())).toArray(TagValue[]::new);
+ }
+
+ private static TagValue[] nullToEmpty(TagValue[] tagValues) {
+ return tagValues == null ? new TagValue[0] : tagValues;
+ }
+
+ private static List<String> tagNamesToUpdate(
+ List<TagValue> tagsToAdd, List<TagValue> tagsToRemove) {
+ Set<String> tagNames = new LinkedHashSet<>();
+ tagsToAdd.stream().map(TagValue::name).forEach(tagNames::add);
+ tagsToRemove.stream().map(TagValue::name).forEach(tagNames::add);
+ return new ArrayList<>(tagNames);
+ }
+
+ private static Map<String, TagPO> tagPOsByName(List<TagPO> tagPOs) {
+ Map<String, TagPO> tagPOsByName = new LinkedHashMap<>();
+ for (TagPO tagPO : tagPOs) {
+ tagPOsByName.put(tagPO.getTagName(), tagPO);
+ }
+ return tagPOsByName;
+ }
+
+ private static void trackExistingAssignment(
+ TagPO tagPO, Map<Long, Set<Optional<String>>> activeValuesByTagId) {
+ activeValuesByTagId
+ .computeIfAbsent(tagPO.getTagId(), ignored -> new LinkedHashSet<>())
+ .add(Optional.ofNullable(tagPO.getAssignmentValue()));
+ }
+
+ private static TagMetadataObjectRelPO tagRelForValue(
+ TagPO tagPO, Long metadataObjectId, MetadataObject metadataObject,
TagValue tagValue) {
+ return POConverters.initializeTagMetadataObjectRelPOWithVersion(
+ tagPO.getTagId(),
+ metadataObjectId,
+ metadataObject.type().toString(),
+ tagValue.value().orElse(null));
+ }
Review Comment:
This helper is used to build delete criteria objects, but it delegates to
`initializeTagMetadataObjectRelPOWithVersion(...)`, which also generates
audit/version fields that are irrelevant for deletion and can be misleading. A
more maintainable approach is to construct a minimal PO (tagId,
metadataObjectId/type, tagValue) or introduce a small dedicated parameter
object for the delete-by-(tag,value) mapper method, so the intent and required
fields are explicit.
##########
scripts/postgresql/schema-2.0.0-postgresql.sql:
##########
@@ -523,21 +525,23 @@ 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,
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 on tag-object relations was removed, but no
replacement uniqueness is added to prevent duplicate active assignments. This
can allow multiple identical rows for the same (tag_id, metadata_object_id,
metadata_object_type, tag_value, deleted_at=0), leading to inconsistent reads
(duplicate assignment values) and race-condition duplicates under concurrent
updates. Consider adding a new uniqueness constraint/index that includes
`tag_value` (and ensures only one active valueless assignment as well), and
apply equivalent constraints across MySQL/H2/PostgreSQL schema + upgrade
scripts.
--
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]
