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: [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]
