zeroshade commented on code in PR #2349:
URL: https://github.com/apache/iceberg-go/pull/2349#discussion_r4414956549
##########
catalog/catalog.go:
##########
@@ -162,6 +159,38 @@ type Catalog interface {
removals []string, updates iceberg.Properties)
(PropertiesUpdateSummary, error)
}
+// Catalog combines universal table and namespace operations with catalog
+// identity and resource lifecycle. Implementations must release catalog-owned
+// resources in Close. Ownership of constructor-supplied dependencies depends
+// on the implementation. Using a catalog after Close is unsupported; repeated
+// Close calls have implementation-specific behavior.
+//
+// After v1, add new capabilities as optional interfaces, never as methods
+// on Catalog, to avoid breaking third-party catalog implementations.
+type Catalog interface {
+ io.Closer
+
+ // Name returns the catalog's configured logical name.
+ Name() string
+
+ // CatalogType returns the type of the catalog.
+ CatalogType() Type
+
+ TableCatalog
+ NamespaceCatalog
+}
+
+// ViewCatalog is an optional capability for standard view operations.
+// Use a type assertion to discover support on a Catalog. RenameView and
+// RegisterView are backend-specific and are not part of this interface.
+type ViewCatalog interface {
Review Comment:
Please keep `UpdateView` out of `ViewCatalog` and add it as its own optional
interface that matches the REST method:
```go
UpdateView(ctx context.Context, ident table.Identifier, requirements
[]view.Requirement, updates []view.Update) (*view.View, error)
```
Add a compile-time check for it in `catalog/rest/rest.go`, and mention it in
the `releases.md` note.
Also, the five methods here need doc comments like the ones on
`TableCatalog` and `NamespaceCatalog`. The new `UpdateView` interface needs
them too.
##########
website/src/releases.md:
##########
@@ -42,6 +42,8 @@ Per-release notes (highlights, breaking changes,
contributors) are published on
### Unreleased compatibility notes
+- **Catalog capabilities split into narrower interfaces.** `catalog.Catalog`
now embeds `TableCatalog` and `NamespaceCatalog` (the existing table and
namespace method signatures are unchanged). All `Catalog` implementations must
add `Name() string` to report their configured name; this is a breaking change
for third-party catalogs. `ViewCatalog` is optional and currently implemented
by REST and Hive. Future catalog capabilities should use optional interfaces
rather than extending `Catalog`.
Review Comment:
Please remove this blank line. The other bullets in this list aren't
separated, and the extra line makes the list render as loose (spaced) items.
##########
catalog/catalog.go:
##########
@@ -162,6 +159,38 @@ type Catalog interface {
removals []string, updates iceberg.Properties)
(PropertiesUpdateSummary, error)
}
+// Catalog combines universal table and namespace operations with catalog
+// identity and resource lifecycle. Implementations must release catalog-owned
+// resources in Close. Ownership of constructor-supplied dependencies depends
+// on the implementation. Using a catalog after Close is unsupported; repeated
+// Close calls have implementation-specific behavior.
+//
+// After v1, add new capabilities as optional interfaces, never as methods
+// on Catalog, to avoid breaking third-party catalog implementations.
+type Catalog interface {
+ io.Closer
+
+ // Name returns the catalog's configured logical name.
+ Name() string
+
+ // CatalogType returns the type of the catalog.
+ CatalogType() Type
+
+ TableCatalog
+ NamespaceCatalog
+}
+
+// ViewCatalog is an optional capability for standard view operations.
+// Use a type assertion to discover support on a Catalog. RenameView and
Review Comment:
Wrapper convention from #2062: a decorator like `struct{ catalog.Catalog }`
hides every optional interface (`ViewCatalog`, `TransactionalCatalog`,
`RegisterableCatalog`, ...) from a type assertion, so for example
`NewMultiTableTransaction` rejects a wrapped REST catalog. Please add an
`Unwrap() Catalog` convention and a `catalog.As[T]` helper (the
`net/http.ResponseController` pattern) that walks the `Unwrap` chain. Then
route `NewMultiTableTransaction` and the other capability checks through it,
and update this doc comment to point at `catalog.As` instead of a bare type
assertion.
##########
catalog/hive/catalog_name_test.go:
##########
@@ -0,0 +1,32 @@
+// 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 hive
+
+import "testing"
+
+func TestHiveCatalogName(t *testing.T) {
+ cat := NewCatalogWithClient(nil, nil)
+ if got := cat.Name(); got != "hive" {
+ t.Fatalf("default catalog name = %q, want %q", got, "hive")
+ }
+
+ WithName("analytics")(cat.opts)
Review Comment:
This mutates `cat.opts` after construction, so it never tests how a
constructor handles the name. Once `NewCatalogWithClient` takes options, use
`NewCatalogWithClient(nil, nil, WithName("analytics"))` for the named case.
##########
catalog/hive/hive.go:
##########
@@ -97,6 +98,8 @@ func NewCatalogWithClient(client HiveClient, props
iceberg.Properties) *Catalog
}
}
+func (c *Catalog) Name() string { return c.opts.Name }
Review Comment:
`NewCatalogWithClient` should accept `opts ...Option` too. As it stands, a
Hive catalog built around an injected client is always named `"hive"`. Please
apply the options the same way `NewCatalog` does.
--
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]
