laskoviymishka commented on code in PR #2172:
URL: https://github.com/apache/iceberg-go/pull/2172#discussion_r4241789290
##########
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 false the suite verifies that RenameTable returns
+ // iceberg.ErrNotImplemented, and the remaining rename subtests are
+ // skipped. See #1097 for Hadoop rename support.
+ SupportsRenameTable bool
Review Comment:
The zero value here is load-bearing: false doesn't mean "rename untested",
it means `RenameTable` must return `iceberg.ErrNotImplemented`. I'd spell that
out in the comment, since any new adopter picking up `Config` at its default
inherits that assertion.
##########
catalog/catalogtest/tables.go:
##########
@@ -0,0 +1,205 @@
+// 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"
+
+ iceberg "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)
+}
+
+func skipWithoutRenameTable(t *testing.T, cfg Config) {
+ t.Helper()
+ if !cfg.SupportsRenameTable {
+ t.Skip("catalog does not support renaming tables")
+ }
+}
+
+// testRenameTableNotSupported asserts that a catalog which does not support
+// rename returns ErrNotImplemented rather than silently succeeding or
+// returning an untyped error.
+func testRenameTableNotSupported(t *testing.T, cfg Config) {
+ if cfg.SupportsRenameTable {
+ t.Skip("catalog supports renaming tables")
+ }
+
+ 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:
This asserts the unsupported contract against a table that was never
created, so it only passes for a catalog that returns `ErrNotImplemented`
before it looks at the source. A perfectly valid unsupported impl that
validates the source first would return `ErrNoSuchTable` here and fail the
suite, and a stub that short-circuits passes trivially without proving
anything. I'd create `from` first, rename a real existing table, then assert
the source still exists afterward so a silent "rename succeeded" regression
also gets caught.
##########
catalog/catalogtest/tables.go:
##########
@@ -0,0 +1,205 @@
+// 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"
+
+ iceberg "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)
+}
+
+func skipWithoutRenameTable(t *testing.T, cfg Config) {
+ t.Helper()
+ if !cfg.SupportsRenameTable {
+ t.Skip("catalog does not support renaming tables")
+ }
+}
+
+// testRenameTableNotSupported asserts that a catalog which does not support
+// rename returns ErrNotImplemented rather than silently succeeding or
+// returning an untyped error.
+func testRenameTableNotSupported(t *testing.T, cfg Config) {
+ if cfg.SupportsRenameTable {
+ t.Skip("catalog supports renaming tables")
+ }
+
+ 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, iceberg.ErrNotImplemented)
+}
+
+// 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"}
Review Comment:
testRenameTable only moves within a single namespace, which skips the
riskiest path: Hive's `toDB != fromDB` case takes separate locks and mutates
`DbName`, and right now that's only exercised by a mock. I'd add a
cross-namespace variant here so the real backends actually run it.
##########
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)
Review Comment:
The pre-check is good for the common case, but it's advisory. Hive's lock is
cooperative, so a non-Iceberg writer that creates `toDB.toTable` via a plain
`create_table` can land between this `GetTable` and the `AlterTable` below.
When that happens HMS rejects the alter itself with `AlreadyExistsException`,
and that path still wraps it as a generic `failed to rename` error, so it never
matches `catalog.ErrTableAlreadyExists`. `isAlreadyExistsError` is already in
this file; I'd map it (and `isNoSuchObjectError` to `ErrNoSuchTable`) on the
`AlterTable` result too, which is what Java's `renameTable` does.
##########
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
Review Comment:
This builds the destination from `testIcebergHiveTable1`, so it only covers
the Iceberg-table conflict. The whole reason for the raw `GetTable` is to also
catch a non-Iceberg destination, and that case is untested; so is the
`!isNoSuchObjectError` branch where the destination lookup returns a transport
error. A case for each would exercise both new branches.
##########
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:
409 to `ErrTableAlreadyExists` matches PyIceberg, so no objection to the
mapping. Two lossy edges worth noting, not blocking: a missing destination
namespace also comes back as 404 and gets reported as `ErrNoSuchTable`, and a
409 can mean a view occupies the destination rather than a table. If we want to
be precise, inspect the error body `type` before picking the sentinel;
otherwise a comment documenting the ambiguity is enough.
--
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]
