joyhaldar commented on code in PR #15300:
URL: https://github.com/apache/iceberg/pull/15300#discussion_r2802732616
##########
bigquery/src/main/java/org/apache/iceberg/gcp/bigquery/BigQueryMetastoreClientImpl.java:
##########
@@ -527,6 +531,76 @@ 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());
+
+ createNewTableDuringRename(toTableReference, renamedTable);
+ deleteOriginalTableDuringRename(fromTableReference, toTableReference);
+
+ } catch (Exception e) {
+ throw new RuntimeIOException(
+ "Failed to rename table from %s to %s: %s",
+ fromTableReference, toTableReference, e.getMessage());
+ }
+ }
+
+ private void createNewTableDuringRename(TableReference toTableReference,
Table renamedTable) {
Review Comment:
`BigQueryMetastoreClient` already has `create(Table)`. Could we reuse it
instead of raw HTTP calls in `createNewTableDuringRename`?
##########
bigquery/src/main/java/org/apache/iceberg/gcp/bigquery/BigQueryMetastoreClientImpl.java:
##########
@@ -527,6 +531,76 @@ public List<Tables> list(DatasetReference
datasetReference, boolean listAllTable
}
}
+ @Override
+ public void rename(TableReference fromTableReference, TableReference
toTableReference) {
Review Comment:
Similar question as ebyhr, does BQMS support renaming tables across
datasets? The current code validates both namespaces exist but doesn't check if
they're the same.
If cross-dataset renames aren't supported by the API, we could fail fast.
If they are supported, there might be another concern, users who had access
to the source dataset may not have access to the destination dataset, so they'd
lose access to the table after rename.
##########
bigquery/src/main/java/org/apache/iceberg/gcp/bigquery/BigQueryMetastoreClientImpl.java:
##########
@@ -527,6 +531,76 @@ 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());
+
+ createNewTableDuringRename(toTableReference, renamedTable);
+ deleteOriginalTableDuringRename(fromTableReference, toTableReference);
+
+ } catch (Exception e) {
+ throw new RuntimeIOException(
+ "Failed to rename table from %s to %s: %s",
+ fromTableReference, toTableReference, e.getMessage());
+ }
+ }
+
+ private void createNewTableDuringRename(TableReference toTableReference,
Table renamedTable) {
+ 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());
+ }
+ }
+
+ private void deleteOriginalTableDuringRename(
+ TableReference fromTableReference, TableReference toTableReference) {
+ try {
+ delete(fromTableReference);
+ } catch (Exception deleteException) {
+ LOG.error("Failed to cleanup original table {}", fromTableReference,
deleteException);
+ cleanupNewlyCreatedTableDuringRename(toTableReference);
+ }
+ }
+
+ private void cleanupNewlyCreatedTableDuringRename(TableReference
toTableReference) {
Review Comment:
`BigQueryMetastoreClient` already has `delete(TableReference)`. Could we
reuse it instead of raw HTTP calls in `cleanupNewlyCreatedTableDuringRename`?
##########
bigquery/src/main/java/org/apache/iceberg/gcp/bigquery/BigQueryMetastoreClientImpl.java:
##########
@@ -527,6 +531,76 @@ 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());
+
+ createNewTableDuringRename(toTableReference, renamedTable);
+ deleteOriginalTableDuringRename(fromTableReference, toTableReference);
+
+ } catch (Exception e) {
+ throw new RuntimeIOException(
+ "Failed to rename table from %s to %s: %s",
+ fromTableReference, toTableReference, e.getMessage());
+ }
+ }
+
+ private void createNewTableDuringRename(TableReference toTableReference,
Table renamedTable) {
+ 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());
+ }
+ }
+
+ private void deleteOriginalTableDuringRename(
+ TableReference fromTableReference, TableReference toTableReference) {
+ try {
+ delete(fromTableReference);
+ } catch (Exception deleteException) {
+ LOG.error("Failed to cleanup original table {}", fromTableReference,
deleteException);
+ cleanupNewlyCreatedTableDuringRename(toTableReference);
Review Comment:
+1 on this concern. It might be worth looking at how other catalogs handle
this scenario, could help inform the approach here.
--
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]
