roryqi commented on code in PR #12354:
URL: https://github.com/apache/gravitino/pull/12354#discussion_r3712270588
##########
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 in the script is the actual PostgreSQL-generated name.
PostgreSQL limits identifiers to 63 bytes and truncates the generated name for
`UNIQUE (tag_id, metadata_object_id, metadata_object_type, deleted_at)` to
`tag_relation_meta_tag_id_metadata_object_id_metadata_object_key`. Therefore
the existing `DROP CONSTRAINT` targets the v1.3.0 constraint; no `DO` block is
needed.
##########
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:
This will be addressed by restoring database-level uniqueness across all
three backends. The no-value case also needs a non-null storage representation
because nullable columns do not reliably enforce uniqueness. The replacement
key will cover `(tag_id, metadata_object_id, metadata_object_type, tag_value,
deleted_at)` in both schema and upgrade scripts.
##########
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:
The delete mapper accepts `TagMetadataObjectRelPO` and its SQL reads only
`tagId`, `metadataObjectId`, `metadataObjectType`, and `tagValue`; the
initialized audit/version fields are not used by the delete statement.
Introducing a dedicated one-use parameter type would expand the mapper/provider
surface without changing behavior, so I prefer to keep the existing PO
construction here.
--
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]
