Copilot commented on code in PR #11292:
URL: https://github.com/apache/gravitino/pull/11292#discussion_r3329877828
##########
catalogs/catalog-hive/src/main/java/org/apache/gravitino/catalog/hive/HiveCatalogOperations.java:
##########
@@ -370,7 +370,7 @@ public boolean dropSchema(NameIdentifier ident, boolean
cascade) throws NonEmpty
@Override
public NameIdentifier[] listTables(Namespace namespace) throws
NoSuchSchemaException {
NameIdentifier schemaIdent = NameIdentifier.of(namespace.levels());
- if (!schemaExists(schemaIdent)) {
+ if (!schemaExists(schemaIdent)) {
Review Comment:
This line introduces trailing whitespace, which will show up in
style/formatting checks and should be removed.
##########
catalogs/catalog-hive/src/main/java/org/apache/gravitino/catalog/hive/HiveCatalogOperations.java:
##########
@@ -675,12 +675,21 @@ public Table alterTable(NameIdentifier tableIdent,
TableChange... changes)
String newComment = currentTable.comment();
Map<String, String> updatedProperties = new
HashMap<>(currentTable.properties());
List<Column> updatedColumns = new
ArrayList<>(Arrays.asList(currentTable.columns()));
+ String targetDatabaseName = schemaIdent.name(); // Define a separate
variable to track the target database name.
+
for (TableChange change : changes) {
if (change instanceof TableChange.RenameTable) {
TableChange.RenameTable rename = (TableChange.RenameTable) change;
- Preconditions.checkArgument(
- rename.getNewSchemaName().isEmpty(), "Does not support rename
schema yet");
+ if (rename.getNewSchemaName().isPresent()) { // Check if a new
schema is specified for the table rename
+ String newSchemaName = rename.getNewSchemaName().get();
+ if (!schemaExists( // Validate that the target schema exists
before moving the table
+ NameIdentifier.of(tableIdent.namespace().levels()[0],
newSchemaName))) {
+ throw new NoSuchSchemaException("Schema %s does not exist",
newSchemaName);
+ }
+ // Move table to the new schema
+ targetDatabaseName = newSchemaName;
Review Comment:
This adds a new cross-schema rename path, but the PR does not add a test
that creates a second Hive schema and verifies the table is loadable from the
new schema (and absent from the old one). Existing `testAlterHiveTable` only
covers same-schema rename, so regressions in the new database move behavior
would not be caught.
##########
catalogs/catalog-hive/src/main/java/org/apache/gravitino/catalog/hive/HiveCatalogOperations.java:
##########
@@ -675,12 +675,21 @@ public Table alterTable(NameIdentifier tableIdent,
TableChange... changes)
String newComment = currentTable.comment();
Map<String, String> updatedProperties = new
HashMap<>(currentTable.properties());
List<Column> updatedColumns = new
ArrayList<>(Arrays.asList(currentTable.columns()));
+ String targetDatabaseName = schemaIdent.name(); // Define a separate
variable to track the target database name.
+
Review Comment:
This blank line contains trailing whitespace; please remove it to keep the
file clean and formatter-friendly.
##########
catalogs/catalog-hive/src/main/java/org/apache/gravitino/catalog/hive/HiveCatalogOperations.java:
##########
@@ -675,12 +675,21 @@ public Table alterTable(NameIdentifier tableIdent,
TableChange... changes)
String newComment = currentTable.comment();
Map<String, String> updatedProperties = new
HashMap<>(currentTable.properties());
List<Column> updatedColumns = new
ArrayList<>(Arrays.asList(currentTable.columns()));
+ String targetDatabaseName = schemaIdent.name(); // Define a separate
variable to track the target database name.
Review Comment:
Initializing the target database from the request identifier changes
non-rename alters from preserving the database returned by HMS to rewriting it
from the caller-supplied name. Use the loaded table's database name as the
default and only override it when a new schema is explicitly requested,
matching the previous behavior for all non-cross-schema alters.
##########
catalogs/catalog-hive/src/main/java/org/apache/gravitino/catalog/hive/HiveCatalogOperations.java:
##########
@@ -675,12 +675,21 @@ public Table alterTable(NameIdentifier tableIdent,
TableChange... changes)
String newComment = currentTable.comment();
Map<String, String> updatedProperties = new
HashMap<>(currentTable.properties());
List<Column> updatedColumns = new
ArrayList<>(Arrays.asList(currentTable.columns()));
+ String targetDatabaseName = schemaIdent.name(); // Define a separate
variable to track the target database name.
+
for (TableChange change : changes) {
if (change instanceof TableChange.RenameTable) {
TableChange.RenameTable rename = (TableChange.RenameTable) change;
- Preconditions.checkArgument(
- rename.getNewSchemaName().isEmpty(), "Does not support rename
schema yet");
+ if (rename.getNewSchemaName().isPresent()) { // Check if a new
schema is specified for the table rename
+ String newSchemaName = rename.getNewSchemaName().get();
+ if (!schemaExists( // Validate that the target schema exists
before moving the table
+ NameIdentifier.of(tableIdent.namespace().levels()[0],
newSchemaName))) {
Review Comment:
The target schema identifier drops the catalog level by using only
`tableIdent.namespace().levels()[0]`. Build the identifier under the current
schema namespace instead so the existence check uses the same `[metalake,
catalog, schema]` shape as the rest of this class.
--
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]
