yuqi1129 commented on code in PR #13880:
URL: https://github.com/apache/gravitino/pull/13880#discussion_r4375581112
##########
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:
Fixed in 2f925cb84d. Moved all three nested result maps into
RoleMetaMapper.xml and TableMetaMapper.xml and removed the public SELECT 1
placeholder methods. The real aggregate statements reference the same
result-map IDs and physical child identities. The H2 service and snapshot
suites pass with the XML maps, including loaded-empty collections and duplicate
privilege targets.
##########
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:
Fixed in 2f925cb84d. The wrapper now rejects a MyBatis <script> root before
constructing/executing the aggregate statement and explains the static-root
contract next to the normalization. Tests cover dynamic roots through both
root-provider overrides, and static full-name SQL with its trailing semicolon.
Removing the guard makes the new test fail.
##########
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:
Addressed by documenting the tradeoff in 2f925cb84d and narrowing the PR's
validation claims. I cannot substantiate a measured full-name cost from the
retained earlier EXPLAIN evidence, so the description no longer implies that it
benchmarks this variant. The full-name route, including when the entity-ID
cache is disabled, still repeats the parent/root joins. The audit now records
that cost explicitly: retaining identical statically bound roots keeps history
bounded to the observed table and avoids backend-specific LATERAL syntax within
one statement snapshot. A new SQL-plan rewrite or full-name throughput
benchmark is outside this comment follow-up; no throughput improvement is
claimed.
--
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]
