laskoviymishka commented on code in PR #1285:
URL: https://github.com/apache/iceberg-go/pull/1285#discussion_r3466961841
##########
catalog/glue/glue_test.go:
##########
@@ -451,6 +459,116 @@ func TestGlueDropTable(t *testing.T) {
assert.NoError(err)
}
+func TestGluePurgeTable(t *testing.T) {
+ assert := require.New(t)
+ ctx := context.Background()
+ tableLocation := filepath.Join(t.TempDir(), "warehouse",
"test_database", "test_table")
+ metadataLocation, dataFile := writeGluePurgeTableFiles(t, tableLocation)
+ glueTable := gluePurgeTable(metadataLocation)
+
+ mockGlueSvc := &mockGlueClient{}
+ mockGlueSvc.On("GetTable", mock.Anything, &glue.GetTableInput{
+ DatabaseName: aws.String("test_database"),
+ Name: aws.String("test_table"),
+ }, mock.Anything).Return(&glue.GetTableOutput{Table: &glueTable},
nil).Twice()
+ mockGlueSvc.On("DeleteTable", mock.Anything, &glue.DeleteTableInput{
+ DatabaseName: aws.String("test_database"),
+ Name: aws.String("test_table"),
+ }, mock.Anything).Return(&glue.DeleteTableOutput{}, nil).Once()
+
+ glueCatalog := &Catalog{glueSvc: mockGlueSvc}
+
+ assert.NoError(glueCatalog.PurgeTable(ctx,
TableIdentifier("test_database", "test_table")))
+ assert.NoFileExists(metadataLocation)
+ assert.NoFileExists(dataFile)
+ mockGlueSvc.AssertExpectations(t)
+}
+
+func TestGluePurgeTableSwallowsPurgeFilesError(t *testing.T) {
+ assert := require.New(t)
+ ctx := context.Background()
+ const scheme = "gluepurgefail"
+ failingFS := failRemoveIO{MemFS: iceio.NewMemFS(), err:
errGluePurgeRemove}
+ iceio.Register(scheme, func(context.Context, *url.URL,
map[string]string) (iceio.IO, error) {
+ return failingFS, nil
+ })
+ t.Cleanup(func() { iceio.Unregister(scheme) })
+
+ tableLocation := scheme + "://bucket/test_database/test_table"
+ metadataLocation := tableLocation + "/metadata/v1.metadata.json"
+ dataFile := tableLocation + "/data/file.parquet"
+ writeGluePurgeTableMetadata(t, failingFS, tableLocation,
metadataLocation)
+ require.NoError(t, failingFS.WriteFile(dataFile, []byte("data")))
+ glueTable := gluePurgeTable(metadataLocation)
+
+ mockGlueSvc := &mockGlueClient{}
+ mockGlueSvc.On("GetTable", mock.Anything, &glue.GetTableInput{
+ DatabaseName: aws.String("test_database"),
+ Name: aws.String("test_table"),
+ }, mock.Anything).Return(&glue.GetTableOutput{Table: &glueTable},
nil).Twice()
+ mockGlueSvc.On("DeleteTable", mock.Anything, &glue.DeleteTableInput{
+ DatabaseName: aws.String("test_database"),
+ Name: aws.String("test_table"),
+ }, mock.Anything).Return(&glue.DeleteTableOutput{}, nil).Once()
+
+ var logs bytes.Buffer
+ originalLogOutput := log.Writer()
+ log.SetOutput(&logs)
+ t.Cleanup(func() { log.SetOutput(originalLogOutput) })
+
+ glueCatalog := &Catalog{glueSvc: mockGlueSvc}
+
+ assert.NoError(glueCatalog.PurgeTable(ctx,
TableIdentifier("test_database", "test_table")))
Review Comment:
These tests don't prove the contract they're named for. We assert the purge
error is swallowed, but nothing asserts `DeleteTable` actually ran *before* the
failing `Remove` — if someone reordered `PurgeTable` to purge first and drop
second, this test still passes and we'd miss the regression.
I'd record call order on the mock (testify `InOrder`, or a counter the
`Remove` stub reads) and assert the table was dropped before any `Remove`. Same
gap in the Hive twin.
##########
catalog/glue/glue_test.go:
##########
@@ -451,6 +459,116 @@ func TestGlueDropTable(t *testing.T) {
assert.NoError(err)
}
+func TestGluePurgeTable(t *testing.T) {
+ assert := require.New(t)
+ ctx := context.Background()
+ tableLocation := filepath.Join(t.TempDir(), "warehouse",
"test_database", "test_table")
+ metadataLocation, dataFile := writeGluePurgeTableFiles(t, tableLocation)
+ glueTable := gluePurgeTable(metadataLocation)
+
+ mockGlueSvc := &mockGlueClient{}
+ mockGlueSvc.On("GetTable", mock.Anything, &glue.GetTableInput{
+ DatabaseName: aws.String("test_database"),
+ Name: aws.String("test_table"),
+ }, mock.Anything).Return(&glue.GetTableOutput{Table: &glueTable},
nil).Twice()
+ mockGlueSvc.On("DeleteTable", mock.Anything, &glue.DeleteTableInput{
+ DatabaseName: aws.String("test_database"),
+ Name: aws.String("test_table"),
+ }, mock.Anything).Return(&glue.DeleteTableOutput{}, nil).Once()
+
+ glueCatalog := &Catalog{glueSvc: mockGlueSvc}
+
+ assert.NoError(glueCatalog.PurgeTable(ctx,
TableIdentifier("test_database", "test_table")))
+ assert.NoFileExists(metadataLocation)
+ assert.NoFileExists(dataFile)
+ mockGlueSvc.AssertExpectations(t)
+}
+
+func TestGluePurgeTableSwallowsPurgeFilesError(t *testing.T) {
+ assert := require.New(t)
+ ctx := context.Background()
+ const scheme = "gluepurgefail"
+ failingFS := failRemoveIO{MemFS: iceio.NewMemFS(), err:
errGluePurgeRemove}
+ iceio.Register(scheme, func(context.Context, *url.URL,
map[string]string) (iceio.IO, error) {
+ return failingFS, nil
+ })
+ t.Cleanup(func() { iceio.Unregister(scheme) })
+
+ tableLocation := scheme + "://bucket/test_database/test_table"
+ metadataLocation := tableLocation + "/metadata/v1.metadata.json"
+ dataFile := tableLocation + "/data/file.parquet"
+ writeGluePurgeTableMetadata(t, failingFS, tableLocation,
metadataLocation)
+ require.NoError(t, failingFS.WriteFile(dataFile, []byte("data")))
+ glueTable := gluePurgeTable(metadataLocation)
+
+ mockGlueSvc := &mockGlueClient{}
+ mockGlueSvc.On("GetTable", mock.Anything, &glue.GetTableInput{
+ DatabaseName: aws.String("test_database"),
+ Name: aws.String("test_table"),
+ }, mock.Anything).Return(&glue.GetTableOutput{Table: &glueTable},
nil).Twice()
+ mockGlueSvc.On("DeleteTable", mock.Anything, &glue.DeleteTableInput{
+ DatabaseName: aws.String("test_database"),
+ Name: aws.String("test_table"),
+ }, mock.Anything).Return(&glue.DeleteTableOutput{}, nil).Once()
+
+ var logs bytes.Buffer
+ originalLogOutput := log.Writer()
+ log.SetOutput(&logs)
+ t.Cleanup(func() { log.SetOutput(originalLogOutput) })
+
+ glueCatalog := &Catalog{glueSvc: mockGlueSvc}
+
+ assert.NoError(glueCatalog.PurgeTable(ctx,
TableIdentifier("test_database", "test_table")))
+ assert.Contains(logs.String(), "failed to purge files")
+ assert.Contains(logs.String(), errGluePurgeRemove.Error())
+ file, err := failingFS.Open(dataFile)
+ assert.NoError(err, "data file should remain when FileIO remove fails")
+ assert.NoError(file.Close())
+ mockGlueSvc.AssertExpectations(t)
+}
+
+type failRemoveIO struct {
+ *iceio.MemFS
+ err error
+}
+
+func (f failRemoveIO) Remove(string) error {
+ return f.err
+}
+
+func gluePurgeTable(metadataLocation string) types.Table {
+ return types.Table{
+ Name: aws.String("test_table"),
+ DatabaseName: aws.String("test_database"),
+ TableType: aws.String("EXTERNAL_TABLE"),
+ Parameters: map[string]string{
+ tableParamTableType: glueTypeIceberg,
+ tableParamMetadataLocation: metadataLocation,
+ },
+ }
+}
+
+func writeGluePurgeTableFiles(t *testing.T, tableLocation string)
(metadataLocation, dataFile string) {
+ t.Helper()
+ metadataLocation = filepath.Join(tableLocation, "metadata",
"v1.metadata.json")
+ dataFile = filepath.Join(tableLocation, "data", "file.parquet")
+ writeGluePurgeTableMetadata(t, iceio.LocalFS{}, tableLocation,
metadataLocation)
+ require.NoError(t, os.MkdirAll(filepath.Dir(dataFile), 0o755))
+ require.NoError(t, os.WriteFile(dataFile, []byte("data"), 0o644))
+
+ return metadataLocation, dataFile
+}
+
+func writeGluePurgeTableMetadata(t *testing.T, fs iceio.WriteFileIO,
tableLocation, metadataLocation string) {
+ t.Helper()
+ schema := iceberg.NewSchema(1, iceberg.NestedField{
+ ID: 1, Name: "foo", Type: iceberg.PrimitiveTypes.String,
Required: true,
+ })
+ meta, err := table.NewMetadata(schema, iceberg.UnpartitionedSpec,
table.UnsortedSortOrder, tableLocation, nil)
Review Comment:
The `gc.enabled=false` branch has no coverage in either suite. `PurgeFiles`
skips data-file removal when `gc.enabled=false` but always removes metadata —
and that's exactly the branch a purge test should pin down, since it's the one
place we deliberately keep data.
The default is `true`, so the happy path only exercises the `true` side. I'd
add a case that builds metadata with `iceberg.Properties{"gc.enabled":
"false"}` and asserts the metadata file is gone but the data file survives.
##########
catalog/glue/glue_test.go:
##########
@@ -451,6 +459,116 @@ func TestGlueDropTable(t *testing.T) {
assert.NoError(err)
}
+func TestGluePurgeTable(t *testing.T) {
+ assert := require.New(t)
+ ctx := context.Background()
+ tableLocation := filepath.Join(t.TempDir(), "warehouse",
"test_database", "test_table")
+ metadataLocation, dataFile := writeGluePurgeTableFiles(t, tableLocation)
+ glueTable := gluePurgeTable(metadataLocation)
+
+ mockGlueSvc := &mockGlueClient{}
+ mockGlueSvc.On("GetTable", mock.Anything, &glue.GetTableInput{
+ DatabaseName: aws.String("test_database"),
+ Name: aws.String("test_table"),
+ }, mock.Anything).Return(&glue.GetTableOutput{Table: &glueTable},
nil).Twice()
+ mockGlueSvc.On("DeleteTable", mock.Anything, &glue.DeleteTableInput{
+ DatabaseName: aws.String("test_database"),
+ Name: aws.String("test_table"),
+ }, mock.Anything).Return(&glue.DeleteTableOutput{}, nil).Once()
+
+ glueCatalog := &Catalog{glueSvc: mockGlueSvc}
+
+ assert.NoError(glueCatalog.PurgeTable(ctx,
TableIdentifier("test_database", "test_table")))
+ assert.NoFileExists(metadataLocation)
+ assert.NoFileExists(dataFile)
+ mockGlueSvc.AssertExpectations(t)
+}
+
+func TestGluePurgeTableSwallowsPurgeFilesError(t *testing.T) {
+ assert := require.New(t)
+ ctx := context.Background()
+ const scheme = "gluepurgefail"
+ failingFS := failRemoveIO{MemFS: iceio.NewMemFS(), err:
errGluePurgeRemove}
+ iceio.Register(scheme, func(context.Context, *url.URL,
map[string]string) (iceio.IO, error) {
+ return failingFS, nil
+ })
+ t.Cleanup(func() { iceio.Unregister(scheme) })
+
+ tableLocation := scheme + "://bucket/test_database/test_table"
+ metadataLocation := tableLocation + "/metadata/v1.metadata.json"
+ dataFile := tableLocation + "/data/file.parquet"
+ writeGluePurgeTableMetadata(t, failingFS, tableLocation,
metadataLocation)
+ require.NoError(t, failingFS.WriteFile(dataFile, []byte("data")))
+ glueTable := gluePurgeTable(metadataLocation)
+
+ mockGlueSvc := &mockGlueClient{}
+ mockGlueSvc.On("GetTable", mock.Anything, &glue.GetTableInput{
+ DatabaseName: aws.String("test_database"),
+ Name: aws.String("test_table"),
+ }, mock.Anything).Return(&glue.GetTableOutput{Table: &glueTable},
nil).Twice()
+ mockGlueSvc.On("DeleteTable", mock.Anything, &glue.DeleteTableInput{
+ DatabaseName: aws.String("test_database"),
+ Name: aws.String("test_table"),
+ }, mock.Anything).Return(&glue.DeleteTableOutput{}, nil).Once()
+
+ var logs bytes.Buffer
+ originalLogOutput := log.Writer()
+ log.SetOutput(&logs)
+ t.Cleanup(func() { log.SetOutput(originalLogOutput) })
+
+ glueCatalog := &Catalog{glueSvc: mockGlueSvc}
+
+ assert.NoError(glueCatalog.PurgeTable(ctx,
TableIdentifier("test_database", "test_table")))
+ assert.Contains(logs.String(), "failed to purge files")
Review Comment:
The substring here is loose enough to false-pass. The actual line is
`WARNING: dropped table %s but failed to purge files: %v`, and since
`log.SetOutput` redirects a process-global buffer, any other line containing
"failed to purge files" lands in the same `logs`.
I'd anchor it to the specific call — assert `Contains` on `WARNING: dropped
table` (or the full `fmt.Sprintf` with the table identifier) so it's tied to
this purge and not just any warning that happens to share the phrase.
##########
catalog/glue/glue_test.go:
##########
@@ -451,6 +459,116 @@ func TestGlueDropTable(t *testing.T) {
assert.NoError(err)
}
+func TestGluePurgeTable(t *testing.T) {
+ assert := require.New(t)
+ ctx := context.Background()
+ tableLocation := filepath.Join(t.TempDir(), "warehouse",
"test_database", "test_table")
+ metadataLocation, dataFile := writeGluePurgeTableFiles(t, tableLocation)
+ glueTable := gluePurgeTable(metadataLocation)
+
+ mockGlueSvc := &mockGlueClient{}
+ mockGlueSvc.On("GetTable", mock.Anything, &glue.GetTableInput{
+ DatabaseName: aws.String("test_database"),
+ Name: aws.String("test_table"),
+ }, mock.Anything).Return(&glue.GetTableOutput{Table: &glueTable},
nil).Twice()
+ mockGlueSvc.On("DeleteTable", mock.Anything, &glue.DeleteTableInput{
+ DatabaseName: aws.String("test_database"),
+ Name: aws.String("test_table"),
+ }, mock.Anything).Return(&glue.DeleteTableOutput{}, nil).Once()
+
+ glueCatalog := &Catalog{glueSvc: mockGlueSvc}
+
+ assert.NoError(glueCatalog.PurgeTable(ctx,
TableIdentifier("test_database", "test_table")))
+ assert.NoFileExists(metadataLocation)
+ assert.NoFileExists(dataFile)
+ mockGlueSvc.AssertExpectations(t)
+}
+
+func TestGluePurgeTableSwallowsPurgeFilesError(t *testing.T) {
+ assert := require.New(t)
+ ctx := context.Background()
+ const scheme = "gluepurgefail"
+ failingFS := failRemoveIO{MemFS: iceio.NewMemFS(), err:
errGluePurgeRemove}
+ iceio.Register(scheme, func(context.Context, *url.URL,
map[string]string) (iceio.IO, error) {
Review Comment:
`iceio.Register` panics if the scheme is already registered, and there's no
guard for a re-run where cleanup didn't fire. If anything between here and
`t.Cleanup` panics, the scheme stays registered and the next run panics on this
line.
Minor, but `defer iceio.Unregister(scheme)` immediately after `Register` (or
a register-if-absent) closes it. Same in the Hive test.
##########
catalog/glue/glue_test.go:
##########
@@ -451,6 +459,116 @@ func TestGlueDropTable(t *testing.T) {
assert.NoError(err)
}
+func TestGluePurgeTable(t *testing.T) {
+ assert := require.New(t)
+ ctx := context.Background()
+ tableLocation := filepath.Join(t.TempDir(), "warehouse",
"test_database", "test_table")
+ metadataLocation, dataFile := writeGluePurgeTableFiles(t, tableLocation)
+ glueTable := gluePurgeTable(metadataLocation)
+
+ mockGlueSvc := &mockGlueClient{}
+ mockGlueSvc.On("GetTable", mock.Anything, &glue.GetTableInput{
+ DatabaseName: aws.String("test_database"),
+ Name: aws.String("test_table"),
+ }, mock.Anything).Return(&glue.GetTableOutput{Table: &glueTable},
nil).Twice()
+ mockGlueSvc.On("DeleteTable", mock.Anything, &glue.DeleteTableInput{
+ DatabaseName: aws.String("test_database"),
+ Name: aws.String("test_table"),
+ }, mock.Anything).Return(&glue.DeleteTableOutput{}, nil).Once()
+
+ glueCatalog := &Catalog{glueSvc: mockGlueSvc}
+
+ assert.NoError(glueCatalog.PurgeTable(ctx,
TableIdentifier("test_database", "test_table")))
+ assert.NoFileExists(metadataLocation)
+ assert.NoFileExists(dataFile)
+ mockGlueSvc.AssertExpectations(t)
+}
+
+func TestGluePurgeTableSwallowsPurgeFilesError(t *testing.T) {
+ assert := require.New(t)
+ ctx := context.Background()
+ const scheme = "gluepurgefail"
+ failingFS := failRemoveIO{MemFS: iceio.NewMemFS(), err:
errGluePurgeRemove}
+ iceio.Register(scheme, func(context.Context, *url.URL,
map[string]string) (iceio.IO, error) {
+ return failingFS, nil
+ })
+ t.Cleanup(func() { iceio.Unregister(scheme) })
+
+ tableLocation := scheme + "://bucket/test_database/test_table"
+ metadataLocation := tableLocation + "/metadata/v1.metadata.json"
+ dataFile := tableLocation + "/data/file.parquet"
+ writeGluePurgeTableMetadata(t, failingFS, tableLocation,
metadataLocation)
+ require.NoError(t, failingFS.WriteFile(dataFile, []byte("data")))
+ glueTable := gluePurgeTable(metadataLocation)
+
+ mockGlueSvc := &mockGlueClient{}
+ mockGlueSvc.On("GetTable", mock.Anything, &glue.GetTableInput{
+ DatabaseName: aws.String("test_database"),
+ Name: aws.String("test_table"),
+ }, mock.Anything).Return(&glue.GetTableOutput{Table: &glueTable},
nil).Twice()
+ mockGlueSvc.On("DeleteTable", mock.Anything, &glue.DeleteTableInput{
+ DatabaseName: aws.String("test_database"),
+ Name: aws.String("test_table"),
+ }, mock.Anything).Return(&glue.DeleteTableOutput{}, nil).Once()
+
+ var logs bytes.Buffer
+ originalLogOutput := log.Writer()
+ log.SetOutput(&logs)
Review Comment:
`log.SetOutput` mutates process-global state with nothing guarding against
parallel execution — a future `t.Parallel()` or `go test -count=2` would race
this `bytes.Buffer`. The existing `table_test.go` does the same thing with
`defer` rather than `t.Cleanup`, so the PR also mixes the two conventions for
one pattern.
The clean fix is to assert only that `PurgeTable` returns nil (the real
contract) and drop the log inspection, or inject a `*slog.Logger` you can scope
per-test. If we keep the global redirect, I'd at least drop a `// do not add
t.Parallel()` note on it.
##########
catalog/glue/glue_test.go:
##########
@@ -451,6 +459,116 @@ func TestGlueDropTable(t *testing.T) {
assert.NoError(err)
}
+func TestGluePurgeTable(t *testing.T) {
+ assert := require.New(t)
+ ctx := context.Background()
+ tableLocation := filepath.Join(t.TempDir(), "warehouse",
"test_database", "test_table")
+ metadataLocation, dataFile := writeGluePurgeTableFiles(t, tableLocation)
+ glueTable := gluePurgeTable(metadataLocation)
+
+ mockGlueSvc := &mockGlueClient{}
+ mockGlueSvc.On("GetTable", mock.Anything, &glue.GetTableInput{
+ DatabaseName: aws.String("test_database"),
+ Name: aws.String("test_table"),
+ }, mock.Anything).Return(&glue.GetTableOutput{Table: &glueTable},
nil).Twice()
+ mockGlueSvc.On("DeleteTable", mock.Anything, &glue.DeleteTableInput{
+ DatabaseName: aws.String("test_database"),
+ Name: aws.String("test_table"),
+ }, mock.Anything).Return(&glue.DeleteTableOutput{}, nil).Once()
+
+ glueCatalog := &Catalog{glueSvc: mockGlueSvc}
+
+ assert.NoError(glueCatalog.PurgeTable(ctx,
TableIdentifier("test_database", "test_table")))
+ assert.NoFileExists(metadataLocation)
+ assert.NoFileExists(dataFile)
+ mockGlueSvc.AssertExpectations(t)
+}
+
+func TestGluePurgeTableSwallowsPurgeFilesError(t *testing.T) {
+ assert := require.New(t)
+ ctx := context.Background()
+ const scheme = "gluepurgefail"
+ failingFS := failRemoveIO{MemFS: iceio.NewMemFS(), err:
errGluePurgeRemove}
+ iceio.Register(scheme, func(context.Context, *url.URL,
map[string]string) (iceio.IO, error) {
+ return failingFS, nil
+ })
+ t.Cleanup(func() { iceio.Unregister(scheme) })
+
+ tableLocation := scheme + "://bucket/test_database/test_table"
+ metadataLocation := tableLocation + "/metadata/v1.metadata.json"
+ dataFile := tableLocation + "/data/file.parquet"
+ writeGluePurgeTableMetadata(t, failingFS, tableLocation,
metadataLocation)
+ require.NoError(t, failingFS.WriteFile(dataFile, []byte("data")))
Review Comment:
Small consistency thing: the test body aliases `assert := require.New(t)`
and uses `assert.NoError(...)` everywhere, then reaches back into package-level
`require.NoError(t, ...)` right here. I'd use
`assert.NoError(failingFS.WriteFile(...))` to match. (The helpers without the
alias are fine as-is.)
##########
catalog/glue/glue_test.go:
##########
@@ -451,6 +459,116 @@ func TestGlueDropTable(t *testing.T) {
assert.NoError(err)
}
+func TestGluePurgeTable(t *testing.T) {
+ assert := require.New(t)
+ ctx := context.Background()
+ tableLocation := filepath.Join(t.TempDir(), "warehouse",
"test_database", "test_table")
+ metadataLocation, dataFile := writeGluePurgeTableFiles(t, tableLocation)
+ glueTable := gluePurgeTable(metadataLocation)
+
+ mockGlueSvc := &mockGlueClient{}
+ mockGlueSvc.On("GetTable", mock.Anything, &glue.GetTableInput{
+ DatabaseName: aws.String("test_database"),
+ Name: aws.String("test_table"),
+ }, mock.Anything).Return(&glue.GetTableOutput{Table: &glueTable},
nil).Twice()
+ mockGlueSvc.On("DeleteTable", mock.Anything, &glue.DeleteTableInput{
+ DatabaseName: aws.String("test_database"),
+ Name: aws.String("test_table"),
+ }, mock.Anything).Return(&glue.DeleteTableOutput{}, nil).Once()
+
+ glueCatalog := &Catalog{glueSvc: mockGlueSvc}
+
+ assert.NoError(glueCatalog.PurgeTable(ctx,
TableIdentifier("test_database", "test_table")))
+ assert.NoFileExists(metadataLocation)
+ assert.NoFileExists(dataFile)
+ mockGlueSvc.AssertExpectations(t)
+}
+
+func TestGluePurgeTableSwallowsPurgeFilesError(t *testing.T) {
+ assert := require.New(t)
+ ctx := context.Background()
+ const scheme = "gluepurgefail"
+ failingFS := failRemoveIO{MemFS: iceio.NewMemFS(), err:
errGluePurgeRemove}
+ iceio.Register(scheme, func(context.Context, *url.URL,
map[string]string) (iceio.IO, error) {
+ return failingFS, nil
+ })
+ t.Cleanup(func() { iceio.Unregister(scheme) })
+
+ tableLocation := scheme + "://bucket/test_database/test_table"
+ metadataLocation := tableLocation + "/metadata/v1.metadata.json"
+ dataFile := tableLocation + "/data/file.parquet"
+ writeGluePurgeTableMetadata(t, failingFS, tableLocation,
metadataLocation)
+ require.NoError(t, failingFS.WriteFile(dataFile, []byte("data")))
+ glueTable := gluePurgeTable(metadataLocation)
+
+ mockGlueSvc := &mockGlueClient{}
+ mockGlueSvc.On("GetTable", mock.Anything, &glue.GetTableInput{
+ DatabaseName: aws.String("test_database"),
+ Name: aws.String("test_table"),
+ }, mock.Anything).Return(&glue.GetTableOutput{Table: &glueTable},
nil).Twice()
+ mockGlueSvc.On("DeleteTable", mock.Anything, &glue.DeleteTableInput{
+ DatabaseName: aws.String("test_database"),
+ Name: aws.String("test_table"),
+ }, mock.Anything).Return(&glue.DeleteTableOutput{}, nil).Once()
+
+ var logs bytes.Buffer
+ originalLogOutput := log.Writer()
+ log.SetOutput(&logs)
+ t.Cleanup(func() { log.SetOutput(originalLogOutput) })
+
+ glueCatalog := &Catalog{glueSvc: mockGlueSvc}
+
+ assert.NoError(glueCatalog.PurgeTable(ctx,
TableIdentifier("test_database", "test_table")))
+ assert.Contains(logs.String(), "failed to purge files")
+ assert.Contains(logs.String(), errGluePurgeRemove.Error())
+ file, err := failingFS.Open(dataFile)
Review Comment:
`file.Close()` on the next line runs without a nil guard — it's only safe
because the `assert` alias is `require.New` (FailNow on the preceding line). If
that alias ever became plain `testify/assert`, this panics on a nil `file`. A
`require.NotNil(t, file)` after `Open` makes the safety explicit rather than
load-bearing on the alias.
##########
catalog/hive/hive_test.go:
##########
@@ -523,6 +530,114 @@ func TestHiveDropTable(t *testing.T) {
mockClient.AssertExpectations(t)
}
+func TestHivePurgeTable(t *testing.T) {
+ assert := require.New(t)
+ ctx := context.Background()
+ tableLocation := filepath.Join(t.TempDir(), "warehouse",
"test_database", "test_table")
+ metadataLocation, dataFile := writeHivePurgeTableFiles(t, tableLocation)
+ hiveTable := hivePurgeTable(metadataLocation, tableLocation)
+
+ mockClient := &mockHiveClient{}
+ mockClient.On("GetTable", mock.Anything, "test_database", "test_table").
+ Return(hiveTable, nil).Twice()
+ mockClient.On("DropTable", mock.Anything, "test_database",
"test_table", false).
+ Return(nil).Once()
+
+ hiveCatalog := NewCatalogWithClient(mockClient, iceberg.Properties{})
+
+ assert.NoError(hiveCatalog.PurgeTable(ctx,
TableIdentifier("test_database", "test_table")))
+ assert.NoFileExists(metadataLocation)
+ assert.NoFileExists(dataFile)
+ mockClient.AssertExpectations(t)
+}
+
+func TestHivePurgeTableSwallowsPurgeFilesError(t *testing.T) {
+ assert := require.New(t)
+ ctx := context.Background()
+ const scheme = "hivepurgefail"
+ failingFS := failRemoveIO{MemFS: iceio.NewMemFS(), err:
errHivePurgeRemove}
+ iceio.Register(scheme, func(context.Context, *url.URL,
map[string]string) (iceio.IO, error) {
+ return failingFS, nil
+ })
+ t.Cleanup(func() { iceio.Unregister(scheme) })
+
+ tableLocation := scheme + "://bucket/test_database/test_table"
+ metadataLocation := tableLocation + "/metadata/v1.metadata.json"
+ dataFile := tableLocation + "/data/file.parquet"
+ writeHivePurgeTableMetadata(t, failingFS, tableLocation,
metadataLocation)
+ require.NoError(t, failingFS.WriteFile(dataFile, []byte("data")))
+ hiveTable := hivePurgeTable(metadataLocation, tableLocation)
+
+ mockClient := &mockHiveClient{}
+ mockClient.On("GetTable", mock.Anything, "test_database", "test_table").
+ Return(hiveTable, nil).Twice()
+ mockClient.On("DropTable", mock.Anything, "test_database",
"test_table", false).
+ Return(nil).Once()
+
+ var logs bytes.Buffer
+ originalLogOutput := log.Writer()
+ log.SetOutput(&logs)
+ t.Cleanup(func() { log.SetOutput(originalLogOutput) })
+
+ hiveCatalog := NewCatalogWithClient(mockClient, iceberg.Properties{})
+
+ assert.NoError(hiveCatalog.PurgeTable(ctx,
TableIdentifier("test_database", "test_table")))
+ assert.Contains(logs.String(), "failed to purge files")
+ assert.Contains(logs.String(), errHivePurgeRemove.Error())
+ file, err := failingFS.Open(dataFile)
+ assert.NoError(err, "data file should remain when FileIO remove fails")
+ assert.NoError(file.Close())
+ mockClient.AssertExpectations(t)
+}
+
+type failRemoveIO struct {
+ *iceio.MemFS
+ err error
+}
+
+func (f failRemoveIO) Remove(string) error {
+ return f.err
+}
+
+func hivePurgeTable(metadataLocation, tableLocation string)
*hive_metastore.Table {
+ return &hive_metastore.Table{
+ TableName: "test_table",
+ DbName: "test_database",
+ TableType: TableTypeExternalTable,
+ Parameters: map[string]string{
Review Comment:
This fixture omits `EXTERNAL=TRUE` and the `storage_handler` parameter. A
real Iceberg table in Hive — whether written by Java's `HiveOperationsBase` or
this client's `catalog/hive/schema.go` — always carries both; with only
`table_type=ICEBERG` and the metadata location, the fixture models a table that
could only exist from an incomplete writer.
Not blocking the purge logic under test, but adding the two keys makes it
the round-trip shape we actually produce. 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]
