jerryshao commented on code in PR #13689:
URL: https://github.com/apache/gravitino/pull/13689#discussion_r4215833322
##########
core/src/test/java/org/apache/gravitino/storage/TestSQLScripts.java:
##########
@@ -255,6 +259,303 @@ public void testUpgradeToTwoZeroBackfillsOccVersions()
throws SQLException, IOEx
}
}
+ /** Verifies a completed upgrade can be repeated without resetting migrated
data. */
+ @TestTemplate
+ public void testUpgradeToTwoZeroIsIdempotent() throws SQLException,
IOException {
+ Path scriptDir = upgradeScriptDirectory();
+ String suffix = "-" + backendType.toLowerCase() + ".sql";
+ File upgrade = scriptDir.resolve("upgrade-1.3.0-to-2.0.0" +
suffix).toFile();
+ dropAllTables();
+ executeScript(scriptDir.resolve("schema-1.3.0" + suffix).toFile());
+ seedUpgradeData();
+ executeScript(upgrade);
+ executeStatements(
+ List.of(
+ "UPDATE idp_user_meta SET enabled = FALSE, audit_info = 'preserved
audit'",
+ "UPDATE idp_group_meta SET group_comment = 'preserved comment'",
+ "UPDATE tag_meta SET allowed_values = '[\"blue\"]'",
+ "UPDATE tag_relation_meta SET tag_value = 'blue'",
+ "UPDATE fileset_meta SET occ_version = 42",
+ "UPDATE policy_meta SET occ_version = 43"),
+ "customized migrated data");
+ Map<String, Set<String>> expectedColumns = readSchemaColumns();
+ List<String> expectedIndexes = readSchemaIndexes(expectedColumns.keySet());
+ Map<String, List<List<String>>> expectedData = readUpgradeData();
+ executeScript(upgrade);
+ executeScript(upgrade);
+ Assertions.assertEquals(expectedColumns, readSchemaColumns());
+ Assertions.assertEquals(expectedIndexes,
readSchemaIndexes(expectedColumns.keySet()));
+ Assertions.assertEquals(expectedData, readUpgradeData(), "Retry must
preserve migrated values");
+ if ("mysql".equals(backendType)) {
+ // Neither rename source nor target exists: this is damage, not an
already-applied DDL.
+ executeStatements(
+ List.of("ALTER TABLE schema_meta DROP INDEX schema_meta_idx_mid"),
+ "missing target index");
+ AssertionFailedError failure =
+ Assertions.assertThrows(AssertionFailedError.class, () ->
executeScript(upgrade));
+ Assertions.assertInstanceOf(SQLException.class, failure.getCause());
+ Assertions.assertTrue(
+ failure.getCause().getMessage().contains("idx_mid"),
failure.getMessage());
Review Comment:
[Nit] `idx_mid` is a substring of `schema_meta_idx_mid`, the rename target
that line 293 just dropped, so this assertion also passes if the script fails
while naming the *target* index (for example a `Duplicate key name
'schema_meta_idx_mid'`) rather than the missing source. That is a different
failure mode from the one the comment on line 292 describes.
MySQL quotes the key name in 1176 (`Key 'idx_mid' doesn't exist in table
'schema_meta'`), so asserting on `"'idx_mid'"` keeps the backend-independence
you wanted here while still distinguishing source from target.
Verified by: read the guard and DDL at
`scripts/mysql/upgrade-1.3.0-to-2.0.0-mysql.sql:137-141` — the rename is
`idx_mid` -> `schema_meta_idx_mid`, and `idx_mid` appears 13 times in
`scripts/mysql/schema-1.3.0-mysql.sql`, so the bare substring is not unique to
this table either.
##########
core/src/test/java/org/apache/gravitino/storage/TestSQLScripts.java:
##########
@@ -255,6 +259,303 @@ public void testUpgradeToTwoZeroBackfillsOccVersions()
throws SQLException, IOEx
}
}
+ /** Verifies a completed upgrade can be repeated without resetting migrated
data. */
+ @TestTemplate
+ public void testUpgradeToTwoZeroIsIdempotent() throws SQLException,
IOException {
+ Path scriptDir = upgradeScriptDirectory();
+ String suffix = "-" + backendType.toLowerCase() + ".sql";
+ File upgrade = scriptDir.resolve("upgrade-1.3.0-to-2.0.0" +
suffix).toFile();
+ dropAllTables();
+ executeScript(scriptDir.resolve("schema-1.3.0" + suffix).toFile());
+ seedUpgradeData();
+ executeScript(upgrade);
+ executeStatements(
+ List.of(
+ "UPDATE idp_user_meta SET enabled = FALSE, audit_info = 'preserved
audit'",
+ "UPDATE idp_group_meta SET group_comment = 'preserved comment'",
+ "UPDATE tag_meta SET allowed_values = '[\"blue\"]'",
+ "UPDATE tag_relation_meta SET tag_value = 'blue'",
+ "UPDATE fileset_meta SET occ_version = 42",
+ "UPDATE policy_meta SET occ_version = 43"),
+ "customized migrated data");
+ Map<String, Set<String>> expectedColumns = readSchemaColumns();
+ List<String> expectedIndexes = readSchemaIndexes(expectedColumns.keySet());
+ Map<String, List<List<String>>> expectedData = readUpgradeData();
+ executeScript(upgrade);
+ executeScript(upgrade);
+ Assertions.assertEquals(expectedColumns, readSchemaColumns());
+ Assertions.assertEquals(expectedIndexes,
readSchemaIndexes(expectedColumns.keySet()));
+ Assertions.assertEquals(expectedData, readUpgradeData(), "Retry must
preserve migrated values");
+ if ("mysql".equals(backendType)) {
+ // Neither rename source nor target exists: this is damage, not an
already-applied DDL.
+ executeStatements(
+ List.of("ALTER TABLE schema_meta DROP INDEX schema_meta_idx_mid"),
+ "missing target index");
+ AssertionFailedError failure =
+ Assertions.assertThrows(AssertionFailedError.class, () ->
executeScript(upgrade));
+ Assertions.assertInstanceOf(SQLException.class, failure.getCause());
+ Assertions.assertTrue(
+ failure.getCause().getMessage().contains("idx_mid"),
failure.getMessage());
+ }
+ assertSchemaMatchesFreshInstall(expectedColumns, expectedIndexes);
+ }
+
+ /** Verifies retries after structural DDL and data updates, including
prepared DDL execution. */
+ @TestTemplate
+ public void testUpgradeToTwoZeroRetriesPartialExecution() throws
SQLException, IOException {
+ Path scriptDir = upgradeScriptDirectory();
+ String suffix = "-" + backendType.toLowerCase() + ".sql";
+ File schema = scriptDir.resolve("schema-1.3.0" + suffix).toFile();
+ File upgrade = scriptDir.resolve("upgrade-1.3.0-to-2.0.0" +
suffix).toFile();
+ List<String> statements = extractStatements(upgrade.toPath());
+ dropAllTables();
+ executeScript(schema);
+ seedUpgradeData();
+ executeScript(upgrade);
+ Map<String, Set<String>> expectedColumns = readSchemaColumns();
+ List<String> expectedIndexes = readSchemaIndexes(expectedColumns.keySet());
+ Map<String, List<List<String>>> expectedData = readUpgradeData();
+ assertSchemaMatchesFreshInstall(expectedColumns, expectedIndexes);
+
+ String lastIndexRename =
+ statements.stream()
+ .filter(sql -> sql.contains("RENAME INDEX"))
+ .reduce((first, last) -> last)
+ .orElse("");
+ boolean testedIndexRename = false;
+ for (int completed = 1; completed <= statements.size(); completed++) {
+ String last = statements.get(completed - 1).toUpperCase();
+ if (!(last.startsWith("ALTER TABLE")
+ || last.startsWith("CREATE ")
+ || last.startsWith("UPDATE ")
+ || last.startsWith("EXECUTE "))) {
+ continue;
+ }
+ // MySQL renames share one guard shape; sample the first and last rename.
+ if (last.startsWith("EXECUTE ") && statements.get(completed -
3).contains("RENAME INDEX")) {
Review Comment:
[Nit] `statements.get(completed - 3)` encodes two assumptions about the
MySQL script's shape: that every `EXECUTE` is the third statement of a `SET
@ddl` / `PREPARE` / `EXECUTE` triple, and that no `EXECUTE` appears among the
first two statements. Both hold today, but if either stops holding the failure
is an `IndexOutOfBoundsException` from the test harness rather than a readable
message about the script.
A `completed >= 3 &&` in front of the call, or scanning backwards for the
nearest `SET @ddl`, costs one line and keeps the failure legible.
Verified by: replayed `extractStatements` over
`scripts/mysql/upgrade-1.3.0-to-2.0.0-mysql.sql` — 203 statements, 48
`EXECUTE`, the first at 0-based index 3 (so `completed - 3` is 1, in range),
and `statements[i-2]` starts with `SET @ddl` for all 48. The same replay
reproduces the 30 / 27 / 21 checkpoint counts in the PR description.
##########
core/src/test/java/org/apache/gravitino/storage/TestSQLScripts.java:
##########
@@ -255,6 +259,303 @@ public void testUpgradeToTwoZeroBackfillsOccVersions()
throws SQLException, IOEx
}
}
+ /** Verifies a completed upgrade can be repeated without resetting migrated
data. */
+ @TestTemplate
+ public void testUpgradeToTwoZeroIsIdempotent() throws SQLException,
IOException {
+ Path scriptDir = upgradeScriptDirectory();
+ String suffix = "-" + backendType.toLowerCase() + ".sql";
+ File upgrade = scriptDir.resolve("upgrade-1.3.0-to-2.0.0" +
suffix).toFile();
+ dropAllTables();
+ executeScript(scriptDir.resolve("schema-1.3.0" + suffix).toFile());
+ seedUpgradeData();
+ executeScript(upgrade);
+ executeStatements(
+ List.of(
+ "UPDATE idp_user_meta SET enabled = FALSE, audit_info = 'preserved
audit'",
+ "UPDATE idp_group_meta SET group_comment = 'preserved comment'",
+ "UPDATE tag_meta SET allowed_values = '[\"blue\"]'",
+ "UPDATE tag_relation_meta SET tag_value = 'blue'",
+ "UPDATE fileset_meta SET occ_version = 42",
+ "UPDATE policy_meta SET occ_version = 43"),
+ "customized migrated data");
+ Map<String, Set<String>> expectedColumns = readSchemaColumns();
+ List<String> expectedIndexes = readSchemaIndexes(expectedColumns.keySet());
+ Map<String, List<List<String>>> expectedData = readUpgradeData();
+ executeScript(upgrade);
+ executeScript(upgrade);
+ Assertions.assertEquals(expectedColumns, readSchemaColumns());
+ Assertions.assertEquals(expectedIndexes,
readSchemaIndexes(expectedColumns.keySet()));
+ Assertions.assertEquals(expectedData, readUpgradeData(), "Retry must
preserve migrated values");
+ if ("mysql".equals(backendType)) {
+ // Neither rename source nor target exists: this is damage, not an
already-applied DDL.
+ executeStatements(
+ List.of("ALTER TABLE schema_meta DROP INDEX schema_meta_idx_mid"),
+ "missing target index");
+ AssertionFailedError failure =
+ Assertions.assertThrows(AssertionFailedError.class, () ->
executeScript(upgrade));
+ Assertions.assertInstanceOf(SQLException.class, failure.getCause());
+ Assertions.assertTrue(
+ failure.getCause().getMessage().contains("idx_mid"),
failure.getMessage());
+ }
+ assertSchemaMatchesFreshInstall(expectedColumns, expectedIndexes);
+ }
+
+ /** Verifies retries after structural DDL and data updates, including
prepared DDL execution. */
+ @TestTemplate
+ public void testUpgradeToTwoZeroRetriesPartialExecution() throws
SQLException, IOException {
+ Path scriptDir = upgradeScriptDirectory();
+ String suffix = "-" + backendType.toLowerCase() + ".sql";
+ File schema = scriptDir.resolve("schema-1.3.0" + suffix).toFile();
+ File upgrade = scriptDir.resolve("upgrade-1.3.0-to-2.0.0" +
suffix).toFile();
+ List<String> statements = extractStatements(upgrade.toPath());
+ dropAllTables();
+ executeScript(schema);
+ seedUpgradeData();
+ executeScript(upgrade);
+ Map<String, Set<String>> expectedColumns = readSchemaColumns();
+ List<String> expectedIndexes = readSchemaIndexes(expectedColumns.keySet());
+ Map<String, List<List<String>>> expectedData = readUpgradeData();
+ assertSchemaMatchesFreshInstall(expectedColumns, expectedIndexes);
+
+ String lastIndexRename =
+ statements.stream()
+ .filter(sql -> sql.contains("RENAME INDEX"))
+ .reduce((first, last) -> last)
+ .orElse("");
+ boolean testedIndexRename = false;
+ for (int completed = 1; completed <= statements.size(); completed++) {
+ String last = statements.get(completed - 1).toUpperCase();
+ if (!(last.startsWith("ALTER TABLE")
+ || last.startsWith("CREATE ")
+ || last.startsWith("UPDATE ")
+ || last.startsWith("EXECUTE "))) {
+ continue;
+ }
+ // MySQL renames share one guard shape; sample the first and last rename.
+ if (last.startsWith("EXECUTE ") && statements.get(completed -
3).contains("RENAME INDEX")) {
+ if (testedIndexRename && !statements.get(completed -
3).equals(lastIndexRename)) {
+ continue;
+ }
+ testedIndexRename = true;
+ }
+ dropAllTables();
+ executeScript(schema);
+ seedUpgradeData();
+ // Close the connection after the prefix, as if the migration process
stopped here.
+ String checkpoint = "Interrupted after statement " + completed + ": " +
last;
+ executeStatements(statements.subList(0, completed), checkpoint);
+ executeScript(upgrade);
+ Assertions.assertEquals(expectedColumns, readSchemaColumns(),
checkpoint);
+ Assertions.assertEquals(
+ expectedIndexes, readSchemaIndexes(expectedColumns.keySet()),
checkpoint);
+ Assertions.assertEquals(expectedData, readUpgradeData(), checkpoint);
+ if ("mysql".equals(backendType)) {
+ try (SqlSession session =
+
SqlSessionFactoryHelper.getInstance().getSqlSessionFactory().openSession(true);
+ Connection connection = session.getConnection();
+ ResultSet keys =
+ connection
+ .getMetaData()
+ .getPrimaryKeys(connection.getCatalog(), null,
"table_version_info")) {
+ List<String> columns = new ArrayList<>();
+ while (keys.next()) {
+ columns.add(keys.getString("COLUMN_NAME"));
+ }
+ Assertions.assertEquals(
+ Set.of("table_id", "version", "deleted_at"),
Set.copyOf(columns), checkpoint);
+ }
+ }
+ }
+ }
+
+ /** Verifies unexpected MySQL primary keys are rejected rather than silently
accepted. */
+ @TestTemplate
+ public void testUpgradeToTwoZeroRejectsUnexpectedPrimaryKey() throws
SQLException, IOException {
+ if (!"mysql".equals(backendType)) {
+ return;
+ }
+ Path scriptDir = upgradeScriptDirectory();
+ for (String key :
+ List.of("table_id", "version, table_id, deleted_at", "table_id,
version, deleted_at")) {
+ dropAllTables();
+ executeScript(scriptDir.resolve("schema-1.3.0-mysql.sql").toFile());
+ seedUpgradeData();
+ // The target key is incomplete while the old unique index remains.
+ executeStatements(
+ List.of("ALTER TABLE table_version_info ADD PRIMARY KEY (" + key +
")"),
+ "unexpected primary key");
+ boolean keepLegacyIndex = !key.startsWith("version");
+ if (!keepLegacyIndex) {
+ // Check column order independently of the legacy-index guard.
+ executeStatements(
+ List.of("ALTER TABLE table_version_info DROP INDEX
uk_table_id_version_deleted_at"),
+ "remove legacy index");
+ }
+ AssertionFailedError failure =
+ Assertions.assertThrows(
+ AssertionFailedError.class,
+ () ->
executeScript(scriptDir.resolve("upgrade-1.3.0-to-2.0.0-mysql.sql").toFile()));
+ Assertions.assertInstanceOf(SQLException.class, failure.getCause());
+ String expectedError = keepLegacyIndex ? "primary key" :
"uk_table_id_version_deleted_at";
+ Assertions.assertTrue(
+
failure.getCause().getMessage().toLowerCase().contains(expectedError),
+ failure.getMessage());
+ Assertions.assertEquals(
+ keepLegacyIndex,
+ readSchemaIndexes(Set.of("table_version_info")).stream()
+ .anyMatch(index ->
index.contains("uk_table_id_version_deleted_at")),
+ "Failed conversion must preserve the existing indexes");
+ }
+ }
+
+ private void assertSchemaMatchesFreshInstall(
+ Map<String, Set<String>> upgradedColumns, List<String> upgradedIndexes)
+ throws SQLException, IOException {
+ Map<String, Set<String>> expectedColumns = new TreeMap<>(upgradedColumns);
+ // The migration intentionally retains this legacy table; fresh 2.0
installs omit it.
+ expectedColumns.keySet().removeIf(table ->
table.equalsIgnoreCase("policy_relation_meta"));
+ List<String> expectedIndexes =
+ upgradedIndexes.stream()
+ .filter(index ->
!index.toLowerCase().startsWith("policy_relation_meta:"))
+ .toList();
+ dropAllTables();
+ executeScript(
+ upgradeScriptDirectory().resolve("schema-2.0.0-" + backendType +
".sql").toFile());
+ Map<String, Set<String>> freshColumns = readSchemaColumns();
+ Assertions.assertEquals(freshColumns, expectedColumns, "Upgrade must match
fresh 2.0 columns");
+ Assertions.assertEquals(
+ readSchemaIndexes(freshColumns.keySet()),
+ expectedIndexes,
+ "Upgrade must match fresh 2.0 indexes");
+ }
+
+ private Path upgradeScriptDirectory() {
+ String home = System.getenv("GRAVITINO_HOME");
+ Assertions.assertNotNull(home, "GRAVITINO_HOME environment variable is not
set");
+ return Path.of(home, "scripts", backendType.toLowerCase());
+ }
+
+ private void seedUpgradeData() throws SQLException {
+ executeStatements(
+ List.of(
+ "INSERT INTO idp_user_meta (user_id, user_name, password_hash, "
+ + "current_version, last_version) VALUES (1, 'user', 'hash',
7, 9)",
+ "INSERT INTO idp_group_meta (group_id, group_name) VALUES (1,
'group')",
+ "INSERT INTO tag_meta (tag_id, tag_name, metalake_id, audit_info)
VALUES (1, "
+ + "'tag', 1, '{}')",
+ "INSERT INTO tag_relation_meta (id, tag_id, metadata_object_id, "
+ + "metadata_object_type, audit_info) VALUES (1, 1, 1, 'TABLE',
'{}')",
+ "INSERT INTO fileset_meta (fileset_id, fileset_name, metalake_id,
catalog_id, "
+ + "schema_id, type, audit_info, current_version, last_version)
VALUES (1, "
+ + "'fileset', 1, 1, 1, 'MANAGED', '{}', 7, 9)",
+ "INSERT INTO policy_meta (policy_id, policy_name, policy_type,
metalake_id, "
+ + "audit_info, current_version, last_version) VALUES (1,
'policy', 'custom', 1, "
+ + "'{}', 7, 9)",
+ "INSERT INTO table_version_info (table_id, version, deleted_at)
VALUES (1, 7, 0)"),
+ "source schema data");
+ }
+
+ private Map<String, List<List<String>>> readUpgradeData() throws
SQLException {
+ Map<String, List<List<String>>> data = new TreeMap<>();
+ Map<String, String> selections =
+ Map.of(
+ "idp_user_meta",
+ "user_id, user_name, password_hash, current_version,
last_version, enabled, audit_info",
+ "idp_group_meta", "group_id, group_name, group_comment,
audit_info",
+ "tag_meta", "tag_id, tag_name, allowed_values",
+ "tag_relation_meta", "id, tag_id, metadata_object_id, tag_value",
+ "fileset_meta", "fileset_id, current_version, last_version,
occ_version",
+ "policy_meta", "policy_id, current_version, last_version,
occ_version",
+ "table_version_info", "table_id, version, deleted_at");
+ try (SqlSession session =
+
SqlSessionFactoryHelper.getInstance().getSqlSessionFactory().openSession(true);
+ Connection connection = session.getConnection();
+ Statement statement = connection.createStatement()) {
+ for (Map.Entry<String, String> entry : selections.entrySet()) {
+ List<List<String>> rows = new ArrayList<>();
+ try (ResultSet result =
+ statement.executeQuery(
+ "SELECT " + entry.getValue() + " FROM " + entry.getKey() + "
ORDER BY 1")) {
+ while (result.next()) {
+ List<String> row = new ArrayList<>();
+ for (int column = 1; column <=
result.getMetaData().getColumnCount(); column++) {
+ row.add(result.getString(column));
+ }
+ rows.add(row);
+ }
+ }
+ data.put(entry.getKey(), rows);
+ }
+ }
+ return data;
+ }
+
+ private Map<String, Set<String>> readSchemaColumns() throws SQLException {
+ Map<String, Set<String>> columns = new TreeMap<>();
+ try (SqlSession session =
+
SqlSessionFactoryHelper.getInstance().getSqlSessionFactory().openSession(true);
+ Connection connection = session.getConnection();
+ ResultSet result =
+ connection
+ .getMetaData()
+ .getColumns(connection.getCatalog(), connection.getSchema(),
"%", "%")) {
+ while (result.next()) {
+ columns
+ .computeIfAbsent(result.getString("TABLE_NAME"), key -> new
TreeSet<>())
+ .add(result.getString("COLUMN_NAME"));
+ }
+ }
+ Assertions.assertFalse(columns.isEmpty(), "Schema metadata must contain
columns");
+ return columns;
+ }
+
+ private List<String> readSchemaIndexes(Set<String> tables) throws
SQLException {
+ List<String> indexes = new ArrayList<>();
+ try (SqlSession session =
+
SqlSessionFactoryHelper.getInstance().getSqlSessionFactory().openSession(true);
+ Connection connection = session.getConnection()) {
+ for (String table : tables) {
+ try (ResultSet result =
+ connection
+ .getMetaData()
+ .getIndexInfo(
+ connection.getCatalog(), connection.getSchema(), table,
false, false)) {
+ while (result.next()) {
+ if (result.getShort("ORDINAL_POSITION") == 0) {
+ continue;
+ }
+ // H2 generates constraint index names from a database-wide
counter. Compare their
+ // definitions and multiplicity, while keeping index names for
MySQL rename checks.
+ String name = "h2".equals(backendType) ? "" :
result.getString("INDEX_NAME");
Review Comment:
[Question] This blanks *every* H2 index name, not only the generated
constraint-index names the comment refers to. That is what makes the new
`assertSchemaMatchesFreshInstall` weaker on H2 than on the other two backends:
it pins index structure and multiplicity, but an explicitly named index created
with the wrong name would still compare equal.
That matters here because the H2 upgrade script does create named indexes —
`uk_ti_mi_mo_tv_del` and `idx_tid_value` at
`scripts/h2/upgrade-1.3.0-to-2.0.0-h2.sql:48-49` — and
`scripts/h2/schema-2.0.0-h2.sql` declares 61 named keys and indexes in total.
Would restricting the blanking to H2's generated names (the counter-suffixed
`..._INDEX_<n>` and `PRIMARY_KEY_<n>` forms) and keeping real names work for
you? That would let the fresh-install assertion check H2 index names too.
I could not run H2 in this environment to confirm the exact generated-name
pattern, hence a question rather than a finding — the H2 run in the PR
description would settle it.
Verified by: read `readSchemaIndexes` in full and
`assertSchemaMatchesFreshInstall` at lines 412-430; counted the named indexes
in `scripts/h2/schema-2.0.0-h2.sql` and read
`scripts/h2/upgrade-1.3.0-to-2.0.0-h2.sql:48-49`.
--
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]
