laskoviymishka commented on code in PR #1461:
URL: https://github.com/apache/iceberg-go/pull/1461#discussion_r3577774365
##########
catalog/rest/rest_integration_test.go:
##########
@@ -289,6 +289,49 @@ func (s *RestIntegrationSuite) TestUpdateView() {
s.Require().NoError(s.cat.DropView(s.ctx,
catalog.ToIdentifier(TestNamespaceIdent, "test-view")))
}
+func (s *RestIntegrationSuite) TestRenameView() {
+ s.ensureNamespace()
+
+ const location = "s3://warehouse/iceberg"
+ tbl, err := s.cat.CreateTable(s.ctx,
+ catalog.ToIdentifier(TestNamespaceIdent, "test-table"),
+ tableSchemaSimple,
catalog.WithProperties(iceberg.Properties{"foobar": "baz"}),
+ catalog.WithLocation(location))
+ s.Require().NoError(err)
+ s.Require().NotNil(tbl)
+
+ // Create a view to rename
+ fromIdent := catalog.ToIdentifier(TestNamespaceIdent, "test-view")
+ toIdent := catalog.ToIdentifier(TestNamespaceIdent, "renamed-view")
+
+ viewRepr := view.NewRepresentation(fmt.Sprintf("SELECT * FROM %s.%s",
TestNamespaceIdent, "test-table"), "trino")
Review Comment:
tiny one — double space in the format string (`FROM %s`). I know it's
copied from the other view tests, but since we're adding a fresh one we could
quietly fix it here. wdyt?
##########
catalog/rest/rest_test.go:
##########
@@ -2660,6 +2660,103 @@ func (r *RestCatalogSuite) TestRegisterView409() {
r.ErrorContains(err, "The given view already exists")
}
+func (r *RestCatalogSuite) TestRenameView204() {
+ const metadataLoc =
"s3://bucket/warehouse/example.db/destination/metadata/00001.metadata.json"
+
+ r.mux.HandleFunc("/v1/views/rename", func(w http.ResponseWriter, req
*http.Request) {
+ r.Require().Equal(http.MethodPost, req.Method)
+
+ for k, v := range TestHeaders {
+ r.Equal(v, req.Header.Values(k))
+ }
+
+ var payload struct {
+ Source struct {
+ Namespace []string `json:"namespace"`
+ Name string `json:"name"`
+ } `json:"source"`
+ Destination struct {
+ Namespace []string `json:"namespace"`
+ Name string `json:"name"`
+ } `json:"destination"`
+ }
+ r.NoError(json.NewDecoder(req.Body).Decode(&payload))
+ r.Equal([]string{"example"}, payload.Source.Namespace)
+ r.Equal("source", payload.Source.Name)
+ r.Equal([]string{"example"}, payload.Destination.Namespace)
+ r.Equal("destination", payload.Destination.Name)
+
+ w.WriteHeader(http.StatusNoContent)
+ })
+
+ // Mock the load view endpoint for loading the renamed view.
+ r.mux.HandleFunc("/v1/namespaces/example/views/destination", func(w
http.ResponseWriter, req *http.Request) {
+ r.Require().Equal(http.MethodGet, req.Method)
+
+ w.Header().Set("Content-Type", "application/json")
+ fmt.Fprintf(w, `{"metadata-location": %q, "metadata": %s,
"config": {}}`,
+ metadataLoc, exampleViewMetadataJSON)
+ })
+
+ cat, err := rest.NewCatalog(context.Background(), "rest", r.srv.URL,
rest.WithOAuthToken(TestToken))
+ r.Require().NoError(err)
+
+ fromIdent := catalog.ToIdentifier("example", "source")
+ toIdent := catalog.ToIdentifier("example", "destination")
+
+ renamed, err := cat.RenameView(context.Background(), fromIdent, toIdent)
+ r.Require().NoError(err)
+
+ r.Equal(toIdent, renamed.Identifier())
+ r.Equal(metadataLoc, renamed.MetadataLocation())
+ r.Equal(uuid.MustParse("a1b2c3d4-e5f6-7890-1234-567890abcdef"),
renamed.Metadata().ViewUUID())
+ r.Equal(exampleViewSQL,
renamed.Metadata().CurrentVersion().Representations[0].Sql)
+}
+
+func (r *RestCatalogSuite) TestRenameView404() {
Review Comment:
RenameView has two `ValidateViewIdentifier` guards but nothing exercises
them — `TestTableAndViewIdentifiersRequireNamespace` covers RenameTable's
namespace-absent case and adds nothing for views, so both branches are
currently unproven. I'd add the mirror of the RenameTable cases there:
```go
_, err = cat.RenameView(context.Background(), table.Identifier{"source"},
table.Identifier{"example", "destination"})
r.ErrorIs(err, catalog.ErrNoSuchView)
_, err = cat.RenameView(context.Background(), table.Identifier{"example",
"source"}, table.Identifier{"destination"})
r.ErrorIs(err, catalog.ErrNoSuchView)
```
This is the one gap I'd want closed before it goes in.
##########
catalog/rest/rest_test.go:
##########
@@ -2660,6 +2660,103 @@ func (r *RestCatalogSuite) TestRegisterView409() {
r.ErrorContains(err, "The given view already exists")
}
+func (r *RestCatalogSuite) TestRenameView204() {
+ const metadataLoc =
"s3://bucket/warehouse/example.db/destination/metadata/00001.metadata.json"
+
+ r.mux.HandleFunc("/v1/views/rename", func(w http.ResponseWriter, req
*http.Request) {
+ r.Require().Equal(http.MethodPost, req.Method)
+
+ for k, v := range TestHeaders {
+ r.Equal(v, req.Header.Values(k))
+ }
+
+ var payload struct {
+ Source struct {
+ Namespace []string `json:"namespace"`
+ Name string `json:"name"`
+ } `json:"source"`
+ Destination struct {
+ Namespace []string `json:"namespace"`
+ Name string `json:"name"`
+ } `json:"destination"`
+ }
+ r.NoError(json.NewDecoder(req.Body).Decode(&payload))
+ r.Equal([]string{"example"}, payload.Source.Namespace)
+ r.Equal("source", payload.Source.Name)
+ r.Equal([]string{"example"}, payload.Destination.Namespace)
+ r.Equal("destination", payload.Destination.Name)
+
+ w.WriteHeader(http.StatusNoContent)
+ })
+
+ // Mock the load view endpoint for loading the renamed view.
+ r.mux.HandleFunc("/v1/namespaces/example/views/destination", func(w
http.ResponseWriter, req *http.Request) {
+ r.Require().Equal(http.MethodGet, req.Method)
Review Comment:
This LoadView leg only checks the method, and the 404/409 handlers below
skip headers entirely — the 204 rename handler and the analogous
TestRegisterView tests all run the `for k, v := range TestHeaders` loop. I'd
add it here and in the two error handlers so we're actually exercising auth on
every leg.
##########
catalog/rest/rest_integration_test.go:
##########
@@ -289,6 +289,49 @@ func (s *RestIntegrationSuite) TestUpdateView() {
s.Require().NoError(s.cat.DropView(s.ctx,
catalog.ToIdentifier(TestNamespaceIdent, "test-view")))
}
+func (s *RestIntegrationSuite) TestRenameView() {
+ s.ensureNamespace()
+
+ const location = "s3://warehouse/iceberg"
+ tbl, err := s.cat.CreateTable(s.ctx,
+ catalog.ToIdentifier(TestNamespaceIdent, "test-table"),
+ tableSchemaSimple,
catalog.WithProperties(iceberg.Properties{"foobar": "baz"}),
+ catalog.WithLocation(location))
+ s.Require().NoError(err)
+ s.Require().NotNil(tbl)
+
+ // Create a view to rename
+ fromIdent := catalog.ToIdentifier(TestNamespaceIdent, "test-view")
+ toIdent := catalog.ToIdentifier(TestNamespaceIdent, "renamed-view")
+
+ viewRepr := view.NewRepresentation(fmt.Sprintf("SELECT * FROM %s.%s",
TestNamespaceIdent, "test-table"), "trino")
+ viewVersion, err := view.NewVersion(1, 1,
[]view.Representation{viewRepr}, table.Identifier{TestNamespaceIdent})
+ s.Require().NoError(err)
+
+ created, err := s.cat.CreateView(s.ctx, fromIdent, viewVersion,
tableSchemaSimple,
+ catalog.WithViewProperties(iceberg.Properties{"foobar": "baz"}))
+ s.Require().NoError(err)
+
+ // Rename it
+ renamed, err := s.cat.RenameView(s.ctx, fromIdent, toIdent)
+ s.Require().NoError(err)
+ s.Equal(toIdent, renamed.Identifier())
+ s.Equal(created.Metadata().ViewUUID(), renamed.Metadata().ViewUUID())
Review Comment:
Could we also capture `created.MetadataLocation()` before the rename and
assert it's unchanged after? We prove the UUID and name carry over, but "a
rename doesn't rewrite metadata" is the property most likely to regress
silently, and it's one more line here.
--
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]
