jerryshao commented on code in PR #13880:
URL: https://github.com/apache/gravitino/pull/13880#discussion_r4291871375
##########
core/src/main/java/org/apache/gravitino/storage/relational/po/RolePO.java:
##########
@@ -31,6 +33,17 @@ public class RolePO {
private Long lastVersion;
private Long deletedAt;
+ private List<SecurableObjectPO> securableObjectPOs = new ArrayList<>();
+
+ /**
+ * Returns privileges read in the same statement as this role.
+ *
+ * @return the privilege snapshot, or an empty list for metadata-only reads
+ */
+ public List<SecurableObjectPO> getSecurableObjectPOs() {
Review Comment:
[Important] `getSecurableObjectPOs()` returns an empty list both for a role
that genuinely has no privileges and for a `RolePO` that came from a
metadata-only read (`selectRoleMetaByMetalakeIdAndName`), and nothing records
which of the two it is. The consumers turn that ambiguity into silent
authorization-data rewrites rather than an error:
- `RoleMetaService.listSecurableObjects` (RoleMetaService.java:521) would
yield an empty privilege list, so `updateRole` computes `oldObjects = {}`
(RoleMetaService.java:253-256) and re-inserts every privilege while deleting
none.
- `UserPO.getRolePOs()` / `GroupPO.getRolePOs()` have the same shape:
`updateUser` diffs role IDs as sets (UserMetaService.java:250, :261-268), so an
unloaded list leaves `deleteRoleIds` empty and re-grants every membership.
Every caller in this PR is correct — I checked all of them, and the three
`listSecurableObjects` sites (RoleMetaService.java:146, :243, :398) plus the
four principal sites all pass the `withPrivileges` / `withRoles` flag — so this
is latent, not a live bug. But the dual-mode read is new here, the two modes
differ only by a boolean, and the compiler gives no help. Suggestion: have the
aggregate mappers mark the PO as loaded and add a `Preconditions.checkState` at
the top of `listSecurableObjects` and the two membership diffs, so a future
metadata-only read fails loudly instead of quietly rewriting privileges or
memberships.
Verified by: grepped every reader of the four new accessors across
core/src/main, then read `updateRole`, `updateUser` and `updateGroup` to
confirm an empty old list produces insert-all / delete-none.
##########
core/src/main/java/org/apache/gravitino/storage/relational/mapper/RoleMetaMapper.java:
##########
@@ -122,4 +128,88 @@ Integer deleteRoleMetasByLegacyTimeline(
@SelectProvider(type = RoleMetaSQLProviderFactory.class, method =
"batchGetRoleUpdatedAt")
List<RoleUpdatedAt> batchGetRoleUpdatedAt(@Param("roleIds") List<Long>
roleIds);
+ /**
+ * Defines the nested result mapping for single-statement role reads.
+ *
+ * @return unused; this declaration is never executed
+ */
+ @Results(
+ id = "rolePrivilegeSnapshot",
+ value = {
+ @Result(property = "id", column = "relationId", id = true),
+ @Result(property = "roleId", column = "roleId"),
+ @Result(property = "metadataObjectId", column = "metadataObjectId"),
+ @Result(property = "type", column = "type"),
+ @Result(property = "privilegeNames", column = "privilegeNames"),
+ @Result(property = "privilegeConditions", column =
"privilegeConditions"),
+ @Result(property = "currentVersion", column = "currentVersion"),
+ @Result(property = "lastVersion", column = "lastVersion"),
+ @Result(property = "deletedAt", column = "deletedAt")
+ })
+ @Select("SELECT 1")
+ SecurableObjectPO rolePrivilegeSnapshot();
Review Comment:
[Nit] `rolePrivilegeSnapshot()`, `membershipRoleSnapshot()`
(RoleMetaMapper.java:214) and `tableColumnSnapshot()` (TableMetaMapper.java:74)
exist only to register a `@Results` id, and the Javadoc says so — but they are
still public mapper methods, so calling one compiles and then fails at runtime
inside the result mapping. Where a shared child map can hang off a real
statement you already avoid this (`tableWithColumns` at TableMetaMapper.java:84
plus `@ResultMap`); for a map that really is shared across mappers, an XML
`<resultMap>` fragment would keep the placeholder off the interface. If the
annotation form stays, a name that reads as unmistakably internal would at
least discourage accidental calls. Note a `default` method will not work as a
substitute: MyBatis skips default methods when parsing, so the result map would
never be registered.
Verified by: read all three placeholder declarations and the
`tableWithColumns` / `@ResultMap` pair in TableMetaMapper.
##########
core/src/main/java/org/apache/gravitino/storage/relational/mapper/provider/base/TableMetaBaseSQLProvider.java:
##########
@@ -426,4 +428,78 @@ public String batchSelectTableByIdentifier(
"""
.formatted(TABLE_NAME, TableVersionMapper.TABLE_NAME);
}
+ /**
+ * Builds a single-statement table and column snapshot by schema ID and name.
+ *
+ * @param schemaId the schema ID
+ * @param tableName the table name
+ * @return the aggregate select SQL
+ */
+ public String selectTableWithColumnsBySchemaIdAndName(
+ @Param("schemaId") Long schemaId, @Param("tableName") String tableName) {
+ return withColumns(selectTableMetaBySchemaIdAndName(schemaId, tableName));
+ }
+
+ /**
+ * Builds a single-statement table and column snapshot by full name.
+ *
+ * @param metalakeName the metalake name
+ * @param catalogName the catalog name
+ * @param schemaName the physical schema name
+ * @param tableName the table name
+ * @return the aggregate select SQL
+ */
+ public String selectTableWithColumnsByFullQualifiedName(
+ @Param("metalakeName") String metalakeName,
+ @Param("catalogName") String catalogName,
+ @Param("schemaName") String schemaName,
+ @Param("tableName") String tableName) {
+ return withColumns(
+ selectTableByFullQualifiedName(metalakeName, catalogName, schemaName,
tableName));
+ }
+
+ private String withColumns(String tableSql) {
+ // One statement gives the root, version and columns a common snapshot.
Overwrite retires old
+ // columns, so a second SELECT cannot reconstruct them. Aggregate history
once and join by the
+ // existing (table_id, table_version, column_id, deleted_at) unique key.
Correlated scans for
+ // each column would be quadratic for long histories. Root subqueries have
independent bound
+ // parameters on all supported backends. Exclude DELETE markers only after
resolving history,
+ // otherwise a dropped column would reappear through its earlier
CREATE/UPDATE row.
+ String rootSql = tableSql.strip().replaceFirst(";\\s*$", "");
+ return "SELECT t.*, "
+ + "c.column_id AS col_columnId, "
+ + "c.column_name AS col_columnName, "
+ + "c.column_position AS col_columnPosition, "
+ + "c.metalake_id AS col_metalakeId, "
+ + "c.catalog_id AS col_catalogId, "
+ + "c.schema_id AS col_schemaId, "
+ + "c.table_id AS col_tableId, "
+ + "c.table_version AS col_tableVersion, "
+ + "c.column_type AS col_columnType, "
+ + "c.column_comment AS col_columnComment, "
+ + "c.column_nullable AS col_nullable, "
+ + "c.column_auto_increment AS col_autoIncrement, "
+ + "c.column_default_value AS col_defaultValue, "
+ + "c.column_op_type AS col_columnOpType, "
+ + "c.deleted_at AS col_deletedAt, "
+ + "c.audit_info AS col_auditInfo "
+ + "FROM ("
+ + rootSql
+ + ") t LEFT JOIN ("
+ + " SELECT history.table_id, history.column_id,
MAX(history.table_version) AS max_version"
+ + " FROM "
+ + TableColumnMapper.COLUMN_TABLE_NAME
+ + " history JOIN ("
+ + rootSql
+ + ") observed"
+ + " ON history.table_id = observed.tableId AND history.table_version
<= observed.currentVersion"
Review Comment:
[Question] The root select is inlined twice, so
`selectTableWithColumnsByFullQualifiedName` evaluates the
metalake-catalog-schema-table-version chain once as `t` and again as
`observed`. `POStorageReadRouting.getPO` falls back to the full-name route
whenever the entity-id cache is off (POStorageReadRouting.java:68, :75), so in
that configuration every table get pays the five-table join twice.
Two things I could not settle from the PR text: (a) was the EXPLAIN ANALYZE
with 45,000 history rows run against this full-name variant, or only the
schema-id one? (b) was a form without the duplicate considered — `LATERAL` is
not portable to all targets, but bounding history with a scalar subquery on
`table_meta.current_version`, or feeding `latest` from
`selectTableIdBySchemaIdAndName`, would pin the history scan without re-running
the parent chain.
Not blocking: the correctness argument for the duplicate holds, since both
occurrences read one statement snapshot at READ_COMMITTED. I would just like
the full-name cost on the record.
Verified by: rendered both provider methods by hand from this file, then
read `POStorageReadRouting.getPO` for the routing and SqlSessions.java:65 for
the isolation level.
##########
core/src/main/java/org/apache/gravitino/storage/relational/service/RoleMetaService.java:
##########
@@ -514,7 +517,8 @@ public int deleteRoleMetasByLegacyTimeline(long
legacyTimeline, int limit) {
}
private static List<SecurableObject> listSecurableObjects(RolePO po) {
- List<SecurableObjectPO> securableObjectPOs =
listSecurableObjectsByRoleId(po.getRoleId());
+ // Full role conversions must use privileges from the same statement as
the role metadata.
+ List<SecurableObjectPO> securableObjectPOs = po.getSecurableObjectPOs();
Review Comment:
[Nit] Now that all three callers read privileges from the role statement,
`listSecurableObjectsByRoleId` (RoleMetaService.java:456) has no production
caller left; the only reference in the tree is the assertion at
core/src/test/java/org/apache/gravitino/storage/relational/service/TestRoleMetaService.java:191.
Its `@Monitored` metric `listSecurableObjectsByRoleId` will therefore go to
zero, which is worth a line in the PR description for anyone alerting on it.
Either drop the method and have the test assert through
`SecurableObjectMapper`, or keep it with a comment saying it is test-only.
Verified by: grep for `listSecurableObjectsByRoleId` across the repository —
the only hits outside its own definition, the mapper and the provider are that
one test assertion.
##########
core/src/main/java/org/apache/gravitino/storage/relational/mapper/provider/base/TableMetaBaseSQLProvider.java:
##########
@@ -426,4 +428,78 @@ public String batchSelectTableByIdentifier(
"""
.formatted(TABLE_NAME, TableVersionMapper.TABLE_NAME);
}
+ /**
+ * Builds a single-statement table and column snapshot by schema ID and name.
+ *
+ * @param schemaId the schema ID
+ * @param tableName the table name
+ * @return the aggregate select SQL
+ */
+ public String selectTableWithColumnsBySchemaIdAndName(
+ @Param("schemaId") Long schemaId, @Param("tableName") String tableName) {
+ return withColumns(selectTableMetaBySchemaIdAndName(schemaId, tableName));
+ }
+
+ /**
+ * Builds a single-statement table and column snapshot by full name.
+ *
+ * @param metalakeName the metalake name
+ * @param catalogName the catalog name
+ * @param schemaName the physical schema name
+ * @param tableName the table name
+ * @return the aggregate select SQL
+ */
+ public String selectTableWithColumnsByFullQualifiedName(
+ @Param("metalakeName") String metalakeName,
+ @Param("catalogName") String catalogName,
+ @Param("schemaName") String schemaName,
+ @Param("tableName") String tableName) {
+ return withColumns(
+ selectTableByFullQualifiedName(metalakeName, catalogName, schemaName,
tableName));
+ }
+
+ private String withColumns(String tableSql) {
+ // One statement gives the root, version and columns a common snapshot.
Overwrite retires old
+ // columns, so a second SELECT cannot reconstruct them. Aggregate history
once and join by the
+ // existing (table_id, table_version, column_id, deleted_at) unique key.
Correlated scans for
+ // each column would be quadratic for long histories. Root subqueries have
independent bound
+ // parameters on all supported backends. Exclude DELETE markers only after
resolving history,
+ // otherwise a dropped column would reappear through its earlier
CREATE/UPDATE row.
+ String rootSql = tableSql.strip().replaceFirst(";\\s*$", "");
Review Comment:
[Nit] `tableSql.strip().replaceFirst(";\\s*$", "")` makes `withColumns`
depend on two undocumented properties of whatever the root provider returns.
The trailing `;` only exists because `selectTableByFullQualifiedName` (line
344) is a text block that happens to end with one, and the wrapping is valid
only while the root is static SQL. A sibling in this same class,
`batchSelectTableByIdentifier` (line 397), is `<script>`-wrapped; if a backend
override ever returns dynamic SQL for either root, the `<script>` tag lands
inside `FROM (...)`, is no longer at position 0, and MyBatis ships the XML to
the database as literal text. A `Preconditions.checkArgument` that the root
carries no `<` (or just a sentence stating the static-SQL contract) would turn
that into a fast, obvious failure.
Verified by: read both root providers and `batchSelectTableByIdentifier` in
this file; MyBatis's XML language driver treats a provider string as dynamic
only when it starts with `<script>`.
--
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]
