laskoviymishka commented on code in PR #2165:
URL: https://github.com/apache/iceberg-go/pull/2165#discussion_r4232826127
##########
catalog/hive/hive.go:
##########
@@ -539,6 +539,14 @@ func (c *Catalog) RenameTable(ctx context.Context, from,
to table.Identifier) (_
table.ErrCommitFailed, fromDB, fromTable,
sourceMetadataLocation, lockedMetadataLocation)
}
+ destExists, err := c.CheckTableExists(ctx, to)
Review Comment:
CheckTableExists here returns isIcebergTable, so a non-Iceberg Hive table
sitting at `to` reads as "not there" and we fall through to AlterTable, which
hands back the raw metastore AlreadyExistsException as a generic rename
failure. That's the same ambiguity this PR is removing, just for a non-Iceberg
destination. I'd check the destination with a raw GetTable and treat any hit as
occupied (isNoSuchObjectError means free), so the ErrTableAlreadyExists mapping
covers both cases.
Worth flagging this is still a pre-check, not a guard: a create or Hive DDL
outside the lock, landing between the check and AlterTable, will still surface
the raw error. Java leans on mapping the AlterTable InvalidOperationException
instead of pre-checking, and doing that as a second layer would close the race
too. I'd treat that part as a follow-up, not a blocker here.
##########
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())
Review Comment:
The 409 to ErrTableAlreadyExists mapping is right and matches both the REST
spec and PyIceberg. Pre-existing and fine to leave: a 404 here can be a missing
source table or a missing destination namespace, and both map to
ErrNoSuchTable, so the suite can't distinguish them. If we want that precision
later, branch on the error type field.
##########
catalog/catalogtest/catalogtest.go:
##########
@@ -89,6 +89,11 @@ 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.
+ // Rename tests are skipped when it is false, as they are for the Hadoop
+ // catalog, which does not implement rename.
+ SupportsRenameTable bool
Review Comment:
This mirrors SupportsNamespaceProperties, so the shape is fine. The gap is
that when the flag is false the whole rename suite is skipped, so a catalog
that half-implements rename, or regresses, stays green instead of getting
caught. I'd have the false branch assert something positive instead:
RenameTable returns ErrNotImplemented, or at least an error with the source
left intact, so "unsupported" is a verified behavior rather than a silent
no-op. A pointer to #1097 in this doc comment would also help, since that's
when Hadoop flips to true.
##########
catalog/catalogtest/tables.go:
##########
@@ -0,0 +1,181 @@
+// 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/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)
+}
+
+func skipWithoutRenameTable(t *testing.T, cfg Config) {
+ t.Helper()
+ if !cfg.SupportsRenameTable {
+ t.Skip("catalog does not support renaming tables")
+ }
+}
+
+// testRenameTable asserts that a renamed table is reachable only under its new
+// identifier and keeps its schema.
+func testRenameTable(t *testing.T, cfg Config) {
+ skipWithoutRenameTable(t, cfg)
+
+ 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)
+ 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, TableSchema.AsStruct(), tbl.Schema().AsStruct(),
"rename should keep the schema")
Review Comment:
The schema check is good, but rename should preserve table identity too, and
that's the cheaper and more meaningful invariant to pin here. CreateTable's
return value is discarded up on line 119; capture it and assert
Metadata().TableUUID() and MetadataLocation() are unchanged after the rename.
That catches a backend that "renames" by recreating, which schema equality
alone wouldn't.
##########
catalog/catalogtest/tables.go:
##########
@@ -0,0 +1,181 @@
+// 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/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)
+}
+
+func skipWithoutRenameTable(t *testing.T, cfg Config) {
+ t.Helper()
+ if !cfg.SupportsRenameTable {
+ t.Skip("catalog does not support renaming tables")
+ }
+}
+
+// testRenameTable asserts that a renamed table is reachable only under its new
+// identifier and keeps its schema.
+func testRenameTable(t *testing.T, cfg Config) {
+ skipWithoutRenameTable(t, cfg)
+
+ 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)
+ 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, 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) {
+ skipWithoutRenameTable(t, cfg)
+
+ 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) {
+ skipWithoutRenameTable(t, cfg)
+
+ 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)
Review Comment:
If you want to push the parity goal further, the paths most likely to hide
cross-catalog divergence are the ones not exercised yet: rename to a missing
destination namespace (Hive checks before locking and returns
ErrNoSuchNamespace, exactly where parity tends to drift) and cross-namespace
rename. Both are natural follow-ups behind the same flag.
--
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]
