badalprasadsingh commented on code in PR #2390:
URL: https://github.com/apache/iceberg-go/pull/2390#discussion_r4523443913
##########
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:
minor: CreateTable here gives implied a table row, so the RenameTable and
CreateView checks below would also pass with the old exact check.
can each of the three use its own namespace that has only a child?
##########
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:
CommitTable also creates tables when called with
`table.RequirementsForCreateTable`, but that branch (Line nymber 959) still
uses namespaceExistsInTx.
So after `CreateNamespace(a.b)`, `CreateTable(a.t)` works while a create
commit for a.t fails. Also, the release note lists only two exceptions, better:
use `listableNamespaceExistsInTx(ctx, tx, ns, nsKey)` there, plus a test.
--
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]
