laskoviymishka commented on code in PR #2173:
URL: https://github.com/apache/iceberg-go/pull/2173#discussion_r4241790729
##########
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)
Review Comment:
Now that we assert `ErrorIs(ErrNotImplemented)`, the `Contains(err.Error(),
"not supported")` on the next line is redundant. Fine to drop it, or keep it if
you want the message text pinned too.
##########
catalog/rest/rest.go:
##########
@@ -1980,8 +1980,18 @@ 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 {
+ // The spec's rename 404 covers both a missing source table and
a
+ // missing destination namespace; discriminate on the error
type so a
+ // missing namespace is not reported as a missing table.
+ var errRsp errorResponse
+ if errors.As(err, &errRsp) && errRsp.Type ==
errTypeNoSuchNamespace {
Review Comment:
This remaps any error whose body type is `NoSuchNamespaceException` to
`ErrNoSuchNamespace` without checking the status, so a 5xx or a 401/403
carrying that type now reports as a missing namespace. `handleNon200` gates its
type refinement on the mapped status for exactly this reason, and a caller like
CDC that treats `ErrNoSuchNamespace` as terminal would mis-handle a failure
that was actually retryable.
I'd route it through the override we already have:
`withErrorTypeOverride(map[string]error{errTypeNoSuchNamespace:
catalog.ErrNoSuchNamespace})` on `doPost`, and drop the `errors.As` block. Same
404 result, status-gated, and it avoids a third hand-rolled copy of this idiom.
If you'd rather keep the `errors.As`, gate it on `errRsp.statusCode ==
http.StatusNotFound`.
##########
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 itself is the right call: raw `GetTable` catches non-Iceberg
destinations that `CheckTableExists` would miss, and the lock ordering is fine.
The gap is the window after it. A writer that doesn't go through these HMS
locks (Hive DDL, say) can still create the destination between this check and
`AlterTable`, and that path surfaces as an unwrapped "failed to rename table
...", so callers can't `errors.Is(err, ErrTableAlreadyExists)` under a race.
`isAlreadyExistsError` already exists at hive.go:1043. I'd map it in the
`AlterTable` error branch and keep the pre-check. Leaving the race as
best-effort is defensible too, but then I'd say so in a comment here rather
than let the code read as a hard guarantee.
##########
catalog/catalogtest/tables.go:
##########
@@ -0,0 +1,254 @@
+// 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)
Review Comment:
This checks both tables still exist, but not that `to` is still the original
table. An implementation that overwrote `to` and then errored would pass this.
I'd record `to`'s UUID (or metadata location) before the rename and assert it's
unchanged here.
##########
catalog/rest/rest_test.go:
##########
@@ -1944,6 +1944,67 @@ 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)
+}
+
+// renameTable404 has the rename endpoint answer 404 with errType and returns
+// the error RenameTable reports.
+func (r *RestCatalogSuite) renameTable404(errType string) error {
+ r.mux.HandleFunc("/v1/tables/rename", func(w http.ResponseWriter, req
*http.Request) {
+ if !r.Equal(http.MethodPost, req.Method) {
+ return
+ }
+
+ w.WriteHeader(http.StatusNotFound)
+ json.NewEncoder(w).Encode(map[string]any{
+ "error": map[string]any{
+ "message": "not found",
+ "type": errType,
+ "code": 404,
+ },
+ })
+ })
+
+ 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("missing",
"destination"))
+
+ return err
+}
+
+func (r *RestCatalogSuite) TestRenameTable404MissingTable() {
Review Comment:
Since `renameTable404` is already parametrized by type, it's cheap to add a
case with an empty or unknown `error.type` and assert it falls back to
`ErrNoSuchTable`. That's the common shape for servers that omit the type, and
it's the branch most likely to regress silently.
--
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]
