laskoviymishka commented on code in PR #2349:
URL: https://github.com/apache/iceberg-go/pull/2349#discussion_r4431212578
##########
catalog/glue/glue.go:
##########
@@ -98,13 +98,13 @@ var (
)
func init() {
- catalog.MustRegister("glue", catalog.RegistrarFunc(func(ctx
context.Context, _ string, props iceberg.Properties) (catalog.Catalog, error) {
+ catalog.MustRegister("glue", catalog.RegistrarFunc(func(ctx
context.Context, name string, props iceberg.Properties) (catalog.Catalog,
error) {
awsConfig, err := toAwsConfig(ctx, props)
if err != nil {
return nil, err
}
- return NewCatalog(WithAWSConfig(awsConfig),
WithAWSProperties(AWSProperties(props)))
+ return NewCatalog(WithName(name), WithAWSConfig(awsConfig),
WithAWSProperties(AWSProperties(props)))
Review Comment:
Could we add a test that loads Glue through `catalog.Load` and checks
`Name()`, like Hadoop's
[`TestRegistration`](https://github.com/apache/iceberg-go/blob/675ad9a299265ea760c50e378d0f6e0f5b9a7a4d/catalog/hadoop/hadoop_test.go#L363-L371)?
On `main` this registrar dropped the name (`_ string`), and
`catalog_name_test.go` only covers `NewCatalog` with `WithName`, so nothing
pins the forwarding added here.
##########
catalog/catalog.go:
##########
@@ -162,11 +159,77 @@ 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
+}
+
+// As finds the first catalog in the wrapping chain that implements T.
+// Catalog decorators can expose a wrapped catalog with Unwrap() Catalog.
+// The outermost catalog is checked first, allowing decorators to intercept
+// optional operations. The Unwrap chain must terminate without cycles.
Review Comment:
Could we document that `Unwrap` may return nil? `As` ends the walk on a nil
result
([catalog.go](https://github.com/apache/iceberg-go/blob/675ad9a299265ea760c50e378d0f6e0f5b9a7a4d/catalog/catalog.go#L183-L203)),
but the doc only mentions cycles, so a decorator author has to read the loop
to find out.
```suggestion
// optional operations. Unwrap may return nil to end the chain, and the
// chain must terminate without cycles.
```
##########
catalog/catalog.go:
##########
@@ -162,11 +159,77 @@ 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.
Review Comment:
Could the doc say what a catalog built without a name returns? Glue and Hive
had no `Name()` on `main`, and here they default to `"glue"` and `"hive"`
([options.go](https://github.com/apache/iceberg-go/blob/675ad9a299265ea760c50e378d0f6e0f5b9a7a4d/catalog/hive/options.go#L87-L95)).
That default becomes part of the contract once we freeze this for v1.
```suggestion
// Name returns the catalog's configured logical name. Glue and Hive
// catalogs built without WithName return "glue" and "hive".
```
--
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]
