You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "mkroll-db (via GitHub)" <gi...@apache.org> on 2026/09/18 15:16:39 UTC
Re: [PR] Core: Add unregister table to RCK [iceberg]
mkroll-db commented on PR #16584:
URL: https://github.com/apache/iceberg/pull/16584#issuecomment-5732083247
Sorry, for the very large delay.
I took another look at the PR and the Catalog changes mentioned by @rdblue and removal of `TableMetadata` are missing.
I took a stab at it and refined it with codex here is the diff as result.
The JDBC changes are so 'big' because they try prevent concurrent modifications. It could also be reduced to a `notImplemented`.
<details>
<summary>Suggested changes</summary>
```diff
diff --git a/api/src/main/java/org/apache/iceberg/Table.java b/api/src/main/java/org/apache/iceberg/Table.java
index 3c0689e89..2ff2784de 100644
--- a/api/src/main/java/org/apache/iceberg/Table.java
+++ b/api/src/main/java/org/apache/iceberg/Table.java
@@ -335,6 +335,16 @@ public interface Table {
/** Returns a {@link FileIO} to read and write table data and metadata files. */
FileIO io();
+ /**
+ * Returns the location of the current table metadata file.
+ *
+ * @return the current table metadata file location
+ */
+ default String metadataFileLocation() {
+ throw new UnsupportedOperationException(
+ getClass().getName() + " doesn't expose its metadata file location");
+ }
+
/**
* Returns an {@link org.apache.iceberg.encryption.EncryptionManager} to encrypt and decrypt data
* files.
diff --git a/api/src/main/java/org/apache/iceberg/catalog/Catalog.java b/api/src/main/java/org/apache/iceberg/catalog/Catalog.java
index 3438f445e..acf2a2d69 100644
--- a/api/src/main/java/org/apache/iceberg/catalog/Catalog.java
+++ b/api/src/main/java/org/apache/iceberg/catalog/Catalog.java
@@ -368,6 +368,21 @@ public interface Catalog {
throw new UnsupportedOperationException("Registering tables with overwrite is not supported");
}
+ /**
+ * Unregister a table without deleting its data or metadata files.
+ *
+ * <p>The returned table is fixed at the last metadata file registered with the catalog and cannot
+ * be modified. Its metadata file location can be used to {@link #registerTable(TableIdentifier,
+ * String) register} the table again.
+ *
+ * @param identifier a table identifier
+ * @return a read-only table fixed at the metadata current when it was unregistered
+ * @throws NoSuchTableException if the table does not exist
+ */
+ default Table unregisterTable(TableIdentifier identifier) {
+ throw new UnsupportedOperationException("Unregistering tables is not supported");
+ }
+
/**
* Instantiate a builder to either create a table or start a create/replace transaction.
*
diff --git a/api/src/main/java/org/apache/iceberg/catalog/SessionCatalog.java b/api/src/main/java/org/apache/iceberg/catalog/SessionCatalog.java
index b54570485..ec7c9293f 100644
--- a/api/src/main/java/org/apache/iceberg/catalog/SessionCatalog.java
+++ b/api/src/main/java/org/apache/iceberg/catalog/SessionCatalog.java
@@ -196,6 +196,21 @@ public interface SessionCatalog {
throw new UnsupportedOperationException("Registering tables with overwrite is not supported");
}
+ /**
+ * Unregister a table without deleting its data or metadata files.
+ *
+ * <p>The returned table is fixed at the last metadata file registered with the catalog and cannot
+ * be modified.
+ *
+ * @param context session context
+ * @param ident a table identifier
+ * @return a read-only table fixed at the metadata current when it was unregistered
+ * @throws NoSuchTableException if the table does not exist
+ */
+ default Table unregisterTable(SessionContext context, TableIdentifier ident) {
+ throw new UnsupportedOperationException("Unregistering tables is not supported");
+ }
+
/**
* Check whether table exists.
*
diff --git a/core/src/main/java/org/apache/iceberg/BaseTable.java b/core/src/main/java/org/apache/iceberg/BaseTable.java
index 4d73c96fe..c3e3ed566 100644
--- a/core/src/main/java/org/apache/iceberg/BaseTable.java
+++ b/core/src/main/java/org/apache/iceberg/BaseTable.java
@@ -69,6 +69,11 @@ public class BaseTable
return ops;
}
+ @Override
+ public String metadataFileLocation() {
+ return ops.current().metadataFileLocation();
+ }
+
@Override
public String name() {
return name;
diff --git a/core/src/main/java/org/apache/iceberg/CachingCatalog.java b/core/src/main/java/org/apache/iceberg/CachingCatalog.java
index 5c92fe7db..b447384ec 100644
--- a/core/src/main/java/org/apache/iceberg/CachingCatalog.java
+++ b/core/src/main/java/org/apache/iceberg/CachingCatalog.java
@@ -205,6 +205,13 @@ public class CachingCatalog implements Catalog {
return table;
}
+ @Override
+ public Table unregisterTable(TableIdentifier identifier) {
+ Table table = catalog.unregisterTable(identifier);
+ invalidateTable(identifier);
+ return table;
+ }
+
private Iterable<TableIdentifier> metadataTableIdentifiers(TableIdentifier ident) {
ImmutableList.Builder<TableIdentifier> builder = ImmutableList.builder();
diff --git a/core/src/main/java/org/apache/iceberg/SerializableTable.java b/core/src/main/java/org/apache/iceberg/SerializableTable.java
index 5b4cd0e55..94fe3153e 100644
--- a/core/src/main/java/org/apache/iceberg/SerializableTable.java
+++ b/core/src/main/java/org/apache/iceberg/SerializableTable.java
@@ -111,6 +111,7 @@ public class SerializableTable implements Table, HasTableOperations, Serializabl
}
}
+ @Override
public String metadataFileLocation() {
if (metadataFileLocation == null) {
throw new UnsupportedOperationException(
diff --git a/core/src/main/java/org/apache/iceberg/catalog/BaseSessionCatalog.java b/core/src/main/java/org/apache/iceberg/catalog/BaseSessionCatalog.java
index ae830791d..dd759c11c 100644
--- a/core/src/main/java/org/apache/iceberg/catalog/BaseSessionCatalog.java
+++ b/core/src/main/java/org/apache/iceberg/catalog/BaseSessionCatalog.java
@@ -95,6 +95,11 @@ public abstract class BaseSessionCatalog implements SessionCatalog {
return BaseSessionCatalog.this.registerTable(context, ident, metadataFileLocation, overwrite);
}
+ @Override
+ public Table unregisterTable(TableIdentifier ident) {
+ return BaseSessionCatalog.this.unregisterTable(context, ident);
+ }
+
@Override
public boolean tableExists(TableIdentifier ident) {
return BaseSessionCatalog.this.tableExists(context, ident);
diff --git a/core/src/main/java/org/apache/iceberg/inmemory/InMemoryCatalog.java b/core/src/main/java/org/apache/iceberg/inmemory/InMemoryCatalog.java
index 2234d418d..e84492d88 100644
--- a/core/src/main/java/org/apache/iceberg/inmemory/InMemoryCatalog.java
+++ b/core/src/main/java/org/apache/iceberg/inmemory/InMemoryCatalog.java
@@ -29,8 +29,11 @@ import java.util.Set;
import java.util.concurrent.ConcurrentMap;
import java.util.stream.Collectors;
import org.apache.iceberg.BaseMetastoreTableOperations;
+import org.apache.iceberg.BaseTable;
import org.apache.iceberg.CatalogProperties;
import org.apache.iceberg.CatalogUtil;
+import org.apache.iceberg.StaticTableOperations;
+import org.apache.iceberg.Table;
import org.apache.iceberg.TableMetadata;
import org.apache.iceberg.TableOperations;
import org.apache.iceberg.catalog.Namespace;
@@ -124,6 +127,27 @@ public class InMemoryCatalog extends BaseMetastoreViewCatalog
}
}
+ @Override
+ public Table unregisterTable(TableIdentifier tableIdentifier) {
+ TableOperations ops = newTableOps(tableIdentifier);
+ TableMetadata metadata;
+
+ synchronized (this) {
+ metadata = ops.current();
+ if (metadata == null) {
+ throw new NoSuchTableException("Table does not exist: %s", tableIdentifier);
+ }
+
+ if (tables.remove(tableIdentifier) == null) {
+ throw new NoSuchTableException("Table does not exist: %s", tableIdentifier);
+ }
+ }
+
+ StaticTableOperations staticOps =
+ new StaticTableOperations(metadata, ops.io(), ops.locationProvider());
+ return new BaseTable(staticOps, fullTableName(name(), tableIdentifier), metricsReporter());
+ }
+
@Override
public boolean dropTable(TableIdentifier tableIdentifier, boolean purge) {
TableOperations ops = newTableOps(tableIdentifier);
diff --git a/core/src/main/java/org/apache/iceberg/jdbc/JdbcCatalog.java b/core/src/main/java/org/apache/iceberg/jdbc/JdbcCatalog.java
index 2d24e5598..e7288170b 100644
--- a/core/src/main/java/org/apache/iceberg/jdbc/JdbcCatalog.java
+++ b/core/src/main/java/org/apache/iceberg/jdbc/JdbcCatalog.java
@@ -18,6 +18,15 @@
*/
package org.apache.iceberg.jdbc;
+import static org.apache.iceberg.TableProperties.COMMIT_MAX_RETRY_WAIT_MS;
+import static org.apache.iceberg.TableProperties.COMMIT_MAX_RETRY_WAIT_MS_DEFAULT;
+import static org.apache.iceberg.TableProperties.COMMIT_MIN_RETRY_WAIT_MS;
+import static org.apache.iceberg.TableProperties.COMMIT_MIN_RETRY_WAIT_MS_DEFAULT;
+import static org.apache.iceberg.TableProperties.COMMIT_NUM_RETRIES;
+import static org.apache.iceberg.TableProperties.COMMIT_NUM_RETRIES_DEFAULT;
+import static org.apache.iceberg.TableProperties.COMMIT_TOTAL_RETRY_TIME_MS;
+import static org.apache.iceberg.TableProperties.COMMIT_TOTAL_RETRY_TIME_MS_DEFAULT;
+
import java.io.IOException;
import java.io.UncheckedIOException;
import java.sql.Connection;
@@ -34,14 +43,18 @@ import java.util.List;
import java.util.Locale;
import java.util.Map;
import java.util.Set;
+import java.util.concurrent.atomic.AtomicReference;
import java.util.function.Consumer;
import java.util.function.Function;
import java.util.function.Predicate;
import java.util.stream.Collectors;
import java.util.stream.Stream;
+import org.apache.iceberg.BaseTable;
import org.apache.iceberg.CatalogProperties;
import org.apache.iceberg.CatalogUtil;
import org.apache.iceberg.Schema;
+import org.apache.iceberg.StaticTableOperations;
+import org.apache.iceberg.Table;
import org.apache.iceberg.TableMetadata;
import org.apache.iceberg.TableOperations;
import org.apache.iceberg.Transaction;
@@ -49,6 +62,7 @@ import org.apache.iceberg.catalog.Namespace;
import org.apache.iceberg.catalog.SupportsNamespaces;
import org.apache.iceberg.catalog.TableIdentifier;
import org.apache.iceberg.exceptions.AlreadyExistsException;
+import org.apache.iceberg.exceptions.CommitFailedException;
import org.apache.iceberg.exceptions.NamespaceNotEmptyException;
import org.apache.iceberg.exceptions.NoSuchNamespaceException;
import org.apache.iceberg.exceptions.NoSuchTableException;
@@ -66,6 +80,7 @@ import org.apache.iceberg.relocated.com.google.common.collect.Lists;
import org.apache.iceberg.relocated.com.google.common.collect.Maps;
import org.apache.iceberg.util.LocationUtil;
import org.apache.iceberg.util.PropertyUtil;
+import org.apache.iceberg.util.Tasks;
import org.apache.iceberg.view.BaseMetastoreViewCatalog;
import org.apache.iceberg.view.ViewMetadata;
import org.apache.iceberg.view.ViewOperations;
@@ -297,6 +312,60 @@ public class JdbcCatalog extends BaseMetastoreViewCatalog
return SLASH.join(defaultNamespaceLocation(table.namespace()), tableLocation);
}
+ @Override
+ public Table unregisterTable(TableIdentifier identifier) {
+ Preconditions.checkArgument(
+ identifier != null && isValidIdentifier(identifier), "Invalid identifier: %s", identifier);
+
+ TableMetadata initialMetadata = newTableOps(identifier).current();
+ if (initialMetadata == null) {
+ throw new NoSuchTableException("Table does not exist: %s", identifier);
+ }
+
+ AtomicReference<Table> unregistered = new AtomicReference<>();
+ Tasks.foreach(identifier)
+ .retry(initialMetadata.propertyAsInt(COMMIT_NUM_RETRIES, COMMIT_NUM_RETRIES_DEFAULT))
+ .exponentialBackoff(
+ initialMetadata.propertyAsInt(
+ COMMIT_MIN_RETRY_WAIT_MS, COMMIT_MIN_RETRY_WAIT_MS_DEFAULT),
+ initialMetadata.propertyAsInt(
+ COMMIT_MAX_RETRY_WAIT_MS, COMMIT_MAX_RETRY_WAIT_MS_DEFAULT),
+ initialMetadata.propertyAsInt(
+ COMMIT_TOTAL_RETRY_TIME_MS, COMMIT_TOTAL_RETRY_TIME_MS_DEFAULT),
+ 2.0 /* exponential */)
+ .onlyRetryOn(CommitFailedException.class)
+ .run(tableIdentifier -> unregistered.set(unregisterTableOnce(tableIdentifier)));
+ return unregistered.get();
+ }
+
+ private Table unregisterTableOnce(TableIdentifier identifier) {
+ TableOperations ops = newTableOps(identifier);
+ TableMetadata metadata = ops.current();
+ if (metadata == null) {
+ throw new NoSuchTableException("Table does not exist: %s", identifier);
+ }
+
+ if (dropTableIfMetadataMatches(identifier, metadata.metadataFileLocation()) == 0) {
+ throw new CommitFailedException(
+ "Cannot unregister table %s: metadata location changed concurrently", identifier);
+ }
+
+ StaticTableOperations staticOps =
+ new StaticTableOperations(metadata, ops.io(), ops.locationProvider());
+ return new BaseTable(staticOps, fullTableName(name(), identifier), metricsReporter());
+ }
+
+ int dropTableIfMetadataMatches(TableIdentifier identifier, String metadataLocation) {
+ return execute(
+ (schemaVersion == JdbcUtil.SchemaVersion.V1)
+ ? JdbcUtil.V1_UNREGISTER_TABLE_SQL
+ : JdbcUtil.V0_UNREGISTER_TABLE_SQL,
+ catalogName,
+ JdbcUtil.namespaceToString(identifier.namespace()),
+ identifier.name(),
+ metadataLocation);
+ }
+
@Override
public boolean dropTable(TableIdentifier identifier, boolean purge) {
TableOperations ops = newTableOps(identifier);
diff --git a/core/src/main/java/org/apache/iceberg/jdbc/JdbcUtil.java b/core/src/main/java/org/apache/iceberg/jdbc/JdbcUtil.java
index 259bd7812..528eccdf1 100644
--- a/core/src/main/java/org/apache/iceberg/jdbc/JdbcUtil.java
+++ b/core/src/main/java/org/apache/iceberg/jdbc/JdbcUtil.java
@@ -321,6 +321,8 @@ final class JdbcUtil {
+ " OR "
+ RECORD_TYPE
+ " IS NULL)";
+ static final String V1_UNREGISTER_TABLE_SQL =
+ V1_DROP_TABLE_SQL + " AND " + BaseMetastoreTableOperations.METADATA_LOCATION_PROP + " = ?";
static final String V0_DROP_TABLE_SQL =
"DELETE FROM "
+ CATALOG_TABLE_VIEW_NAME
@@ -331,6 +333,8 @@ final class JdbcUtil {
+ " = ? AND "
+ TABLE_NAME
+ " = ?";
+ static final String V0_UNREGISTER_TABLE_SQL =
+ V0_DROP_TABLE_SQL + " AND " + BaseMetastoreTableOperations.METADATA_LOCATION_PROP + " = ?";
private static final String GET_NAMESPACE_SQL =
"SELECT "
+ TABLE_NAMESPACE
diff --git a/core/src/main/java/org/apache/iceberg/rest/CatalogHandlers.java b/core/src/main/java/org/apache/iceberg/rest/CatalogHandlers.java
index 427560384..0bd1cd219 100644
--- a/core/src/main/java/org/apache/iceberg/rest/CatalogHandlers.java
+++ b/core/src/main/java/org/apache/iceberg/rest/CatalogHandlers.java
@@ -496,23 +496,15 @@ public class CatalogHandlers {
throw new NoSuchTableException("Table does not exist: %s", ident);
}
- // capture the last metadata before dropping so it can be returned for re-registration
- Table table = catalog.loadTable(ident);
+ Table table = catalog.unregisterTable(ident);
if (!(table instanceof BaseTable)) {
throw new IllegalStateException("Cannot wrap catalog that does not produce BaseTable");
}
TableMetadata metadata = ((BaseTable) table).operations().current();
- // catalog implementations should preserve the table's metadata/data files
- boolean dropped = catalog.dropTable(ident, false /* do not purge */);
- if (!dropped) {
- throw new IllegalStateException(
- String.format("Unregister failed. Table not dropped: %s", ident));
- }
-
return ImmutableUnregisterTableResponse.builder()
- .metadataLocation(metadata.metadataFileLocation())
+ .metadataLocation(table.metadataFileLocation())
.metadata(metadata)
.build();
}
diff --git a/core/src/main/java/org/apache/iceberg/rest/RESTCatalog.java b/core/src/main/java/org/apache/iceberg/rest/RESTCatalog.java
index 94883ab7b..a2c8e3f1f 100644
--- a/core/src/main/java/org/apache/iceberg/rest/RESTCatalog.java
+++ b/core/src/main/java/org/apache/iceberg/rest/RESTCatalog.java
@@ -28,7 +28,6 @@ import org.apache.iceberg.CatalogProperties;
import org.apache.iceberg.PartitionSpec;
import org.apache.iceberg.Schema;
import org.apache.iceberg.Table;
-import org.apache.iceberg.TableMetadata;
import org.apache.iceberg.Transaction;
import org.apache.iceberg.catalog.Catalog;
import org.apache.iceberg.catalog.Namespace;
@@ -247,18 +246,9 @@ public class RESTCatalog
return delegate.registerTable(ident, metadataFileLocation, overwrite);
}
- /**
- * Unregister a table from the catalog.
- *
- * <p>This is the opposite of {@link #registerTable(TableIdentifier, String)}. The underlying data
- * and metadata files should be left in place so that the table can be registered in another
- * catalog.
- *
- * @param ident a table identifier
- * @return the last metadata for the unregistered table
- */
- public TableMetadata unregisterTable(TableIdentifier ident) {
- return sessionCatalog.unregisterTable(context, ident);
+ @Override
+ public Table unregisterTable(TableIdentifier ident) {
+ return delegate.unregisterTable(ident);
}
@Override
diff --git a/core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java b/core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java
index 60ae12b58..d22a1ccaf 100644
--- a/core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java
+++ b/core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java
@@ -41,6 +41,7 @@ import org.apache.iceberg.MetadataUpdate;
import org.apache.iceberg.PartitionSpec;
import org.apache.iceberg.Schema;
import org.apache.iceberg.SortOrder;
+import org.apache.iceberg.StaticTableOperations;
import org.apache.iceberg.Table;
import org.apache.iceberg.TableMetadata;
import org.apache.iceberg.TableOperations;
@@ -736,14 +737,15 @@ public class RESTSessionCatalog extends BaseViewSessionCatalog
* Unregister a table from the catalog without removing its data or metadata files.
*
* <p>This is the opposite of {@link #registerTable(SessionContext, TableIdentifier, String)}. On
- * success, the table no longer exists in the catalog and the returned metadata can be used to
- * register the table in another catalog.
+ * success, the table no longer exists in the catalog and the returned table is fixed at the last
+ * metadata registered with the catalog.
*
* @param context session context
* @param identifier a table identifier
- * @return the last metadata for the unregistered table
+ * @return a read-only table fixed at the metadata current when it was unregistered
*/
- public TableMetadata unregisterTable(SessionContext context, TableIdentifier identifier) {
+ @Override
+ public Table unregisterTable(SessionContext context, TableIdentifier identifier) {
Endpoint.check(endpoints, Endpoint.V1_UNREGISTER_TABLE);
checkIdentifierIsValid(identifier);
@@ -758,7 +760,8 @@ public class RESTSessionCatalog extends BaseViewSessionCatalog
UnregisterTableResponse.class,
mutationHeaders,
ErrorHandlers.tableErrorHandler());
- return response.metadata();
+ StaticTableOperations ops = new StaticTableOperations(response.metadata(), io);
+ return new BaseTable(ops, fullTableName(identifier));
} finally {
invalidateTable(context, identifier);
}
diff --git a/core/src/test/java/org/apache/iceberg/catalog/CatalogTests.java b/core/src/test/java/org/apache/iceberg/catalog/CatalogTests.java
index 6c6949ce4..491af0e97 100644
--- a/core/src/test/java/org/apache/iceberg/catalog/CatalogTests.java
+++ b/core/src/test/java/org/apache/iceberg/catalog/CatalogTests.java
@@ -203,6 +203,10 @@ public abstract class CatalogTests<C extends Catalog & SupportsNamespaces> {
return false;
}
+ protected boolean supportsUnregister() {
+ return false;
+ }
+
protected String baseTableLocation(TableIdentifier identifier) {
return BASE_TABLE_LOCATION + "/" + identifier.namespace() + "/" + identifier.name();
}
@@ -3327,6 +3331,46 @@ public abstract class CatalogTests<C extends Catalog & SupportsNamespaces> {
assertThat(catalog.dropTable(identifier)).isTrue();
}
+ @Test
+ public void unregisterTable() {
+ assumeThat(supportsUnregister()).isTrue();
+
+ C catalog = catalog();
+ if (requiresNamespaceCreate()) {
+ catalog.createNamespace(TABLE.namespace());
+ }
+
+ Table original = catalog.buildTable(TABLE, SCHEMA).withPartitionSpec(SPEC).create();
+ original.newFastAppend().appendFile(FILE_A).commit();
+ String metadataLocation = original.metadataFileLocation();
+
+ Table unregistered = catalog.unregisterTable(TABLE);
+
+ assertThat(unregistered.metadataFileLocation()).isEqualTo(metadataLocation);
+ assertThat(unregistered.currentSnapshot()).isEqualTo(original.currentSnapshot());
+ assertThat(catalog.tableExists(TABLE)).isFalse();
+ assertThat(unregistered.io().newInputFile(metadataLocation).exists()).isTrue();
+ assertThatThrownBy(() -> unregistered.updateProperties().set("unregistered", "true").commit())
+ .isInstanceOf(UnsupportedOperationException.class)
+ .hasMessageContaining("Cannot modify a static table");
+
+ TableIdentifier registeredIdentifier =
+ TableIdentifier.of(TABLE.namespace(), "registered-after-unregister");
+ Table registered = catalog.registerTable(registeredIdentifier, metadataLocation);
+ assertThat(registered.currentSnapshot()).isEqualTo(original.currentSnapshot());
+ assertFiles(registered, FILE_A);
+ assertThat(catalog.dropTable(registeredIdentifier)).isTrue();
+ }
+
+ @Test
+ public void unregisterMissingTable() {
+ assumeThat(supportsUnregister()).isTrue();
+
+ assertThatThrownBy(() -> catalog().unregisterTable(TABLE))
+ .isInstanceOf(NoSuchTableException.class)
+ .hasMessageContaining("Table does not exist");
+ }
+
@Test
public void testCatalogWithCustomMetricsReporter() throws IOException {
C catalogWithCustomReporter =
diff --git a/core/src/test/java/org/apache/iceberg/inmemory/TestInMemoryCatalog.java b/core/src/test/java/org/apache/iceberg/inmemory/TestInMemoryCatalog.java
index 827450d4a..9cd361aa2 100644
--- a/core/src/test/java/org/apache/iceberg/inmemory/TestInMemoryCatalog.java
+++ b/core/src/test/java/org/apache/iceberg/inmemory/TestInMemoryCatalog.java
@@ -87,6 +87,11 @@ public class TestInMemoryCatalog extends CatalogTests<InMemoryCatalog> {
return true;
}
+ @Override
+ protected boolean supportsUnregister() {
+ return true;
+ }
+
@Test
@Override
public void testLoadTableWithMissingMetadataFile(@TempDir Path tempDir) throws IOException {
diff --git a/core/src/test/java/org/apache/iceberg/jdbc/TestJdbcCatalog.java b/core/src/test/java/org/apache/iceberg/jdbc/TestJdbcCatalog.java
index ce22c8089..674fccb90 100644
--- a/core/src/test/java/org/apache/iceberg/jdbc/TestJdbcCatalog.java
+++ b/core/src/test/java/org/apache/iceberg/jdbc/TestJdbcCatalog.java
@@ -61,6 +61,7 @@ import org.apache.iceberg.catalog.CatalogTests;
import org.apache.iceberg.catalog.Namespace;
import org.apache.iceberg.catalog.TableIdentifier;
import org.apache.iceberg.exceptions.AlreadyExistsException;
+import org.apache.iceberg.exceptions.CommitFailedException;
import org.apache.iceberg.exceptions.NamespaceNotEmptyException;
import org.apache.iceberg.exceptions.NoSuchNamespaceException;
import org.apache.iceberg.exceptions.NoSuchTableException;
@@ -112,6 +113,11 @@ public class TestJdbcCatalog extends CatalogTests<JdbcCatalog> {
return true;
}
+ @Override
+ protected boolean supportsUnregister() {
+ return true;
+ }
+
@Override
protected boolean supportsNamesWithDot() {
// namespaces with a dot are not supported
@@ -594,6 +600,63 @@ public class TestJdbcCatalog extends CatalogTests<JdbcCatalog> {
"Failed to load table db.table from catalog test_jdbc_catalog: dropped by another process");
}
+ @Test
+ public void testUnregisterRetriesAfterConcurrentCommit() {
+ TableIdentifier tableIdentifier = TableIdentifier.of("db", "table");
+ catalog.createTable(tableIdentifier, SCHEMA, PartitionSpec.unpartitioned());
+
+ JdbcCatalog unregisteringCatalog = Mockito.spy(catalog);
+ int[] attempts = {0};
+ String[] committedMetadataLocation = {null};
+ Mockito.doAnswer(
+ invocation -> {
+ if (attempts[0]++ == 0) {
+ Table concurrentTable = catalog.loadTable(tableIdentifier);
+ concurrentTable.updateProperties().set("concurrent", "commit").commit();
+ committedMetadataLocation[0] = concurrentTable.metadataFileLocation();
+ }
+
+ return invocation.callRealMethod();
+ })
+ .when(unregisteringCatalog)
+ .dropTableIfMetadataMatches(any(TableIdentifier.class), any(String.class));
+
+ Table unregistered = unregisteringCatalog.unregisterTable(tableIdentifier);
+
+ assertThat(attempts[0]).isEqualTo(2);
+ assertThat(unregistered.metadataFileLocation()).isEqualTo(committedMetadataLocation[0]);
+ assertThat(unregistered.properties()).containsEntry("concurrent", "commit");
+ assertThat(catalog.tableExists(tableIdentifier)).isFalse();
+ }
+
+ @Test
+ public void testUnregisterStopsAfterRetryTimeout() {
+ TableIdentifier tableIdentifier = TableIdentifier.of("db", "table");
+ catalog
+ .buildTable(tableIdentifier, SCHEMA)
+ .withProperty(TableProperties.COMMIT_NUM_RETRIES, "1")
+ .withProperty(TableProperties.COMMIT_MIN_RETRY_WAIT_MS, "0")
+ .withProperty(TableProperties.COMMIT_MAX_RETRY_WAIT_MS, "0")
+ .withProperty(TableProperties.COMMIT_TOTAL_RETRY_TIME_MS, "0")
+ .create();
+
+ JdbcCatalog unregisteringCatalog = Mockito.spy(catalog);
+ int[] attempts = {0};
+ Mockito.doAnswer(
+ invocation -> {
+ attempts[0] += 1;
+ return 0;
+ })
+ .when(unregisteringCatalog)
+ .dropTableIfMetadataMatches(any(TableIdentifier.class), any(String.class));
+
+ assertThatThrownBy(() -> unregisteringCatalog.unregisterTable(tableIdentifier))
+ .isInstanceOf(CommitFailedException.class)
+ .hasMessage("Cannot unregister table db.table: metadata location changed concurrently");
+ assertThat(attempts[0]).isEqualTo(2);
+ assertThat(catalog.tableExists(tableIdentifier)).isTrue();
+ }
+
@Test
public void testCommitHistory() throws IOException {
TableIdentifier testTable = TableIdentifier.of("db", "ns", "tbl");
diff --git a/open-api/src/test/java/org/apache/iceberg/rest/RESTCompatibilityKitCatalogTests.java b/open-api/src/test/java/org/apache/iceberg/rest/RESTCompatibilityKitCatalogTests.java
index 4e5aedfba..c9d837836 100644
--- a/open-api/src/test/java/org/apache/iceberg/rest/RESTCompatibilityKitCatalogTests.java
+++ b/open-api/src/test/java/org/apache/iceberg/rest/RESTCompatibilityKitCatalogTests.java
@@ -23,7 +23,6 @@ import static org.assertj.core.api.Assertions.assertThatThrownBy;
import java.util.Map;
import org.apache.iceberg.Table;
-import org.apache.iceberg.TableMetadata;
import org.apache.iceberg.catalog.CatalogTests;
import org.apache.iceberg.exceptions.NoSuchTableException;
import org.apache.iceberg.util.PropertyUtil;
@@ -113,6 +112,11 @@ public class RESTCompatibilityKitCatalogTests extends CatalogTests<RESTCatalog>
return false;
}
+ @Override
+ protected boolean supportsUnregister() {
+ return true;
+ }
+
@Test
public void testUnregisterTable() {
if (requiresNamespaceCreate()) {
@@ -128,15 +132,21 @@ public class RESTCompatibilityKitCatalogTests extends CatalogTests<RESTCatalog>
original.newFastAppend().appendFile(FILE_A).commit();
original.newFastAppend().appendFile(FILE_B).commit();
- TableMetadata metadata = restCatalog.unregisterTable(TABLE);
+ Table unregistered = restCatalog.unregisterTable(TABLE);
- assertThat(metadata).as("Returned metadata must not be null").isNotNull();
+ assertThat(unregistered.currentSnapshot())
+ .as("Current snapshot must match the unregistered table")
+ .isEqualTo(original.currentSnapshot());
+ assertFiles(unregistered, FILE_A, FILE_B);
assertThat(restCatalog.tableExists(TABLE))
.as("Table must not exist after being unregistered")
.isFalse();
+ assertThatThrownBy(() -> unregistered.updateProperties().set("unregistered", "true").commit())
+ .isInstanceOf(UnsupportedOperationException.class)
+ .hasMessageContaining("Cannot modify a static table");
// the underlying files are left in place, so the table can be registered again
- Table registered = restCatalog.registerTable(TABLE, metadata.metadataFileLocation());
+ Table registered = restCatalog.registerTable(TABLE, unregistered.metadataFileLocation());
assertThat(registered.currentSnapshot())
.as("Current snapshot must match the unregistered table")
.isEqualTo(original.currentSnapshot());
```
</details>
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org