DerGut opened a new pull request, #2999: URL: https://github.com/apache/iceberg-rust/pull/2999
## Which issue does this PR close? This is another part in the chain towards resolving issue https://github.com/apache/iceberg-rust/issues/2774 and is based on PR https://github.com/apache/iceberg-rust/pull/2920 In a follow-up, I will introduce the `SessionCatalog` to the `integrations/datafusion` crate. It will use a `dyn SessionCatalog` similar to the existing `dyn Catalog` . ### Prevent Duplication of APIs The purpose of this PR is to break the need to update all `Catalog`-based APIs to get a `SessionCatalog` equivalent. It does so by introducing a conversion from `SessionCatalog` to `Catalog`. As an example, the [`[Transaction::commit](https://github.com/apache/iceberg-rust/blob/a500a2e731ac0f1c9bf43d6613a8dbe08417465f/crates/iceberg/src/transaction/mod.rs#L175)`](https://github.com/apache/iceberg-rust/blob/a500a2e731ac0f1c9bf43d6613a8dbe08417465f/crates/iceberg/src/transaction/mod.rs#L175) function today takes a `&dyn Catalog`. To write any data with a `SessionCatalog`, we'd need to introduce new, duplicate APIs. ## What changes are included in this PR? It adds this new implementation for all SessionCatalog's: ```rust impl dyn SessionCatalog { /// Bind this catalog to a session, exposing the ordinary Catalog API. pub fn into_catalog(self: Arc<Self>, session: SessionContext) -> Arc<dyn Catalog> { Arc::new(SessionBoundCatalog { inner: self, session, }) } } ``` where a `SessionBoundCatalog` is a private wrapper around an inner `SessionCatalog` that adapts it to the `Catalog` trait by always providing the same _bound_ session context. ### RestCatalog Note, this is basically mirroring what the `RestCatalog` implementation is now doing. It is also a wrapper with an inner `SessionCatalog` and `SessionContext`. The reason I'm introducing a separate type here (that is not used by the `RestCatalog` implementation) are: 1. we need to have the `SessionCatalog` trait be convertible to `Catalog` (the Datafusion integration uses traits for its providers, and not concrete catalog implementations) 2. to use the `SessionBoundCatalog` for the `RestCatalog` we'd need to make it `pub`. IMO the limited amount of duplication warrants the ability to keep it private for now. Users shouldn't really depend on the `SessionBoundCatalog`'s direct API (beyond its `Catalog` trait implementation), and so the export would only serve to remove some duplication. We can also always change this later without breaking compatibility. ## Are these changes tested? This is very similar to the `RestCatalog` implementation. The `SessionBoundCatalog` is not adding behavior and the delegation is exercised by the compiler. ## AI Disclosure Prototyped different approaches with Codex and Claude. Ended up typing the result by hand. -- 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]
