DerGut commented on code in PR #2920:
URL: https://github.com/apache/iceberg-rust/pull/2920#discussion_r3786376755


##########
crates/catalog/rest/src/catalog.rs:
##########
@@ -1349,6 +1501,176 @@ impl Catalog for RestCatalog {
     }
 }
 
+/// Builder for an unbound [`RestSessionCatalog`].
+///
+/// Unlike [`RestCatalogBuilder`], the resulting catalog accepts a
+/// [`SessionContext`] with each [`SessionCatalog`] operation.
+#[derive(Debug)]
+pub struct RestSessionCatalogBuilder {

Review Comment:
   Good point! I have a local draft ready that I can clean up later. 
   
   The main open question IMO is how to support providers that were passed a 
`SessionCatalog` and ones that were just passed a `Catalog`.
   
   My current version uses a
   
   ```rust
   enum CatalogBackend {
       Bound(Arc<dyn Catalog>),
       Session(Arc<dyn SessionCatalog>, Arc<dyn SessionContextResolver>),
   }
   ```
   
   to share the Provider implementation and branch via the enum.
   
   `SessionContextResolver` will be something like
   
   ```rust
   pub trait SessionContextResolver {
     fn resolve_session_context(&self, state: DFSession) -> 
DFResult<SessionContext>;
   }
   ```
   
   because Datafusion doesn't have a canonical way to represent query context 
like Trino does (beyond its `Session.session_id()`). Users instead pass 
metadata via extensions which they will need to extract in custom 
implementations and convert to `SessionContext` themselves.
   
   More on this hopefully this weekend!



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