badalprasadsingh commented on code in PR #2390:
URL: https://github.com/apache/iceberg-go/pull/2390#discussion_r4440382341
##########
catalog/sql/sql.go:
##########
@@ -1455,15 +1483,25 @@ func (c *Catalog) ListNamespaces(ctx context.Context,
parent table.Identifier) (
return nil, err
}
+ // Like Java's JdbcCatalog, return only the level below parent: the
+ // distinct first levels at the root, and direct children otherwise.
ret := make([]table.Identifier, 0, len(namespaces))
+ seen := make(map[string]struct{}, len(namespaces))
for _, n := range namespaces {
ident, err := namespaceFromString(n)
if err != nil {
return nil, err
}
- if len(parent) == 0 || (len(ident) >= len(parent) &&
slices.Equal(ident[:len(parent)], parent)) {
- ret = append(ret, ident)
+ if len(ident) <= len(parent) ||
!slices.Equal(ident[:len(parent)], parent) {
+ continue
+ }
+ ident = ident[:len(parent)+1]
Review Comment:
P1: the root listing now returns parents the catalog says doesn't exist.
- `CreateNamespace(a.b)` does not need a: so `ListNamespaces(nil)` returns
a. Then `ListNamespaces(a)` fails with `ErrNoSuchNamespace`.
- `a.b` can't be found by walking, though: main's root listing shows it.
- fails the same way: iceberg list a
Can we counts descendants as existing? I'd do that in ListNamespaces,
ListTables, ListViews and CheckNamespaceExists, and return ErrNamespaceNotEmpty
from DropNamespace here. CreateNamespace keeps its exact check.
Could you please add this walk to TestListNamespaces?
##########
catalog/sql/sql.go:
##########
@@ -144,6 +144,16 @@ func whereNamespaceParent(query *bun.SelectQuery, column
string, parent table.Id
})
}
+func whereNamespaceDescendant(query *bun.SelectQuery, column string, namespace
table.Identifier) *bun.SelectQuery {
+ return query.WhereGroup(" AND ", func(query *bun.SelectQuery)
*bun.SelectQuery {
+ for _, prefix := range namespaceDescendantPrefixes(namespace) {
+ query = query.WhereOr("? LIKE ? ESCAPE '!'",
bun.Ident(column), namespaceLikePrefix(prefix))
+ }
Review Comment:
P2: this repeats the loop in `whereNamespaceParent`, and that helper's =
branch is no longer needed: `ListNamespaces` now throws away the parent's own
rows in its filter.
If `ListNamespaces` uses `whereNamespaceDescendant`, we can delete
`whereNamespaceParent` IMO, and keep one predicate. `resolveNamespaceKey`
already handles existence separately.
##########
catalog/sql/sql.go:
##########
@@ -1345,6 +1355,24 @@ func (c *Catalog) DropNamespace(ctx context.Context,
namespace table.Identifier)
return fmt.Errorf("%w: %s", catalog.ErrNoSuchNamespace,
nsToDelete)
}
+ // A child namespace exists if it has properties of its own or
holds a table.
+ childExists, err :=
whereNamespaceDescendant(tx.NewSelect().Model((*sqlIcebergNamespaceProps)(nil)).
+ ColumnExpr("1").
+ Where("catalog_name = ?", c.name), "namespace",
namespace).
+ Limit(1).Exists(ctx)
+ if err == nil && !childExists {
+ childExists, err =
whereNamespaceDescendant(tx.NewSelect().Model((*sqlIcebergTable)(nil)).
Review Comment:
P1: drop should not disagree with list.
sqlite's "LIKE" ignores case, so: it treats `SALES.eu` as a child of
`sales`. ListNamespaces(sales) returns nothing, yet DropNamespace(sales)
returns ErrNamespaceNotEmpty. Today that drop succeeds.
ListNamespaces gets this right because it compares levels exactly after the
query. Can DropNamespace share that logic, the way Java's dropNamespace uses
listNamespaces? We can move the query and filter into a helper that takes the
transaction, call it from both, and remove these two Exists queries.
Also, can you please add a case-variant test?
##########
catalog/sql/sql_test.go:
##########
@@ -1202,30 +1202,51 @@ func (s *SqliteCatalogTestSuite) TestListNamespaces() {
s.Require().NoError(cat.CreateNamespace(ctx, ns1, nil))
s.Require().NoError(cat.CreateNamespace(ctx, ns2, nil))
+ // The root lists first levels only, so ns2 appears as its
first level.
nslist, err := cat.ListNamespaces(ctx, nil)
s.Require().NoError(err)
- s.Len(nslist, 2)
- s.Contains(nslist, ns1)
- s.Contains(nslist, ns2)
+ s.ElementsMatch([]table.Identifier{ns1, ns2[:1]}, nslist)
ns, err := cat.ListNamespaces(ctx, ns1)
s.Require().NoError(err)
- s.Len(ns, 1)
- s.Equal(ns1, ns[0])
+ s.Empty(ns)
ns, err = cat.ListNamespaces(ctx, ns2)
s.Require().NoError(err)
- s.Len(ns, 1)
- s.Equal(ns2, ns[0])
+ s.Empty(ns)
_, err = cat.ListNamespaces(ctx,
table.Identifier{"does_not_exist"})
s.ErrorIs(err, catalog.ErrNoSuchNamespace)
+ }
+}
+
+func (s *SqliteCatalogTestSuite) TestNestedNamespaces() {
+ catalogs := []*sqlcat.Catalog{s.getCatalogMemory(),
s.getCatalogSqlite()}
+
+ for _, cat := range catalogs {
+ ctx := context.Background()
+ parent := table.Identifier{databaseName()}
+ child := append(slices.Clone(parent), "child")
+ grandchild := append(slices.Clone(child), "grandchild")
+ for _, ns := range []table.Identifier{parent, child,
grandchild} {
+ s.Require().NoError(cat.CreateNamespace(ctx, ns, nil))
+ }
- ns, err = cat.ListNamespaces(ctx, nil)
+ root, err := cat.ListNamespaces(ctx, nil)
+ s.Require().NoError(err)
+ s.Equal([]table.Identifier{parent}, root)
+ children, err := cat.ListNamespaces(ctx, parent)
s.Require().NoError(err)
- s.Len(ns, 2)
- s.Equal(ns1, ns[0])
- s.Equal(ns2, ns[1])
+ s.Equal([]table.Identifier{child}, children)
+ children, err = cat.ListNamespaces(ctx, child)
+ s.Require().NoError(err)
+ s.Equal([]table.Identifier{grandchild}, children)
+
+ s.ErrorIs(cat.DropNamespace(ctx, parent),
catalog.ErrNamespaceNotEmpty)
+ s.ErrorIs(cat.DropNamespace(ctx, child),
catalog.ErrNamespaceNotEmpty)
+ for _, ns := range []table.Identifier{grandchild, child,
parent} {
+ s.Require().NoError(cat.DropNamespace(ctx, ns))
Review Comment:
right now, the ErrNamespaceNotEmpty checks above cover only children created
with CreateNamespace under plain keys. Two cases have no test: (a) a child that
exists only through a table row, and, (b) a dotted child with a JSON-encoded
key.
could you please add 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]
