Core: Add tests for catalogs supporting empty namespaces #9890

nastra · 2024-03-07T09:19:32Z

From all the catalogs that extend CatalogTests, only Hive + REST don't support creating a table/view and also listing tables/views in an empty namespace.

JDBC + InMemory do support that and therefore override the supportsEmptyNamespace() feature in their tests.

Additionally, this PR also fixes listing tables/views for InMemory as previously this would list all available tables/views, instead of just the ones in that namespace.

I've decided to not adjust the Nessie tests, since the behavior deviates from the current behavior that can be found for JDBC + InMemory. While Nessie doesn't support creation/removal of empty namespaces, it does support creating a table/view in an empty namespace. Listing tables/views in an empty namespace returns all available tables/views.

nastra · 2024-03-07T09:21:37Z

core/src/test/java/org/apache/iceberg/jdbc/TestJdbcCatalog.java

@@ -102,12 +102,12 @@ protected JdbcCatalog catalog() {
  }

  @Override
-  protected boolean supportsNamespaceProperties() {


Intellij flags this because supportsNamespaceProperties() is true by default in CatalogTests, so no need to override this method here

nastra · 2024-03-07T09:22:00Z

nessie/src/test/java/org/apache/iceberg/nessie/TestNessieCatalog.java

@@ -140,17 +142,17 @@ protected boolean supportsNestedNamespaces() {
    return true;
  }

-  @Override
-  protected boolean supportsNamespaceProperties() {
-    return true;


Intellij flags this because supportsNamespaceProperties() is true by default in CatalogTests, so no need to override this method here

nastra · 2024-03-07T11:21:02Z

core/src/main/java/org/apache/iceberg/jdbc/JdbcUtil.java

@@ -468,6 +468,10 @@ private JdbcUtil() {}

  static Namespace stringToNamespace(String namespace) {
    Preconditions.checkArgument(namespace != null, "Invalid namespace %s", namespace);
+    if (namespace.isEmpty()) {


previously this method would return the identifier as .tblName instead of tblName when using an empty namespace, because L475 passes an array with a single empty string to Namespace.of().

I guess the question here is: is Namespace.of("") the same as Namespace.empty() and should we maybe fix Namespace.of()?
It seems there are currently no existing tests that would indicate what the correct behavior should be in this case

I think they should be treated the same, I also hope it doesn't come up very often

nastra · 2024-03-07T11:49:31Z

core/src/test/java/org/apache/iceberg/catalog/CatalogTests.java

@@ -987,6 +991,38 @@ public void testListTables() {
    assertEmpty("Should not contain ns_2.table_1 after drop", catalog, ns2);
  }

+  @Test
+  public void emptyNamespace() {


added this test because JDBC was previously returning namespace "" (empty string) when listing all namespaces

ajantha-bhat · 2024-03-07T14:25:56Z

nessie/src/test/java/org/apache/iceberg/nessie/TestNessieCatalog.java

+
+    // TODO this lists all available tables and deviates from the behavior in other catalogs and
+    // should probably be fixed
+    Assertions.assertThat(catalog().listTables(Namespace.empty()))


@snazy, @adutra, @dimas-b: Nessie doesn't support empty namespace creation. So, should we allow listing tables from empty namespace?

But nessie supports creating a table/view withing the empty namespace, which is a valid use case. If creating a table/view in an empty namespace is supported, then listing should be supported as well

An empty namespace is not an "entity", right? Why would we want to "create" it?

That said, creating tables in the "empty" / "root" namespace should work with Nessie, IIRC.

core/src/main/java/org/apache/iceberg/inmemory/InMemoryCatalog.java

core/src/test/java/org/apache/iceberg/catalog/CatalogTests.java

nastra · 2024-03-08T10:59:47Z

core/src/test/java/org/apache/iceberg/catalog/CatalogTests.java

@@ -987,6 +995,61 @@ public void testListTables() {
    assertEmpty("Should not contain ns_2.table_1 after drop", catalog, ns2);
  }

+  @Test
+  public void listNamespacesWithEmptyNamespace() {


this behavior applies for all catalogs

nastra · 2024-03-08T11:59:22Z

core/src/main/java/org/apache/iceberg/inmemory/InMemoryCatalog.java

@@ -288,6 +288,7 @@ public List<Namespace> listNamespaces(Namespace namespace) throws NoSuchNamespac

    List<Namespace> filteredNamespaces =
        namespaces.keySet().stream()
+            .filter(n -> !n.isEmpty())


this would previously fail in L303 when calling listNamespaces(Namespace.empty())

So does this one support empty namespaces or not?

Just wondering what the behavior should be here, since it is a valid namespace so shouldn't we be able to list it?

yes, InMemoryCatalog supports empty namespaces and this comes back to the TODO I added in https://github.com/apache/iceberg/pull/9890/files#diff-de393039ffcf049ebb22892ac1b9aad88d53274c1e86431b45e776e4568246f8R1037 to determine what the listing behavior with empty namespaces should be

RussellSpitzer · 2024-03-28T14:42:54Z

spark/v3.5/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestViews.java

+    Object[] v1 = row(NAMESPACE.toString(), "v1", false);
+    Object[] v2 = row(NAMESPACE.toString(), "prefixV2", false);
+    Object[] v3 = row(NAMESPACE.toString(), "prefixV3", false);
+    assertThat(sql("SHOW VIEWS")).contains(v2, v3, v1, tempView);


nit: (ocd) It tares me up that you define v1, v2, v3 and then check contains v2, v3, v1

RussellSpitzer · 2024-03-28T14:43:20Z

core/src/test/java/org/apache/iceberg/jdbc/TestJdbcUtil.java

@@ -45,4 +47,10 @@ public void testFilterAndRemovePrefix() {

    assertThat(expected).isEqualTo(actual);
  }
+
+  @Test
+  public void emptyNamespaceInIdentifier() {


RussellSpitzer · 2024-03-28T14:47:30Z

spark/v3.4/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestViews.java

+    Object[] v1 = row(NAMESPACE.toString(), "v1", false);
+    Object[] v2 = row(NAMESPACE.toString(), "prefixV2", false);
+    Object[] v3 = row(NAMESPACE.toString(), "prefixV3", false);
+    assertThat(sql("SHOW VIEWS")).contains(v2, v3, v1, tempView);


the order here

RussellSpitzer

LGTM, only small issue with test rows being defined, 1, 2, 3 then used 2, 3, 1. I keep thinking there is a secret there but I know there isn't.

github-actions · 2024-10-22T00:15:41Z

This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions.

Fokko

Looks good, thanks @nastra. Thanks @RussellSpitzer for the review, let's get this in and follow up on the TODO in a separate PR

github-actions bot added core NESSIE labels Mar 7, 2024

nastra commented Mar 7, 2024

View reviewed changes

nastra force-pushed the empty-namespace-handling branch from a3ddfab to 61cfa9d Compare March 7, 2024 09:41

nastra changed the title ~~Handle listing tables/views in empty namespaces~~ Core: Handle listing tables/views in empty namespaces Mar 7, 2024

nastra force-pushed the empty-namespace-handling branch from 61cfa9d to 6fecd82 Compare March 7, 2024 10:42

github-actions bot added the spark label Mar 7, 2024

nastra force-pushed the empty-namespace-handling branch from 6fecd82 to ea594d8 Compare March 7, 2024 11:14

nastra commented Mar 7, 2024

View reviewed changes

nastra force-pushed the empty-namespace-handling branch 2 times, most recently from 3a9fde1 to e4cee23 Compare March 7, 2024 11:46

nastra commented Mar 7, 2024

View reviewed changes

ajantha-bhat reviewed Mar 7, 2024

View reviewed changes

rdblue reviewed Mar 7, 2024

View reviewed changes

core/src/main/java/org/apache/iceberg/inmemory/InMemoryCatalog.java Outdated Show resolved Hide resolved

nastra commented Mar 8, 2024

View reviewed changes

core/src/test/java/org/apache/iceberg/catalog/CatalogTests.java Outdated Show resolved Hide resolved

nastra force-pushed the empty-namespace-handling branch from f062549 to 95b0416 Compare March 8, 2024 10:58

nastra commented Mar 8, 2024

View reviewed changes

nastra force-pushed the empty-namespace-handling branch from 95b0416 to b6129ef Compare March 8, 2024 11:37

github-actions bot removed the NESSIE label Mar 8, 2024

nastra commented Mar 8, 2024

View reviewed changes

nastra changed the title ~~Core: Handle listing tables/views in empty namespaces~~ Core: Add tests for catalogs supporting empty namespaces Mar 8, 2024

RussellSpitzer reviewed Mar 28, 2024

View reviewed changes

RussellSpitzer approved these changes Mar 28, 2024

View reviewed changes

nastra mentioned this pull request Jul 3, 2024

View Spec implementation apache/iceberg-rust#331

Merged

github-actions bot added the stale label Oct 22, 2024

nastra removed the stale label Oct 22, 2024

nastra added the not-stale label Oct 22, 2024

nastra force-pushed the empty-namespace-handling branch 3 times, most recently from 6ae6aec to 39f4033 Compare December 4, 2024 14:09

Core: Add tests for catalogs supporting empty namespaces

e02ef8a

nastra force-pushed the empty-namespace-handling branch from 39f4033 to e02ef8a Compare December 4, 2024 14:28

Fokko approved these changes Jan 13, 2025

View reviewed changes

Fokko merged commit 472ec6c into apache:main Jan 13, 2025
49 checks passed

nastra deleted the empty-namespace-handling branch January 14, 2025 06:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Core: Add tests for catalogs supporting empty namespaces #9890

Core: Add tests for catalogs supporting empty namespaces #9890

nastra commented Mar 7, 2024 •

edited

Loading

nastra Mar 7, 2024

nastra Mar 7, 2024

nastra Mar 7, 2024

RussellSpitzer Mar 28, 2024

nastra Mar 7, 2024

ajantha-bhat Mar 7, 2024

nastra Mar 7, 2024

dimas-b Mar 18, 2024

nastra Mar 8, 2024

nastra Mar 8, 2024

RussellSpitzer Mar 28, 2024

RussellSpitzer Mar 28, 2024

nastra Mar 28, 2024

RussellSpitzer Mar 28, 2024

RussellSpitzer Mar 28, 2024

RussellSpitzer Mar 28, 2024

RussellSpitzer left a comment

github-actions bot commented Oct 22, 2024

Fokko left a comment

Core: Add tests for catalogs supporting empty namespaces #9890

Core: Add tests for catalogs supporting empty namespaces #9890

Conversation

nastra commented Mar 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RussellSpitzer left a comment

Choose a reason for hiding this comment

github-actions bot commented Oct 22, 2024

Fokko left a comment

Choose a reason for hiding this comment

nastra commented Mar 7, 2024 •

edited

Loading