laskoviymishka commented on code in PR #1211:
URL: https://github.com/apache/iceberg-go/pull/1211#discussion_r3423760799
##########
catalog/sql/sql_test.go:
##########
@@ -322,6 +322,28 @@ func (s *SqliteCatalogTestSuite)
TestCreationAllTablesExist() {
s.confirmTablesExist(sqldb)
}
+func (s *SqliteCatalogTestSuite) TestCatalogNameMatchesLoaderArg() {
+ const catalogName = "test_catalog"
+ cat, err := catalog.Load(context.Background(), catalogName,
iceberg.Properties{
+ "uri": ":memory:",
+ sqlcat.DriverKey: sqliteshim.ShimName,
+ sqlcat.DialectKey: string(sqlcat.SQLite),
+ "type": "sql",
+ "warehouse": "file://" + s.warehouse,
+ })
+ s.Require().NoError(err)
+ sqlCat := cat.(*sqlcat.Catalog)
+ s.Equal(catalogName, sqlCat.Name(), "SQL catalog must surface the name
passed to catalog.Load")
+
+ ctx := context.Background()
+ ns := table.Identifier{"test"}
+ s.Require().NoError(sqlCat.CreateNamespace(ctx, ns,
iceberg.Properties{"created_by": "iceberg-go"}))
+
+ got, err := sqlCat.ListNamespaces(ctx, table.Identifier{})
+ s.Require().NoError(err)
+ s.Contains(got, ns, "iceberg-go must list a namespace it just created
under its own catalog name")
Review Comment:
This is the main thing I'd want reworked. As written, this asserts a
single-instance round-trip — create under "test_catalog", list under
"test_catalog" — which passes under the *old* code too, since one catalog is
always self-consistent regardless of what name it stored.
The regression is cross-instance isolation. I'd open a second catalog with
name "sql" against the same `:memory:` DB and assert it does *not* see the
namespace created under "test_catalog". That's the assertion that actually
fails before the fix and passes after.
##########
catalog/sql/sql.go:
##########
@@ -92,7 +92,7 @@ func init() {
}
}()
- return NewCatalog(p.Get(name, "sql"), sqldb,
SupportedDialect(dialect), p)
+ return NewCatalog(name, sqldb, SupportedDialect(dialect), p)
Review Comment:
This is the right fix — `p.Get(name, "sql")` was treating `name` as a lookup
key (never set), so it always fell back to "sql". `name` flows straight into
`c.name` as the `catalog_name` partition key on every query, which matches REST
and Hadoop here.
The one thing this changes silently: existing deployments that loaded under
a name other than "sql" had everything written under `catalog_name='sql'`, and
now those rows won't be found. I'd document the upgrade in the CHANGELOG with
the re-key `UPDATE iceberg_tables SET catalog_name='<name>' WHERE
catalog_name='sql'` (and the same for `iceberg_namespace_properties`), and note
warehouse paths are unchanged. A one-line comment here explaining `name` is the
catalog_name partition key would also save the next person from re-deriving the
`p.Get` confusion. wdyt?
##########
catalog/sql/sql_test.go:
##########
@@ -322,6 +322,28 @@ func (s *SqliteCatalogTestSuite)
TestCreationAllTablesExist() {
s.confirmTablesExist(sqldb)
}
+func (s *SqliteCatalogTestSuite) TestCatalogNameMatchesLoaderArg() {
+ const catalogName = "test_catalog"
+ cat, err := catalog.Load(context.Background(), catalogName,
iceberg.Properties{
+ "uri": ":memory:",
+ sqlcat.DriverKey: sqliteshim.ShimName,
+ sqlcat.DialectKey: string(sqlcat.SQLite),
+ "type": "sql",
+ "warehouse": "file://" + s.warehouse,
+ })
+ s.Require().NoError(err)
+ sqlCat := cat.(*sqlcat.Catalog)
Review Comment:
Minor: the bare assertion panics instead of failing cleanly if `Load` ever
returns a different type. The two-value form reads better in a test:
```go
sqlCat, ok := cat.(*sqlcat.Catalog)
s.Require().True(ok, "expected *sqlcat.Catalog")
```
Matches the pattern in TestPurgeTable. Not blocking.
##########
catalog/sql/sql_test.go:
##########
@@ -322,6 +322,28 @@ func (s *SqliteCatalogTestSuite)
TestCreationAllTablesExist() {
s.confirmTablesExist(sqldb)
}
+func (s *SqliteCatalogTestSuite) TestCatalogNameMatchesLoaderArg() {
+ const catalogName = "test_catalog"
+ cat, err := catalog.Load(context.Background(), catalogName,
iceberg.Properties{
+ "uri": ":memory:",
+ sqlcat.DriverKey: sqliteshim.ShimName,
+ sqlcat.DialectKey: string(sqlcat.SQLite),
+ "type": "sql",
+ "warehouse": "file://" + s.warehouse,
+ })
+ s.Require().NoError(err)
+ sqlCat := cat.(*sqlcat.Catalog)
+ s.Equal(catalogName, sqlCat.Name(), "SQL catalog must surface the name
passed to catalog.Load")
+
+ ctx := context.Background()
+ ns := table.Identifier{"test"}
+ s.Require().NoError(sqlCat.CreateNamespace(ctx, ns,
iceberg.Properties{"created_by": "iceberg-go"}))
+
+ got, err := sqlCat.ListNamespaces(ctx, table.Identifier{})
Review Comment:
`s.Contains` alone won't notice a phantom or duplicated namespace. An
`s.Len(got, 1)` before the Contains (like TestListNamespaces) makes it strict.
Also `context.Background()` is called twice in this test — the surrounding
multi-call tests declare `ctx` once and reuse it. Both small, fold them in if
you're touching this anyway.
--
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]
