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: [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]
