slachiewicz commented on code in PR #2390:
URL: https://github.com/apache/iceberg-go/pull/2390#discussion_r4440754573
##########
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:
Done in 62a475c. Drop and list share `childNamespacesInTx`, which compares
levels exactly after the query. `TestNamespaceLevelsCompareCase` covers
`SALES.eu` vs `sales`. In a local run against MySQL 8.4 the same case failed
before this commit and passes now; PostgreSQL 17 passes either way, since its
`LIKE` is case-sensitive.
##########
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:
Done in 62a475c. A namespace with children exists for `ListNamespaces`,
`ListTables`, `ListViews`, `CheckNamespaceExists` and `LoadNamespaceProperties`
(empty properties), and `DropNamespace` returns `ErrNamespaceNotEmpty`.
`CreateNamespace` and `UpdateNamespaceProperties` keep the exact check. The
walk is in `TestListNamespaces`; in a local run it also passes on PostgreSQL 17
and MySQL 8.4, as does the catalogtest conformance with #2389 merged and nested
namespaces on.
##########
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:
Done in 62a475c. `whereNamespaceParent` is gone; listing uses
`whereNamespaceDescendant`.
##########
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:
Added in 62a475c: `TestDropNamespaceWithChildOnlyInTableRow` and
`TestDropNamespaceWithDottedChild`. In a local run both scenarios also pass on
PostgreSQL 17 and MySQL 8.4.
--
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]
