Add RestSessionCatalog - #2920
Conversation
9f36a56 to
75caec7
Compare
beedd7a to
2dd57e5
Compare
2dd57e5 to
f154271
Compare
|
|
||
| /// Configures the session that will be used with this catalog. | ||
| /// Overwrites the default empty session from SessionContext::empty(). | ||
| pub fn with_session(mut self, session: SessionContext) -> Self { |
There was a problem hiding this comment.
As mentioned in the "Welcome Feedback ❓" section, I could use some opinions on the session vs. context/ ctx variable naming. I feel like what we introduce here will be reused throughout.
| pub fn with_session(mut self, session: SessionContext) -> Self { | |
| pub fn with_session_context(mut self, ctx: SessionContext) -> Self { |
There was a problem hiding this comment.
This is a good point and I agree that naming is important, I'm thinking of:
session_context or context: SessionContext
auth_session: AuthSession (some existing fields are named session which I think can be more explicit)
RestContext -> RestClient sounds like a good direction to address the naming conflict
I think a small PR to address the naming before the feature change would be very helpful
4215d14 to
6890cd2
Compare
| &RestSessionCatalog::auth_props(user_config), | ||
| ) | ||
| .await?; | ||
| RestSessionCatalog::load_config( |
There was a problem hiding this comment.
I'm leaning towards moving the two functions RestSessionCatalog::auth_props and RestSessionCatalog::load_config over to the RestClient too.
They're only used in the RestClient::init and aren't part of the catalog API (not part of the Catalog trait).
There was a problem hiding this comment.
makes sense to me, let's do it!
6890cd2 to
49cb24e
Compare
CTTY
left a comment
CTTY
left a comment
There was a problem hiding this comment.
LGTM in general! there are some inconsistencies in doc, we should fix them before moving on.
Also I'm assuming we will actually propagate the context in the followup PR?
| &RestSessionCatalog::auth_props(user_config), | ||
| ) | ||
| .await?; | ||
| RestSessionCatalog::load_config( |
There was a problem hiding this comment.
makes sense to me, let's do it!
|
|
||
| assert!(result.is_ok()); | ||
|
|
||
| // Without `with_session`, the catalog falls back to `SessionContext::empty()`, |
There was a problem hiding this comment.
| // Without `with_session`, the catalog falls back to `SessionContext::empty()`, | |
| // Without `with_session_context`, the catalog falls back to `SessionContext::empty()`, |
| // to loading the namespace (GET) and treating a missing namespace as | ||
| // `false`, so this still works against servers that don't advertise the | ||
| // HEAD route. | ||
| if !self.supports_endpoint(&V1_NAMESPACE_EXISTS).await? { |
There was a problem hiding this comment.
There are some documentation need to be updated since we are moving functions into RestSessionCatalog: https://github.com/apache/iceberg-rust/blob/main/crates/catalog/rest/src/endpoint.rs#L27
| @@ -1083,7 +1219,11 @@ impl Catalog for RestCatalog { | |||
| /// If there are any config properties that are present in both the response from the REST | |||
| /// server and the config provided when creating this `RestCatalog` instance, then the value | |||
| @@ -998,6 +1133,7 @@ impl Catalog for RestCatalog { | |||
| /// the value provided locally to the `RestCatalog` will take precedence. | |||
2 participants
Which issue does this PR close?
What changes are included in this PR?
This PR adds
RestSessionCatalog, implementing theSessionCatalogAPI introduced in #2836.RestSessionCatalogbecomes the core REST implementation. The existingRestCatalogdelegates to it with an internalSessionContext, preserving session-unaware behavior. The REST test suite is refactored to exerciseRestSessionCatalogdirectly.Welcome Feedback ❓
The main question I had while implementing this is which canonical variable name we should introduce for the
SessionContext. The current implementation usessessionbecause there's already acontextin theRestSessionCatalog's scope (which deals with the lazy creation of the client via the/v1/configendpoint).Arguably, the
SessionContextisn't equivalent to a session. It may carry state from a query engine's session, and it may be converted to anAuthSessionwhich is a session in the Iceberg sense. But maybe it's close enough.An alternative way to name the
SessionContextin variables would be to rename theRestContexttype to something likeRestClient, we'd free the concept of a context in that scope.Follow-ups
This PR is going to be followed by a
integrations/datafusionPR that starts using aSessionCatalogto propagate Datafusion sessions.Are these changes tested?
The existing test suite passes after backing the
RestCatalogwith theRestSessionCatalog(first commit c7fd2fe). Afterwards, I've refactored the test suite to test theRestSessionCatalogdirectly (as it now contains the core logic), and added some additional tests for session catalog creation and integration-style tests that assert the delegation is working.