laskoviymishka commented on code in PR #1489:
URL: https://github.com/apache/iceberg-go/pull/1489#discussion_r3659912813
##########
catalog/sql/sql.go:
##########
@@ -793,6 +846,14 @@ func (c *Catalog) CreateTable(ctx context.Context, ident
table.Identifier, sc *i
return nil
})
if err != nil {
+ if collisionErr := c.checkIdentifierAvailableInCatalog(ctx, ns,
tblIdent, ident); errors.Is(collisionErr, catalog.ErrTableAlreadyExists) ||
Review Comment:
When the in-tx `checkIdentifierAvailable` already returned a sentinel, this
opens a second read tx and re-derives the same collision — an extra round-trip
on the contention-free duplicate case, which is the common one. I'd only
recheck when the error isn't already one of ours:
```go
if !errors.Is(err, catalog.ErrTableAlreadyExists) && !errors.Is(err,
catalog.ErrViewAlreadyExists) {
if collisionErr := c.checkIdentifierAvailableInCatalog(ctx, ns,
tblIdent, ident); ... {
err = collisionErr
}
}
```
Same thing over in `CreateView`.
##########
catalog/sql/sql.go:
##########
@@ -736,6 +735,56 @@ func checkValidNamespace(ident table.Identifier) error {
return nil
}
+func removeUncommittedMetadata(ctx context.Context, metadataLocation string,
loadFS table.FSysF) error {
+ fs, err := loadFS(context.WithoutCancel(ctx))
+ if err != nil {
+ return fmt.Errorf("failed to load filesystem while removing
uncommitted metadata %s: %w", metadataLocation, err)
+ }
+
+ if err := fs.Remove(metadataLocation); err != nil {
+ return fmt.Errorf("failed to remove uncommitted metadata %s:
%w", metadataLocation, err)
+ }
+
+ return nil
+}
+
+func (c *Catalog) checkIdentifierAvailable(
+ ctx context.Context, tx bun.Tx, namespace, name string, identifier
table.Identifier,
+) error {
+ entry := &sqlIcebergTable{
+ CatalogName: c.name,
+ TableNamespace: namespace,
+ TableName: name,
+ }
+ query := tx.NewSelect().Model(entry).WherePK()
+ if c.isV0() {
+ query = query.ExcludeColumn("iceberg_type")
+ }
+
+ err := query.Scan(ctx)
+ if errors.Is(err, sql.ErrNoRows) {
+ return nil
+ }
+ if err != nil {
+ return fmt.Errorf("error checking identifier availability: %w",
err)
+ }
+
+ identifierName := strings.Join(identifier, ".")
+ if !c.isV0() && entry.IcebergType.String == ViewType {
Review Comment:
This is correct today — a NULL `iceberg_type` on a legacy row gives
`Valid:false, String:""`, and `"" == ViewType` is false, so we fall through to
the table sentinel. But it leans on the zero-value of `String` when `Valid` is
false, and it's the only NullString read in this file that doesn't gate on
`.Valid` first.
I'd add the explicit check so a future driver that puts garbage in `String`
on a NULL column can't misclassify a legacy table row as a view:
```go
if !c.isV0() && entry.IcebergType.Valid && entry.IcebergType.String ==
ViewType {
```
##########
catalog/catalog.go:
##########
@@ -135,7 +135,8 @@ type Catalog interface {
// RenameTable tells the catalog to rename a given table by the
identifiers
// provided, and then loads and returns the destination table
RenameTable(ctx context.Context, from, to table.Identifier)
(*table.Table, error)
- // CheckTableExists returns if the table exists
+ // CheckTableExists reports whether the identifier is registered as a
table in
+ // the catalog. It does not validate that the table metadata file is
readable.
Review Comment:
This reads as an interface-wide guarantee, but the REST impl falls back to
`LoadTable` when the server has no HEAD endpoint, which does fetch and parse
the metadata file.
Since the SQL impl already carries its own doc comment saying this just
below, I'd scope the strict wording there and keep the interface doc looser
here. wdyt?
##########
catalog/sql/sql_test.go:
##########
@@ -2123,6 +2123,135 @@ 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))
+ _, err := db.CreateTable(ctx, identifier, tableSchemaNested)
+ s.Require().NoError(err)
+
+ metadataDir := filepath.Join(s.warehouse, identifier[0]+".db",
identifier[1], "metadata")
+ before, err := os.ReadDir(metadataDir)
+ s.Require().NoError(err)
+
+ err = db.CreateView(ctx, identifier, tableSchemaNested, "SELECT 1", nil)
+ s.ErrorIs(err, catalog.ErrTableAlreadyExists)
+
+ after, err := os.ReadDir(metadataDir)
+ s.Require().NoError(err)
+ s.Len(after, len(before))
+}
+
+func (s *SqliteCatalogTestSuite)
TestConcurrentTableViewCollisionReturnsCatalogSentinel() {
+ ctx := context.Background()
+ sqlDB := s.getDB()
+ _, err := sqlDB.Exec("PRAGMA journal_mode=WAL")
+ s.Require().NoError(err)
+ s.Require().NoError(sqlDB.Close())
+
+ db := s.getCatalogSqlite()
+ identifier := s.randomTableIdentifier()
+ s.Require().NoError(db.CreateNamespace(ctx,
catalog.NamespaceFromIdent(identifier), nil))
+
+ type createResult struct {
+ objectType string
+ err error
+ }
+ start := make(chan struct{})
+ results := make(chan createResult, 2)
+ go func() {
+ <-start
+ _, err := db.CreateTable(ctx, identifier, tableSchemaNested)
+ results <- createResult{objectType: sqlcat.TableType, err: err}
+ }()
+ go func() {
+ <-start
+ err := db.CreateView(ctx, identifier, tableSchemaNested,
"SELECT 1", nil)
+ results <- createResult{objectType: sqlcat.ViewType, err: err}
+ }()
+ close(start)
+
+ first, second := <-results, <-results
Review Comment:
If both creates happen to fail, the swap leaves `first.err` non-nil and this
just blows up here with no hint about what actually broke. Worth asserting
exactly one of the two succeeded (or at least a message on this line) so a
regression points at the real failure. wdyt?
--
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]
