laskoviymishka commented on code in PR #1489:
URL: https://github.com/apache/iceberg-go/pull/1489#discussion_r3601862857
##########
catalog/sql/sql.go:
##########
@@ -1285,6 +1333,12 @@ func (c *Catalog) CreateView(ctx context.Context,
identifier table.Identifier, s
return nil
})
+ if err != nil {
+ loadFS := io.LoadFSFunc(props, metadataLocation)
Review Comment:
This cleanup won't run on any backend that keeps credentials in the catalog
config.
`props` here is the caller's view-level props, which is nil in the common
case — the S3/GCS/ADLS credentials live in `c.props`, which is what `LoadView`
and `CheckTableExists` use. So `io.LoadFSFunc(props, ...)` fails to
authenticate, `removeUncommittedMetadata` returns an error that just gets
joined and returned, and the orphaned metadata file is exactly what stays
behind. The table path already gets this right via `staged.FS` (built from
`c.props`).
I'd switch this to `io.LoadFSFunc(c.props, metadataLocation)`.
##########
catalog/sql/sql.go:
##########
@@ -627,6 +640,14 @@ func (c *Catalog) CreateTable(ctx context.Context, ident
table.Identifier, sc *i
return nil, fmt.Errorf("%w: %s", catalog.ErrNoSuchNamespace, ns)
}
+ viewExists, err := c.CheckViewExists(ctx, ident)
Review Comment:
The symmetry with `CreateView` isn't quite complete. `CreateView` now
returns typed sentinels for both collisions, but `CreateTable` only prechecks
the view case — a duplicate *table* still falls through to the raw DB
unique-constraint error rather than `ErrTableAlreadyExists`, so callers
switching on the error for idempotent-create or "already exists" handling will
misclassify it.
I'd add a `CheckTableExists` precheck right after this block. Worth a test
for the real duplicate-table case too — the trigger test forces every insert to
fail, which is a different path. wdyt?
##########
catalog/sql/sql_test.go:
##########
@@ -1972,6 +1972,95 @@ func (s *SqliteCatalogTestSuite) TestCreateView() {
s.True(exists)
}
+func (s *SqliteCatalogTestSuite) TestCreateTableConflictsWithView() {
+ ctx := context.Background()
+ db := s.getCatalogSqlite()
+ identifier := s.randomTableIdentifier()
+ s.Require().NoError(db.CreateNamespace(ctx,
catalog.NamespaceFromIdent(identifier), nil))
+ s.Require().NoError(db.CreateView(ctx, identifier, tableSchemaNested,
"SELECT 1", nil))
+
+ metadataDir := filepath.Join(s.warehouse, identifier[0]+".db",
identifier[1], "metadata")
+ before, err := os.ReadDir(metadataDir)
+ s.Require().NoError(err)
+
+ _, err = db.CreateTable(ctx, identifier, tableSchemaNested)
+ s.ErrorIs(err, catalog.ErrViewAlreadyExists)
+
+ after, err := os.ReadDir(metadataDir)
+ s.Require().NoError(err)
+ s.Len(after, len(before))
+}
+
+func (s *SqliteCatalogTestSuite) TestCreateViewConflictsWithTable() {
+ ctx := context.Background()
+ db := s.getCatalogSqlite()
+ identifier := s.randomTableIdentifier()
+ s.Require().NoError(db.CreateNamespace(ctx,
catalog.NamespaceFromIdent(identifier), nil))
+ tbl, err := db.CreateTable(ctx, identifier, tableSchemaNested)
+ s.Require().NoError(err)
+
s.Require().NoError(os.Remove(strings.TrimPrefix(tbl.MetadataLocation(),
"file://")))
Review Comment:
This `os.Remove` makes the file-count assertion prove nothing.
After deleting the metadata file, `before` is empty, `CreateView` bails at
the `CheckTableExists` guard before it ever writes metadata, so `after` is also
empty and `s.Len(after, len(before))` is just `0 == 0`. It never touches the
cleanup path, and it leaves the catalog in a state that can't occur in practice
(row present, file gone).
I'd drop the `os.Remove`, take `before` right after `CreateTable` (one file
present), then assert `after` still equals `before` after the failed
`CreateView`.
##########
catalog/sql/sql.go:
##########
@@ -919,16 +944,31 @@ func (c *Catalog) RenameTable(ctx context.Context, from,
to table.Identifier) (*
}
func (c *Catalog) CheckTableExists(ctx context.Context, identifier
table.Identifier) (bool, error) {
- _, err := c.LoadTable(ctx, identifier)
+ ns, err := c.namespaceKey(ctx, catalog.NamespaceFromIdent(identifier))
Review Comment:
This quietly changes the contract of a public interface method. Before, it
went through `LoadTable`, so a table with a missing or unreadable metadata file
returned false; now it's a catalog-row `EXISTS` check that returns true
whenever the row is present, regardless of metadata reachability.
That's the right behavior for collision detection and matches Java's
`JdbcUtil.tableExists`, but it's a behavioral break for anyone relying on the
old reachability semantics. I'd add a one-line doc comment here (and on the
interface in `catalog/catalog.go`) noting it checks catalog-registry existence,
not metadata-file validity.
--
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]
