DerGut commented on code in PR #3000:
URL: https://github.com/apache/iceberg-rust/pull/3000#discussion_r3826369894
##########
crates/integrations/datafusion/src/table/mod.rs:
##########
@@ -80,18 +83,21 @@ impl IcebergTableProvider {
/// Loads the table once to get the initial schema, then stores the catalog
/// reference for future metadata refreshes on each operation.
pub(crate) async fn try_new(
- catalog: Arc<dyn Catalog>,
+ catalog_access: CatalogAccess,
Review Comment:
I tend to agree with @gabotechs that an `Arc<SessionCatalog>` may be the
better default than `Arc<Catalog>`.
> Would using Arc<dyn SessionCatalog> directly here block users from using
non-session catalog?
Directly, yes. Changing the constructor would be breaking, but we could
provide an adapter type for users:
```rust
let session_catalog = DroppingSessionCatalogAdapter::new(plain_catalog);
let provider = IcebergCatalogProvider(session_catalog);
```
I think a `SessionBoundCatalog` is a great internal utility type. It
provides an easy way to cache a shared fallback session context and we can
access session-aware and -unaware functionality. But users passing that type
would now have to wrap a `SessionCatalog` in that type.
--
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]
