From 3bb76cd731ad8254c01f79e7d4f440dce88498c9 Mon Sep 17 00:00:00 2001 From: Alex Stephen Date: Wed, 27 May 2026 21:05:17 +0000 Subject: [PATCH 1/9] Add unregister table to RCK --- .../apache/iceberg/rest/CatalogHandlers.java | 23 +++ .../org/apache/iceberg/rest/Endpoint.java | 2 + .../org/apache/iceberg/rest/RESTCatalog.java | 13 ++ .../apache/iceberg/rest/RESTSerializers.java | 27 ++++ .../iceberg/rest/RESTSessionCatalog.java | 33 ++++ .../apache/iceberg/rest/ResourcePaths.java | 13 ++ .../responses/UnregisterTableResponse.java | 41 +++++ .../UnregisterTableResponseParser.java | 78 ++++++++++ .../iceberg/rest/RESTCatalogAdapter.java | 10 ++ .../java/org/apache/iceberg/rest/Route.java | 6 + .../TestUnregisterTableResponseParser.java | 143 ++++++++++++++++++ .../RESTCompatibilityKitCatalogTests.java | 46 ++++++ 12 files changed, 435 insertions(+) create mode 100644 core/src/main/java/org/apache/iceberg/rest/responses/UnregisterTableResponse.java create mode 100644 core/src/main/java/org/apache/iceberg/rest/responses/UnregisterTableResponseParser.java create mode 100644 core/src/test/java/org/apache/iceberg/rest/responses/TestUnregisterTableResponseParser.java 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 13089fc07ded..f0dc436671f2 100644 --- a/core/src/main/java/org/apache/iceberg/rest/CatalogHandlers.java +++ b/core/src/main/java/org/apache/iceberg/rest/CatalogHandlers.java @@ -95,11 +95,13 @@ import org.apache.iceberg.rest.responses.FetchScanTasksResponse; import org.apache.iceberg.rest.responses.GetNamespaceResponse; import org.apache.iceberg.rest.responses.ImmutableLoadViewResponse; +import org.apache.iceberg.rest.responses.ImmutableUnregisterTableResponse; import org.apache.iceberg.rest.responses.ListNamespacesResponse; import org.apache.iceberg.rest.responses.ListTablesResponse; import org.apache.iceberg.rest.responses.LoadTableResponse; import org.apache.iceberg.rest.responses.LoadViewResponse; import org.apache.iceberg.rest.responses.PlanTableScanResponse; +import org.apache.iceberg.rest.responses.UnregisterTableResponse; import org.apache.iceberg.rest.responses.UpdateNamespacePropertiesResponse; import org.apache.iceberg.util.Pair; import org.apache.iceberg.util.Tasks; @@ -488,6 +490,27 @@ public static void dropTable(Catalog catalog, TableIdentifier ident) { } } + public static UnregisterTableResponse unregisterTable(Catalog catalog, TableIdentifier ident) { + // capture the last metadata before dropping so it can be returned for re-registration + Table table = catalog.loadTable(ident); + if (!(table instanceof BaseTable)) { + throw new IllegalStateException("Cannot wrap catalog that does not produce BaseTable"); + } + + TableMetadata metadata = ((BaseTable) table).operations().current(); + + // unregister without removing the underlying data and metadata files + boolean dropped = catalog.dropTable(ident, false /* do not purge */); + if (!dropped) { + throw new NoSuchTableException("Table does not exist: %s", ident); + } + + return ImmutableUnregisterTableResponse.builder() + .metadataLocation(metadata.metadataFileLocation()) + .metadata(metadata) + .build(); + } + public static void purgeTable(Catalog catalog, TableIdentifier ident) { boolean dropped = catalog.dropTable(ident, true); if (!dropped) { diff --git a/core/src/main/java/org/apache/iceberg/rest/Endpoint.java b/core/src/main/java/org/apache/iceberg/rest/Endpoint.java index d56a14d18954..ef071c228ea4 100644 --- a/core/src/main/java/org/apache/iceberg/rest/Endpoint.java +++ b/core/src/main/java/org/apache/iceberg/rest/Endpoint.java @@ -62,6 +62,8 @@ public class Endpoint { Endpoint.create("POST", ResourcePaths.V1_TABLE_RENAME); public static final Endpoint V1_REGISTER_TABLE = Endpoint.create("POST", ResourcePaths.V1_TABLE_REGISTER); + public static final Endpoint V1_UNREGISTER_TABLE = + Endpoint.create("POST", ResourcePaths.V1_TABLE_UNREGISTER); public static final Endpoint V1_REPORT_METRICS = Endpoint.create("POST", ResourcePaths.V1_TABLE_METRICS); public static final Endpoint V1_TABLE_CREDENTIALS = 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 02bceab4d2a0..3416e3dc2824 100644 --- a/core/src/main/java/org/apache/iceberg/rest/RESTCatalog.java +++ b/core/src/main/java/org/apache/iceberg/rest/RESTCatalog.java @@ -252,6 +252,19 @@ public Table registerTable( return delegate.registerTable(ident, metadataFileLocation, overwrite); } + /** + * Unregister a table from the catalog without removing its data or metadata files. + * + *

This is the opposite of {@link #registerTable(TableIdentifier, String)}. The underlying data + * and metadata files are left in place so that the table can be registered in another catalog. + * + * @param ident a table identifier + * @return the last metadata location for the unregistered table + */ + public String unregisterTable(TableIdentifier ident) { + return sessionCatalog.unregisterTable(context, ident); + } + @Override public void createNamespace(Namespace ns, Map props) { nsDelegate.createNamespace(ns, props); diff --git a/core/src/main/java/org/apache/iceberg/rest/RESTSerializers.java b/core/src/main/java/org/apache/iceberg/rest/RESTSerializers.java index c97f4aa2e522..ad3aa936c5b9 100644 --- a/core/src/main/java/org/apache/iceberg/rest/RESTSerializers.java +++ b/core/src/main/java/org/apache/iceberg/rest/RESTSerializers.java @@ -80,6 +80,7 @@ import org.apache.iceberg.rest.responses.ImmutableLoadCredentialsResponse; import org.apache.iceberg.rest.responses.ImmutableLoadViewResponse; import org.apache.iceberg.rest.responses.ImmutableRemoteSignResponse; +import org.apache.iceberg.rest.responses.ImmutableUnregisterTableResponse; import org.apache.iceberg.rest.responses.LoadCredentialsResponse; import org.apache.iceberg.rest.responses.LoadCredentialsResponseParser; import org.apache.iceberg.rest.responses.LoadTableResponse; @@ -91,6 +92,8 @@ import org.apache.iceberg.rest.responses.PlanTableScanResponseParser; import org.apache.iceberg.rest.responses.RemoteSignResponse; import org.apache.iceberg.rest.responses.RemoteSignResponseParser; +import org.apache.iceberg.rest.responses.UnregisterTableResponse; +import org.apache.iceberg.rest.responses.UnregisterTableResponseParser; import org.apache.iceberg.util.JsonUtil; public class RESTSerializers { @@ -153,6 +156,12 @@ public static void registerAll(ObjectMapper mapper) { .addDeserializer(ConfigResponse.class, new ConfigResponseDeserializer<>()) .addSerializer(LoadTableResponse.class, new LoadTableResponseSerializer<>()) .addDeserializer(LoadTableResponse.class, new LoadTableResponseDeserializer<>()) + .addSerializer(UnregisterTableResponse.class, new UnregisterTableResponseSerializer<>()) + .addSerializer( + ImmutableUnregisterTableResponse.class, new UnregisterTableResponseSerializer<>()) + .addDeserializer(UnregisterTableResponse.class, new UnregisterTableResponseDeserializer<>()) + .addDeserializer( + ImmutableUnregisterTableResponse.class, new UnregisterTableResponseDeserializer<>()) .addSerializer(PlanTableScanRequest.class, new PlanTableScanRequestSerializer<>()) .addDeserializer(PlanTableScanRequest.class, new PlanTableScanRequestDeserializer<>()) .addSerializer(FetchScanTasksRequest.class, new FetchScanTasksRequestSerializer<>()) @@ -532,6 +541,24 @@ public void serialize(T request, JsonGenerator gen, SerializerProvider serialize } } + static class UnregisterTableResponseSerializer + extends JsonSerializer { + @Override + public void serialize(T response, JsonGenerator gen, SerializerProvider serializers) + throws IOException { + UnregisterTableResponseParser.toJson(response, gen); + } + } + + static class UnregisterTableResponseDeserializer + extends JsonDeserializer { + @Override + public T deserialize(JsonParser p, DeserializationContext context) throws IOException { + JsonNode jsonNode = p.getCodec().readTree(p); + return (T) UnregisterTableResponseParser.fromJson(jsonNode); + } + } + static class LoadTableResponseDeserializer extends JsonDeserializer { @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 0b30ef9d88ef..36c6130ccc44 100644 --- a/core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java +++ b/core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java @@ -100,6 +100,7 @@ import org.apache.iceberg.rest.responses.ListTablesResponse; import org.apache.iceberg.rest.responses.LoadTableResponse; import org.apache.iceberg.rest.responses.LoadViewResponse; +import org.apache.iceberg.rest.responses.UnregisterTableResponse; import org.apache.iceberg.rest.responses.UpdateNamespacePropertiesResponse; import org.apache.iceberg.util.EnvironmentUtil; import org.apache.iceberg.util.PropertyUtil; @@ -805,6 +806,38 @@ public Table registerTable( response.labels()); } + /** + * Unregister a table from the catalog without removing its data or metadata files. + * + *

This is the opposite of {@link #registerTable(SessionContext, TableIdentifier, String)}. On + * success, the table no longer exists in the catalog and the returned metadata location can be + * used to register the table in another catalog. + * + * @param context session context + * @param identifier a table identifier + * @return the last metadata location for the unregistered table + */ + public String unregisterTable(SessionContext context, TableIdentifier identifier) { + Endpoint.check(endpoints, Endpoint.V1_UNREGISTER_TABLE); + checkIdentifierIsValid(identifier); + + try { + AuthSession contextualSession = authManager.contextualSession(context, catalogAuth); + UnregisterTableResponse response = + client + .withAuthSession(contextualSession) + .post( + paths.unregister(identifier), + null, + UnregisterTableResponse.class, + mutationHeaders, + ErrorHandlers.tableErrorHandler()); + return response.metadataLocation(); + } finally { + invalidateTable(context, identifier); + } + } + @Override public void createNamespace( SessionContext context, Namespace namespace, Map metadata) { diff --git a/core/src/main/java/org/apache/iceberg/rest/ResourcePaths.java b/core/src/main/java/org/apache/iceberg/rest/ResourcePaths.java index 5757a6d17624..63637e75da2b 100644 --- a/core/src/main/java/org/apache/iceberg/rest/ResourcePaths.java +++ b/core/src/main/java/org/apache/iceberg/rest/ResourcePaths.java @@ -38,6 +38,8 @@ public class ResourcePaths { public static final String V1_TABLE_REMOTE_SIGN = "/v1/{prefix}/namespaces/{namespace}/tables/{table}/sign"; public static final String V1_TABLE_REGISTER = "/v1/{prefix}/namespaces/{namespace}/register"; + public static final String V1_TABLE_UNREGISTER = + "/v1/{prefix}/namespaces/{namespace}/tables/{table}/unregister"; public static final String V1_TABLE_METRICS = "/v1/{prefix}/namespaces/{namespace}/tables/{table}/metrics"; public static final String V1_TABLE_RENAME = "/v1/{prefix}/tables/rename"; @@ -108,6 +110,17 @@ public String register(Namespace ns) { return SLASH.join("v1", prefix, "namespaces", pathEncode(ns), "register"); } + public String unregister(TableIdentifier ident) { + return SLASH.join( + "v1", + prefix, + "namespaces", + pathEncode(ident.namespace()), + "tables", + RESTUtil.encodeString(ident.name()), + "unregister"); + } + public String rename() { return SLASH.join("v1", prefix, "tables", "rename"); } diff --git a/core/src/main/java/org/apache/iceberg/rest/responses/UnregisterTableResponse.java b/core/src/main/java/org/apache/iceberg/rest/responses/UnregisterTableResponse.java new file mode 100644 index 000000000000..336d361b5327 --- /dev/null +++ b/core/src/main/java/org/apache/iceberg/rest/responses/UnregisterTableResponse.java @@ -0,0 +1,41 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.iceberg.rest.responses; + +import org.apache.iceberg.TableMetadata; +import org.apache.iceberg.rest.RESTResponse; +import org.immutables.value.Value; + +/** + * Represents the response when a table is successfully unregistered from a catalog. + * + *

The response carries the table's last metadata location and the corresponding table metadata + * so that the underlying (still present) files can be registered in another catalog. + */ +@Value.Immutable +public interface UnregisterTableResponse extends RESTResponse { + String metadataLocation(); + + TableMetadata metadata(); + + @Override + default void validate() { + // nothing to validate as it's not possible to create an invalid instance + } +} diff --git a/core/src/main/java/org/apache/iceberg/rest/responses/UnregisterTableResponseParser.java b/core/src/main/java/org/apache/iceberg/rest/responses/UnregisterTableResponseParser.java new file mode 100644 index 000000000000..95479fa018c1 --- /dev/null +++ b/core/src/main/java/org/apache/iceberg/rest/responses/UnregisterTableResponseParser.java @@ -0,0 +1,78 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.iceberg.rest.responses; + +import com.fasterxml.jackson.core.JsonGenerator; +import com.fasterxml.jackson.databind.JsonNode; +import java.io.IOException; +import org.apache.iceberg.TableMetadata; +import org.apache.iceberg.TableMetadataParser; +import org.apache.iceberg.relocated.com.google.common.base.Preconditions; +import org.apache.iceberg.util.JsonUtil; + +public class UnregisterTableResponseParser { + + private static final String METADATA_LOCATION = "metadata-location"; + private static final String METADATA = "metadata"; + + private UnregisterTableResponseParser() {} + + public static String toJson(UnregisterTableResponse response) { + return toJson(response, false); + } + + public static String toJson(UnregisterTableResponse response, boolean pretty) { + return JsonUtil.generate(gen -> toJson(response, gen), pretty); + } + + public static void toJson(UnregisterTableResponse response, JsonGenerator gen) + throws IOException { + Preconditions.checkArgument(null != response, "Invalid unregister table response: null"); + + gen.writeStartObject(); + + gen.writeStringField(METADATA_LOCATION, response.metadataLocation()); + + gen.writeFieldName(METADATA); + TableMetadataParser.toJson(response.metadata(), gen); + + gen.writeEndObject(); + } + + public static UnregisterTableResponse fromJson(String json) { + return JsonUtil.parse(json, UnregisterTableResponseParser::fromJson); + } + + public static UnregisterTableResponse fromJson(JsonNode json) { + Preconditions.checkArgument( + null != json, "Cannot parse unregister table response from null object"); + + String metadataLocation = JsonUtil.getString(METADATA_LOCATION, json); + TableMetadata metadata = TableMetadataParser.fromJson(JsonUtil.get(METADATA, json)); + + if (null == metadata.metadataFileLocation()) { + metadata = TableMetadata.buildFrom(metadata).withMetadataLocation(metadataLocation).build(); + } + + return ImmutableUnregisterTableResponse.builder() + .metadataLocation(metadataLocation) + .metadata(metadata) + .build(); + } +} diff --git a/core/src/test/java/org/apache/iceberg/rest/RESTCatalogAdapter.java b/core/src/test/java/org/apache/iceberg/rest/RESTCatalogAdapter.java index b99d7ffb63f4..e06f07d445b5 100644 --- a/core/src/test/java/org/apache/iceberg/rest/RESTCatalogAdapter.java +++ b/core/src/test/java/org/apache/iceberg/rest/RESTCatalogAdapter.java @@ -391,6 +391,16 @@ public T handleRequest( }); } + case UNREGISTER_TABLE: + { + return CatalogHandlers.withIdempotency( + httpRequest, + () -> + castResponse( + responseType, + CatalogHandlers.unregisterTable(catalog, tableIdentFromPathVars(vars)))); + } + case UPDATE_TABLE: { return CatalogHandlers.withIdempotency( diff --git a/core/src/test/java/org/apache/iceberg/rest/Route.java b/core/src/test/java/org/apache/iceberg/rest/Route.java index 8680915bff64..4a51fc256c98 100644 --- a/core/src/test/java/org/apache/iceberg/rest/Route.java +++ b/core/src/test/java/org/apache/iceberg/rest/Route.java @@ -45,6 +45,7 @@ import org.apache.iceberg.rest.responses.LoadViewResponse; import org.apache.iceberg.rest.responses.OAuthTokenResponse; import org.apache.iceberg.rest.responses.PlanTableScanResponse; +import org.apache.iceberg.rest.responses.UnregisterTableResponse; import org.apache.iceberg.rest.responses.UpdateNamespacePropertiesResponse; import org.apache.iceberg.util.Pair; @@ -82,6 +83,11 @@ enum Route { ResourcePaths.V1_TABLE_REGISTER, RegisterTableRequest.class, LoadTableResponse.class), + UNREGISTER_TABLE( + HTTPRequest.HTTPMethod.POST, + ResourcePaths.V1_TABLE_UNREGISTER, + null, + UnregisterTableResponse.class), UPDATE_TABLE( HTTPRequest.HTTPMethod.POST, ResourcePaths.V1_TABLE, diff --git a/core/src/test/java/org/apache/iceberg/rest/responses/TestUnregisterTableResponseParser.java b/core/src/test/java/org/apache/iceberg/rest/responses/TestUnregisterTableResponseParser.java new file mode 100644 index 000000000000..af1a0ed36d72 --- /dev/null +++ b/core/src/test/java/org/apache/iceberg/rest/responses/TestUnregisterTableResponseParser.java @@ -0,0 +1,143 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.iceberg.rest.responses; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +import com.fasterxml.jackson.databind.JsonNode; +import org.apache.iceberg.PartitionSpec; +import org.apache.iceberg.Schema; +import org.apache.iceberg.SortOrder; +import org.apache.iceberg.TableMetadata; +import org.apache.iceberg.types.Types; +import org.junit.jupiter.api.Test; + +public class TestUnregisterTableResponseParser { + + @Test + public void nullAndEmptyCheck() { + assertThatThrownBy(() -> UnregisterTableResponseParser.toJson(null)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("Invalid unregister table response: null"); + + assertThatThrownBy(() -> UnregisterTableResponseParser.fromJson((JsonNode) null)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("Cannot parse unregister table response from null object"); + + assertThatThrownBy(() -> UnregisterTableResponseParser.fromJson("{}")) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("Cannot parse missing string: metadata-location"); + } + + @Test + public void missingFields() { + assertThatThrownBy( + () -> + UnregisterTableResponseParser.fromJson( + "{\"metadata-location\": \"custom-location\"}")) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("Cannot parse missing field: metadata"); + } + + @Test + public void roundTripSerde() { + String uuid = "386b9f01-002b-4d8c-b77f-42c3fd3b7c9b"; + TableMetadata metadata = + TableMetadata.buildFromEmpty(1) + .assignUUID(uuid) + .setLocation("location") + .setCurrentSchema( + new Schema(Types.NestedField.required(1, "x", Types.LongType.get())), 1) + .addPartitionSpec(PartitionSpec.unpartitioned()) + .addSortOrder(SortOrder.unsorted()) + .discardChanges() + .withMetadataLocation("metadata-location") + .build(); + + UnregisterTableResponse response = + ImmutableUnregisterTableResponse.builder() + .metadataLocation("metadata-location") + .metadata(metadata) + .build(); + + String expectedJson = + String.format( + "{\n" + + " \"metadata-location\" : \"metadata-location\",\n" + + " \"metadata\" : {\n" + + " \"format-version\" : 1,\n" + + " \"table-uuid\" : \"386b9f01-002b-4d8c-b77f-42c3fd3b7c9b\",\n" + + " \"location\" : \"location\",\n" + + " \"last-updated-ms\" : %s,\n" + + " \"last-column-id\" : 1,\n" + + " \"schema\" : {\n" + + " \"type\" : \"struct\",\n" + + " \"schema-id\" : 0,\n" + + " \"fields\" : [ {\n" + + " \"id\" : 1,\n" + + " \"name\" : \"x\",\n" + + " \"required\" : true,\n" + + " \"type\" : \"long\"\n" + + " } ]\n" + + " },\n" + + " \"current-schema-id\" : 0,\n" + + " \"schemas\" : [ {\n" + + " \"type\" : \"struct\",\n" + + " \"schema-id\" : 0,\n" + + " \"fields\" : [ {\n" + + " \"id\" : 1,\n" + + " \"name\" : \"x\",\n" + + " \"required\" : true,\n" + + " \"type\" : \"long\"\n" + + " } ]\n" + + " } ],\n" + + " \"partition-spec\" : [ ],\n" + + " \"default-spec-id\" : 0,\n" + + " \"partition-specs\" : [ {\n" + + " \"spec-id\" : 0,\n" + + " \"fields\" : [ ]\n" + + " } ],\n" + + " \"last-partition-id\" : 999,\n" + + " \"default-sort-order-id\" : 0,\n" + + " \"sort-orders\" : [ {\n" + + " \"order-id\" : 0,\n" + + " \"fields\" : [ ]\n" + + " } ],\n" + + " \"properties\" : { },\n" + + " \"current-snapshot-id\" : -1,\n" + + " \"refs\" : { },\n" + + " \"snapshots\" : [ ],\n" + + " \"statistics\" : [ ],\n" + + " \"partition-statistics\" : [ ],\n" + + " \"snapshot-log\" : [ ],\n" + + " \"metadata-log\" : [ ]\n" + + " }\n" + + "}", + metadata.lastUpdatedMillis()); + + String json = UnregisterTableResponseParser.toJson(response, true); + assertThat(json).isEqualTo(expectedJson); + // can't do an equality comparison because Schema doesn't implement equals/hashCode + assertThat( + UnregisterTableResponseParser.toJson( + UnregisterTableResponseParser.fromJson(json), true)) + .isEqualTo(expectedJson); + } +} 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 611a6a20da05..ec093a7b8f1a 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 @@ -19,9 +19,12 @@ package org.apache.iceberg.rest; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; import java.util.Map; +import org.apache.iceberg.Table; import org.apache.iceberg.catalog.CatalogTests; +import org.apache.iceberg.exceptions.NoSuchTableException; import org.apache.iceberg.util.PropertyUtil; import org.junit.jupiter.api.AfterAll; import org.junit.jupiter.api.BeforeAll; @@ -115,6 +118,49 @@ protected boolean supportsVariant() { restCatalog.properties(), RESTCompatibilityKitSuite.RCK_SUPPORTS_VARIANT, false); } + @Test + public void testUnregisterTable() { + if (requiresNamespaceCreate()) { + restCatalog.createNamespace(TABLE.namespace()); + } + + Table original = + restCatalog + .buildTable(TABLE, SCHEMA) + .withPartitionSpec(SPEC) + .withSortOrder(WRITE_ORDER) + .create(); + original.newFastAppend().appendFile(FILE_A).commit(); + original.newFastAppend().appendFile(FILE_B).commit(); + + String metadataLocation = restCatalog.unregisterTable(TABLE); + + assertThat(metadataLocation).as("Returned metadata location must not be null").isNotNull(); + assertThat(restCatalog.tableExists(TABLE)) + .as("Table must not exist after being unregistered") + .isFalse(); + + // the underlying files are left in place, so the table can be registered again + Table registered = restCatalog.registerTable(TABLE, metadataLocation); + assertThat(registered.currentSnapshot()) + .as("Current snapshot must match the unregistered table") + .isEqualTo(original.currentSnapshot()); + assertFiles(registered, FILE_A, FILE_B); + + assertThat(restCatalog.dropTable(TABLE)).isTrue(); + } + + @Test + public void testUnregisterMissingTable() { + if (requiresNamespaceCreate()) { + restCatalog.createNamespace(TABLE.namespace()); + } + + assertThatThrownBy(() -> restCatalog.unregisterTable(TABLE)) + .isInstanceOf(NoSuchTableException.class) + .hasMessageContaining("Table does not exist"); + } + @Disabled("RESTServerExtension isn’t configurable per test") @Test public void createTableInUniqueLocation() { From c6b13f1fb50e07f273562781cc07bac0255f1a07 Mon Sep 17 00:00:00 2001 From: Alex Stephen <1325798+rambleraptor@users.noreply.github.com> Date: Tue, 30 Jun 2026 15:33:31 -0700 Subject: [PATCH 2/9] Update core/src/main/java/org/apache/iceberg/rest/RESTCatalog.java Co-authored-by: Daniel Weeks --- core/src/main/java/org/apache/iceberg/rest/RESTCatalog.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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 3416e3dc2824..552fffc1b3cd 100644 --- a/core/src/main/java/org/apache/iceberg/rest/RESTCatalog.java +++ b/core/src/main/java/org/apache/iceberg/rest/RESTCatalog.java @@ -253,10 +253,10 @@ public Table registerTable( } /** - * Unregister a table from the catalog without removing its data or metadata files. + * Unregister a table from the catalog. * *

This is the opposite of {@link #registerTable(TableIdentifier, String)}. The underlying data - * and metadata files are left in place so that the table can be registered in another catalog. + * 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 location for the unregistered table From 87a43332b12e95413d839ae53aa35244c644c4cf Mon Sep 17 00:00:00 2001 From: Alex Stephen <1325798+rambleraptor@users.noreply.github.com> Date: Tue, 30 Jun 2026 15:33:55 -0700 Subject: [PATCH 3/9] Update core/src/main/java/org/apache/iceberg/rest/CatalogHandlers.java Co-authored-by: Marco Kroll --- core/src/main/java/org/apache/iceberg/rest/CatalogHandlers.java | 1 + 1 file changed, 1 insertion(+) 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 f0dc436671f2..ccd894ef399c 100644 --- a/core/src/main/java/org/apache/iceberg/rest/CatalogHandlers.java +++ b/core/src/main/java/org/apache/iceberg/rest/CatalogHandlers.java @@ -51,6 +51,7 @@ import org.apache.iceberg.BaseTransaction; import org.apache.iceberg.FileScanTask; import org.apache.iceberg.IncrementalAppendScan; +import org.apache.iceberg.MetadataTableType; import org.apache.iceberg.MetadataUpdate.UpgradeFormatVersion; import org.apache.iceberg.PartitionSpec; import org.apache.iceberg.RetryableValidationException; From 94cac1ff79470e4228d63abc0cd87f9770a4400b Mon Sep 17 00:00:00 2001 From: Alex Stephen <1325798+rambleraptor@users.noreply.github.com> Date: Tue, 30 Jun 2026 15:34:06 -0700 Subject: [PATCH 4/9] Update core/src/main/java/org/apache/iceberg/rest/CatalogHandlers.java Co-authored-by: Marco Kroll --- .../main/java/org/apache/iceberg/rest/CatalogHandlers.java | 4 ++++ 1 file changed, 4 insertions(+) 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 ccd894ef399c..14ca098de0cc 100644 --- a/core/src/main/java/org/apache/iceberg/rest/CatalogHandlers.java +++ b/core/src/main/java/org/apache/iceberg/rest/CatalogHandlers.java @@ -492,6 +492,10 @@ public static void dropTable(Catalog catalog, TableIdentifier ident) { } public static UnregisterTableResponse unregisterTable(Catalog catalog, TableIdentifier ident) { + if (MetadataTableType.from(ident.name()) != null) { + 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); if (!(table instanceof BaseTable)) { From 4bdab8d29f96d9801fe98a96183574605237bf4f Mon Sep 17 00:00:00 2001 From: Alex Stephen <1325798+rambleraptor@users.noreply.github.com> Date: Tue, 30 Jun 2026 15:41:13 -0700 Subject: [PATCH 5/9] Update core/src/main/java/org/apache/iceberg/rest/CatalogHandlers.java Co-authored-by: Daniel Weeks --- core/src/main/java/org/apache/iceberg/rest/CatalogHandlers.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 14ca098de0cc..149c52665620 100644 --- a/core/src/main/java/org/apache/iceberg/rest/CatalogHandlers.java +++ b/core/src/main/java/org/apache/iceberg/rest/CatalogHandlers.java @@ -504,7 +504,7 @@ public static UnregisterTableResponse unregisterTable(Catalog catalog, TableIden TableMetadata metadata = ((BaseTable) table).operations().current(); - // unregister without removing the underlying data and metadata files + // catalog implementations should preserve the table's metadata/data files boolean dropped = catalog.dropTable(ident, false /* do not purge */); if (!dropped) { throw new NoSuchTableException("Table does not exist: %s", ident); From bb4dbd7e48f4bc3ec4cfdeb3d0f3a2873fc5c037 Mon Sep 17 00:00:00 2001 From: Alex Stephen Date: Tue, 30 Jun 2026 22:54:52 +0000 Subject: [PATCH 6/9] PR comments --- .../java/org/apache/iceberg/rest/CatalogHandlers.java | 3 ++- .../main/java/org/apache/iceberg/rest/RESTCatalog.java | 8 +++++--- .../org/apache/iceberg/rest/RESTSessionCatalog.java | 10 +++++----- .../rest/responses/UnregisterTableResponse.java | 4 +++- .../iceberg/rest/RESTCompatibilityKitCatalogTests.java | 7 ++++--- 5 files changed, 19 insertions(+), 13 deletions(-) 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 149c52665620..33e58c95045f 100644 --- a/core/src/main/java/org/apache/iceberg/rest/CatalogHandlers.java +++ b/core/src/main/java/org/apache/iceberg/rest/CatalogHandlers.java @@ -507,7 +507,8 @@ public static UnregisterTableResponse unregisterTable(Catalog catalog, TableIden // catalog implementations should preserve the table's metadata/data files boolean dropped = catalog.dropTable(ident, false /* do not purge */); if (!dropped) { - throw new NoSuchTableException("Table does not exist: %s", ident); + throw new IllegalStateException( + String.format("Unregister failed. Table not dropped: %s", ident)); } return ImmutableUnregisterTableResponse.builder() 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 552fffc1b3cd..1fdfa1678770 100644 --- a/core/src/main/java/org/apache/iceberg/rest/RESTCatalog.java +++ b/core/src/main/java/org/apache/iceberg/rest/RESTCatalog.java @@ -28,6 +28,7 @@ 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.LoadContext; @@ -256,12 +257,13 @@ public Table registerTable( * Unregister a table from the catalog. * *

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. + * 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 location for the unregistered table + * @return the last metadata for the unregistered table */ - public String unregisterTable(TableIdentifier ident) { + public TableMetadata unregisterTable(TableIdentifier ident) { return sessionCatalog.unregisterTable(context, ident); } 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 36c6130ccc44..b6bb315bb82a 100644 --- a/core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java +++ b/core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java @@ -810,14 +810,14 @@ public Table registerTable( * Unregister a table from the catalog without removing its data or metadata files. * *

This is the opposite of {@link #registerTable(SessionContext, TableIdentifier, String)}. On - * success, the table no longer exists in the catalog and the returned metadata location can be - * used to register the table in another catalog. + * success, the table no longer exists in the catalog and the returned metadata can be used to + * register the table in another catalog. * * @param context session context * @param identifier a table identifier - * @return the last metadata location for the unregistered table + * @return the last metadata for the unregistered table */ - public String unregisterTable(SessionContext context, TableIdentifier identifier) { + public TableMetadata unregisterTable(SessionContext context, TableIdentifier identifier) { Endpoint.check(endpoints, Endpoint.V1_UNREGISTER_TABLE); checkIdentifierIsValid(identifier); @@ -832,7 +832,7 @@ public String unregisterTable(SessionContext context, TableIdentifier identifier UnregisterTableResponse.class, mutationHeaders, ErrorHandlers.tableErrorHandler()); - return response.metadataLocation(); + return response.metadata(); } finally { invalidateTable(context, identifier); } diff --git a/core/src/main/java/org/apache/iceberg/rest/responses/UnregisterTableResponse.java b/core/src/main/java/org/apache/iceberg/rest/responses/UnregisterTableResponse.java index 336d361b5327..a555a6996275 100644 --- a/core/src/main/java/org/apache/iceberg/rest/responses/UnregisterTableResponse.java +++ b/core/src/main/java/org/apache/iceberg/rest/responses/UnregisterTableResponse.java @@ -19,6 +19,7 @@ package org.apache.iceberg.rest.responses; import org.apache.iceberg.TableMetadata; +import org.apache.iceberg.relocated.com.google.common.base.Preconditions; import org.apache.iceberg.rest.RESTResponse; import org.immutables.value.Value; @@ -36,6 +37,7 @@ public interface UnregisterTableResponse extends RESTResponse { @Override default void validate() { - // nothing to validate as it's not possible to create an invalid instance + Preconditions.checkArgument(metadataLocation() != null, "Invalid metadata location: null"); + Preconditions.checkArgument(metadata() != null, "Invalid metadata: null"); } } 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 ec093a7b8f1a..c60a6552c1e0 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,6 +23,7 @@ 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; @@ -133,15 +134,15 @@ public void testUnregisterTable() { original.newFastAppend().appendFile(FILE_A).commit(); original.newFastAppend().appendFile(FILE_B).commit(); - String metadataLocation = restCatalog.unregisterTable(TABLE); + TableMetadata metadata = restCatalog.unregisterTable(TABLE); - assertThat(metadataLocation).as("Returned metadata location must not be null").isNotNull(); + assertThat(metadata).as("Returned metadata must not be null").isNotNull(); assertThat(restCatalog.tableExists(TABLE)) .as("Table must not exist after being unregistered") .isFalse(); // the underlying files are left in place, so the table can be registered again - Table registered = restCatalog.registerTable(TABLE, metadataLocation); + Table registered = restCatalog.registerTable(TABLE, metadata.metadataFileLocation()); assertThat(registered.currentSnapshot()) .as("Current snapshot must match the unregistered table") .isEqualTo(original.currentSnapshot()); From feea383885077553fcccd2bbd356e34abd11a650 Mon Sep 17 00:00:00 2001 From: Marco Kroll Date: Fri, 18 Sep 2026 07:00:50 +0000 Subject: [PATCH 7/9] Address feedback and implement unregister in catalog --- .../main/java/org/apache/iceberg/Table.java | 10 +++ .../org/apache/iceberg/catalog/Catalog.java | 15 ++++ .../iceberg/catalog/SessionCatalog.java | 15 ++++ .../java/org/apache/iceberg/BaseTable.java | 5 ++ .../org/apache/iceberg/CachingCatalog.java | 7 ++ .../org/apache/iceberg/SerializableTable.java | 1 + .../iceberg/catalog/BaseSessionCatalog.java | 5 ++ .../iceberg/inmemory/InMemoryCatalog.java | 24 +++++++ .../org/apache/iceberg/jdbc/JdbcCatalog.java | 69 +++++++++++++++++++ .../org/apache/iceberg/jdbc/JdbcUtil.java | 4 ++ .../apache/iceberg/rest/CatalogHandlers.java | 12 +--- .../org/apache/iceberg/rest/RESTCatalog.java | 16 +---- .../iceberg/rest/RESTSessionCatalog.java | 13 ++-- .../apache/iceberg/catalog/CatalogTests.java | 44 ++++++++++++ .../iceberg/inmemory/TestInMemoryCatalog.java | 5 ++ .../apache/iceberg/jdbc/TestJdbcCatalog.java | 63 +++++++++++++++++ .../RESTCompatibilityKitCatalogTests.java | 18 +++-- 17 files changed, 294 insertions(+), 32 deletions(-) diff --git a/api/src/main/java/org/apache/iceberg/Table.java b/api/src/main/java/org/apache/iceberg/Table.java index 3c0689e89288..2ff2784de90d 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 @@ default UpdatePartitionStatistics updatePartitionStatistics() { /** 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 fddae97125f5..231bc57087aa 100644 --- a/api/src/main/java/org/apache/iceberg/catalog/Catalog.java +++ b/api/src/main/java/org/apache/iceberg/catalog/Catalog.java @@ -382,6 +382,21 @@ default Table registerTable( throw new UnsupportedOperationException("Registering tables with overwrite is not supported"); } + /** + * Unregister a table without deleting its data or metadata files. + * + *

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 a3e269613591..a9132e1d6136 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 @@ default Table registerTable( throw new UnsupportedOperationException("Registering tables with overwrite is not supported"); } + /** + * Unregister a table without deleting its data or metadata files. + * + *

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 a98d4ef1afe8..a73136b97f58 100644 --- a/core/src/main/java/org/apache/iceberg/BaseTable.java +++ b/core/src/main/java/org/apache/iceberg/BaseTable.java @@ -81,6 +81,11 @@ public TableOperations operations() { 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 700cfc6cb582..4ee6d61c0910 100644 --- a/core/src/main/java/org/apache/iceberg/CachingCatalog.java +++ b/core/src/main/java/org/apache/iceberg/CachingCatalog.java @@ -204,6 +204,13 @@ public Table registerTable( return table; } + @Override + public Table unregisterTable(TableIdentifier identifier) { + Table table = catalog.unregisterTable(identifier); + invalidateTable(identifier); + return table; + } + private Iterable metadataTableIdentifiers(TableIdentifier ident) { ImmutableList.Builder 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 5b4cd0e55396..94fe3153e332 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 static Table copyOf(Table table) { } } + @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 69e0990cbf3b..bf8bce4e4da2 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 Table registerTable( 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 80297310e82f..71917a4a52fe 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.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; @@ -125,6 +128,27 @@ private String defaultNamespaceLocation(Namespace namespace) { } } + @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 2d24e5598ac7..e7288170b4e6 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.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.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.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 @@ protected String defaultWarehouseLocation(TableIdentifier table) { 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 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 259bd7812555..528eccdf16c9 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 @@ enum SchemaVersion { + " 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 @@ enum SchemaVersion { + " = ? 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 33e58c95045f..d9a7e406adc9 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 static UnregisterTableResponse unregisterTable(Catalog catalog, TableIden 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 1fdfa1678770..dca43aa217cd 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.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.LoadContext; @@ -253,18 +252,9 @@ public Table registerTable( return delegate.registerTable(ident, metadataFileLocation, overwrite); } - /** - * Unregister a table from the catalog. - * - *

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 b6bb315bb82a..16a4f4239560 100644 --- a/core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java +++ b/core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java @@ -43,6 +43,7 @@ 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; @@ -810,14 +811,15 @@ public Table registerTable( * Unregister a table from the catalog without removing its data or metadata files. * *

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); @@ -832,7 +834,8 @@ public TableMetadata unregisterTable(SessionContext context, TableIdentifier ide 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 ff40527ebedd..afc4d44e245f 100644 --- a/core/src/test/java/org/apache/iceberg/catalog/CatalogTests.java +++ b/core/src/test/java/org/apache/iceberg/catalog/CatalogTests.java @@ -208,6 +208,10 @@ protected boolean supportsVariant() { return false; } + protected boolean supportsUnregister() { + return false; + } + protected String baseTableLocation(TableIdentifier identifier) { return BASE_TABLE_LOCATION + "/" + identifier.namespace() + "/" + identifier.name(); } @@ -3454,6 +3458,46 @@ public void testRegisterExistingTable() { 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 827450d4a398..9cd361aa2638 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 @@ protected boolean supportsNestedNamespaces() { 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 ce22c8089bc3..674fccb90302 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.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 @@ protected boolean supportsEmptyNamespace() { return true; } + @Override + protected boolean supportsUnregister() { + return true; + } + @Override protected boolean supportsNamesWithDot() { // namespaces with a dot are not supported @@ -594,6 +600,63 @@ public void testConcurrentCommit() throws IOException { "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 c60a6552c1e0..989273c53430 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 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; @@ -119,6 +118,11 @@ protected boolean supportsVariant() { restCatalog.properties(), RESTCompatibilityKitSuite.RCK_SUPPORTS_VARIANT, false); } + @Override + protected boolean supportsUnregister() { + return true; + } + @Test public void testUnregisterTable() { if (requiresNamespaceCreate()) { @@ -134,15 +138,21 @@ public void testUnregisterTable() { 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()); From a86231e6bf40885fe42fd1b99ca563464544f848 Mon Sep 17 00:00:00 2001 From: Marco Kroll Date: Wed, 23 Sep 2026 08:47:47 +0000 Subject: [PATCH 8/9] Address implementation feedback --- api/src/main/java/org/apache/iceberg/Table.java | 10 ---------- core/src/main/java/org/apache/iceberg/BaseTable.java | 5 ----- .../java/org/apache/iceberg/SerializableTable.java | 1 - .../org/apache/iceberg/inmemory/InMemoryCatalog.java | 2 +- .../main/java/org/apache/iceberg/jdbc/JdbcCatalog.java | 2 +- .../java/org/apache/iceberg/rest/CatalogHandlers.java | 2 +- .../org/apache/iceberg/rest/RESTSessionCatalog.java | 2 +- .../java/org/apache/iceberg/rest/ResourcePaths.java | 2 +- .../java/org/apache/iceberg/catalog/CatalogTests.java | 5 +++-- .../java/org/apache/iceberg/jdbc/TestJdbcCatalog.java | 6 ++++-- .../iceberg/rest/RESTCompatibilityKitCatalogTests.java | 4 +++- 11 files changed, 15 insertions(+), 26 deletions(-) diff --git a/api/src/main/java/org/apache/iceberg/Table.java b/api/src/main/java/org/apache/iceberg/Table.java index 2ff2784de90d..3c0689e89288 100644 --- a/api/src/main/java/org/apache/iceberg/Table.java +++ b/api/src/main/java/org/apache/iceberg/Table.java @@ -335,16 +335,6 @@ default UpdatePartitionStatistics updatePartitionStatistics() { /** 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/core/src/main/java/org/apache/iceberg/BaseTable.java b/core/src/main/java/org/apache/iceberg/BaseTable.java index a73136b97f58..a98d4ef1afe8 100644 --- a/core/src/main/java/org/apache/iceberg/BaseTable.java +++ b/core/src/main/java/org/apache/iceberg/BaseTable.java @@ -81,11 +81,6 @@ public TableOperations operations() { 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/SerializableTable.java b/core/src/main/java/org/apache/iceberg/SerializableTable.java index 94fe3153e332..5b4cd0e55396 100644 --- a/core/src/main/java/org/apache/iceberg/SerializableTable.java +++ b/core/src/main/java/org/apache/iceberg/SerializableTable.java @@ -111,7 +111,6 @@ public static Table copyOf(Table table) { } } - @Override public String metadataFileLocation() { if (metadataFileLocation == null) { throw new UnsupportedOperationException( 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 71917a4a52fe..18b205ec249a 100644 --- a/core/src/main/java/org/apache/iceberg/inmemory/InMemoryCatalog.java +++ b/core/src/main/java/org/apache/iceberg/inmemory/InMemoryCatalog.java @@ -146,7 +146,7 @@ public Table unregisterTable(TableIdentifier tableIdentifier) { StaticTableOperations staticOps = new StaticTableOperations(metadata, ops.io(), ops.locationProvider()); - return new BaseTable(staticOps, fullTableName(name(), tableIdentifier), metricsReporter()); + return new BaseTable(staticOps, tableIdentifier.name(), metricsReporter()); } @Override 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 e7288170b4e6..24d7c56f857f 100644 --- a/core/src/main/java/org/apache/iceberg/jdbc/JdbcCatalog.java +++ b/core/src/main/java/org/apache/iceberg/jdbc/JdbcCatalog.java @@ -352,7 +352,7 @@ private Table unregisterTableOnce(TableIdentifier identifier) { StaticTableOperations staticOps = new StaticTableOperations(metadata, ops.io(), ops.locationProvider()); - return new BaseTable(staticOps, fullTableName(name(), identifier), metricsReporter()); + return new BaseTable(staticOps, identifier.name(), metricsReporter()); } int dropTableIfMetadataMatches(TableIdentifier identifier, String metadataLocation) { 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 d9a7e406adc9..8d738874cd98 100644 --- a/core/src/main/java/org/apache/iceberg/rest/CatalogHandlers.java +++ b/core/src/main/java/org/apache/iceberg/rest/CatalogHandlers.java @@ -504,7 +504,7 @@ public static UnregisterTableResponse unregisterTable(Catalog catalog, TableIden TableMetadata metadata = ((BaseTable) table).operations().current(); return ImmutableUnregisterTableResponse.builder() - .metadataLocation(table.metadataFileLocation()) + .metadataLocation(metadata.metadataFileLocation()) .metadata(metadata) .build(); } 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 16a4f4239560..03302ce687e2 100644 --- a/core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java +++ b/core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java @@ -835,7 +835,7 @@ public Table unregisterTable(SessionContext context, TableIdentifier identifier) mutationHeaders, ErrorHandlers.tableErrorHandler()); StaticTableOperations ops = new StaticTableOperations(response.metadata(), io); - return new BaseTable(ops, fullTableName(identifier)); + return new BaseTable(ops, identifier.name()); } finally { invalidateTable(context, identifier); } diff --git a/core/src/main/java/org/apache/iceberg/rest/ResourcePaths.java b/core/src/main/java/org/apache/iceberg/rest/ResourcePaths.java index 63637e75da2b..970e29e0eb8b 100644 --- a/core/src/main/java/org/apache/iceberg/rest/ResourcePaths.java +++ b/core/src/main/java/org/apache/iceberg/rest/ResourcePaths.java @@ -117,7 +117,7 @@ public String unregister(TableIdentifier ident) { "namespaces", pathEncode(ident.namespace()), "tables", - RESTUtil.encodeString(ident.name()), + RESTUtil.encodePathSegment(ident.name()), "unregister"); } 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 afc4d44e245f..9e6ad888c601 100644 --- a/core/src/test/java/org/apache/iceberg/catalog/CatalogTests.java +++ b/core/src/test/java/org/apache/iceberg/catalog/CatalogTests.java @@ -3469,11 +3469,12 @@ public void unregisterTable() { Table original = catalog.buildTable(TABLE, SCHEMA).withPartitionSpec(SPEC).create(); original.newFastAppend().appendFile(FILE_A).commit(); - String metadataLocation = original.metadataFileLocation(); + String metadataLocation = TableUtil.metadataFileLocation(original); Table unregistered = catalog.unregisterTable(TABLE); - assertThat(unregistered.metadataFileLocation()).isEqualTo(metadataLocation); + assertThat(unregistered.name()).isEqualTo(TABLE.name()); + assertThat(TableUtil.metadataFileLocation(unregistered)).isEqualTo(metadataLocation); assertThat(unregistered.currentSnapshot()).isEqualTo(original.currentSnapshot()); assertThat(catalog.tableExists(TABLE)).isFalse(); assertThat(unregistered.io().newInputFile(metadataLocation).exists()).isTrue(); 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 674fccb90302..0ca2a8d9f4a8 100644 --- a/core/src/test/java/org/apache/iceberg/jdbc/TestJdbcCatalog.java +++ b/core/src/test/java/org/apache/iceberg/jdbc/TestJdbcCatalog.java @@ -56,6 +56,7 @@ import org.apache.iceberg.TableMetadata; import org.apache.iceberg.TableOperations; import org.apache.iceberg.TableProperties; +import org.apache.iceberg.TableUtil; import org.apache.iceberg.Transaction; import org.apache.iceberg.catalog.CatalogTests; import org.apache.iceberg.catalog.Namespace; @@ -613,7 +614,7 @@ public void testUnregisterRetriesAfterConcurrentCommit() { if (attempts[0]++ == 0) { Table concurrentTable = catalog.loadTable(tableIdentifier); concurrentTable.updateProperties().set("concurrent", "commit").commit(); - committedMetadataLocation[0] = concurrentTable.metadataFileLocation(); + committedMetadataLocation[0] = TableUtil.metadataFileLocation(concurrentTable); } return invocation.callRealMethod(); @@ -624,7 +625,8 @@ public void testUnregisterRetriesAfterConcurrentCommit() { Table unregistered = unregisteringCatalog.unregisterTable(tableIdentifier); assertThat(attempts[0]).isEqualTo(2); - assertThat(unregistered.metadataFileLocation()).isEqualTo(committedMetadataLocation[0]); + assertThat(TableUtil.metadataFileLocation(unregistered)) + .isEqualTo(committedMetadataLocation[0]); assertThat(unregistered.properties()).containsEntry("concurrent", "commit"); assertThat(catalog.tableExists(tableIdentifier)).isFalse(); } 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 989273c53430..83343c4e42d0 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,6 +23,7 @@ import java.util.Map; import org.apache.iceberg.Table; +import org.apache.iceberg.TableUtil; import org.apache.iceberg.catalog.CatalogTests; import org.apache.iceberg.exceptions.NoSuchTableException; import org.apache.iceberg.util.PropertyUtil; @@ -152,7 +153,8 @@ public void testUnregisterTable() { .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, unregistered.metadataFileLocation()); + Table registered = + restCatalog.registerTable(TABLE, TableUtil.metadataFileLocation(unregistered)); assertThat(registered.currentSnapshot()) .as("Current snapshot must match the unregistered table") .isEqualTo(original.currentSnapshot()); From 40c08461c3c768a2d5cdaed6660831076b49596f Mon Sep 17 00:00:00 2001 From: Marco Kroll Date: Thu, 24 Sep 2026 15:03:48 +0000 Subject: [PATCH 9/9] Address test feedback --- .../apache/iceberg/jdbc/TestJdbcCatalog.java | 60 ---------------- .../TestUnregisterTableResponseParser.java | 71 +++---------------- 2 files changed, 10 insertions(+), 121 deletions(-) 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 0ca2a8d9f4a8..225e6da20a9e 100644 --- a/core/src/test/java/org/apache/iceberg/jdbc/TestJdbcCatalog.java +++ b/core/src/test/java/org/apache/iceberg/jdbc/TestJdbcCatalog.java @@ -56,13 +56,11 @@ import org.apache.iceberg.TableMetadata; import org.apache.iceberg.TableOperations; import org.apache.iceberg.TableProperties; -import org.apache.iceberg.TableUtil; import org.apache.iceberg.Transaction; 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; @@ -601,64 +599,6 @@ public void testConcurrentCommit() throws IOException { "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] = TableUtil.metadataFileLocation(concurrentTable); - } - - return invocation.callRealMethod(); - }) - .when(unregisteringCatalog) - .dropTableIfMetadataMatches(any(TableIdentifier.class), any(String.class)); - - Table unregistered = unregisteringCatalog.unregisterTable(tableIdentifier); - - assertThat(attempts[0]).isEqualTo(2); - assertThat(TableUtil.metadataFileLocation(unregistered)) - .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/core/src/test/java/org/apache/iceberg/rest/responses/TestUnregisterTableResponseParser.java b/core/src/test/java/org/apache/iceberg/rest/responses/TestUnregisterTableResponseParser.java index af1a0ed36d72..997be87e1de2 100644 --- a/core/src/test/java/org/apache/iceberg/rest/responses/TestUnregisterTableResponseParser.java +++ b/core/src/test/java/org/apache/iceberg/rest/responses/TestUnregisterTableResponseParser.java @@ -26,6 +26,7 @@ import org.apache.iceberg.Schema; import org.apache.iceberg.SortOrder; import org.apache.iceberg.TableMetadata; +import org.apache.iceberg.TableMetadataParser; import org.apache.iceberg.types.Types; import org.junit.jupiter.api.Test; @@ -68,76 +69,24 @@ public void roundTripSerde() { .addPartitionSpec(PartitionSpec.unpartitioned()) .addSortOrder(SortOrder.unsorted()) .discardChanges() - .withMetadataLocation("metadata-location") + .withMetadataLocation("metadataTestLocation") .build(); UnregisterTableResponse response = ImmutableUnregisterTableResponse.builder() - .metadataLocation("metadata-location") + .metadataLocation("metadataTestLocation") .metadata(metadata) .build(); String expectedJson = String.format( - "{\n" - + " \"metadata-location\" : \"metadata-location\",\n" - + " \"metadata\" : {\n" - + " \"format-version\" : 1,\n" - + " \"table-uuid\" : \"386b9f01-002b-4d8c-b77f-42c3fd3b7c9b\",\n" - + " \"location\" : \"location\",\n" - + " \"last-updated-ms\" : %s,\n" - + " \"last-column-id\" : 1,\n" - + " \"schema\" : {\n" - + " \"type\" : \"struct\",\n" - + " \"schema-id\" : 0,\n" - + " \"fields\" : [ {\n" - + " \"id\" : 1,\n" - + " \"name\" : \"x\",\n" - + " \"required\" : true,\n" - + " \"type\" : \"long\"\n" - + " } ]\n" - + " },\n" - + " \"current-schema-id\" : 0,\n" - + " \"schemas\" : [ {\n" - + " \"type\" : \"struct\",\n" - + " \"schema-id\" : 0,\n" - + " \"fields\" : [ {\n" - + " \"id\" : 1,\n" - + " \"name\" : \"x\",\n" - + " \"required\" : true,\n" - + " \"type\" : \"long\"\n" - + " } ]\n" - + " } ],\n" - + " \"partition-spec\" : [ ],\n" - + " \"default-spec-id\" : 0,\n" - + " \"partition-specs\" : [ {\n" - + " \"spec-id\" : 0,\n" - + " \"fields\" : [ ]\n" - + " } ],\n" - + " \"last-partition-id\" : 999,\n" - + " \"default-sort-order-id\" : 0,\n" - + " \"sort-orders\" : [ {\n" - + " \"order-id\" : 0,\n" - + " \"fields\" : [ ]\n" - + " } ],\n" - + " \"properties\" : { },\n" - + " \"current-snapshot-id\" : -1,\n" - + " \"refs\" : { },\n" - + " \"snapshots\" : [ ],\n" - + " \"statistics\" : [ ],\n" - + " \"partition-statistics\" : [ ],\n" - + " \"snapshot-log\" : [ ],\n" - + " \"metadata-log\" : [ ]\n" - + " }\n" - + "}", - metadata.lastUpdatedMillis()); + "{\"metadata-location\":\"metadataTestLocation\",\"metadata\":%s}", + TableMetadataParser.toJson(metadata)); + String actualJson = UnregisterTableResponseParser.toJson(response); + assertThat(actualJson).isEqualTo(expectedJson); - String json = UnregisterTableResponseParser.toJson(response, true); - assertThat(json).isEqualTo(expectedJson); - // can't do an equality comparison because Schema doesn't implement equals/hashCode - assertThat( - UnregisterTableResponseParser.toJson( - UnregisterTableResponseParser.fromJson(json), true)) - .isEqualTo(expectedJson); + UnregisterTableResponse parsed = UnregisterTableResponseParser.fromJson(actualJson); + assertThat(parsed.metadata().metadataFileLocation()).isEqualTo("metadataTestLocation"); + assertThat(UnregisterTableResponseParser.toJson(parsed)).isEqualTo(expectedJson); } }