This is an automated email from the ASF dual-hosted git repository.
laskoviymishka pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/iceberg-go.git
The following commit(s) were added to refs/heads/main by this push:
new 97720316a feat(catalog): add table lifecycle conformance tests and fix
rename-to-existing errors (#2165)
97720316a is described below
commit 97720316a4a206c600b9357c18d158d8eda570be
Author: Richie Bachala <[email protected]>
AuthorDate: Sun Oct 11 11:19:06 2026 -0700
feat(catalog): add table lifecycle conformance tests and fix
rename-to-existing errors (#2165)
* feat(catalog): add table lifecycle tests to the catalog conformance suite
Add ListTables, DropTable, DropMissingTable, RenameTable,
RenameTableToExisting and RenameMissingTable to catalogtest, gated by a
new Config.SupportsRenameTable flag. SQL, REST and Hive opt in; Hadoop
does not implement rename.
Related to #1691
* fix(catalog): return ErrTableAlreadyExists when renaming onto an existing
table
The new RenameTableToExisting conformance test found that REST and Hive
returned untyped errors for this case. REST now maps 409 to
ErrTableAlreadyExists, matching CreateTable. Hive checks the destination
while holding the rename locks, matching its create path.
Related to #1691
* fix(hive): detect non-Iceberg tables at rename destination
CheckTableExists only recognises Iceberg tables, so a non-Iceberg Hive
table at the destination was invisible to the pre-check and AlterTable
would return a raw metastore error. Switch to a raw GetTable call so
any table (Iceberg or not) at the destination is rejected with
ErrTableAlreadyExists.
Also strengthen the rename conformance test to assert that the table
UUID survives the rename, catching backends that silently recreate
instead of renaming.
* test(catalog): verify unsupported rename and rename metadata preservation
Address review feedback on the table lifecycle conformance tests:
- When SupportsRenameTable is false, run RenameTableNotSupported instead
of skipping the rename suite, asserting RenameTable returns
iceberg.ErrNotImplemented and leaves the source table in place. The
Hadoop catalog now wraps that sentinel. Reference #1097 in the field
doc.
- Assert that rename preserves the table metadata location alongside
its UUID, catching backends that rename by recreating the table.
Signed-off-by: Richie Bachala <[email protected]>
---------
Signed-off-by: Richie Bachala <[email protected]>
---
catalog/catalogtest/catalogtest.go | 17 ++++
catalog/catalogtest/tables.go | 198 +++++++++++++++++++++++++++++++++++++
catalog/hadoop/hadoop.go | 2 +-
catalog/hadoop/hadoop_test.go | 2 +-
catalog/hive/conformance_test.go | 1 +
catalog/hive/hive.go | 10 ++
catalog/hive/hive_test.go | 27 +++++
catalog/rest/conformance_test.go | 1 +
catalog/rest/rest.go | 2 +-
catalog/rest/rest_test.go | 23 +++++
catalog/sql/conformance_test.go | 1 +
11 files changed, 281 insertions(+), 3 deletions(-)
diff --git a/catalog/catalogtest/catalogtest.go
b/catalog/catalogtest/catalogtest.go
index 0521f24a2..79bf7a6e2 100644
--- a/catalog/catalogtest/catalogtest.go
+++ b/catalog/catalogtest/catalogtest.go
@@ -89,6 +89,12 @@ type Config struct {
// are directories with nowhere to keep them. Loading is required of
every
// catalog and is tested either way.
SupportsNamespaceProperties bool
+
+ // SupportsRenameTable reports whether the catalog can rename tables.
+ // When it is false, as it is for the Hadoop catalog until #1097 lands,
+ // the rename tests are replaced by one asserting that RenameTable
returns
+ // iceberg.ErrNotImplemented and leaves the source table in place.
+ SupportsRenameTable bool
}
// RunCatalogTests runs the conformance suite against the catalog described by
@@ -102,6 +108,17 @@ func RunCatalogTests(t *testing.T, cfg Config) {
t.Run("LoadMissingTable", func(t *testing.T) { testLoadMissingTable(t,
cfg) })
t.Run("LoadTableWithNonExistingNamespace", func(t *testing.T) {
testLoadTableWithNonExistingNamespace(t, cfg) })
+ t.Run("ListTables", func(t *testing.T) { testListTables(t, cfg) })
+ t.Run("DropTable", func(t *testing.T) { testDropTable(t, cfg) })
+ t.Run("DropMissingTable", func(t *testing.T) { testDropMissingTable(t,
cfg) })
+ if cfg.SupportsRenameTable {
+ t.Run("RenameTable", func(t *testing.T) { testRenameTable(t,
cfg) })
+ t.Run("RenameTableToExisting", func(t *testing.T) {
testRenameTableToExisting(t, cfg) })
+ t.Run("RenameMissingTable", func(t *testing.T) {
testRenameMissingTable(t, cfg) })
+ } else {
+ t.Run("RenameTableNotSupported", func(t *testing.T) {
testRenameTableNotSupported(t, cfg) })
+ }
+
t.Run("CreateNamespace", func(t *testing.T) { testCreateNamespace(t,
cfg) })
t.Run("CreateNamespaceThatAlreadyExists", func(t *testing.T) {
testCreateNamespaceThatAlreadyExists(t, cfg) })
t.Run("DropNamespace", func(t *testing.T) { testDropNamespace(t, cfg) })
diff --git a/catalog/catalogtest/tables.go b/catalog/catalogtest/tables.go
new file mode 100644
index 000000000..bfc03cc75
--- /dev/null
+++ b/catalog/catalogtest/tables.go
@@ -0,0 +1,198 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package catalogtest
+
+import (
+ "context"
+ "testing"
+
+ "github.com/apache/iceberg-go"
+ "github.com/apache/iceberg-go/catalog"
+ "github.com/apache/iceberg-go/table"
+ "github.com/stretchr/testify/assert"
+ "github.com/stretchr/testify/require"
+)
+
+// listTables collects every table identifier the catalog reports for
namespace.
+func listTables(t *testing.T, cat catalog.Catalog, namespace table.Identifier)
[]table.Identifier {
+ t.Helper()
+
+ var out []table.Identifier
+ for ident, err := range cat.ListTables(context.Background(), namespace)
{
+ require.NoError(t, err)
+ out = append(out, ident)
+ }
+
+ return out
+}
+
+// testListTables asserts that ListTables reflects creates and drops.
+func testListTables(t *testing.T, cfg Config) {
+ ctx := context.Background()
+ cat := cfg.NewCatalog(t)
+ namespace, ident := newIdentifiers()
+
+ require.NoError(t, cat.CreateNamespace(ctx, namespace, nil))
+ t.Cleanup(func() { _ = cat.DropNamespace(ctx, namespace) })
+
+ assert.Empty(t, listTables(t, cat, namespace), "new namespace should
have no tables")
+
+ _, err := cat.CreateTable(ctx, ident, Schema)
+ require.NoError(t, err)
+ t.Cleanup(func() { _ = cat.DropTable(ctx, ident) })
+
+ assert.Equal(t, []table.Identifier{ident}, listTables(t, cat,
namespace))
+
+ require.NoError(t, cat.DropTable(ctx, ident))
+ assert.Empty(t, listTables(t, cat, namespace), "dropped table should
not be listed")
+}
+
+// testDropTable asserts that a dropped table is no longer visible.
+func testDropTable(t *testing.T, cfg Config) {
+ ctx := context.Background()
+ cat := cfg.NewCatalog(t)
+ namespace, ident := newIdentifiers()
+
+ require.NoError(t, cat.CreateNamespace(ctx, namespace, nil))
+ t.Cleanup(func() { _ = cat.DropNamespace(ctx, namespace) })
+
+ _, err := cat.CreateTable(ctx, ident, Schema)
+ require.NoError(t, err)
+
+ require.NoError(t, cat.DropTable(ctx, ident))
+
+ exists, err := cat.CheckTableExists(ctx, ident)
+ require.NoError(t, err)
+ assert.False(t, exists, "table should not exist after drop")
+
+ _, err = cat.LoadTable(ctx, ident)
+ assert.ErrorIs(t, err, catalog.ErrNoSuchTable)
+}
+
+// testDropMissingTable asserts that dropping a table that was never created
+// reports that the table does not exist.
+func testDropMissingTable(t *testing.T, cfg Config) {
+ ctx := context.Background()
+ cat := cfg.NewCatalog(t)
+ namespace, ident := newIdentifiers()
+
+ require.NoError(t, cat.CreateNamespace(ctx, namespace, nil))
+ t.Cleanup(func() { _ = cat.DropNamespace(ctx, namespace) })
+
+ assert.ErrorIs(t, cat.DropTable(ctx, ident), catalog.ErrNoSuchTable)
+}
+
+// testRenameTableNotSupported asserts that a catalog without rename support
+// reports it with iceberg.ErrNotImplemented and leaves the source table in
+// place, so an unsupported rename is a verified behavior rather than a skip.
+func testRenameTableNotSupported(t *testing.T, cfg Config) {
+ ctx := context.Background()
+ cat := cfg.NewCatalog(t)
+ namespace, from := newIdentifiers()
+ to := table.Identifier{namespace[0], "renamed"}
+
+ require.NoError(t, cat.CreateNamespace(ctx, namespace, nil))
+ t.Cleanup(func() { _ = cat.DropNamespace(ctx, namespace) })
+
+ _, err := cat.CreateTable(ctx, from, Schema)
+ require.NoError(t, err)
+ t.Cleanup(func() { _ = cat.DropTable(ctx, from) })
+
+ _, err = cat.RenameTable(ctx, from, to)
+ assert.ErrorIs(t, err, iceberg.ErrNotImplemented)
+
+ exists, err := cat.CheckTableExists(ctx, from)
+ require.NoError(t, err)
+ assert.True(t, exists, "source table should still exist after an
unsupported rename")
+}
+
+// testRenameTable asserts that a renamed table is reachable only under its new
+// identifier and keeps its identity, metadata and schema.
+func testRenameTable(t *testing.T, cfg Config) {
+ ctx := context.Background()
+ cat := cfg.NewCatalog(t)
+ namespace, from := newIdentifiers()
+ to := table.Identifier{namespace[0], "renamed"}
+
+ require.NoError(t, cat.CreateNamespace(ctx, namespace, nil))
+ t.Cleanup(func() { _ = cat.DropNamespace(ctx, namespace) })
+
+ created, err := cat.CreateTable(ctx, from, Schema)
+ require.NoError(t, err)
+ t.Cleanup(func() { _ = cat.DropTable(ctx, from) })
+
+ originalUUID := created.Metadata().TableUUID()
+ originalMetadataLocation := created.MetadataLocation()
+
+ _, err = cat.RenameTable(ctx, from, to)
+ require.NoError(t, err)
+ t.Cleanup(func() { _ = cat.DropTable(ctx, to) })
+
+ exists, err := cat.CheckTableExists(ctx, from)
+ require.NoError(t, err)
+ assert.False(t, exists, "source table should not exist after rename")
+
+ tbl, err := cat.LoadTable(ctx, to)
+ require.NoError(t, err)
+ assert.Equal(t, to, tbl.Identifier())
+ assert.Equal(t, originalUUID, tbl.Metadata().TableUUID(), "rename must
preserve table identity")
+ assert.Equal(t, originalMetadataLocation, tbl.MetadataLocation(),
"rename should not rewrite table metadata")
+ assert.Equal(t, TableSchema.AsStruct(), tbl.Schema().AsStruct(),
"rename should keep the schema")
+}
+
+// testRenameTableToExisting asserts that renaming onto an existing table is
+// rejected and leaves both tables in place.
+func testRenameTableToExisting(t *testing.T, cfg Config) {
+ ctx := context.Background()
+ cat := cfg.NewCatalog(t)
+ namespace, from := newIdentifiers()
+ to := table.Identifier{namespace[0], "other"}
+
+ require.NoError(t, cat.CreateNamespace(ctx, namespace, nil))
+ t.Cleanup(func() { _ = cat.DropNamespace(ctx, namespace) })
+
+ for _, ident := range []table.Identifier{from, to} {
+ _, err := cat.CreateTable(ctx, ident, Schema)
+ require.NoError(t, err)
+ t.Cleanup(func() { _ = cat.DropTable(ctx, ident) })
+ }
+
+ _, err := cat.RenameTable(ctx, from, to)
+ assert.ErrorIs(t, err, catalog.ErrTableAlreadyExists)
+
+ for _, ident := range []table.Identifier{from, to} {
+ exists, err := cat.CheckTableExists(ctx, ident)
+ require.NoError(t, err)
+ assert.True(t, exists, "%v should still exist after a rejected
rename", ident)
+ }
+}
+
+// testRenameMissingTable asserts that renaming a table that was never created
+// reports that the table does not exist.
+func testRenameMissingTable(t *testing.T, cfg Config) {
+ ctx := context.Background()
+ cat := cfg.NewCatalog(t)
+ namespace, from := newIdentifiers()
+ to := table.Identifier{namespace[0], "renamed"}
+
+ require.NoError(t, cat.CreateNamespace(ctx, namespace, nil))
+ t.Cleanup(func() { _ = cat.DropNamespace(ctx, namespace) })
+
+ _, err := cat.RenameTable(ctx, from, to)
+ assert.ErrorIs(t, err, catalog.ErrNoSuchTable)
+}
diff --git a/catalog/hadoop/hadoop.go b/catalog/hadoop/hadoop.go
index ec25158f8..ecc718aa6 100644
--- a/catalog/hadoop/hadoop.go
+++ b/catalog/hadoop/hadoop.go
@@ -928,7 +928,7 @@ func (c *Catalog) PurgeTable(ctx context.Context,
identifier table.Identifier) e
}
func (c *Catalog) RenameTable(_ context.Context, _, _ table.Identifier)
(*table.Table, error) {
- return nil, errors.New("hadoop catalog: rename table is not supported")
+ return nil, fmt.Errorf("%w: hadoop catalog: rename table is not
supported", iceberg.ErrNotImplemented)
}
func (c *Catalog) CreateNamespace(_ context.Context, ns table.Identifier,
props iceberg.Properties) error {
diff --git a/catalog/hadoop/hadoop_test.go b/catalog/hadoop/hadoop_test.go
index 2007ab402..1390a34df 100644
--- a/catalog/hadoop/hadoop_test.go
+++ b/catalog/hadoop/hadoop_test.go
@@ -2105,7 +2105,7 @@ func (s *HadoopCatalogTestSuite)
TestDropTableNamespacePreserved() {
func (s *HadoopCatalogTestSuite) TestRenameTableUnsupported() {
_, err := s.cat.RenameTable(context.Background(), []string{"ns",
"old"}, []string{"ns", "new"})
- s.Require().Error(err)
+ s.Require().ErrorIs(err, iceberg.ErrNotImplemented)
s.Contains(err.Error(), "not supported")
}
diff --git a/catalog/hive/conformance_test.go b/catalog/hive/conformance_test.go
index 7aab243fe..46e4a2ae9 100644
--- a/catalog/hive/conformance_test.go
+++ b/catalog/hive/conformance_test.go
@@ -31,6 +31,7 @@ import (
func TestHiveCatalogConformance(t *testing.T) {
catalogtest.RunCatalogTests(t, catalogtest.Config{
SupportsNamespaceProperties: true,
+ SupportsRenameTable: true,
NewCatalog: func(t *testing.T) catalog.Catalog {
cat, err := NewCatalog(iceberg.Properties{
URI: getTestHiveURI(),
diff --git a/catalog/hive/hive.go b/catalog/hive/hive.go
index 30670bd96..2b00770f2 100644
--- a/catalog/hive/hive.go
+++ b/catalog/hive/hive.go
@@ -539,6 +539,16 @@ func (c *Catalog) RenameTable(ctx context.Context, from,
to table.Identifier) (_
table.ErrCommitFailed, fromDB, fromTable,
sourceMetadataLocation, lockedMetadataLocation)
}
+ // Use a raw GetTable instead of CheckTableExists so that non-Iceberg
+ // Hive tables at the destination are also detected as conflicts.
+ _, err = c.client.GetTable(ctx, toDB, toTable)
+ if err == nil {
+ return nil, fmt.Errorf("%w: %s.%s",
catalog.ErrTableAlreadyExists, toDB, toTable)
+ }
+ if !isNoSuchObjectError(err) {
+ return nil, fmt.Errorf("failed to check destination table
%s.%s: %w", toDB, toTable, err)
+ }
+
hiveTbl.TableName = toTable
hiveTbl.DbName = toDB
diff --git a/catalog/hive/hive_test.go b/catalog/hive/hive_test.go
index 85ab555e7..d8c06a684 100644
--- a/catalog/hive/hive_test.go
+++ b/catalog/hive/hive_test.go
@@ -891,6 +891,33 @@ func TestHiveRenameTableRejectsChangedMetadataAfterLock(t
*testing.T) {
mockClient.AssertExpectations(t)
}
+func TestHiveRenameTableToExistingTable(t *testing.T) {
+ mockClient := &mockHiveClient{}
+ destination := *testIcebergHiveTable1
+ destination.TableName = "renamed"
+ destination.DbName = "target_database"
+
+ mockClient.On("GetDatabase", mock.Anything, "target_database").
+ Return(&hive_metastore.Database{Name: "target_database"},
nil).Once()
+ mockClient.On("GetTable", mock.Anything, "test_database", "test_table").
+ Return(testIcebergHiveTable1, nil).Twice()
+ mockClient.On("Lock", mock.Anything, mock.Anything).
+ Return(&hive_metastore.LockResponse{Lockid: 2, State:
hive_metastore.LockState_ACQUIRED}, nil).Once()
+ mockClient.On("GetTable", mock.Anything, "target_database", "renamed").
+ Return(&destination, nil).Once()
+ mockClient.On("Unlock", mock.Anything, int64(2)).Return(nil).Once()
+ hiveCatalog := NewCatalogWithClient(mockClient, iceberg.Properties{})
+
+ _, err := hiveCatalog.RenameTable(
+ context.Background(),
+ TableIdentifier("test_database", "test_table"),
+ TableIdentifier("target_database", "renamed"),
+ )
+ require.ErrorIs(t, err, catalog.ErrTableAlreadyExists)
+ mockClient.AssertNotCalled(t, "AlterTable", mock.Anything,
mock.Anything, mock.Anything, mock.Anything)
+ mockClient.AssertExpectations(t)
+}
+
func TestHivePurgeTable(t *testing.T) {
assert := require.New(t)
ctx := context.Background()
diff --git a/catalog/rest/conformance_test.go b/catalog/rest/conformance_test.go
index 3e8bda67e..15f4e17f5 100644
--- a/catalog/rest/conformance_test.go
+++ b/catalog/rest/conformance_test.go
@@ -33,6 +33,7 @@ import (
func TestRestCatalogConformance(t *testing.T) {
catalogtest.RunCatalogTests(t, catalogtest.Config{
SupportsNamespaceProperties: true,
+ SupportsRenameTable: true,
NewCatalog: func(t *testing.T) catalog.Catalog {
cat, err := catalog.Load(context.Background(), "local",
iceberg.Properties{
"type": "rest",
diff --git a/catalog/rest/rest.go b/catalog/rest/rest.go
index 06bd3eaae..83c5d9855 100644
--- a/catalog/rest/rest.go
+++ b/catalog/rest/rest.go
@@ -1980,7 +1980,7 @@ func (r *Catalog) RenameTable(ctx context.Context, from,
to table.Identifier) (*
}
_, err = doPost[payload, any](ctx, r.baseURI, path, payload{Source:
src, Destination: dst}, r.cl,
- map[int]error{http.StatusNotFound: catalog.ErrNoSuchTable},
allowNoContent())
+ map[int]error{http.StatusNotFound: catalog.ErrNoSuchTable,
http.StatusConflict: catalog.ErrTableAlreadyExists}, allowNoContent())
if err != nil {
return nil, err
}
diff --git a/catalog/rest/rest_test.go b/catalog/rest/rest_test.go
index 0655da244..8ace73360 100644
--- a/catalog/rest/rest_test.go
+++ b/catalog/rest/rest_test.go
@@ -1944,6 +1944,29 @@ func (r *RestCatalogSuite)
TestLoadTableWithSnapshotModeAll() {
r.EqualValues(3497810964824022504, tbl.CurrentSnapshot().SnapshotID)
}
+func (r *RestCatalogSuite) TestRenameTable409() {
+ r.mux.HandleFunc("/v1/tables/rename", func(w http.ResponseWriter, req
*http.Request) {
+ if !r.Equal(http.MethodPost, req.Method) {
+ return
+ }
+
+ w.WriteHeader(http.StatusConflict)
+ json.NewEncoder(w).Encode(map[string]any{
+ "error": map[string]any{
+ "message": "Table already exists:
fokko.destination",
+ "type": "AlreadyExistsException",
+ "code": 409,
+ },
+ })
+ })
+
+ cat, err := rest.NewCatalog(context.Background(), "rest", r.srv.URL,
rest.WithOAuthToken(TestToken))
+ r.Require().NoError(err)
+
+ _, err = cat.RenameTable(context.Background(),
catalog.ToIdentifier("fokko", "source"), catalog.ToIdentifier("fokko",
"destination"))
+ r.ErrorIs(err, catalog.ErrTableAlreadyExists)
+}
+
func (r *RestCatalogSuite) TestRenameTable204() {
// Mock the rename table endpoint
r.mux.HandleFunc("/v1/tables/rename", func(w http.ResponseWriter, req
*http.Request) {
diff --git a/catalog/sql/conformance_test.go b/catalog/sql/conformance_test.go
index 2fca4fb89..2c4129041 100644
--- a/catalog/sql/conformance_test.go
+++ b/catalog/sql/conformance_test.go
@@ -33,6 +33,7 @@ import (
func TestSqlCatalogConformance(t *testing.T) {
catalogtest.RunCatalogTests(t, catalogtest.Config{
SupportsNamespaceProperties: true,
+ SupportsRenameTable: true,
NewCatalog: func(t *testing.T) catalog.Catalog {
warehouse := t.TempDir()
