Skip to content

Add RestSessionCatalog - #2920

Open
DerGut wants to merge 3 commits into
apache:mainfrom
DerGut:rest-session-catalog
Open

Add RestSessionCatalog#2920
DerGut wants to merge 3 commits into
apache:mainfrom
DerGut:rest-session-catalog

Conversation

@DerGut

@DerGut DerGut commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

What changes are included in this PR?

This PR adds RestSessionCatalog, implementing the SessionCatalog API introduced in #2836.

RestSessionCatalog becomes the core REST implementation. The existing RestCatalog delegates to it with an internal SessionContext, preserving session-unaware behavior. The REST test suite is refactored to exercise RestSessionCatalog directly.

Welcome Feedback ❓

The main question I had while implementing this is which canonical variable name we should introduce for the SessionContext. The current implementation uses session because there's already a context in the RestSessionCatalog's scope (which deals with the lazy creation of the client via the /v1/config endpoint).

Arguably, the SessionContext isn't equivalent to a session. It may carry state from a query engine's session, and it may be converted to an AuthSession which is a session in the Iceberg sense. But maybe it's close enough.

An alternative way to name the SessionContext in variables would be to rename the RestContext type to something like RestClient, we'd free the concept of a context in that scope.

Follow-ups

This PR is going to be followed by a integrations/datafusion PR that starts using a SessionCatalog to propagate Datafusion sessions.

Are these changes tested?

The existing test suite passes after backing the RestCatalog with the RestSessionCatalog (first commit c7fd2fe). Afterwards, I've refactored the test suite to test the RestSessionCatalog directly (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.

@DerGut
DerGut force-pushed the rest-session-catalog branch from 9f36a56 to 75caec7 Compare July 28, 2026 22:21
@DerGut
DerGut force-pushed the rest-session-catalog branch 2 times, most recently from beedd7a to 2dd57e5 Compare August 5, 2026 15:57
@DerGut
DerGut force-pushed the rest-session-catalog branch from 2dd57e5 to f154271 Compare August 5, 2026 16:30

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
pub fn with_session(mut self, session: SessionContext) -> Self {
pub fn with_session_context(mut self, ctx: SessionContext) -> Self {

@DerGut
DerGut marked this pull request as ready for review August 5, 2026 17:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant