You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "richiebachala (via GitHub)" <gi...@apache.org> on 2026/10/10 16:00:10 UTC
[PR] test(catalog): add cross-namespace rename conformance tests [iceberg-go]
richiebachala opened a new pull request, #2173:
URL: https://github.com/apache/iceberg-go/pull/2173
Follow-up to #2165, addressing @laskoviymishka's suggestion to cover the rename paths most likely to hide cross-catalog divergence. Part of #1691.
> **Stacked on #2165.** This branch includes its commits; only the last commit (`658efca`) is new. I'll rebase onto `main` once #2165 merges and mark this ready for review then.
## Changes
- **`RenameTableAcrossNamespaces`**: a table moves to another namespace, keeps its UUID, and is listed only in the destination.
- **`RenameTableToMissingNamespace`**: renaming into a nonexistent namespace returns `ErrNoSuchNamespace` and leaves the source table in place.
- **REST fix**: the spec's rename 404 covers both a missing source table and a missing destination namespace, and the client mapped every 404 to `ErrNoSuchTable`. It now discriminates on `error.type`, following the existing `loadFunction` pattern, so a missing namespace reports `ErrNoSuchNamespace`, matching Hive and SQL. Unit tests cover both 404 types.
The new conformance test caught the REST divergence: against the Java REST fixture, the server returns `NoSuchNamespaceException`, which the client previously reported as a missing table.
## Testing
- `go test ./catalog/...`
- Conformance for SQL and Hadoop, plus REST and Hive with `-tags integration` against `internal/recipe` containers: all pass, including both new cases.
- `golangci-lint`: 0 issues.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] test(catalog): table-lifecycle conformance slice with cross-namespace rename and error-mapping fixes [iceberg-go]
Posted by "richiebachala (via GitHub)" <gi...@apache.org>.
richiebachala commented on PR #2173:
URL: https://github.com/apache/iceberg-go/pull/2173#issuecomment-6152310256
Thanks for the approval @zeroshade. A correction to my last comment: I said I'd open an issue proposing a spec clarification for `from == to`. After digging further, I don't think one is warranted.
This was settled deliberately when REST rename was added in apache/iceberg#4448. A draft of `RenameTableRequest` rejected `source == destination`; @rdblue asked whether it should be invalid or a no-op, and the check was dropped so that implementors keep discretion. The `JdbcCatalog` and `InMemoryCatalog` early returns came later (#9487, #6927). Across Java's catalogs, Hive, DynamoDB and ECS reject a self-rename, and JDBC and InMemory no-op, so accepting both outcomes matches upstream intent rather than papering over a gap.
8e354d3 is a comment-only change citing #4448 in `testRenameTableToSelf` so the reasoning lives next to the test. No behavior change; conformance still passes locally.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] test(catalog): add cross-namespace rename conformance tests [iceberg-go]
Posted by "laskoviymishka (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] test(catalog): add cross-namespace rename conformance tests and harden rename error mapping [iceberg-go]
Posted by "laskoviymishka (via GitHub)" <gi...@apache.org>.
laskoviymishka commented on code in PR #2173:
URL: https://github.com/apache/iceberg-go/pull/2173#discussion_r4249100968
##########
catalog/catalogtest/catalogtest.go:
##########
@@ -115,6 +115,8 @@ func RunCatalogTests(t *testing.T, cfg Config) {
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) })
+ t.Run("RenameTableAcrossNamespaces", func(t *testing.T) { testRenameTableAcrossNamespaces(t, cfg) })
+ t.Run("RenameTableToMissingNamespace", func(t *testing.T) { testRenameTableToMissingNamespace(t, cfg) })
Review Comment:
the from==to self-rename case I flagged last round is still missing from the matrix. Hive now reports already-exists through the new pre-check while REST/SQL are unspecified, so it's exactly the divergence this suite exists to catch. a `testRenameTableToSelf` asserting `ErrTableAlreadyExists` with an unchanged UUID, or an explicit follow-up note if the catalogs genuinely disagree, would close it.
##########
catalog/rest/rest_test.go:
##########
@@ -1967,6 +1967,87 @@ func (r *RestCatalogSuite) TestRenameTable409() {
r.ErrorIs(err, catalog.ErrTableAlreadyExists)
}
+// renameTableErrors registers a rename endpoint that fails and returns a
+// function that sets the failure's status and error.type, renames a table, and
+// returns the error RenameTable reports. The handler is registered once, so the
+// returned function can be called repeatedly within one test.
+func (r *RestCatalogSuite) renameTableErrors() func(status int, errType string) error {
+ var status int
+ var errType string
+ r.mux.HandleFunc("/v1/tables/rename", func(w http.ResponseWriter, req *http.Request) {
+ if !r.Equal(http.MethodPost, req.Method) {
+ return
+ }
+
+ w.WriteHeader(status)
+ json.NewEncoder(w).Encode(map[string]any{
+ "error": map[string]any{
+ "message": "rename failed",
+ "type": errType,
+ "code": status,
+ },
+ })
+ })
+
+ cat, err := rest.NewCatalog(context.Background(), "rest", r.srv.URL, rest.WithOAuthToken(TestToken))
+ r.Require().NoError(err)
+
+ return func(s int, t string) error {
+ status, errType = s, t
+ _, err := cat.RenameTable(context.Background(), catalog.ToIdentifier("fokko", "source"), catalog.ToIdentifier("missing", "destination"))
+
+ return err
+ }
+}
+
+func (r *RestCatalogSuite) TestRenameTable404MissingTable() {
+ err := r.renameTableErrors()(http.StatusNotFound, "NoSuchTableException")
+ r.ErrorIs(err, catalog.ErrNoSuchTable)
+ r.NotErrorIs(err, catalog.ErrNoSuchNamespace)
+}
+
+func (r *RestCatalogSuite) TestRenameTable404MissingNamespace() {
+ err := r.renameTableErrors()(http.StatusNotFound, "NoSuchNamespaceException")
+ r.ErrorIs(err, catalog.ErrNoSuchNamespace)
+ r.NotErrorIs(err, catalog.ErrNoSuchTable)
+}
+
+// TestRenameTable404UntypedFallsBackToNoSuchTable pins the fallback for servers
+// that omit or send an unrecognized error.type on a rename 404.
+func (r *RestCatalogSuite) TestRenameTable404UntypedFallsBackToNoSuchTable() {
+ rename := r.renameTableErrors()
+ for _, errType := range []string{"", "SomeUnknownException"} {
+ r.Run("type="+errType, func() {
+ err := rename(http.StatusNotFound, errType)
+ r.ErrorIs(err, catalog.ErrNoSuchTable)
+ r.NotErrorIs(err, catalog.ErrNoSuchNamespace)
+ })
+ }
+}
+
+// TestRenameTableNoSuchNamespaceTypeIsStatusGated asserts that only a 404 is
Review Comment:
"only a 404 is refined" is a bit broader than what's implemented: 409 is also in the rename status map, so a 409 carrying `NoSuchNamespaceException` would refine to `ErrNoSuchNamespace` too. no real server sends that combo so it's harmless, but the comment and the table only cover unmapped statuses (503/500/401/403). I'd either add a 409 case and decide what it should resolve to, or reword to "a status this call doesn't map."
##########
catalog/hive/hive_test.go:
##########
@@ -918,6 +918,33 @@ func TestHiveRenameTableToExistingTable(t *testing.T) {
mockClient.AssertExpectations(t)
}
+// TestHiveRenameTableDestinationCreatedConcurrently covers a destination that
+// appears after the pre-check, e.g. from Hive DDL that skips the HMS locks: the
+// metastore rejects AlterTable, and that must surface as ErrTableAlreadyExists.
+func TestHiveRenameTableDestinationCreatedConcurrently(t *testing.T) {
+ mockClient := &mockHiveClient{}
+ 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(nil, errNoSuchObject).Once()
+ mockClient.On("AlterTable", mock.Anything, "test_database", "test_table", mock.Anything).
+ Return(errors.New("InvalidOperationException: new table target_database.renamed already exists")).Once()
Review Comment:
this passes only because `isAlreadyExistsError` substring-matches the message text: the mock returns a plain `errors.New`, not the typed `hive_metastore.InvalidOperationException` HMS actually sends. worth a one-line comment that the string is verbatim HMS text (or returning the typed exception if it's available), so a later tightening of the helper to `errors.As` doesn't silently break this while production stays fine.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] test(catalog): add cross-namespace rename conformance tests and harden rename error mapping [iceberg-go]
Posted by "richiebachala (via GitHub)" <gi...@apache.org>.
richiebachala commented on PR #2173:
URL: https://github.com/apache/iceberg-go/pull/2173#issuecomment-6113918304
Thanks @laskoviymishka. With #2165 merged, I merged `main` into this branch (no force-push), so the diff is now only the cross-namespace work: 8 files, +202/-17. The ListTables/DropTable/DropMissingTable slice you mentioned landed in #2165, so I've retitled this to what it now adds rather than widening it. Addressed in edcc0d0:
- **REST 404 discrimination (blocker):** routed through `withErrorTypeOverride` on `doPost` and dropped the `errors.As` block. Since `handleNon200` only refines a status the caller mapped, a 5xx or 401/403 carrying `NoSuchNamespaceException` no longer reads as a missing namespace. New tests cover 503, 500, 401 and 403 with that type; all four fail against the previous code and pass now. I also added the empty and unknown `error.type` cases you suggested, which fall back to `ErrNoSuchTable`.
- **Hive AlterTable race:** mapped `isAlreadyExistsError` on the `AlterTable` branch to `ErrTableAlreadyExists` and kept the pre-check, with a comment saying the pre-check is best-effort. A new unit test covers a destination that appears between the pre-check and `AlterTable`.
- **Rejected-rename identity:** `RenameTableToExisting` now asserts that both `from` and `to` still carry their original table UUIDs, not just that they exist.
- **Hadoop:** dropped the redundant message match, and switched the integration test to assert `ErrNotImplemented` (carried over from #2172, now closed as superseded).
**Same-identifier rename (`from == to`):** I'd like to take this as a follow-up rather than pin it here. Hive now reports already-exists, while REST and SQL are unspecified, and I don't think the spec or Java's `CatalogTests` settle the expected result. I'll open an issue proposing a `RenameTableToSelf` case so the expected behavior is agreed before it's tested.
Verified locally: `go test ./catalog/...`, lint, and the conformance suite against all four catalogs, with REST and Hive running against the integration containers.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] test(catalog): table-lifecycle conformance slice with cross-namespace rename and error-mapping fixes [iceberg-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade merged PR #2173:
URL: https://github.com/apache/iceberg-go/pull/2173
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] test(catalog): table-lifecycle conformance slice with cross-namespace rename and error-mapping fixes [iceberg-go]
Posted by "richiebachala (via GitHub)" <gi...@apache.org>.
richiebachala commented on PR #2173:
URL: https://github.com/apache/iceberg-go/pull/2173#issuecomment-6147268005
Thanks @zeroshade, good catch. Went with the first option in 66a98b4.
Checking the Java side settled it: the catalogs genuinely disagree. `JdbcCatalog` (behind the REST fixture) and `InMemoryCatalog` both start `renameTable` with `if (from.equals(to)) return;`, so a self-rename is a no-op success, while `HiveCatalog` has no such check and throws `AlreadyExistsException`. Neither the REST spec nor Java's `CatalogTests` defines the outcome; `CatalogTests` has no self-rename case at all.
So `RenameTableToSelf` now asserts only the shared invariant:
- the rename succeeds or returns `ErrTableAlreadyExists`; any other error fails;
- either way, the table still loads with its original UUID **and** metadata location, and `ListTables` shows it exactly once.
The doc comment records the divergence. Verified against all four catalogs: REST passes on the no-op path; Hive and SQL pass on the rejection path. `go test ./catalog/...` and lint pass.
I'll open the follow-up issue proposing a spec clarification for `from == to`, and link it from the test once it exists.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org