badalprasadsingh commented on code in PR #2389:
URL: https://github.com/apache/iceberg-go/pull/2389#discussion_r4451062337
##########
catalog/catalogtest/namespaces.go:
##########
@@ -168,6 +169,94 @@ func testListNamespaces(t *testing.T, cfg Config) {
assert.Contains(t, namespaces, second, "remaining namespace should
still be listed")
}
+// testListNestedNamespaces asserts that listing a namespace returns its direct
+// children as full identifiers, that the children stay out of the top-level
+// listing, and that a dropped child disappears from its parent's listing.
+func testListNestedNamespaces(t *testing.T, cfg Config) {
+ ctx := context.Background()
+ cat := cfg.NewCatalog(t)
+ parent, _ := newIdentifiers()
+ child1 := append(slices.Clone(parent), "child1")
+ child2 := append(slices.Clone(parent), "child2")
+
+ require.NoError(t, cat.CreateNamespace(ctx, parent, nil))
+ t.Cleanup(func() { _ = cat.DropNamespace(ctx, parent) })
+
+ children, err := cat.ListNamespaces(ctx, parent)
+ require.NoError(t, err)
+ assert.Empty(t, children, "a new namespace should have no children")
+
+ require.NoError(t, cat.CreateNamespace(ctx, child1, nil))
+ t.Cleanup(func() { _ = cat.DropNamespace(ctx, child1) })
+
+ children, err = cat.ListNamespaces(ctx, parent)
+ require.NoError(t, err)
+ assert.ElementsMatch(t, []table.Identifier{child1}, children, "listing
the parent should return child1")
+
+ require.NoError(t, cat.CreateNamespace(ctx, child2, nil))
+ t.Cleanup(func() { _ = cat.DropNamespace(ctx, child2) })
+
+ children, err = cat.ListNamespaces(ctx, parent)
+ require.NoError(t, err)
+ assert.ElementsMatch(t, []table.Identifier{child1, child2}, children,
"listing the parent should return both children")
Review Comment:
a partial #2388 fix could pass this test while still listing grandchildren.
could we add a grand child and check that the parent still lists only its
two direct children?
##########
catalog/rest/conformance_test.go:
##########
@@ -34,6 +34,9 @@ func TestRestCatalogConformance(t *testing.T) {
catalogtest.RunCatalogTests(t, catalogtest.Config{
SupportsNamespaceProperties: true,
SupportsRenameTable: true,
+ // SupportsNestedNamespaces stays off: the published fixture
(1.10.1)
+ // drops a namespace that still has children, which
apache/iceberg#16061
+ // fixed in 1.11.0, and no newer fixture image is published yet.
Review Comment:
this note covers fixture, but ci also runs this suite on Polaris, whose [own
tests](https://github.com/apache/polaris/blob/apache-polaris-1.8.0/integration-tests/src/main/java/org/apache/polaris/service/it/test/PolarisRestCatalogIntegrationBase.java#L460)
enable nested namespaces.
Could you set the flag in `TestExternalRestCatalogConformance` from an
off-by-default `REST_CATALOG_NESTED_NAMESPACES` env var, and turn it on in
`go-integration.yml`? We must not hard-code it as it would fail CONTRIBUTING's
fixture command.
##########
catalog/catalogtest/catalogtest.go:
##########
@@ -96,6 +96,15 @@ type Config struct {
// the rename tests are replaced by one asserting that RenameTable
returns
// iceberg.ErrNotImplemented and leaves the source table in place.
SupportsRenameTable bool
+
+ // SupportsNestedNamespaces reports whether the catalog can create a
+ // namespace inside another one. When it is true, the suite also checks
+ // listing a namespace's children and refusing to drop a namespace that
Review Comment:
nit: sql and rest can both create nested-namespaces, but the flag stays off
for them, so "can create" doesn't match how it's used.
I'd describe what the flag actually gates, and keep the rest of the comment:
```go
// SupportsNestedNamespaces reports whether the catalog follows Java's
nested namespace behavior:
// listing a namespace returns only its direct children, and
// a namespace that still has children cannot be dropped. When it is true,
the suite checks both.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
