Unblock test listNamespacesWithEmptyNamespace
#1289
Conversation
liamzwbao
requested review from
adutra,
ashvina,
dennishuo,
dimas-b,
eric-maynard,
jackye1995,
jbonofre,
vvcephei,
collado-mike,
snazy,
RussellSpitzer,
takidau,
MonkeyCanCode,
flyrain and
ebyhr
as code owners
...kus/service/src/test/java/org/apache/polaris/service/quarkus/catalog/IcebergCatalogTest.java
Outdated
Show resolved
Hide resolved
...kus/service/src/test/java/org/apache/polaris/service/quarkus/catalog/IcebergCatalogTest.java
Outdated
Show resolved
Hide resolved
| Assertions.assertThat(catalog().namespaceExists(Namespace.empty())).isTrue(); | ||
| Assertions.assertThat(catalog().listNamespaces()) | ||
| .contains(NS) | ||
| .doesNotContain(Namespace.empty()); |
There was a problem hiding this comment.
This looks like a bug to me. The test clearly indicates that the empty namespace exists, but is not returned when all namespaces are listed.
There was a problem hiding this comment.
I think the reason is that Namespace.empty() represents the root namespace in Polaris and is created implicitly. When we call listNamespaces without any arg, it returns namespaces under the root—i.e., under Namespace.empty(). In this case, we only created NS, so it's the only one listed. Since creating an "empty" namespace isn’t something we officially support I guess, there’s no empty namespace under the root empty namespace.
That said, I’m happy to clarify what behavior we actually want here and fix it if it turns out to be a bug.
There was a problem hiding this comment.
One interesting behavior is that we can drop the empty namespace using catalog().dropNamespace(Namespace.empty()). But after doing that, we can’t create any new namespaces because it fails, saying the parent namespace doesn't exist. Even worse, we also can’t recreate the empty namespace itself since it's explicitly blocked here.
I think we should consider aligning the behavior between creating and dropping the empty namespace anyway.
There was a problem hiding this comment.
IMHO, catalog().namespaceExists(Namespace.empty()) should not be true (as the Iceberg tests expects).
There was a problem hiding this comment.
Just checked the RESTSessionCatalog implementation in Iceberg, seems like it will throw exception for empty namespaces. Fix the behavior in Polaris to match how RESTCatalog handles empty namespaces in Iceberg.
liamzwbao
force-pushed
the
issue-1272-test-empty-ns
branch
from
3e9f4bb to
df8486b
Compare
There was a problem hiding this comment.
Thanks, @liamzwbao ! These changes LGTM. Just a couple minor comments.
...kus/service/src/test/java/org/apache/polaris/service/quarkus/catalog/IcebergCatalogTest.java
Outdated
Show resolved
Hide resolved
| @@ -462,7 +462,7 @@ public boolean dropTable(TableIdentifier tableIdentifier, boolean purge) { | |||
|
|
|||
| @Override | |||
| public List<TableIdentifier> 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); | |||
There was a problem hiding this comment.
Maybe add quotes around %s (same for views)? The empty namespace will result in an awkward message otherwise.
...kus/service/src/test/java/org/apache/polaris/service/quarkus/catalog/IcebergCatalogTest.java
Outdated
Show resolved
Hide resolved
liamzwbao
force-pushed
the
issue-1272-test-empty-ns
branch
from
df8486b to
7012aeb
Compare
github-project-automation
bot
moved this from PRs In Progress
to Ready to merge
in Basic Kanban Board
github-project-automation
bot
moved this from Ready to merge
to Done
in Basic Kanban Board
This PR fixes #1272
In Polaris, the empty namespace implicitly exists by default, see link. However, the superclass test assumes that the empty namespace does not exist initially, which causes the test to fail as expected in Polaris. This test was originally introduced in apache/iceberg#9890.
Changes in this PR:
namespaceExists,dropNamespace,listTables,listViews