Copilot commented on code in PR #10913:
URL: https://github.com/apache/gravitino/pull/10913#discussion_r3199662654
##########
core/src/main/java/org/apache/gravitino/storage/relational/mapper/provider/base/OwnerMetaBaseSQLProvider.java:
##########
@@ -247,4 +248,23 @@ public String deleteOwnerMetasByLegacyTimeline(
+ OWNER_TABLE_NAME
+ " WHERE deleted_at > 0 AND deleted_at < #{legacyTimeline} LIMIT
#{limit}";
}
+
+ public String selectOwnerByMetadataObjectIdAndType(
+ @Param("metadataObjectId") long metadataObjectId,
+ @Param("metadataObjectType") String metadataObjectType) {
+ return "SELECT owner_id as ownerId, owner_type as ownerType FROM "
+ + OWNER_TABLE_NAME
+ + " WHERE metadata_object_id = #{metadataObjectId}"
+ + " AND metadata_object_type = #{metadataObjectType}"
+ + " AND deleted_at = 0"
+ + " ORDER BY updated_at DESC, id DESC LIMIT 1";
+ }
+
+ public String selectChangedOwners(@Param("updatedAtAfter") long
updatedAtAfter) {
+ return "SELECT metadata_object_id as metadataObjectId, updated_at as
updatedAt"
+ + " FROM "
+ + OWNER_TABLE_NAME
+ + " WHERE deleted_at = 0 AND updated_at >= #{updatedAtAfter}"
+ + " ORDER BY updated_at, id LIMIT 1000";
Review Comment:
`selectChangedOwners` only returns `metadata_object_id` and `updated_at`.
Since `owner_meta` is keyed by both `metadata_object_id` and
`metadata_object_type` (and this PR even adds APIs that require filtering by
type to avoid ID collisions), a change poller result without
`metadata_object_type` is ambiguous and can mis-associate changes when IDs
collide across object types. Consider selecting `metadata_object_type` too and
extending `ChangedOwnerInfo` accordingly (and updating the mapper/test).
##########
core/src/main/java/org/apache/gravitino/storage/relational/mapper/provider/base/UserMetaBaseSQLProvider.java:
##########
@@ -187,4 +187,21 @@ public String deleteUserMetasByLegacyTimeline(
+ USER_TABLE_NAME
+ " WHERE deleted_at > 0 AND deleted_at < #{legacyTimeline} LIMIT
#{limit}";
}
+
+ public String touchUserUpdatedAt(@Param("userId") long userId, @Param("now")
long now) {
+ return "UPDATE " + USER_TABLE_NAME + " SET updated_at = #{now} WHERE
user_id = #{userId}";
Review Comment:
`touchUserUpdatedAt` updates by `user_id` without filtering out soft-deleted
rows. To keep deleted records immutable and consistent with other update
statements in this file, add `AND deleted_at = 0` to the WHERE clause (and
consider whether the update should be a no-op vs. error when the user is
deleted).
##########
core/src/main/java/org/apache/gravitino/storage/relational/mapper/provider/base/RoleMetaBaseSQLProvider.java:
##########
@@ -192,4 +193,20 @@ public String deleteRoleMetasByLegacyTimeline(
+ ROLE_TABLE_NAME
+ " WHERE deleted_at > 0 AND deleted_at < #{legacyTimeline} LIMIT
#{limit}";
}
+
+ public String touchRoleUpdatedAt(@Param("roleId") long roleId, @Param("now")
long now) {
+ return "UPDATE " + ROLE_TABLE_NAME + " SET updated_at = #{now} WHERE
role_id = #{roleId}";
Review Comment:
`touchRoleUpdatedAt` updates by `role_id` without filtering out soft-deleted
rows. To avoid mutating deleted roles and to match the rest of the role-meta
update patterns, add `AND deleted_at = 0` to the WHERE clause.
##########
core/src/main/java/org/apache/gravitino/storage/relational/mapper/provider/base/OwnerMetaBaseSQLProvider.java:
##########
@@ -247,4 +248,23 @@ public String deleteOwnerMetasByLegacyTimeline(
+ OWNER_TABLE_NAME
+ " WHERE deleted_at > 0 AND deleted_at < #{legacyTimeline} LIMIT
#{limit}";
}
+
+ public String selectOwnerByMetadataObjectIdAndType(
+ @Param("metadataObjectId") long metadataObjectId,
+ @Param("metadataObjectType") String metadataObjectType) {
+ return "SELECT owner_id as ownerId, owner_type as ownerType FROM "
+ + OWNER_TABLE_NAME
+ + " WHERE metadata_object_id = #{metadataObjectId}"
+ + " AND metadata_object_type = #{metadataObjectType}"
+ + " AND deleted_at = 0"
+ + " ORDER BY updated_at DESC, id DESC LIMIT 1";
+ }
+
+ public String selectChangedOwners(@Param("updatedAtAfter") long
updatedAtAfter) {
+ return "SELECT metadata_object_id as metadataObjectId, updated_at as
updatedAt"
+ + " FROM "
+ + OWNER_TABLE_NAME
+ + " WHERE deleted_at = 0 AND updated_at >= #{updatedAtAfter}"
Review Comment:
The parameter name `updatedAtAfter` suggests an exclusive lower bound, but
the query uses `updated_at >= #{updatedAtAfter}` (inclusive). Either switch to
`>` or rename the parameter/method to reflect inclusive semantics to avoid
misuse by future callers.
--
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]
