From ba402dc849e54c6524c64907786493eb69fa563f Mon Sep 17 00:00:00 2001 From: Eric Maynard Date: Fri, 18 Apr 2025 12:17:34 -0700 Subject: [PATCH 01/19] initial commit --- .../catalog/iceberg/IcebergCatalog.java | 8 +- .../iceberg/IcebergCatalogHandler.java | 76 +++++++++++++++++-- 2 files changed, 77 insertions(+), 7 deletions(-) diff --git a/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java b/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java index f80d4077ab..a1e0e467e4 100644 --- a/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java +++ b/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java @@ -169,8 +169,10 @@ public class IcebergCatalog extends BaseMetastoreViewCatalog private Map tableDefaultProperties; private FileIOFactory fileIOFactory; private PolarisMetaStoreManager metaStoreManager; + private TableIdentifier identifier; + private PolarisEntity entity; - /** + /** * @param entityManager provides handle to underlying PolarisMetaStoreManager with which to * perform mutations on entities. * @param callContext the current CallContext @@ -1768,7 +1770,9 @@ private void renameTableLike( * method will fill in the parentId if needed upon resolution. */ private void createTableLike(TableIdentifier identifier, PolarisEntity entity) { - PolarisResolvedPathWrapper resolvedParent = + this.identifier = identifier; + this.entity = entity; + PolarisResolvedPathWrapper resolvedParent = resolvedEntityView.getResolvedPath(identifier.namespace()); if (resolvedParent == null) { // Illegal state because the namespace should've already been in the static resolution set. diff --git a/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java b/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java index 716d68fdbc..63d6ef8da0 100644 --- a/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java +++ b/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java @@ -20,8 +20,10 @@ import com.google.common.base.Preconditions; import com.google.common.collect.Maps; +import jakarta.annotation.Nonnull; import jakarta.ws.rs.core.SecurityContext; import java.io.Closeable; +import java.lang.reflect.Field; import java.time.OffsetDateTime; import java.time.ZoneOffset; import java.util.ArrayList; @@ -36,6 +38,8 @@ import org.apache.iceberg.BaseTable; import org.apache.iceberg.MetadataUpdate; import org.apache.iceberg.PartitionSpec; +import org.apache.iceberg.Snapshot; +import org.apache.iceberg.SnapshotRef; import org.apache.iceberg.SortOrder; import org.apache.iceberg.Table; import org.apache.iceberg.TableMetadata; @@ -117,6 +121,8 @@ public class IcebergCatalogHandler extends CatalogHandler implements AutoCloseable { private static final Logger LOGGER = LoggerFactory.getLogger(IcebergCatalogHandler.class); + private static final Field tableMetadataSnapshotsField; + private final PolarisMetaStoreManager metaStoreManager; private final UserSecretsManager userSecretsManager; private final CallContextCatalogFactory catalogFactory; @@ -127,6 +133,21 @@ public class IcebergCatalogHandler extends CatalogHandler implements AutoCloseab protected SupportsNamespaces namespaceCatalog = null; protected ViewCatalog viewCatalog = null; + public static final String SNAPSHOTS_ALL = "all"; + public static final String SNAPSHOTS_REFS = "refs"; + + static { + Field snapshotsField; + try { + snapshotsField = TableMetadata.class.getField("snapshots"); + snapshotsField.setAccessible(true); + } catch (NoSuchFieldException e) { + LOGGER.error("Could not load snapshots field for snapshot filtering", e); + snapshotsField = null; + } + tableMetadataSnapshotsField = snapshotsField; + } + public IcebergCatalogHandler( CallContext callContext, PolarisEntityManager entityManager, @@ -380,7 +401,9 @@ public LoadTableResponse createTableDirectWithWriteDelegation( Set.of( PolarisStorageActions.READ, PolarisStorageActions.WRITE, - PolarisStorageActions.LIST)) + PolarisStorageActions.LIST), + SNAPSHOTS_ALL + ) .build(); } else if (table instanceof BaseMetadataTable) { // metadata tables are loaded on the client side, return NoSuchTableException for now @@ -471,7 +494,7 @@ public LoadTableResponse createTableStagedWithWriteDelegation( TableMetadata metadata = stageTableCreateHelper(namespace, request); return buildLoadTableResponseWithDelegationCredentials( - ident, metadata, Set.of(PolarisStorageActions.ALL)) + ident, metadata, Set.of(PolarisStorageActions.ALL), SNAPSHOTS_ALL) .build(); } @@ -580,7 +603,17 @@ public Optional loadTableIfStale( } } - return Optional.of(CatalogHandlers.loadTable(baseCatalog, tableIdentifier)); + Table table = baseCatalog.loadTable(tableIdentifier); + if (table instanceof BaseMetadataTable) { + throw new NoSuchTableException("Table does not exist: %s", tableIdentifier.toString()); + } else if (!(table instanceof BaseTable)) { + throw new IllegalStateException("Cannot wrap catalog that does not produce BaseTable"); + } else { + LoadTableResponse rawResponse = LoadTableResponse.builder() + .withTableMetadata(((BaseTable) table).operations().current()) + .build(); + return Optional.of(filterResponseToSnapshots(rawResponse, snapshots)); + } } public LoadTableResponse loadTableWithAccessDelegation( @@ -679,7 +712,7 @@ public Optional loadTableWithAccessDelegationIfStale( TableMetadata tableMetadata = baseTable.operations().current(); return Optional.of( buildLoadTableResponseWithDelegationCredentials( - tableIdentifier, tableMetadata, actionsRequested) + tableIdentifier, tableMetadata, actionsRequested, snapshots) .build()); } else if (table instanceof BaseMetadataTable) { // metadata tables are loaded on the client side, return NoSuchTableException for now @@ -692,7 +725,8 @@ public Optional loadTableWithAccessDelegationIfStale( private LoadTableResponse.Builder buildLoadTableResponseWithDelegationCredentials( TableIdentifier tableIdentifier, TableMetadata tableMetadata, - Set actions) { + Set actions, + String snapshots) { LoadTableResponse.Builder responseBuilder = LoadTableResponse.builder().withTableMetadata(tableMetadata); if (baseCatalog instanceof SupportsCredentialDelegation credentialDelegation) { @@ -1010,6 +1044,38 @@ public void renameView(RenameTableRequest request) { CatalogHandlers.renameView(viewCatalog, request); } + private @Nonnull LoadTableResponse filterResponseToSnapshots( + LoadTableResponse loadTableResponse, String snapshots) { + if (snapshots.equalsIgnoreCase(SNAPSHOTS_ALL)) { + return loadTableResponse; + } else if (snapshots.equalsIgnoreCase(SNAPSHOTS_REFS)) { + if (tableMetadataSnapshotsField == null) { + // We should have already logged an error in the static block + return loadTableResponse; + } + + TableMetadata metadata = loadTableResponse.tableMetadata(); + + Set referencedSnapshotIds = metadata.refs().values().stream() + .map(SnapshotRef::snapshotId) + .collect(Collectors.toSet()); + + List filterSnapshots = metadata.snapshots().stream() + .filter(snapshot -> referencedSnapshotIds.contains(snapshot.snapshotId())) + .collect(Collectors.toList()); + + try { + tableMetadataSnapshotsField.set(metadata, filterSnapshots); + } catch (IllegalAccessException e) { + LOGGER.error("Error setting filtered snapshots", e); + } + + return loadTableResponse; + } else { + throw new IllegalArgumentException("Unrecognized snapshots: " + snapshots); + } + } + @Override public void close() throws Exception { if (baseCatalog instanceof Closeable closeable) { From f3986ae6e873567d801d49ab00da774a58e8d0a1 Mon Sep 17 00:00:00 2001 From: Eric Maynard Date: Fri, 18 Apr 2025 12:19:21 -0700 Subject: [PATCH 02/19] autolint --- .../catalog/iceberg/IcebergCatalog.java | 12 +++++----- .../iceberg/IcebergCatalogHandler.java | 24 ++++++++++--------- 2 files changed, 19 insertions(+), 17 deletions(-) diff --git a/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java b/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java index a1e0e467e4..d29a137311 100644 --- a/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java +++ b/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java @@ -169,10 +169,10 @@ public class IcebergCatalog extends BaseMetastoreViewCatalog private Map tableDefaultProperties; private FileIOFactory fileIOFactory; private PolarisMetaStoreManager metaStoreManager; - private TableIdentifier identifier; - private PolarisEntity entity; + private TableIdentifier identifier; + private PolarisEntity entity; - /** + /** * @param entityManager provides handle to underlying PolarisMetaStoreManager with which to * perform mutations on entities. * @param callContext the current CallContext @@ -1770,9 +1770,9 @@ private void renameTableLike( * method will fill in the parentId if needed upon resolution. */ private void createTableLike(TableIdentifier identifier, PolarisEntity entity) { - this.identifier = identifier; - this.entity = entity; - PolarisResolvedPathWrapper resolvedParent = + this.identifier = identifier; + this.entity = entity; + PolarisResolvedPathWrapper resolvedParent = resolvedEntityView.getResolvedPath(identifier.namespace()); if (resolvedParent == null) { // Illegal state because the namespace should've already been in the static resolution set. diff --git a/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java b/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java index 63d6ef8da0..82b4fa69b1 100644 --- a/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java +++ b/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java @@ -402,8 +402,7 @@ public LoadTableResponse createTableDirectWithWriteDelegation( PolarisStorageActions.READ, PolarisStorageActions.WRITE, PolarisStorageActions.LIST), - SNAPSHOTS_ALL - ) + SNAPSHOTS_ALL) .build(); } else if (table instanceof BaseMetadataTable) { // metadata tables are loaded on the client side, return NoSuchTableException for now @@ -609,9 +608,10 @@ public Optional loadTableIfStale( } else if (!(table instanceof BaseTable)) { throw new IllegalStateException("Cannot wrap catalog that does not produce BaseTable"); } else { - LoadTableResponse rawResponse = LoadTableResponse.builder() - .withTableMetadata(((BaseTable) table).operations().current()) - .build(); + LoadTableResponse rawResponse = + LoadTableResponse.builder() + .withTableMetadata(((BaseTable) table).operations().current()) + .build(); return Optional.of(filterResponseToSnapshots(rawResponse, snapshots)); } } @@ -1056,13 +1056,15 @@ public void renameView(RenameTableRequest request) { TableMetadata metadata = loadTableResponse.tableMetadata(); - Set referencedSnapshotIds = metadata.refs().values().stream() - .map(SnapshotRef::snapshotId) - .collect(Collectors.toSet()); + Set referencedSnapshotIds = + metadata.refs().values().stream() + .map(SnapshotRef::snapshotId) + .collect(Collectors.toSet()); - List filterSnapshots = metadata.snapshots().stream() - .filter(snapshot -> referencedSnapshotIds.contains(snapshot.snapshotId())) - .collect(Collectors.toList()); + List filterSnapshots = + metadata.snapshots().stream() + .filter(snapshot -> referencedSnapshotIds.contains(snapshot.snapshotId())) + .collect(Collectors.toList()); try { tableMetadataSnapshotsField.set(metadata, filterSnapshots); From f3e4fcef126c9edef58744abefd169f4045c154d Mon Sep 17 00:00:00 2001 From: Eric Maynard Date: Fri, 18 Apr 2025 12:20:19 -0700 Subject: [PATCH 03/19] small revert --- .../polaris/service/catalog/iceberg/IcebergCatalog.java | 4 ---- 1 file changed, 4 deletions(-) diff --git a/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java b/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java index d29a137311..f80d4077ab 100644 --- a/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java +++ b/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java @@ -169,8 +169,6 @@ public class IcebergCatalog extends BaseMetastoreViewCatalog private Map tableDefaultProperties; private FileIOFactory fileIOFactory; private PolarisMetaStoreManager metaStoreManager; - private TableIdentifier identifier; - private PolarisEntity entity; /** * @param entityManager provides handle to underlying PolarisMetaStoreManager with which to @@ -1770,8 +1768,6 @@ private void renameTableLike( * method will fill in the parentId if needed upon resolution. */ private void createTableLike(TableIdentifier identifier, PolarisEntity entity) { - this.identifier = identifier; - this.entity = entity; PolarisResolvedPathWrapper resolvedParent = resolvedEntityView.getResolvedPath(identifier.namespace()); if (resolvedParent == null) { From e1d28372951042317cd80560534e3ced96fe1050 Mon Sep 17 00:00:00 2001 From: Eric Maynard Date: Fri, 18 Apr 2025 13:27:09 -0700 Subject: [PATCH 04/19] rebase --- .../iceberg/IcebergCatalogHandler.java | 40 +++++++------------ 1 file changed, 14 insertions(+), 26 deletions(-) diff --git a/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java b/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java index 82b4fa69b1..85ffde5ae4 100644 --- a/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java +++ b/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java @@ -121,8 +121,6 @@ public class IcebergCatalogHandler extends CatalogHandler implements AutoCloseable { private static final Logger LOGGER = LoggerFactory.getLogger(IcebergCatalogHandler.class); - private static final Field tableMetadataSnapshotsField; - private final PolarisMetaStoreManager metaStoreManager; private final UserSecretsManager userSecretsManager; private final CallContextCatalogFactory catalogFactory; @@ -136,18 +134,6 @@ public class IcebergCatalogHandler extends CatalogHandler implements AutoCloseab public static final String SNAPSHOTS_ALL = "all"; public static final String SNAPSHOTS_REFS = "refs"; - static { - Field snapshotsField; - try { - snapshotsField = TableMetadata.class.getField("snapshots"); - snapshotsField.setAccessible(true); - } catch (NoSuchFieldException e) { - LOGGER.error("Could not load snapshots field for snapshot filtering", e); - snapshotsField = null; - } - tableMetadataSnapshotsField = snapshotsField; - } - public IcebergCatalogHandler( CallContext callContext, PolarisEntityManager entityManager, @@ -1049,11 +1035,6 @@ public void renameView(RenameTableRequest request) { if (snapshots.equalsIgnoreCase(SNAPSHOTS_ALL)) { return loadTableResponse; } else if (snapshots.equalsIgnoreCase(SNAPSHOTS_REFS)) { - if (tableMetadataSnapshotsField == null) { - // We should have already logged an error in the static block - return loadTableResponse; - } - TableMetadata metadata = loadTableResponse.tableMetadata(); Set referencedSnapshotIds = @@ -1061,18 +1042,25 @@ public void renameView(RenameTableRequest request) { .map(SnapshotRef::snapshotId) .collect(Collectors.toSet()); - List filterSnapshots = + List filteredSnapshots = metadata.snapshots().stream() .filter(snapshot -> referencedSnapshotIds.contains(snapshot.snapshotId())) .collect(Collectors.toList()); - try { - tableMetadataSnapshotsField.set(metadata, filterSnapshots); - } catch (IllegalAccessException e) { - LOGGER.error("Error setting filtered snapshots", e); - } + TableMetadata filteredMetadata = + TableMetadata.buildFrom(metadata) + .withMetadataLocation(loadTableResponse.metadataLocation()) + .setPreviousFileLocation(null) + .setSnapshotsSupplier( + () -> filteredSnapshots) + .discardChanges() + .build(); - return loadTableResponse; + return LoadTableResponse.builder() + .withTableMetadata(filteredMetadata) + .addAllConfig(loadTableResponse.config()) + .addAllCredentials(loadTableResponse.credentials()) + .build(); } else { throw new IllegalArgumentException("Unrecognized snapshots: " + snapshots); } From 0c60c729174c80e2837cb6853ba9a8f26785bee4 Mon Sep 17 00:00:00 2001 From: Eric Maynard Date: Fri, 18 Apr 2025 13:27:12 -0700 Subject: [PATCH 05/19] autolint --- .../service/catalog/iceberg/IcebergCatalogHandler.java | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java b/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java index 85ffde5ae4..f2fc5ba7b5 100644 --- a/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java +++ b/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java @@ -23,7 +23,6 @@ import jakarta.annotation.Nonnull; import jakarta.ws.rs.core.SecurityContext; import java.io.Closeable; -import java.lang.reflect.Field; import java.time.OffsetDateTime; import java.time.ZoneOffset; import java.util.ArrayList; @@ -1051,8 +1050,7 @@ public void renameView(RenameTableRequest request) { TableMetadata.buildFrom(metadata) .withMetadataLocation(loadTableResponse.metadataLocation()) .setPreviousFileLocation(null) - .setSnapshotsSupplier( - () -> filteredSnapshots) + .setSnapshotsSupplier(() -> filteredSnapshots) .discardChanges() .build(); From ddf8162c2acde780e01e4150a3ab63e60d1b4ea0 Mon Sep 17 00:00:00 2001 From: Eric Maynard Date: Fri, 18 Apr 2025 13:29:11 -0700 Subject: [PATCH 06/19] simpler --- .../catalog/iceberg/IcebergCatalogHandler.java | 12 +----------- 1 file changed, 1 insertion(+), 11 deletions(-) diff --git a/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java b/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java index f2fc5ba7b5..191a432c83 100644 --- a/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java +++ b/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java @@ -1041,18 +1041,8 @@ public void renameView(RenameTableRequest request) { .map(SnapshotRef::snapshotId) .collect(Collectors.toSet()); - List filteredSnapshots = - metadata.snapshots().stream() - .filter(snapshot -> referencedSnapshotIds.contains(snapshot.snapshotId())) - .collect(Collectors.toList()); - TableMetadata filteredMetadata = - TableMetadata.buildFrom(metadata) - .withMetadataLocation(loadTableResponse.metadataLocation()) - .setPreviousFileLocation(null) - .setSnapshotsSupplier(() -> filteredSnapshots) - .discardChanges() - .build(); + metadata.removeSnapshotsIf(s -> referencedSnapshotIds.contains(s.snapshotId())); return LoadTableResponse.builder() .withTableMetadata(filteredMetadata) From 357e825423857e068f72d3d5bbe97b6577e41c73 Mon Sep 17 00:00:00 2001 From: Eric Maynard Date: Fri, 18 Apr 2025 13:29:13 -0700 Subject: [PATCH 07/19] autolint --- .../polaris/service/catalog/iceberg/IcebergCatalogHandler.java | 1 - 1 file changed, 1 deletion(-) diff --git a/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java b/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java index 191a432c83..edd5ba0d3f 100644 --- a/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java +++ b/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java @@ -37,7 +37,6 @@ import org.apache.iceberg.BaseTable; import org.apache.iceberg.MetadataUpdate; import org.apache.iceberg.PartitionSpec; -import org.apache.iceberg.Snapshot; import org.apache.iceberg.SnapshotRef; import org.apache.iceberg.SortOrder; import org.apache.iceberg.Table; From 5c12912c76ebff5e3341abf1b9edbb257a3048c2 Mon Sep 17 00:00:00 2001 From: Eric Maynard Date: Fri, 18 Apr 2025 13:55:42 -0700 Subject: [PATCH 08/19] tests --- .../polaris/service/it/env/CatalogApi.java | 12 ++++ .../PolarisRestCatalogIntegrationTest.java | 66 +++++++++++++++++++ 2 files changed, 78 insertions(+) diff --git a/integration-tests/src/main/java/org/apache/polaris/service/it/env/CatalogApi.java b/integration-tests/src/main/java/org/apache/polaris/service/it/env/CatalogApi.java index 8312e488bb..ebb38870ef 100644 --- a/integration-tests/src/main/java/org/apache/polaris/service/it/env/CatalogApi.java +++ b/integration-tests/src/main/java/org/apache/polaris/service/it/env/CatalogApi.java @@ -149,6 +149,18 @@ public void dropTable(String catalog, TableIdentifier id) { } } + public int loadTable(String catalog, TableIdentifier id, String snapshots) { + String ns = RESTUtil.encodeNamespace(id.namespace()); + try (Response res = + request( + "v1/{cat}/namespaces/" + ns + "/tables/{table}", + Map.of("cat", catalog, "table", id.name())) + .header("snapshots", snapshots) + .get()) { + return res.getStatus(); + } + } + public List listViews(String catalog, Namespace namespace) { String ns = RESTUtil.encodeNamespace(namespace); try (Response res = diff --git a/integration-tests/src/main/java/org/apache/polaris/service/it/test/PolarisRestCatalogIntegrationTest.java b/integration-tests/src/main/java/org/apache/polaris/service/it/test/PolarisRestCatalogIntegrationTest.java index 33bc779541..8784d6675f 100644 --- a/integration-tests/src/main/java/org/apache/polaris/service/it/test/PolarisRestCatalogIntegrationTest.java +++ b/integration-tests/src/main/java/org/apache/polaris/service/it/test/PolarisRestCatalogIntegrationTest.java @@ -19,6 +19,8 @@ package org.apache.polaris.service.it.test; import static jakarta.ws.rs.core.Response.Status.NOT_FOUND; +import static javax.ws.rs.core.Response.Status.BAD_REQUEST; +import static javax.ws.rs.core.Response.Status.OK; import static org.apache.polaris.service.it.env.PolarisClient.polarisClient; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatThrownBy; @@ -34,6 +36,7 @@ import java.lang.reflect.Method; import java.net.URI; import java.nio.file.Path; +import java.util.ArrayList; import java.util.Arrays; import java.util.HashMap; import java.util.List; @@ -64,6 +67,7 @@ import org.apache.iceberg.rest.RESTUtil; import org.apache.iceberg.rest.requests.CreateTableRequest; import org.apache.iceberg.rest.responses.ErrorResponse; +import org.apache.iceberg.rest.responses.LoadTableResponse; import org.apache.iceberg.types.Types; import org.apache.polaris.core.admin.model.AwsStorageConfigInfo; import org.apache.polaris.core.admin.model.Catalog; @@ -1343,4 +1347,66 @@ public void testDropNonExistingGenericTable() { genericTableApi.purge(currentCatalogName, namespace); } + + @Test + public void testLoadTableWithSnapshots() { + Namespace namespace = Namespace.of("ns1"); + restCatalog.createNamespace(namespace); + TableIdentifier tableIdentifier = TableIdentifier.of(namespace, "tbl1"); + restCatalog.createTable(tableIdentifier, SCHEMA); + + assertThat(catalogApi.loadTable(currentCatalogName, tableIdentifier, "ALL")) + .isEqualTo(OK.getStatusCode()); + assertThat(catalogApi.loadTable(currentCatalogName, tableIdentifier, "all")) + .isEqualTo(OK.getStatusCode()); + assertThat(catalogApi.loadTable(currentCatalogName, tableIdentifier, "refs")) + .isEqualTo(OK.getStatusCode()); + assertThat(catalogApi.loadTable(currentCatalogName, tableIdentifier, "REFS")) + .isEqualTo(OK.getStatusCode()); + assertThat(catalogApi.loadTable(currentCatalogName, tableIdentifier, "not-real")) + .isEqualTo(BAD_REQUEST.getStatusCode()); + + catalogApi.purge(currentCatalogName, namespace); + } + + @Test + public void testLoadTableWithRefFiltering() { + Namespace namespace = Namespace.of("ns1"); + restCatalog.createNamespace(namespace); + TableIdentifier tableIdentifier = TableIdentifier.of(namespace, "tbl1"); + + restCatalog.createTable(tableIdentifier, SCHEMA); + + Table table = restCatalog.loadTable(tableIdentifier); + + table.newAppend().appendFile(FILE_A).commit(); + table.newAppend().appendFile(FILE_B).commit(); + + // Expire current snapshot to simulate orphaned commit + long orphanedSnapshotId = table.currentSnapshot().snapshotId(); + table.expireSnapshots().expireSnapshotId(orphanedSnapshotId).commit(); + + String ns = RESTUtil.encodeNamespace(tableIdentifier.namespace()); + try (Response res = + catalogApi.request( + "v1/{cat}/namespaces/" + ns + "/tables/{table}", + Map.of("cat", currentCatalogName, "table", tableIdentifier.name())) + .header("snapshots", "all") + .get()) { + LoadTableResponse responseContent = res.readEntity(LoadTableResponse.class); + assertThat(responseContent.tableMetadata().snapshots().size()).isEqualTo(2); + } + + try (Response res = + catalogApi.request( + "v1/{cat}/namespaces/" + ns + "/tables/{table}", + Map.of("cat", currentCatalogName, "table", tableIdentifier.name())) + .header("snapshots", "refs") + .get()) { + LoadTableResponse responseContent = res.readEntity(LoadTableResponse.class); + assertThat(responseContent.tableMetadata().snapshots().size()).isEqualTo(1); + } + + catalogApi.purge(currentCatalogName, namespace); + } } From 024cdacc9a3662807aac98f68d8483a81bbcb6da Mon Sep 17 00:00:00 2001 From: Eric Maynard Date: Fri, 18 Apr 2025 13:55:44 -0700 Subject: [PATCH 09/19] autolint --- .../polaris/service/it/env/CatalogApi.java | 10 ++++---- .../PolarisRestCatalogIntegrationTest.java | 23 ++++++++++--------- 2 files changed, 17 insertions(+), 16 deletions(-) diff --git a/integration-tests/src/main/java/org/apache/polaris/service/it/env/CatalogApi.java b/integration-tests/src/main/java/org/apache/polaris/service/it/env/CatalogApi.java index ebb38870ef..d086ab2e16 100644 --- a/integration-tests/src/main/java/org/apache/polaris/service/it/env/CatalogApi.java +++ b/integration-tests/src/main/java/org/apache/polaris/service/it/env/CatalogApi.java @@ -152,11 +152,11 @@ public void dropTable(String catalog, TableIdentifier id) { public int loadTable(String catalog, TableIdentifier id, String snapshots) { String ns = RESTUtil.encodeNamespace(id.namespace()); try (Response res = - request( - "v1/{cat}/namespaces/" + ns + "/tables/{table}", - Map.of("cat", catalog, "table", id.name())) - .header("snapshots", snapshots) - .get()) { + request( + "v1/{cat}/namespaces/" + ns + "/tables/{table}", + Map.of("cat", catalog, "table", id.name())) + .header("snapshots", snapshots) + .get()) { return res.getStatus(); } } diff --git a/integration-tests/src/main/java/org/apache/polaris/service/it/test/PolarisRestCatalogIntegrationTest.java b/integration-tests/src/main/java/org/apache/polaris/service/it/test/PolarisRestCatalogIntegrationTest.java index 8784d6675f..5b55c7df21 100644 --- a/integration-tests/src/main/java/org/apache/polaris/service/it/test/PolarisRestCatalogIntegrationTest.java +++ b/integration-tests/src/main/java/org/apache/polaris/service/it/test/PolarisRestCatalogIntegrationTest.java @@ -36,7 +36,6 @@ import java.lang.reflect.Method; import java.net.URI; import java.nio.file.Path; -import java.util.ArrayList; import java.util.Arrays; import java.util.HashMap; import java.util.List; @@ -1388,21 +1387,23 @@ public void testLoadTableWithRefFiltering() { String ns = RESTUtil.encodeNamespace(tableIdentifier.namespace()); try (Response res = - catalogApi.request( - "v1/{cat}/namespaces/" + ns + "/tables/{table}", - Map.of("cat", currentCatalogName, "table", tableIdentifier.name())) - .header("snapshots", "all") - .get()) { + catalogApi + .request( + "v1/{cat}/namespaces/" + ns + "/tables/{table}", + Map.of("cat", currentCatalogName, "table", tableIdentifier.name())) + .header("snapshots", "all") + .get()) { LoadTableResponse responseContent = res.readEntity(LoadTableResponse.class); assertThat(responseContent.tableMetadata().snapshots().size()).isEqualTo(2); } try (Response res = - catalogApi.request( - "v1/{cat}/namespaces/" + ns + "/tables/{table}", - Map.of("cat", currentCatalogName, "table", tableIdentifier.name())) - .header("snapshots", "refs") - .get()) { + catalogApi + .request( + "v1/{cat}/namespaces/" + ns + "/tables/{table}", + Map.of("cat", currentCatalogName, "table", tableIdentifier.name())) + .header("snapshots", "refs") + .get()) { LoadTableResponse responseContent = res.readEntity(LoadTableResponse.class); assertThat(responseContent.tableMetadata().snapshots().size()).isEqualTo(1); } From 9352aca0698eaa7035f634edc9d26c688936d445 Mon Sep 17 00:00:00 2001 From: Eric Maynard Date: Fri, 18 Apr 2025 14:20:04 -0700 Subject: [PATCH 10/19] stable --- .../polaris/service/it/env/CatalogApi.java | 4 +-- .../PolarisRestCatalogIntegrationTest.java | 32 +++++++++---------- .../iceberg/IcebergCatalogAdapter.java | 1 + .../iceberg/IcebergCatalogHandler.java | 2 +- 4 files changed, 19 insertions(+), 20 deletions(-) diff --git a/integration-tests/src/main/java/org/apache/polaris/service/it/env/CatalogApi.java b/integration-tests/src/main/java/org/apache/polaris/service/it/env/CatalogApi.java index d086ab2e16..8704e377c1 100644 --- a/integration-tests/src/main/java/org/apache/polaris/service/it/env/CatalogApi.java +++ b/integration-tests/src/main/java/org/apache/polaris/service/it/env/CatalogApi.java @@ -154,8 +154,8 @@ public int loadTable(String catalog, TableIdentifier id, String snapshots) { try (Response res = request( "v1/{cat}/namespaces/" + ns + "/tables/{table}", - Map.of("cat", catalog, "table", id.name())) - .header("snapshots", snapshots) + Map.of("cat", catalog, "table", id.name()), + Map.of("snapshots", snapshots)) .get()) { return res.getStatus(); } diff --git a/integration-tests/src/main/java/org/apache/polaris/service/it/test/PolarisRestCatalogIntegrationTest.java b/integration-tests/src/main/java/org/apache/polaris/service/it/test/PolarisRestCatalogIntegrationTest.java index 5b55c7df21..926650527b 100644 --- a/integration-tests/src/main/java/org/apache/polaris/service/it/test/PolarisRestCatalogIntegrationTest.java +++ b/integration-tests/src/main/java/org/apache/polaris/service/it/test/PolarisRestCatalogIntegrationTest.java @@ -20,7 +20,6 @@ import static jakarta.ws.rs.core.Response.Status.NOT_FOUND; import static javax.ws.rs.core.Response.Status.BAD_REQUEST; -import static javax.ws.rs.core.Response.Status.OK; import static org.apache.polaris.service.it.env.PolarisClient.polarisClient; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatThrownBy; @@ -1354,14 +1353,14 @@ public void testLoadTableWithSnapshots() { TableIdentifier tableIdentifier = TableIdentifier.of(namespace, "tbl1"); restCatalog.createTable(tableIdentifier, SCHEMA); - assertThat(catalogApi.loadTable(currentCatalogName, tableIdentifier, "ALL")) - .isEqualTo(OK.getStatusCode()); - assertThat(catalogApi.loadTable(currentCatalogName, tableIdentifier, "all")) - .isEqualTo(OK.getStatusCode()); - assertThat(catalogApi.loadTable(currentCatalogName, tableIdentifier, "refs")) - .isEqualTo(OK.getStatusCode()); - assertThat(catalogApi.loadTable(currentCatalogName, tableIdentifier, "REFS")) - .isEqualTo(OK.getStatusCode()); + // assertThat(catalogApi.loadTable(currentCatalogName, tableIdentifier, "ALL")) + // .isEqualTo(OK.getStatusCode()); + // assertThat(catalogApi.loadTable(currentCatalogName, tableIdentifier, "all")) + // .isEqualTo(OK.getStatusCode()); + // assertThat(catalogApi.loadTable(currentCatalogName, tableIdentifier, "refs")) + // .isEqualTo(OK.getStatusCode()); + // assertThat(catalogApi.loadTable(currentCatalogName, tableIdentifier, "REFS")) + // .isEqualTo(OK.getStatusCode()); assertThat(catalogApi.loadTable(currentCatalogName, tableIdentifier, "not-real")) .isEqualTo(BAD_REQUEST.getStatusCode()); @@ -1378,20 +1377,19 @@ public void testLoadTableWithRefFiltering() { Table table = restCatalog.loadTable(tableIdentifier); + // Create an orphaned snapshot: table.newAppend().appendFile(FILE_A).commit(); + long snapshotIdA = table.currentSnapshot().snapshotId(); table.newAppend().appendFile(FILE_B).commit(); - - // Expire current snapshot to simulate orphaned commit - long orphanedSnapshotId = table.currentSnapshot().snapshotId(); - table.expireSnapshots().expireSnapshotId(orphanedSnapshotId).commit(); + table.manageSnapshots().setCurrentSnapshot(snapshotIdA).commit(); String ns = RESTUtil.encodeNamespace(tableIdentifier.namespace()); try (Response res = catalogApi .request( "v1/{cat}/namespaces/" + ns + "/tables/{table}", - Map.of("cat", currentCatalogName, "table", tableIdentifier.name())) - .header("snapshots", "all") + Map.of("cat", currentCatalogName, "table", tableIdentifier.name()), + Map.of("snapshots", "all")) .get()) { LoadTableResponse responseContent = res.readEntity(LoadTableResponse.class); assertThat(responseContent.tableMetadata().snapshots().size()).isEqualTo(2); @@ -1401,8 +1399,8 @@ public void testLoadTableWithRefFiltering() { catalogApi .request( "v1/{cat}/namespaces/" + ns + "/tables/{table}", - Map.of("cat", currentCatalogName, "table", tableIdentifier.name())) - .header("snapshots", "refs") + Map.of("cat", currentCatalogName, "table", tableIdentifier.name()), + Map.of("snapshots", "refs")) .get()) { LoadTableResponse responseContent = res.readEntity(LoadTableResponse.class); assertThat(responseContent.tableMetadata().snapshots().size()).isEqualTo(1); diff --git a/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogAdapter.java b/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogAdapter.java index 6895351395..c4aaea3859 100644 --- a/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogAdapter.java +++ b/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogAdapter.java @@ -369,6 +369,7 @@ public Response loadTable( String namespace, String table, String accessDelegationMode, + String ifNoneMatchString, String snapshots, RealmContext realmContext, SecurityContext securityContext) { diff --git a/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java b/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java index edd5ba0d3f..17edb33e15 100644 --- a/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java +++ b/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java @@ -1030,7 +1030,7 @@ public void renameView(RenameTableRequest request) { private @Nonnull LoadTableResponse filterResponseToSnapshots( LoadTableResponse loadTableResponse, String snapshots) { - if (snapshots.equalsIgnoreCase(SNAPSHOTS_ALL)) { + if (snapshots == null || snapshots.equalsIgnoreCase(SNAPSHOTS_ALL)) { return loadTableResponse; } else if (snapshots.equalsIgnoreCase(SNAPSHOTS_REFS)) { TableMetadata metadata = loadTableResponse.tableMetadata(); From 1803df5a7ddbb1a911369bd2a024f76deefee91f Mon Sep 17 00:00:00 2001 From: Eric Maynard Date: Fri, 18 Apr 2025 14:24:25 -0700 Subject: [PATCH 11/19] fix leak --- .../polaris/service/catalog/iceberg/IcebergCatalogAdapter.java | 1 - 1 file changed, 1 deletion(-) diff --git a/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogAdapter.java b/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogAdapter.java index c4aaea3859..6895351395 100644 --- a/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogAdapter.java +++ b/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogAdapter.java @@ -369,7 +369,6 @@ public Response loadTable( String namespace, String table, String accessDelegationMode, - String ifNoneMatchString, String snapshots, RealmContext realmContext, SecurityContext securityContext) { From 00c34e5ce83848d19401e9a348d23b6017c123e9 Mon Sep 17 00:00:00 2001 From: Eric Maynard Date: Fri, 18 Apr 2025 14:24:53 -0700 Subject: [PATCH 12/19] ready for review --- .../test/PolarisRestCatalogIntegrationTest.java | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/integration-tests/src/main/java/org/apache/polaris/service/it/test/PolarisRestCatalogIntegrationTest.java b/integration-tests/src/main/java/org/apache/polaris/service/it/test/PolarisRestCatalogIntegrationTest.java index 926650527b..e49547901e 100644 --- a/integration-tests/src/main/java/org/apache/polaris/service/it/test/PolarisRestCatalogIntegrationTest.java +++ b/integration-tests/src/main/java/org/apache/polaris/service/it/test/PolarisRestCatalogIntegrationTest.java @@ -20,6 +20,7 @@ import static jakarta.ws.rs.core.Response.Status.NOT_FOUND; import static javax.ws.rs.core.Response.Status.BAD_REQUEST; +import static javax.ws.rs.core.Response.Status.OK; import static org.apache.polaris.service.it.env.PolarisClient.polarisClient; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatThrownBy; @@ -1353,14 +1354,14 @@ public void testLoadTableWithSnapshots() { TableIdentifier tableIdentifier = TableIdentifier.of(namespace, "tbl1"); restCatalog.createTable(tableIdentifier, SCHEMA); - // assertThat(catalogApi.loadTable(currentCatalogName, tableIdentifier, "ALL")) - // .isEqualTo(OK.getStatusCode()); - // assertThat(catalogApi.loadTable(currentCatalogName, tableIdentifier, "all")) - // .isEqualTo(OK.getStatusCode()); - // assertThat(catalogApi.loadTable(currentCatalogName, tableIdentifier, "refs")) - // .isEqualTo(OK.getStatusCode()); - // assertThat(catalogApi.loadTable(currentCatalogName, tableIdentifier, "REFS")) - // .isEqualTo(OK.getStatusCode()); + assertThat(catalogApi.loadTable(currentCatalogName, tableIdentifier, "ALL")) + .isEqualTo(OK.getStatusCode()); + assertThat(catalogApi.loadTable(currentCatalogName, tableIdentifier, "all")) + .isEqualTo(OK.getStatusCode()); + assertThat(catalogApi.loadTable(currentCatalogName, tableIdentifier, "refs")) + .isEqualTo(OK.getStatusCode()); + assertThat(catalogApi.loadTable(currentCatalogName, tableIdentifier, "REFS")) + .isEqualTo(OK.getStatusCode()); assertThat(catalogApi.loadTable(currentCatalogName, tableIdentifier, "not-real")) .isEqualTo(BAD_REQUEST.getStatusCode()); From edcf8f437dc2c5739d9904f8b724aa316c7b0ed4 Mon Sep 17 00:00:00 2001 From: Eric Maynard Date: Fri, 18 Apr 2025 22:41:26 -0700 Subject: [PATCH 13/19] improved test --- .../service/it/test/PolarisRestCatalogIntegrationTest.java | 1 + 1 file changed, 1 insertion(+) diff --git a/integration-tests/src/main/java/org/apache/polaris/service/it/test/PolarisRestCatalogIntegrationTest.java b/integration-tests/src/main/java/org/apache/polaris/service/it/test/PolarisRestCatalogIntegrationTest.java index e49547901e..7d5a9cd434 100644 --- a/integration-tests/src/main/java/org/apache/polaris/service/it/test/PolarisRestCatalogIntegrationTest.java +++ b/integration-tests/src/main/java/org/apache/polaris/service/it/test/PolarisRestCatalogIntegrationTest.java @@ -1405,6 +1405,7 @@ public void testLoadTableWithRefFiltering() { .get()) { LoadTableResponse responseContent = res.readEntity(LoadTableResponse.class); assertThat(responseContent.tableMetadata().snapshots().size()).isEqualTo(1); + assertThat(responseContent.tableMetadata().snapshots().get(0).snapshotId()).isEqualTo(snapshotIdA); } catalogApi.purge(currentCatalogName, namespace); From 6736d60ad5522e84c00538991f6107f95a874f1e Mon Sep 17 00:00:00 2001 From: Eric Maynard Date: Fri, 18 Apr 2025 22:41:39 -0700 Subject: [PATCH 14/19] autolint --- .../service/it/test/PolarisRestCatalogIntegrationTest.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/integration-tests/src/main/java/org/apache/polaris/service/it/test/PolarisRestCatalogIntegrationTest.java b/integration-tests/src/main/java/org/apache/polaris/service/it/test/PolarisRestCatalogIntegrationTest.java index 7d5a9cd434..63d25db5ad 100644 --- a/integration-tests/src/main/java/org/apache/polaris/service/it/test/PolarisRestCatalogIntegrationTest.java +++ b/integration-tests/src/main/java/org/apache/polaris/service/it/test/PolarisRestCatalogIntegrationTest.java @@ -1405,7 +1405,8 @@ public void testLoadTableWithRefFiltering() { .get()) { LoadTableResponse responseContent = res.readEntity(LoadTableResponse.class); assertThat(responseContent.tableMetadata().snapshots().size()).isEqualTo(1); - assertThat(responseContent.tableMetadata().snapshots().get(0).snapshotId()).isEqualTo(snapshotIdA); + assertThat(responseContent.tableMetadata().snapshots().get(0).snapshotId()) + .isEqualTo(snapshotIdA); } catalogApi.purge(currentCatalogName, namespace); From acefb481976b657e320bc89531ead9602d7acaa1 Mon Sep 17 00:00:00 2001 From: Eric Maynard Date: Fri, 18 Apr 2025 23:53:16 -0700 Subject: [PATCH 15/19] logic flip again --- .../polaris/service/catalog/iceberg/IcebergCatalogHandler.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java b/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java index 17edb33e15..49e5121b28 100644 --- a/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java +++ b/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java @@ -1041,7 +1041,7 @@ public void renameView(RenameTableRequest request) { .collect(Collectors.toSet()); TableMetadata filteredMetadata = - metadata.removeSnapshotsIf(s -> referencedSnapshotIds.contains(s.snapshotId())); + metadata.removeSnapshotsIf(s -> !referencedSnapshotIds.contains(s.snapshotId())); return LoadTableResponse.builder() .withTableMetadata(filteredMetadata) From 0ed31962601c2316776fb89a99abe4a2278cc82b Mon Sep 17 00:00:00 2001 From: Eric Maynard Date: Mon, 21 Apr 2025 09:52:54 -0700 Subject: [PATCH 16/19] Update service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java Co-authored-by: Alexandre Dutra --- .../catalog/iceberg/IcebergCatalogHandler.java | 14 ++------------ 1 file changed, 2 insertions(+), 12 deletions(-) diff --git a/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java b/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java index 49e5121b28..e9eb5c4f72 100644 --- a/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java +++ b/service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java @@ -586,18 +586,8 @@ public Optional loadTableIfStale( } } - Table table = baseCatalog.loadTable(tableIdentifier); - if (table instanceof BaseMetadataTable) { - throw new NoSuchTableException("Table does not exist: %s", tableIdentifier.toString()); - } else if (!(table instanceof BaseTable)) { - throw new IllegalStateException("Cannot wrap catalog that does not produce BaseTable"); - } else { - LoadTableResponse rawResponse = - LoadTableResponse.builder() - .withTableMetadata(((BaseTable) table).operations().current()) - .build(); - return Optional.of(filterResponseToSnapshots(rawResponse, snapshots)); - } + LoadTableResponse rawResponse = CatalogHandlers.loadTable(baseCatalog, tableIdentifier); + return Optional.of(filterResponseToSnapshots(rawResponse, snapshots)); } public LoadTableResponse loadTableWithAccessDelegation( From fd90a7b18859e355cbeb98af341cd7be7bee8b42 Mon Sep 17 00:00:00 2001 From: Eric Maynard Date: Mon, 21 Apr 2025 09:53:01 -0700 Subject: [PATCH 17/19] Update integration-tests/src/main/java/org/apache/polaris/service/it/env/CatalogApi.java Co-authored-by: Alexandre Dutra --- .../apache/polaris/service/it/env/CatalogApi.java | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/integration-tests/src/main/java/org/apache/polaris/service/it/env/CatalogApi.java b/integration-tests/src/main/java/org/apache/polaris/service/it/env/CatalogApi.java index 8704e377c1..9774aad849 100644 --- a/integration-tests/src/main/java/org/apache/polaris/service/it/env/CatalogApi.java +++ b/integration-tests/src/main/java/org/apache/polaris/service/it/env/CatalogApi.java @@ -149,18 +149,24 @@ public void dropTable(String catalog, TableIdentifier id) { } } - public int loadTable(String catalog, TableIdentifier id, String snapshots) { + public LoadTableResponse loadTable(String catalog, TableIdentifier id, String snapshots) { String ns = RESTUtil.encodeNamespace(id.namespace()); try (Response res = request( "v1/{cat}/namespaces/" + ns + "/tables/{table}", Map.of("cat", catalog, "table", id.name()), - Map.of("snapshots", snapshots)) + snapshots == null ? Map.of() : Map.of("snapshots", snapshots)) .get()) { - return res.getStatus(); + if (res.getStatus() == Response.Status.OK.getStatusCode()) { + return res.readEntity(LoadTableResponse.class); + } + throw new RESTException( + "Unhandled error: %s", + ((ErrorHandler) ErrorHandlers.defaultErrorHandler()) + .parseResponse(res.getStatus(), res.readEntity(String.class))); } } - + public List listViews(String catalog, Namespace namespace) { String ns = RESTUtil.encodeNamespace(namespace); try (Response res = From 8c6ab9e539896858736032a92cc6f2e0c386331e Mon Sep 17 00:00:00 2001 From: Eric Maynard Date: Mon, 21 Apr 2025 10:02:09 -0700 Subject: [PATCH 18/19] adjustments for committed suggestions --- .../polaris/service/it/env/CatalogApi.java | 4 ++ .../PolarisRestCatalogIntegrationTest.java | 56 ++++++++----------- 2 files changed, 26 insertions(+), 34 deletions(-) diff --git a/integration-tests/src/main/java/org/apache/polaris/service/it/env/CatalogApi.java b/integration-tests/src/main/java/org/apache/polaris/service/it/env/CatalogApi.java index 9774aad849..8ab52485ac 100644 --- a/integration-tests/src/main/java/org/apache/polaris/service/it/env/CatalogApi.java +++ b/integration-tests/src/main/java/org/apache/polaris/service/it/env/CatalogApi.java @@ -34,10 +34,14 @@ import java.util.Map; import org.apache.iceberg.catalog.Namespace; import org.apache.iceberg.catalog.TableIdentifier; +import org.apache.iceberg.exceptions.RESTException; +import org.apache.iceberg.rest.ErrorHandler; +import org.apache.iceberg.rest.ErrorHandlers; import org.apache.iceberg.rest.RESTUtil; import org.apache.iceberg.rest.requests.CreateNamespaceRequest; 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.OAuthTokenResponse; /** diff --git a/integration-tests/src/main/java/org/apache/polaris/service/it/test/PolarisRestCatalogIntegrationTest.java b/integration-tests/src/main/java/org/apache/polaris/service/it/test/PolarisRestCatalogIntegrationTest.java index 63d25db5ad..e3482e2ae3 100644 --- a/integration-tests/src/main/java/org/apache/polaris/service/it/test/PolarisRestCatalogIntegrationTest.java +++ b/integration-tests/src/main/java/org/apache/polaris/service/it/test/PolarisRestCatalogIntegrationTest.java @@ -23,6 +23,7 @@ import static javax.ws.rs.core.Response.Status.OK; import static org.apache.polaris.service.it.env.PolarisClient.polarisClient; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatCode; import static org.assertj.core.api.Assertions.assertThatThrownBy; import com.google.common.collect.ImmutableMap; @@ -60,6 +61,7 @@ import org.apache.iceberg.catalog.TableIdentifier; import org.apache.iceberg.exceptions.CommitFailedException; import org.apache.iceberg.exceptions.ForbiddenException; +import org.apache.iceberg.exceptions.RESTException; import org.apache.iceberg.expressions.Expressions; import org.apache.iceberg.io.ResolvingFileIO; import org.apache.iceberg.rest.RESTCatalog; @@ -1270,7 +1272,7 @@ public void testDropGenericTable() { genericTableApi.dropGenericTable(currentCatalogName, tableIdentifier); - Assertions.assertThatCode( + assertThatCode( () -> genericTableApi.getGenericTable(currentCatalogName, tableIdentifier)) .isInstanceOf(ProcessingException.class); @@ -1354,16 +1356,18 @@ public void testLoadTableWithSnapshots() { TableIdentifier tableIdentifier = TableIdentifier.of(namespace, "tbl1"); restCatalog.createTable(tableIdentifier, SCHEMA); - assertThat(catalogApi.loadTable(currentCatalogName, tableIdentifier, "ALL")) - .isEqualTo(OK.getStatusCode()); - assertThat(catalogApi.loadTable(currentCatalogName, tableIdentifier, "all")) - .isEqualTo(OK.getStatusCode()); - assertThat(catalogApi.loadTable(currentCatalogName, tableIdentifier, "refs")) - .isEqualTo(OK.getStatusCode()); - assertThat(catalogApi.loadTable(currentCatalogName, tableIdentifier, "REFS")) - .isEqualTo(OK.getStatusCode()); - assertThat(catalogApi.loadTable(currentCatalogName, tableIdentifier, "not-real")) - .isEqualTo(BAD_REQUEST.getStatusCode()); + assertThatCode(() -> catalogApi.loadTable(currentCatalogName, tableIdentifier, "ALL")) + .doesNotThrowAnyException(); + assertThatCode(() -> catalogApi.loadTable(currentCatalogName, tableIdentifier, "all")) + .doesNotThrowAnyException(); + assertThatCode(() -> catalogApi.loadTable(currentCatalogName, tableIdentifier, "refs")) + .doesNotThrowAnyException(); + assertThatCode(() -> catalogApi.loadTable(currentCatalogName, tableIdentifier, "REFS")) + .doesNotThrowAnyException(); + assertThatCode(() -> catalogApi.loadTable(currentCatalogName, tableIdentifier, "not-real")) + .isInstanceOf(RESTException.class) + .hasMessageContaining("Unrecognized snapshots") + .hasMessageContaining("code=" + BAD_REQUEST.getStatusCode()); catalogApi.purge(currentCatalogName, namespace); } @@ -1384,30 +1388,14 @@ public void testLoadTableWithRefFiltering() { table.newAppend().appendFile(FILE_B).commit(); table.manageSnapshots().setCurrentSnapshot(snapshotIdA).commit(); - String ns = RESTUtil.encodeNamespace(tableIdentifier.namespace()); - try (Response res = - catalogApi - .request( - "v1/{cat}/namespaces/" + ns + "/tables/{table}", - Map.of("cat", currentCatalogName, "table", tableIdentifier.name()), - Map.of("snapshots", "all")) - .get()) { - LoadTableResponse responseContent = res.readEntity(LoadTableResponse.class); - assertThat(responseContent.tableMetadata().snapshots().size()).isEqualTo(2); - } + var allSnapshots = + catalogApi.loadTable(currentCatalogName, tableIdentifier, "ALL").tableMetadata().snapshots(); + assertThat(allSnapshots).hasSize(2); - try (Response res = - catalogApi - .request( - "v1/{cat}/namespaces/" + ns + "/tables/{table}", - Map.of("cat", currentCatalogName, "table", tableIdentifier.name()), - Map.of("snapshots", "refs")) - .get()) { - LoadTableResponse responseContent = res.readEntity(LoadTableResponse.class); - assertThat(responseContent.tableMetadata().snapshots().size()).isEqualTo(1); - assertThat(responseContent.tableMetadata().snapshots().get(0).snapshotId()) - .isEqualTo(snapshotIdA); - } + var refsSnapshots = + catalogApi.loadTable(currentCatalogName, tableIdentifier, "REFS").tableMetadata().snapshots(); + assertThat(refsSnapshots).hasSize(1); + assertThat(refsSnapshots.getFirst().snapshotId()).isEqualTo(snapshotIdA); catalogApi.purge(currentCatalogName, namespace); } From 480d02873c39e8b089d53bdca9b24c821285ab12 Mon Sep 17 00:00:00 2001 From: Eric Maynard Date: Mon, 21 Apr 2025 10:02:20 -0700 Subject: [PATCH 19/19] autolint --- .../apache/polaris/service/it/env/CatalogApi.java | 2 +- .../test/PolarisRestCatalogIntegrationTest.java | 15 +++++++++------ 2 files changed, 10 insertions(+), 7 deletions(-) diff --git a/integration-tests/src/main/java/org/apache/polaris/service/it/env/CatalogApi.java b/integration-tests/src/main/java/org/apache/polaris/service/it/env/CatalogApi.java index 8ab52485ac..7be67f1947 100644 --- a/integration-tests/src/main/java/org/apache/polaris/service/it/env/CatalogApi.java +++ b/integration-tests/src/main/java/org/apache/polaris/service/it/env/CatalogApi.java @@ -170,7 +170,7 @@ public LoadTableResponse loadTable(String catalog, TableIdentifier id, String sn .parseResponse(res.getStatus(), res.readEntity(String.class))); } } - + public List listViews(String catalog, Namespace namespace) { String ns = RESTUtil.encodeNamespace(namespace); try (Response res = diff --git a/integration-tests/src/main/java/org/apache/polaris/service/it/test/PolarisRestCatalogIntegrationTest.java b/integration-tests/src/main/java/org/apache/polaris/service/it/test/PolarisRestCatalogIntegrationTest.java index e3482e2ae3..7b02a9baad 100644 --- a/integration-tests/src/main/java/org/apache/polaris/service/it/test/PolarisRestCatalogIntegrationTest.java +++ b/integration-tests/src/main/java/org/apache/polaris/service/it/test/PolarisRestCatalogIntegrationTest.java @@ -20,7 +20,6 @@ import static jakarta.ws.rs.core.Response.Status.NOT_FOUND; import static javax.ws.rs.core.Response.Status.BAD_REQUEST; -import static javax.ws.rs.core.Response.Status.OK; import static org.apache.polaris.service.it.env.PolarisClient.polarisClient; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatCode; @@ -68,7 +67,6 @@ import org.apache.iceberg.rest.RESTUtil; import org.apache.iceberg.rest.requests.CreateTableRequest; import org.apache.iceberg.rest.responses.ErrorResponse; -import org.apache.iceberg.rest.responses.LoadTableResponse; import org.apache.iceberg.types.Types; import org.apache.polaris.core.admin.model.AwsStorageConfigInfo; import org.apache.polaris.core.admin.model.Catalog; @@ -1272,8 +1270,7 @@ public void testDropGenericTable() { genericTableApi.dropGenericTable(currentCatalogName, tableIdentifier); - assertThatCode( - () -> genericTableApi.getGenericTable(currentCatalogName, tableIdentifier)) + assertThatCode(() -> genericTableApi.getGenericTable(currentCatalogName, tableIdentifier)) .isInstanceOf(ProcessingException.class); genericTableApi.purge(currentCatalogName, namespace); @@ -1389,11 +1386,17 @@ public void testLoadTableWithRefFiltering() { table.manageSnapshots().setCurrentSnapshot(snapshotIdA).commit(); var allSnapshots = - catalogApi.loadTable(currentCatalogName, tableIdentifier, "ALL").tableMetadata().snapshots(); + catalogApi + .loadTable(currentCatalogName, tableIdentifier, "ALL") + .tableMetadata() + .snapshots(); assertThat(allSnapshots).hasSize(2); var refsSnapshots = - catalogApi.loadTable(currentCatalogName, tableIdentifier, "REFS").tableMetadata().snapshots(); + catalogApi + .loadTable(currentCatalogName, tableIdentifier, "REFS") + .tableMetadata() + .snapshots(); assertThat(refsSnapshots).hasSize(1); assertThat(refsSnapshots.getFirst().snapshotId()).isEqualTo(snapshotIdA);