Copilot commented on code in PR #9572:
URL: https://github.com/apache/gravitino/pull/9572#discussion_r2647449566
##########
catalogs/catalog-lakehouse-iceberg/src/test/java/org/apache/gravitino/catalog/lakehouse/iceberg/TestIcebergTable.java:
##########
@@ -491,6 +491,91 @@ public void testAlterIcebergTable() {
Assertions.assertArrayEquals(createdTable.partitioning(),
alteredTable.partitioning());
}
+ @Test
+ public void testRenameTableAcrossSchema() {
+ NameIdentifier tableIdentifier =
+ NameIdentifier.of(
+ META_LAKE_NAME, icebergCatalog.name(), icebergSchema.name(),
"test_iceberg_table");
+ Map<String, String> properties = Maps.newHashMap();
+ properties.put("key1", "val1");
+
+ IcebergColumn col1 =
+ IcebergColumn.builder()
+ .withName("col_1")
+ .withType(Types.IntegerType.get())
+ .withComment(ICEBERG_COMMENT)
+ .build();
+ Column[] columns = new Column[] {col1};
+
+ String targetSchemaName = "target_schema_" + genRandomName();
+ NameIdentifier targetSchemaIdent =
+ NameIdentifier.of(META_LAKE_NAME, icebergCatalog.name(),
targetSchemaName);
+ NameIdentifier renamedIdentifier =
+ NameIdentifier.of(META_LAKE_NAME, icebergCatalog.name(),
targetSchemaName, "renamed_table");
+
+ try {
+ icebergCatalogOperations.createSchema(targetSchemaIdent,
ICEBERG_COMMENT, Maps.newHashMap());
+ icebergCatalogOperations.createTable(
+ tableIdentifier,
+ columns,
+ ICEBERG_COMMENT,
+ properties,
+ new Transform[0],
+ Distributions.NONE,
+ new SortOrder[0]);
+
+ icebergCatalogOperations.alterTable(
+ tableIdentifier, TableChange.rename("renamed_table",
targetSchemaName));
+
+
Assertions.assertFalse(icebergCatalogOperations.tableExists(tableIdentifier));
+
Assertions.assertTrue(icebergCatalogOperations.tableExists(renamedIdentifier));
+ Assertions.assertEquals(
+ "renamed_table",
icebergCatalogOperations.loadTable(renamedIdentifier).name());
+ } finally {
+ if (icebergCatalogOperations.tableExists(renamedIdentifier)) {
+ icebergCatalogOperations.dropTable(renamedIdentifier);
+ }
+ icebergCatalogOperations.dropSchema(targetSchemaIdent, false);
+ }
+ }
+
+ @Test
+ public void testRenameTableToMissingSchema() {
+ NameIdentifier tableIdentifier =
+ NameIdentifier.of(
+ META_LAKE_NAME, icebergCatalog.name(), icebergSchema.name(),
"test_iceberg_table");
+ Map<String, String> properties = Maps.newHashMap();
+ properties.put("key1", "val1");
+
+ IcebergColumn col1 =
+ IcebergColumn.builder()
+ .withName("col_1")
+ .withType(Types.IntegerType.get())
+ .withComment(ICEBERG_COMMENT)
+ .build();
+ Column[] columns = new Column[] {col1};
+
+ icebergCatalogOperations.createTable(
+ tableIdentifier,
+ columns,
+ ICEBERG_COMMENT,
+ properties,
+ new Transform[0],
+ Distributions.NONE,
+ new SortOrder[0]);
+
+ String missingSchemaName = "missing_schema_" + genRandomName();
+ Throwable exception =
+ Assertions.assertThrows(
+ NoSuchSchemaException.class,
+ () ->
+ icebergCatalogOperations.alterTable(
+ tableIdentifier, TableChange.rename("renamed_table",
missingSchemaName)));
+ Assertions.assertTrue(
+ exception.getMessage().contains("Iceberg Schema (database) does not
exist"));
+
Assertions.assertTrue(icebergCatalogOperations.tableExists(tableIdentifier));
+ }
Review Comment:
The test does not clean up the table created in the original schema. The
table should be dropped in a finally block to ensure proper cleanup even if the
test fails. This is important to prevent test pollution and resource leaks.
##########
catalogs/catalog-lakehouse-iceberg/src/test/java/org/apache/gravitino/catalog/lakehouse/iceberg/TestIcebergTable.java:
##########
@@ -491,6 +491,91 @@ public void testAlterIcebergTable() {
Assertions.assertArrayEquals(createdTable.partitioning(),
alteredTable.partitioning());
}
+ @Test
+ public void testRenameTableAcrossSchema() {
+ NameIdentifier tableIdentifier =
+ NameIdentifier.of(
+ META_LAKE_NAME, icebergCatalog.name(), icebergSchema.name(),
"test_iceberg_table");
+ Map<String, String> properties = Maps.newHashMap();
+ properties.put("key1", "val1");
+
+ IcebergColumn col1 =
+ IcebergColumn.builder()
+ .withName("col_1")
+ .withType(Types.IntegerType.get())
+ .withComment(ICEBERG_COMMENT)
+ .build();
+ Column[] columns = new Column[] {col1};
+
+ String targetSchemaName = "target_schema_" + genRandomName();
+ NameIdentifier targetSchemaIdent =
+ NameIdentifier.of(META_LAKE_NAME, icebergCatalog.name(),
targetSchemaName);
+ NameIdentifier renamedIdentifier =
+ NameIdentifier.of(META_LAKE_NAME, icebergCatalog.name(),
targetSchemaName, "renamed_table");
+
+ try {
+ icebergCatalogOperations.createSchema(targetSchemaIdent,
ICEBERG_COMMENT, Maps.newHashMap());
+ icebergCatalogOperations.createTable(
+ tableIdentifier,
+ columns,
+ ICEBERG_COMMENT,
+ properties,
+ new Transform[0],
+ Distributions.NONE,
+ new SortOrder[0]);
+
+ icebergCatalogOperations.alterTable(
+ tableIdentifier, TableChange.rename("renamed_table",
targetSchemaName));
+
+
Assertions.assertFalse(icebergCatalogOperations.tableExists(tableIdentifier));
+
Assertions.assertTrue(icebergCatalogOperations.tableExists(renamedIdentifier));
+ Assertions.assertEquals(
+ "renamed_table",
icebergCatalogOperations.loadTable(renamedIdentifier).name());
+ } finally {
+ if (icebergCatalogOperations.tableExists(renamedIdentifier)) {
+ icebergCatalogOperations.dropTable(renamedIdentifier);
+ }
+ icebergCatalogOperations.dropSchema(targetSchemaIdent, false);
Review Comment:
The finally block should check if the target schema exists before attempting
to drop it. If the test fails before the schema is created (e.g., during table
creation), the dropSchema call will throw an exception and mask the actual test
failure.
```suggestion
try {
icebergCatalogOperations.dropSchema(targetSchemaIdent, false);
} catch (NoSuchSchemaException e) {
// Ignore: schema may not exist if creation or earlier steps failed
}
```
##########
catalogs/catalog-lakehouse-iceberg/src/main/java/org/apache/gravitino/catalog/lakehouse/iceberg/IcebergCatalogOperations.java:
##########
@@ -449,15 +449,30 @@ private Table internalUpdateTable(NameIdentifier
tableIdent, TableChange... chan
private Table renameTable(NameIdentifier tableIdent, TableChange.RenameTable
renameTable)
throws NoSuchTableException, IllegalArgumentException {
try {
+ Namespace destinationNamespace = tableIdent.namespace();
+ if (renameTable.getNewSchemaName().isPresent()) {
+ String[] namespaceLevels = tableIdent.namespace().levels();
+ String[] destinationLevels = Arrays.copyOf(namespaceLevels,
namespaceLevels.length);
+ destinationLevels[destinationLevels.length - 1] =
renameTable.getNewSchemaName().get();
+ NameIdentifier destinationSchemaIdent =
NameIdentifier.of(destinationLevels);
+ if (!schemaExists(destinationSchemaIdent)) {
+ throw new NoSuchSchemaException(
+ "Iceberg Schema (database) does not exist %s",
destinationSchemaIdent);
Review Comment:
The error message format is inconsistent with existing patterns in this
file. Line 245 uses "Iceberg schema (database) does not exist" with lowercase
's', and line 351 uses "Schema (database) does not exist" without the "Iceberg"
prefix. Consider using "Iceberg schema (database) does not exist: %s" for
consistency with line 245, or follow the simpler pattern from line 351 with
"Schema (database) does not exist %s".
```suggestion
"Iceberg schema (database) does not exist: %s",
destinationSchemaIdent);
```
--
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]
