yuqi1129 commented on code in PR #13689:
URL: https://github.com/apache/gravitino/pull/13689#discussion_r4216239746
##########
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:
Fixed in d8fd07b. The assertion now requires the quoted source key name
('idx_mid'), so a message naming only schema_meta_idx_mid cannot satisfy it.
The same idempotency test passed on MySQL.
##########
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:
Fixed in d8fd07b. Added completed >= 3 before accessing
statements.get(completed - 3), preserving today's first/last rename sampling
while preventing an out-of-range lookup for an early EXECUTE. This boundary
change was reviewed in source; the interruption matrix was not rerun in this
follow-up.
##########
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:
Fixed in d8fd07b. H2 now uses INFORMATION_SCHEMA.INDEXES.IS_GENERATED to
identify generated names, removes their generated suffixes, and preserves
explicit index names. A fresh-install constraint's generated backing index
normalizes to the same logical name as the upgrade's named unique index. Added
a regression inside the existing idempotency test: renaming idx_tid_value must
change the schema comparison, then the name is restored. That one test passed
on H2, MySQL, and PostgreSQL (three invocations); no full core suite was run.
--
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]
