zeroshade commented on issue #2062:
URL: https://github.com/apache/iceberg-go/issues/2062#issuecomment-6476657503

   ## Proposal: split `Catalog` into capability interfaces
   
   This is the decision for the "Catalog capabilities" item in section 2. 
`Catalog` has 17 methods. Adding a method after v1 breaks every implementer, 
including third-party catalogs, so the shape needs to be settled before the 
tag. The split below composes the interface rather than stripping it, so 
existing callers see the same method set.
   
   ### What we have today
   
   - **Optional interfaces already in place:** `TransactionalCatalog`, 
`RegisterableCatalog` and `RefreshableCredentialCatalog`, checked with a type 
assertion. `PurgeTable` is embedded through `PurgeableCatalog`.
   - **Views have no interface.** Each backend has concrete methods:
   
   | Backend | List / Load / Create / Drop / CheckExists | Rename / Register 
view | `Name()` |
   |---|---|---|---|
   | REST | ✓ | ✓ / ✓ | ✓ |
   | Hive | ✓ | – | – |
   | SQL | ✓, but with different signatures | – | ✓ |
   | Glue, Hadoop | – | – | – |
   
   - **SQL's view signatures differ** from REST and Hive:
     - `CreateView` takes `(ctx, ident, *iceberg.Schema, view.Representation, 
iceberg.Properties) error` instead of `(ctx, ident, *view.Version, 
*iceberg.Schema, ...CreateViewOpt) (*view.View, error)`.
     - `LoadView` returns `view.Metadata` instead of `*view.View`.
   
     A shared view interface can't cover SQL until those are aligned.
   
   ### Proposed shape
   
   This mirrors Java's `Catalog` / `SupportsNamespaces` / `ViewCatalog`:
   
   ```go
   // Core table operations.
   type TableCatalog interface {
        CreateTable(...); LoadTable(...); CommitTable(...)
        ListTables(...); DropTable(...); RenameTable(...); CheckTableExists(...)
        PurgeableCatalog
   }
   
   type NamespaceCatalog interface {
        ListNamespaces(...); CreateNamespace(...); DropNamespace(...)
        CheckNamespaceExists(...); LoadNamespaceProperties(...); 
UpdateNamespaceProperties(...)
   }
   
   // What every backend implements. New methods after v1 go into new optional
   // interfaces, never here.
   type Catalog interface {
        io.Closer
        Name() string
        CatalogType() Type
        TableCatalog
        NamespaceCatalog
   }
   
   // Optional, checked with a type assertion like RegisterableCatalog.
   type ViewCatalog interface {
        ListViews(...); LoadView(...) (*view.View, error); CreateView(...) 
(*view.View, error)
        DropView(...); CheckViewExists(...) (bool, error)
   }
   ```
   
   **Decisions:**
   
   - **`Name()` joins the core interface.** REST and SQL already have it, and 
Hive, Glue and Hadoop store the name but don't expose it. Adding it breaks 
third-party implementers, which is why it has to happen before v1.
   - **Namespaces stay part of `Catalog`.** All five backends implement them, 
and making them optional would force a type assertion for a universal feature. 
`NamespaceCatalog` exists so code can accept only what it needs.
   - **`RenameView` and `RegisterView` stay out of `ViewCatalog`.** Only REST 
has them; they can become separate optional interfaces later.
   - **The `Catalog` doc comment should state the rule** that new methods after 
v1 go into new optional interfaces.
   
   ### Compatibility
   
   | Change | Breaking? |
   |---|---|
   | Split `Catalog` into embedded `TableCatalog` / `NamespaceCatalog` | No: 
same method set |
   | Add `ViewCatalog`; REST and Hive satisfy it as-is | No: additive |
   | Add `Name()` to `Catalog`, implemented by Hive, Glue and Hadoop | Yes, for 
third-party `Catalog` implementations |
   | Align SQL `CreateView` / `LoadView` with REST and Hive | Yes, for 
`sql.Catalog` callers |
   
   ### Suggested PRs
   
   1. **`refactor(catalog)!:` split the interface, add `Name()` and 
`ViewCatalog`.**
      - Changes: `catalog/catalog.go`, plus `Name()` on Hive, Glue and Hadoop.
      - Add compile-time checks such as `var _ catalog.ViewCatalog = 
(*Catalog)(nil)` in REST and Hive.
      - Add a release note for `Name()`.
   2. **`refactor(catalog/sql)!:` align SQL view signatures with 
`ViewCatalog`.**
      - Add the SQL compile-time check and a release note.
   
   These can be claimed separately. PR 2 depends on PR 1. Please claim here 
before starting.
   


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

Reply via email to