From e9c348a532a7ee1d8736f6d642ce4844d895ee37 Mon Sep 17 00:00:00 2001 From: Liam Bao Date: Tue, 1 Apr 2025 20:22:32 -0400 Subject: [PATCH 1/6] Unblock test `listNamespacesWithEmptyNamespace` --- .../quarkus/catalog/IcebergCatalogTest.java | 29 ++++++++++++------- 1 file changed, 19 insertions(+), 10 deletions(-) diff --git a/quarkus/service/src/test/java/org/apache/polaris/service/quarkus/catalog/IcebergCatalogTest.java b/quarkus/service/src/test/java/org/apache/polaris/service/quarkus/catalog/IcebergCatalogTest.java index 4a5506fa2..f517381f3 100644 --- a/quarkus/service/src/test/java/org/apache/polaris/service/quarkus/catalog/IcebergCatalogTest.java +++ b/quarkus/service/src/test/java/org/apache/polaris/service/quarkus/catalog/IcebergCatalogTest.java @@ -127,13 +127,13 @@ import org.junit.jupiter.api.Assumptions; import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.BeforeEach; -import org.junit.jupiter.api.Disabled; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.TestInfo; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.Arguments; import org.junit.jupiter.params.provider.MethodSource; import org.mockito.Mockito; +import org.opentest4j.AssertionFailedError; import software.amazon.awssdk.core.exception.NonRetryableException; import software.amazon.awssdk.core.exception.RetryableException; import software.amazon.awssdk.services.sts.StsClient; @@ -386,18 +386,27 @@ public Map purgeRealms(Iterable realms) { }; } - /** TODO: Unblock this test, see: https://github.com/apache/polaris/issues/1272 */ @Override @Test - @Disabled( - """ - Disabled because the behavior is not applicable to Polaris. - To unblock: - 1) Align Polaris behavior with the superclass by handling empty namespaces the same way, or - 2) Modify this test to expect an exception and add a Polaris-specific version. - """) public void listNamespacesWithEmptyNamespace() { - super.listNamespacesWithEmptyNamespace(); + // In Polaris, the empty namespace implicitly exists by default. + // The superclass test assumes the empty namespace does not exist initially, + // so running it will fail as expected. + Assertions.assertThatThrownBy(super::listNamespacesWithEmptyNamespace) + .isInstanceOf(AssertionFailedError.class); + } + + @Test + public void listNamespacesWithEmptyNamespaceInPolaris() { + catalog().createNamespace(NS); + + Assertions.assertThat(catalog().namespaceExists(Namespace.empty())).isTrue(); + Assertions.assertThat(catalog().listNamespaces()) + .contains(NS) + .doesNotContain(Namespace.empty()); + Assertions.assertThat(catalog().listNamespaces(Namespace.empty())) + .contains(NS) + .doesNotContain(Namespace.empty()); } @Test From 7beab98eeff29809ce3fb8f639eb917ebfa9044f Mon Sep 17 00:00:00 2001 From: Liam Bao Date: Wed, 2 Apr 2025 18:18:41 -0400 Subject: [PATCH 2/6] Use `containsExactly` to simplify the test --- .../service/quarkus/catalog/IcebergCatalogTest.java | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/quarkus/service/src/test/java/org/apache/polaris/service/quarkus/catalog/IcebergCatalogTest.java b/quarkus/service/src/test/java/org/apache/polaris/service/quarkus/catalog/IcebergCatalogTest.java index f517381f3..281b0d4bb 100644 --- a/quarkus/service/src/test/java/org/apache/polaris/service/quarkus/catalog/IcebergCatalogTest.java +++ b/quarkus/service/src/test/java/org/apache/polaris/service/quarkus/catalog/IcebergCatalogTest.java @@ -401,12 +401,8 @@ public void listNamespacesWithEmptyNamespaceInPolaris() { catalog().createNamespace(NS); Assertions.assertThat(catalog().namespaceExists(Namespace.empty())).isTrue(); - Assertions.assertThat(catalog().listNamespaces()) - .contains(NS) - .doesNotContain(Namespace.empty()); - Assertions.assertThat(catalog().listNamespaces(Namespace.empty())) - .contains(NS) - .doesNotContain(Namespace.empty()); + Assertions.assertThat(catalog().listNamespaces()).containsExactly(NS); + Assertions.assertThat(catalog().listNamespaces(Namespace.empty())).containsExactly(NS); } @Test From 6c80b54fc763754ec6b43fe7f4780cd82990d610 Mon Sep 17 00:00:00 2001 From: Liam Bao Date: Thu, 3 Apr 2025 21:18:20 -0400 Subject: [PATCH 3/6] Fix empty namespace behavior --- .../quarkus/catalog/IcebergCatalogTest.java | 41 +++++++++++-------- .../catalog/iceberg/IcebergCatalog.java | 9 ++-- 2 files changed, 30 insertions(+), 20 deletions(-) diff --git a/quarkus/service/src/test/java/org/apache/polaris/service/quarkus/catalog/IcebergCatalogTest.java b/quarkus/service/src/test/java/org/apache/polaris/service/quarkus/catalog/IcebergCatalogTest.java index 281b0d4bb..4a6049725 100644 --- a/quarkus/service/src/test/java/org/apache/polaris/service/quarkus/catalog/IcebergCatalogTest.java +++ b/quarkus/service/src/test/java/org/apache/polaris/service/quarkus/catalog/IcebergCatalogTest.java @@ -123,6 +123,7 @@ import org.apache.polaris.service.types.NotificationType; import org.apache.polaris.service.types.TableUpdateNotification; import org.assertj.core.api.Assertions; +import org.assertj.core.api.ThrowableAssert.ThrowingCallable; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.Assumptions; import org.junit.jupiter.api.BeforeAll; @@ -133,7 +134,6 @@ import org.junit.jupiter.params.provider.Arguments; import org.junit.jupiter.params.provider.MethodSource; import org.mockito.Mockito; -import org.opentest4j.AssertionFailedError; import software.amazon.awssdk.core.exception.NonRetryableException; import software.amazon.awssdk.core.exception.RetryableException; import software.amazon.awssdk.services.sts.StsClient; @@ -164,6 +164,7 @@ public Map getConfigOverrides() { new Schema( required(3, "id", Types.IntegerType.get(), "unique ID 🤪"), required(4, "data", Types.StringType.get())); + protected static final String VIEW_QUERY = "select * from ns1.layer1_table"; public static final String CATALOG_NAME = "polaris-catalog"; public static final String TEST_ACCESS_KEY = "test_access_key"; public static final String SECRET_ACCESS_KEY = "secret_access_key"; @@ -386,23 +387,29 @@ public Map purgeRealms(Iterable realms) { }; } - @Override - @Test - public void listNamespacesWithEmptyNamespace() { - // In Polaris, the empty namespace implicitly exists by default. - // The superclass test assumes the empty namespace does not exist initially, - // so running it will fail as expected. - Assertions.assertThatThrownBy(super::listNamespacesWithEmptyNamespace) - .isInstanceOf(AssertionFailedError.class); - } - @Test - public void listNamespacesWithEmptyNamespaceInPolaris() { - catalog().createNamespace(NS); - - Assertions.assertThat(catalog().namespaceExists(Namespace.empty())).isTrue(); - Assertions.assertThat(catalog().listNamespaces()).containsExactly(NS); - Assertions.assertThat(catalog().listNamespaces(Namespace.empty())).containsExactly(NS); + public void testEmptyNamespace() { + IcebergCatalog catalog = catalog(); + TableIdentifier tableInRootNs = TableIdentifier.of("table"); + + ThrowingCallable createTable = () -> catalog.createTable(tableInRootNs, SCHEMA); + Assertions.assertThatThrownBy(createTable).isInstanceOf(NoSuchNamespaceException.class); + + ThrowingCallable createView = + () -> + catalog + .buildView(tableInRootNs) + .withSchema(SCHEMA) + .withDefaultNamespace(Namespace.empty()) + .withQuery("spark", VIEW_QUERY) + .create(); + Assertions.assertThatThrownBy(createView).isInstanceOf(NoSuchNamespaceException.class); + + ThrowingCallable listTables = () -> catalog.listTables(Namespace.empty()); + Assertions.assertThatThrownBy(listTables).isInstanceOf(NoSuchNamespaceException.class); + + ThrowingCallable listViews = () -> catalog.listViews(Namespace.empty()); + Assertions.assertThatThrownBy(listViews).isInstanceOf(NoSuchNamespaceException.class); } @Test 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 3f9722f79..fd9ced20f 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 @@ -462,7 +462,7 @@ public boolean dropTable(TableIdentifier tableIdentifier, boolean purge) { @Override public List listTables(Namespace namespace) { - if (!namespaceExists(namespace) && !namespace.isEmpty()) { + if (!namespaceExists(namespace)) { throw new NoSuchNamespaceException( "Cannot list tables for namespace. Namespace does not exist: %s", namespace); } @@ -633,7 +633,10 @@ private PolarisResolvedPathWrapper getResolvedParentNamespace(Namespace namespac @Override public boolean namespaceExists(Namespace namespace) { - return resolvedEntityView.getResolvedPath(namespace) != null; + return Optional.ofNullable(namespace) + .filter(ns -> !ns.isEmpty()) + .map(resolvedEntityView::getResolvedPath) + .isPresent(); } @Override @@ -798,7 +801,7 @@ public void close() throws IOException { @Override public List listViews(Namespace namespace) { - if (!namespaceExists(namespace) && !namespace.isEmpty()) { + if (!namespaceExists(namespace)) { throw new NoSuchNamespaceException( "Cannot list views for namespace. Namespace does not exist: %s", namespace); } From f9bf115d7d10f284afcfcbb8b8200e4a26f7fcc1 Mon Sep 17 00:00:00 2001 From: Liam Bao Date: Sun, 6 Apr 2025 09:24:59 -0400 Subject: [PATCH 4/6] Address comments --- .../quarkus/catalog/IcebergCatalogTest.java | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/quarkus/service/src/test/java/org/apache/polaris/service/quarkus/catalog/IcebergCatalogTest.java b/quarkus/service/src/test/java/org/apache/polaris/service/quarkus/catalog/IcebergCatalogTest.java index 4a6049725..521624ad9 100644 --- a/quarkus/service/src/test/java/org/apache/polaris/service/quarkus/catalog/IcebergCatalogTest.java +++ b/quarkus/service/src/test/java/org/apache/polaris/service/quarkus/catalog/IcebergCatalogTest.java @@ -164,7 +164,7 @@ public Map getConfigOverrides() { new Schema( required(3, "id", Types.IntegerType.get(), "unique ID 🤪"), required(4, "data", Types.StringType.get())); - protected static final String VIEW_QUERY = "select * from ns1.layer1_table"; + private static final String VIEW_QUERY = "select * from ns1.layer1_table"; public static final String CATALOG_NAME = "polaris-catalog"; public static final String TEST_ACCESS_KEY = "test_access_key"; public static final String SECRET_ACCESS_KEY = "secret_access_key"; @@ -391,9 +391,12 @@ public Map purgeRealms(Iterable realms) { public void testEmptyNamespace() { IcebergCatalog catalog = catalog(); TableIdentifier tableInRootNs = TableIdentifier.of("table"); + String expectedMessage = "Namespace does not exist: "; ThrowingCallable createTable = () -> catalog.createTable(tableInRootNs, SCHEMA); - Assertions.assertThatThrownBy(createTable).isInstanceOf(NoSuchNamespaceException.class); + Assertions.assertThatThrownBy(createTable) + .isInstanceOf(NoSuchNamespaceException.class) + .hasMessageContaining(expectedMessage); ThrowingCallable createView = () -> @@ -403,13 +406,19 @@ public void testEmptyNamespace() { .withDefaultNamespace(Namespace.empty()) .withQuery("spark", VIEW_QUERY) .create(); - Assertions.assertThatThrownBy(createView).isInstanceOf(NoSuchNamespaceException.class); + Assertions.assertThatThrownBy(createView) + .isInstanceOf(NoSuchNamespaceException.class) + .hasMessageContaining(expectedMessage); ThrowingCallable listTables = () -> catalog.listTables(Namespace.empty()); - Assertions.assertThatThrownBy(listTables).isInstanceOf(NoSuchNamespaceException.class); + Assertions.assertThatThrownBy(listTables) + .isInstanceOf(NoSuchNamespaceException.class) + .hasMessageContaining(expectedMessage); ThrowingCallable listViews = () -> catalog.listViews(Namespace.empty()); - Assertions.assertThatThrownBy(listViews).isInstanceOf(NoSuchNamespaceException.class); + Assertions.assertThatThrownBy(listViews) + .isInstanceOf(NoSuchNamespaceException.class) + .hasMessageContaining(expectedMessage); } @Test From 591133c84df112cf3c32fdb0e74549647bc4cc2d Mon Sep 17 00:00:00 2001 From: Liam Bao Date: Sun, 6 Apr 2025 10:43:45 -0400 Subject: [PATCH 5/6] Block dropping empty namespace --- .../service/quarkus/catalog/IcebergCatalogTest.java | 10 ++++++++++ .../service/catalog/iceberg/IcebergCatalog.java | 3 +++ 2 files changed, 13 insertions(+) diff --git a/quarkus/service/src/test/java/org/apache/polaris/service/quarkus/catalog/IcebergCatalogTest.java b/quarkus/service/src/test/java/org/apache/polaris/service/quarkus/catalog/IcebergCatalogTest.java index 521624ad9..fd44332ab 100644 --- a/quarkus/service/src/test/java/org/apache/polaris/service/quarkus/catalog/IcebergCatalogTest.java +++ b/quarkus/service/src/test/java/org/apache/polaris/service/quarkus/catalog/IcebergCatalogTest.java @@ -393,6 +393,16 @@ public void testEmptyNamespace() { TableIdentifier tableInRootNs = TableIdentifier.of("table"); String expectedMessage = "Namespace does not exist: "; + ThrowingCallable createEmptyNamespace = () -> catalog.createNamespace(Namespace.empty()); + Assertions.assertThatThrownBy(createEmptyNamespace) + .isInstanceOf(AlreadyExistsException.class) + .hasMessage("Cannot create root namespace, as it already exists implicitly."); + + ThrowingCallable dropEmptyNamespace = () -> catalog.dropNamespace(Namespace.empty()); + Assertions.assertThatThrownBy(dropEmptyNamespace) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("Cannot drop root namespace"); + ThrowingCallable createTable = () -> catalog.createTable(tableInRootNs, SCHEMA); Assertions.assertThatThrownBy(createTable) .isInstanceOf(NoSuchNamespaceException.class) 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 fd9ced20f..de279d76d 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 @@ -641,6 +641,9 @@ public boolean namespaceExists(Namespace namespace) { @Override public boolean dropNamespace(Namespace namespace) throws NamespaceNotEmptyException { + if (namespace.isEmpty()) { + throw new IllegalArgumentException("Cannot drop root namespace"); + } PolarisResolvedPathWrapper resolvedEntities = resolvedEntityView.getResolvedPath(namespace); if (resolvedEntities == null) { return false; From 7012aeb676503c09c5cd269a6512118138902c70 Mon Sep 17 00:00:00 2001 From: Liam Bao Date: Sun, 6 Apr 2025 10:57:02 -0400 Subject: [PATCH 6/6] Improve error messages --- .../service/quarkus/catalog/IcebergCatalogTest.java | 2 +- .../polaris/service/catalog/iceberg/IcebergCatalog.java | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/quarkus/service/src/test/java/org/apache/polaris/service/quarkus/catalog/IcebergCatalogTest.java b/quarkus/service/src/test/java/org/apache/polaris/service/quarkus/catalog/IcebergCatalogTest.java index fd44332ab..eb5a88d9e 100644 --- a/quarkus/service/src/test/java/org/apache/polaris/service/quarkus/catalog/IcebergCatalogTest.java +++ b/quarkus/service/src/test/java/org/apache/polaris/service/quarkus/catalog/IcebergCatalogTest.java @@ -391,7 +391,7 @@ public Map purgeRealms(Iterable realms) { public void testEmptyNamespace() { IcebergCatalog catalog = catalog(); TableIdentifier tableInRootNs = TableIdentifier.of("table"); - String expectedMessage = "Namespace does not exist: "; + String expectedMessage = "Namespace does not exist: ''"; ThrowingCallable createEmptyNamespace = () -> catalog.createNamespace(Namespace.empty()); Assertions.assertThatThrownBy(createEmptyNamespace) 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 de279d76d..f80d4077a 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 @@ -464,7 +464,7 @@ public boolean dropTable(TableIdentifier tableIdentifier, boolean purge) { public List listTables(Namespace namespace) { if (!namespaceExists(namespace)) { throw new NoSuchNamespaceException( - "Cannot list tables for namespace. Namespace does not exist: %s", namespace); + "Cannot list tables for namespace. Namespace does not exist: '%s'", namespace); } return listTableLike(PolarisEntitySubType.ICEBERG_TABLE, namespace); @@ -806,7 +806,7 @@ public void close() throws IOException { public List listViews(Namespace namespace) { if (!namespaceExists(namespace)) { throw new NoSuchNamespaceException( - "Cannot list views for namespace. Namespace does not exist: %s", namespace); + "Cannot list views for namespace. Namespace does not exist: '%s'", namespace); } return listTableLike(PolarisEntitySubType.ICEBERG_VIEW, namespace); @@ -1257,7 +1257,7 @@ public void doCommit(TableMetadata base, TableMetadata metadata) { // TODO: Maybe avoid writing metadata if there's definitely a transaction conflict if (null == base && !namespaceExists(tableIdentifier.namespace())) { throw new NoSuchNamespaceException( - "Cannot create table %s. Namespace does not exist: %s", + "Cannot create table '%s'. Namespace does not exist: '%s'", tableIdentifier, tableIdentifier.namespace()); } @@ -1498,7 +1498,7 @@ public void doCommit(ViewMetadata base, ViewMetadata metadata) { LOGGER.debug("doCommit for view {} with base {}, metadata {}", identifier, base, metadata); if (null == base && !namespaceExists(identifier.namespace())) { throw new NoSuchNamespaceException( - "Cannot create view %s. Namespace does not exist: %s", + "Cannot create view '%s'. Namespace does not exist: '%s'", identifier, identifier.namespace()); }