Copilot commented on code in PR #10913:
URL: https://github.com/apache/gravitino/pull/10913#discussion_r3195151718
##########
core/src/main/java/org/apache/gravitino/storage/relational/mapper/provider/base/OwnerMetaBaseSQLProvider.java:
##########
@@ -247,4 +248,19 @@ public String deleteOwnerMetasByLegacyTimeline(
+ OWNER_TABLE_NAME
+ " WHERE deleted_at > 0 AND deleted_at < #{legacyTimeline} LIMIT
#{limit}";
}
+
+ public String selectOwnerByMetadataObjectId(@Param("metadataObjectId") long
metadataObjectId) {
+ return "SELECT owner_id as ownerId, owner_type as ownerType FROM "
+ + OWNER_TABLE_NAME
+ + " WHERE metadata_object_id = #{metadataObjectId} AND deleted_at = 0"
Review Comment:
`selectOwnerByMetadataObjectId` filters only by `metadata_object_id`. In the
relational store, numeric IDs can collide across different
`metadata_object_type`s, so this query can return the wrong owner when multiple
entity types share the same ID. Consider requiring `metadataObjectType` (and/or
metalake/catalog/schema scope if applicable) as an input and adding it to the
WHERE clause to make the lookup unambiguous.
##########
core/src/main/java/org/apache/gravitino/storage/relational/mapper/provider/base/RoleMetaBaseSQLProvider.java:
##########
@@ -192,4 +193,15 @@ 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}";
+ }
+
+ public String batchGetRoleUpdatedAt(@Param("roleIds") List<Long> roleIds) {
+ return "<script>SELECT role_id as roleId, role_name as roleName,
updated_at as updatedAt FROM "
+ + ROLE_TABLE_NAME
+ + " WHERE role_id IN <foreach item='id' collection='roleIds' open='('
separator=',' close=')'>#{id}</foreach>"
+ + " AND deleted_at = 0</script>";
Review Comment:
`batchGetRoleUpdatedAt` will generate `WHERE role_id IN ()` when `roleIds`
is empty, which is invalid SQL and will fail at runtime. Please guard against
empty lists (e.g., return a query that yields 0 rows, or handle the empty-list
case before invoking the mapper).
##########
core/src/main/java/org/apache/gravitino/storage/relational/service/UserMetaService.java:
##########
@@ -252,6 +252,11 @@ public <E extends Entity & HasIdentifier> UserEntity
updateUser(
mapper ->
mapper.softDeleteUserRoleRelByUserAndRoles(
newEntity.id(), Lists.newArrayList(deleteRoleIds)));
+ },
+ () -> {
+ long now = System.currentTimeMillis();
+ SessionUtils.doWithoutCommit(
+ UserMetaMapper.class, mapper ->
mapper.touchUpdatedAt(oldUserPO.getUserId(), now));
});
Review Comment:
This change introduces a new persistence-side behavior (touching
`user_meta.updated_at` on user updates) but there is no service-level test
asserting the column changes when `updateUser` runs. Please add/extend a test
(e.g., in `TestUserMetaService`) to validate `updated_at` is updated as part of
the update transaction, since this is the cache staleness sentinel.
##########
core/src/main/java/org/apache/gravitino/storage/relational/service/RoleMetaService.java:
##########
@@ -259,6 +259,11 @@ public <E extends Entity & HasIdentifier> RoleEntity
updateRole(
SessionUtils.doWithoutCommit(
SecurableObjectMapper.class,
mapper ->
mapper.batchInsertSecurableObjects(insertSecurableObjectPOs));
+ },
+ () -> {
+ long now = System.currentTimeMillis();
+ SessionUtils.doWithoutCommit(
+ RoleMetaMapper.class, mapper ->
mapper.touchUpdatedAt(rolePO.getRoleId(), now));
});
Review Comment:
This change introduces a new persistence-side behavior (touching
`role_meta.updated_at` on role updates) but there is no service-level test
asserting the column changes when `updateRole` runs. Please add/extend a test
(e.g., in `TestRoleMetaService`) to validate `updated_at` is updated as part of
the update transaction, since this is the cache staleness sentinel.
--
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]
