Copilot commented on code in PR #14159:
URL: https://github.com/apache/iceberg/pull/14159#discussion_r2370201143
##########
bigquery/src/main/java/org/apache/iceberg/gcp/bigquery/BigQueryMetastoreClientImpl.java:
##########
@@ -527,6 +527,57 @@ public List<Tables> list(DatasetReference
datasetReference, boolean listAllTable
}
}
+ @Override
+ public void rename(TableReference fromTableReference, TableReference
toTableReference) {
+ try {
+ Table sourceTable = load(fromTableReference);
+
+ DatasetReference toDatasetReference =
+ new DatasetReference()
+ .setProjectId(toTableReference.getProjectId())
+ .setDatasetId(toTableReference.getDatasetId());
+ load(toDatasetReference);
+
+ Table renamedTable = new Table();
+ renamedTable.setTableReference(toTableReference);
+
renamedTable.setExternalCatalogTableOptions(sourceTable.getExternalCatalogTableOptions());
+ renamedTable.setSchema(sourceTable.getSchema());
+
+ try {
+ HttpResponse createResponse =
+ client
+ .tables()
+ .insert(
+ toTableReference.getProjectId(),
toTableReference.getDatasetId(), renamedTable)
+ .executeUnparsed();
+
+ convertExceptionIfUnsuccessful(createResponse);
+
+ } catch (Exception createException) {
+ throw new RuntimeIOException(
+ "Failed to create new table %s during rename: %s",
+ toTableReference, createException.getMessage());
+ }
+
+ try {
+ delete(fromTableReference);
+ } catch (Exception deleteException) {
+ client
+ .tables()
+ .delete(
+ toTableReference.getProjectId(),
+ toTableReference.getDatasetId(),
+ toTableReference.getTableId())
+ .executeUnparsed();
Review Comment:
The cleanup delete operation in the catch block doesn't handle potential
exceptions. If this delete fails, it could throw an exception that masks the
original deleteException, making debugging difficult. Consider wrapping this
cleanup operation in a try-catch block and logging any failures.
```suggestion
try {
client
.tables()
.delete(
toTableReference.getProjectId(),
toTableReference.getDatasetId(),
toTableReference.getTableId())
.executeUnparsed();
} catch (Exception cleanupException) {
System.err.println("Failed to cleanup newly created table " +
toTableReference + " after failed rename: " + cleanupException.getMessage());
}
```
##########
bigquery/src/main/java/org/apache/iceberg/gcp/bigquery/BigQueryMetastoreClientImpl.java:
##########
@@ -527,6 +527,57 @@ public List<Tables> list(DatasetReference
datasetReference, boolean listAllTable
}
}
+ @Override
+ public void rename(TableReference fromTableReference, TableReference
toTableReference) {
+ try {
+ Table sourceTable = load(fromTableReference);
+
+ DatasetReference toDatasetReference =
+ new DatasetReference()
+ .setProjectId(toTableReference.getProjectId())
+ .setDatasetId(toTableReference.getDatasetId());
+ load(toDatasetReference);
+
+ Table renamedTable = new Table();
+ renamedTable.setTableReference(toTableReference);
+
renamedTable.setExternalCatalogTableOptions(sourceTable.getExternalCatalogTableOptions());
+ renamedTable.setSchema(sourceTable.getSchema());
+
+ try {
+ HttpResponse createResponse =
+ client
+ .tables()
+ .insert(
+ toTableReference.getProjectId(),
toTableReference.getDatasetId(), renamedTable)
+ .executeUnparsed();
+
+ convertExceptionIfUnsuccessful(createResponse);
+
+ } catch (Exception createException) {
+ throw new RuntimeIOException(
+ "Failed to create new table %s during rename: %s",
+ toTableReference, createException.getMessage());
+ }
+
+ try {
+ delete(fromTableReference);
+ } catch (Exception deleteException) {
+ client
+ .tables()
+ .delete(
+ toTableReference.getProjectId(),
+ toTableReference.getDatasetId(),
+ toTableReference.getTableId())
+ .executeUnparsed();
+ }
Review Comment:
The original deleteException is caught but not propagated or logged. Users
won't know why the source table deletion failed. Consider logging the
deleteException and then throwing it or including it in a more comprehensive
error message.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
