You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "fallintoplace (via GitHub)" <gi...@apache.org> on 2026/07/23 12:13:08 UTC
[PR] Fix/glue reject nonempty namespace [iceberg-go]
fallintoplace opened a new pull request, #1520:
URL: https://github.com/apache/iceberg-go/pull/1520
## Summary
- check a Glue database for tables before dropping its namespace
- return `catalog.ErrNamespaceNotEmpty` when any table exists
- avoid deleting the database when Glue cannot determine whether it is empty
The check includes both Iceberg and non-Iceberg tables because deleting a Glue database makes all tables in that database inaccessible.
## Testing
- `go test ./catalog/glue`
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(glue): reject dropping non-empty namespaces [iceberg-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade merged PR #1520:
URL: https://github.com/apache/iceberg-go/pull/1520
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(glue): reject dropping non-empty namespaces [iceberg-go]
Posted by "laskoviymishka (via GitHub)" <gi...@apache.org>.
laskoviymishka commented on code in PR #1520:
URL: https://github.com/apache/iceberg-go/pull/1520#discussion_r3645170971
##########
catalog/glue/glue_test.go:
##########
@@ -907,6 +905,11 @@ func TestGlueDropNamespace(t *testing.T) {
},
}, nil).Once()
+ mockGlueSvc.On("GetTables", mock.Anything, &glue.GetTablesInput{
Review Comment:
All three new tests build the catalog as `&Catalog{glueSvc: mockGlueSvc}`, so `c.catalogId` is nil and none of the `GetTables` mock expectations pin `CatalogId`. The production call sets `CatalogId: c.catalogId` correctly — but it means the cross-account path is entirely uncovered, and a refactor that dropped the field would still go green.
Could we add a case that constructs the catalog with a non-nil `catalogId` and asserts it flows into the `GetTables` input? wdyt?
##########
catalog/glue/glue.go:
##########
@@ -685,6 +685,18 @@ func (c *Catalog) DropNamespace(ctx context.Context, namespace table.Identifier)
return err
}
+ tables, err := c.glueSvc.GetTables(ctx, &glue.GetTablesInput{
+ CatalogId: c.catalogId,
+ DatabaseName: aws.String(databaseName),
+ MaxResults: aws.Int32(1),
+ })
+ if err != nil {
+ return fmt.Errorf("failed to check namespace %s for tables: %w", databaseName, err)
+ }
+ if len(tables.TableList) > 0 {
Review Comment:
This is a best-effort guard — Glue has no conditional delete, so there's still a window where a concurrent `CreateTable` lands between this check and `DeleteDatabase`. That's unavoidable here and I'm fine shipping it, but I'd drop a one-line comment noting the check is best-effort and can race, so the next reader doesn't assume it's airtight.
##########
catalog/glue/glue.go:
##########
@@ -685,6 +685,18 @@ func (c *Catalog) DropNamespace(ctx context.Context, namespace table.Identifier)
return err
}
+ tables, err := c.glueSvc.GetTables(ctx, &glue.GetTablesInput{
+ CatalogId: c.catalogId,
+ DatabaseName: aws.String(databaseName),
+ MaxResults: aws.Int32(1),
+ })
+ if err != nil {
+ return fmt.Errorf("failed to check namespace %s for tables: %w", databaseName, err)
+ }
+ if len(tables.TableList) > 0 {
+ return fmt.Errorf("%w: %s", catalog.ErrNamespaceNotEmpty, databaseName)
Review Comment:
Java's `GlueCatalog` and PyIceberg both distinguish the message here — "still contains Iceberg tables" vs "non-Iceberg tables" — by reading the table's `table_type` parameter. Programmatic callers key off `errors.Is(ErrNamespaceNotEmpty)` either way, so this is purely about the human-readable signal.
Worth folding in while we're here, or leave it? wdyt?
##########
catalog/glue/glue_test.go:
##########
@@ -916,7 +919,60 @@ func TestGlueDropNamespace(t *testing.T) {
}
err := glueCatalog.DropNamespace(context.TODO(), DatabaseIdentifier("test_namespace"))
- assert.NoError(err)
+ require.NoError(t, err)
+ mockGlueSvc.AssertExpectations(t)
+}
+
+func TestGlueDropNamespaceNotEmpty(t *testing.T) {
+ for _, tableType := range []string{glueTypeIceberg, "EXTERNAL_TABLE"} {
+ t.Run(tableType, func(t *testing.T) {
+ mockGlueSvc := &mockGlueClient{}
+
+ mockGlueSvc.On("GetDatabase", mock.Anything, &glue.GetDatabaseInput{
+ Name: aws.String("test_namespace"),
+ }, mock.Anything).Return(&glue.GetDatabaseOutput{
+ Database: &types.Database{Name: aws.String("test_namespace")},
+ }, nil).Once()
+ mockGlueSvc.On("GetTables", mock.Anything, &glue.GetTablesInput{
+ DatabaseName: aws.String("test_namespace"),
+ MaxResults: aws.Int32(1),
+ }, mock.Anything).Return(&glue.GetTablesOutput{
+ TableList: []types.Table{{
+ Name: aws.String("test_table"),
+ Parameters: map[string]string{tableParamTableType: tableType},
Review Comment:
`Parameters` here isn't read by anything — production just does `len(tables.TableList) > 0` with no type filtering, so the fixture reads as if the table type matters when it doesn't. I'd either drop it, or keep it and wire up the Iceberg-vs-non-Iceberg message distinction I flagged over in glue.go so it's actually load-bearing. Either is fine, just not the middle state.
##########
catalog/glue/glue.go:
##########
@@ -685,6 +685,18 @@ func (c *Catalog) DropNamespace(ctx context.Context, namespace table.Identifier)
return err
}
+ tables, err := c.glueSvc.GetTables(ctx, &glue.GetTablesInput{
+ CatalogId: c.catalogId,
+ DatabaseName: aws.String(databaseName),
+ MaxResults: aws.Int32(1),
+ })
+ if err != nil {
+ return fmt.Errorf("failed to check namespace %s for tables: %w", databaseName, err)
Review Comment:
Small consistency thing: the sibling errors in this file follow "failed to <verb> <noun>: %w" — e.g. "failed to list tables in namespace %s". I'd match that here, something like `failed to list tables in namespace %s: %w`.
##########
catalog/glue/glue_test.go:
##########
@@ -916,7 +919,60 @@ func TestGlueDropNamespace(t *testing.T) {
}
err := glueCatalog.DropNamespace(context.TODO(), DatabaseIdentifier("test_namespace"))
- assert.NoError(err)
+ require.NoError(t, err)
+ mockGlueSvc.AssertExpectations(t)
+}
+
+func TestGlueDropNamespaceNotEmpty(t *testing.T) {
+ for _, tableType := range []string{glueTypeIceberg, "EXTERNAL_TABLE"} {
Review Comment:
`glueTypeIceberg` is a named constant sitting right next to a raw `"EXTERNAL_TABLE"`. If there's a matching constant for the external type I'd use it here for consistency; if there isn't one, no big deal.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org