Copilot commented on code in PR #9481:
URL: https://github.com/apache/gravitino/pull/9481#discussion_r2638560729
##########
core/src/test/java/org/apache/gravitino/catalog/TestManagedTableOperations.java:
##########
@@ -328,13 +328,15 @@ public void testAlterTable() {
Assertions.assertEquals("table1_renamed", loadedRenamedTable.name());
// Test rename the table to another schema
+ Table renamedTable1 =
+ tableOperations.alterTable(
+ NameIdentifierUtil.ofTable(METALAKE_NAME, CATALOG_NAME,
SCHEMA_NAME, "table1_renamed"),
+ TableChange.rename("table1_moved", "schema2"));
+
NameIdentifier renamedTable1Ident =
- NameIdentifierUtil.ofTable(METALAKE_NAME, CATALOG_NAME, SCHEMA_NAME,
"table1_renamed");
- Assertions.assertThrows(
- IllegalArgumentException.class,
- () ->
- tableOperations.alterTable(
- renamedTable1Ident, TableChange.rename("table1_moved",
"schema2")));
+ NameIdentifierUtil.ofTable(METALAKE_NAME, CATALOG_NAME, "schema2",
"table1_moved");
+ Table loadedRenamedTable1 = tableOperations.loadTable(renamedTable1Ident);
+ Assertions.assertEquals(renamedTable1.name(), loadedRenamedTable1.name());
Review Comment:
After renaming the table to a new schema, the test should verify that the
table no longer exists at the old location. Add an assertion to confirm that
loading the table from the old identifier throws NoSuchTableException.
##########
core/src/main/java/org/apache/gravitino/catalog/ManagedTableOperations.java:
##########
@@ -225,13 +227,13 @@ private TableEntity applyChanges(TableEntity
oldTableEntity, TableChange... chan
for (TableChange change : tableChanges) {
if (change instanceof TableChange.RenameTable rename) {
- if (rename.getNewSchemaName().isPresent()) {
- throw new IllegalArgumentException(
- "Gravitino managed table doesn't support renaming "
- + "the table across schemas for now");
- }
-
+ Namespace oldNs = oldTableEntity.namespace();
newName = rename.getNewName();
+ newNs =
+ rename
+ .getNewSchemaName()
+ .map(s -> NamespaceUtil.ofTable(oldNs.level(0),
oldNs.level(1), s))
+ .orElse(oldNs);
Review Comment:
When renaming a table to a new schema, there is no validation that the
target schema exists. Consider adding a schema existence check before applying
the rename operation to provide a clear error message if the target schema does
not exist, similar to how createTable validates schema existence.
##########
core/src/test/java/org/apache/gravitino/catalog/TestManagedTableOperations.java:
##########
@@ -328,13 +328,15 @@ public void testAlterTable() {
Assertions.assertEquals("table1_renamed", loadedRenamedTable.name());
// Test rename the table to another schema
+ Table renamedTable1 =
+ tableOperations.alterTable(
+ NameIdentifierUtil.ofTable(METALAKE_NAME, CATALOG_NAME,
SCHEMA_NAME, "table1_renamed"),
+ TableChange.rename("table1_moved", "schema2"));
+
Review Comment:
The test attempts to rename a table to schema2, but schema2 is never created
in the test setup. The test should create schema2 before attempting to move the
table to it. Without validating that the target schema exists, this test may
pass but doesn't properly test the real-world scenario where schema validation
should occur.
--
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]
