laskoviymishka commented on code in PR #2390:
URL: https://github.com/apache/iceberg-go/pull/2390#discussion_r4537113644
##########
catalog/sql/sql.go:
##########
@@ -841,7 +836,7 @@ func (c *Catalog) CreateTable(ctx context.Context, ident
table.Identifier, sc *i
}
err = withSerializableWriteTx(ctx, c.db, func(ctx context.Context, tx
bun.Tx) error {
- exists, err := c.namespaceExistsInTx(ctx, tx, ns)
+ exists, err := c.listableNamespaceExistsInTx(ctx, tx, nsIdent,
ns)
Review Comment:
Could we take @badalprasadsingh's thread here in this PR? The `CommitTable`
create branch
([L959-L965](https://github.com/apache/iceberg-go/blob/7be3e2ea28a51a7e14699ed0de36a6a9713253c9/catalog/sql/sql.go#L959-L965))
still calls the exact `namespaceExistsInTx`: on this head, after
`CreateNamespace(a.child)`, `CreateTable(a.t)` passes but `CommitTable(a.t,
[AssertCreate()], ...)` fails with `namespace does not exist`. `main` was exact
on both paths. The listable check plus a test also makes the release note's
two-exception line true.
##########
catalog/sql/sql_test.go:
##########
@@ -1202,30 +1202,203 @@ 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)
- ns, err = cat.ListNamespaces(ctx, nil)
+ // A parent created only through its child is listed, and
exists for
+ // every read, so the hierarchy can be walked from the root.
+ implied := table.Identifier{databaseName()}
+ impliedChild := append(slices.Clone(implied), "child")
+ s.Require().NoError(cat.CreateNamespace(ctx, impliedChild, nil))
+
+ nslist, err = cat.ListNamespaces(ctx, nil)
+ s.Require().NoError(err)
+ s.Contains(nslist, implied)
+ ns, err = cat.ListNamespaces(ctx, implied)
+ s.Require().NoError(err)
+ s.Equal([]table.Identifier{impliedChild}, ns)
+ ns, err = cat.ListNamespaces(ctx, impliedChild)
+ s.Require().NoError(err)
+ s.Empty(ns)
+
+ exists, err := cat.CheckNamespaceExists(ctx, implied)
+ s.Require().NoError(err)
+ s.True(exists)
+ exists, err = cat.CheckNamespaceExists(ctx, nil)
+ s.Require().NoError(err)
+ s.False(exists)
+ props, err := cat.LoadNamespaceProperties(ctx, implied)
+ s.Require().NoError(err)
+ s.Empty(props)
+ for _, err := range cat.ListTables(ctx, implied) {
+ s.Require().NoError(err)
+ }
+ for _, err := range cat.ListViews(ctx, implied) {
+ s.Require().NoError(err)
+ }
+ s.ErrorIs(cat.DropNamespace(ctx, implied),
catalog.ErrNamespaceNotEmpty)
+
+ // Creating the parent itself still succeeds.
+ s.Require().NoError(cat.CreateNamespace(ctx, implied, nil))
+ s.ErrorIs(cat.CreateNamespace(ctx, implied, nil),
catalog.ErrNamespaceAlreadyExists)
+ }
+}
+
+func (s *SqliteCatalogTestSuite) TestCreateInNamespaceWithOnlyChildren() {
+ for _, cat := range []*sqlcat.Catalog{s.getCatalogMemory(),
s.getCatalogSqlite()} {
+ ctx := context.Background()
+ implied := table.Identifier{databaseName()}
+ s.Require().NoError(cat.CreateNamespace(ctx,
append(slices.Clone(implied), "child"), nil))
+
+ exists, err := cat.CheckNamespaceExists(ctx, implied)
+ s.Require().NoError(err)
+ s.Require().True(exists)
+
+ // CheckNamespaceExists is true, so tables and views can be
created
+ // there, and a table can be renamed into it.
+ tblName := tableName()
+ _, err = cat.CreateTable(ctx, append(slices.Clone(implied),
tblName), tableSchemaNested)
Review Comment:
Could each operation here get its own children-only namespace, as
@badalprasadsingh asks? This `CreateTable` gives `implied` a table row, so the
`RenameTable` and `CreateView` checks below never see a children-only
namespace. With both reverted to the exact `resolveNamespaceKey` /
`namespaceExistsInTx`, the test still passes; on a fresh children-only
namespace the same calls fail with `namespace does not exist`.
--
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]
