refactor: move catalog traits to session crate - #23703
Conversation
Move catalog, schema, table provider, and table function contracts into datafusion-session while preserving datafusion-catalog re-exports. Expose catalog access as a required Session capability, including across ForeignSession and the FFI boundary. AI Disclosure: This code was written in part by an AI agent.
|
Thank you for opening this pull request! Reviewer note: cargo-semver-checks reported the current version number is not SemVer-compatible with the changes in this pull request (compared against the base branch). Details |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #23703 +/- ##
==========================================
- Coverage 80.75% 80.75% -0.01%
==========================================
Files 1089 1089
Lines 368809 368866 +57
Branches 368809 368866 +57
==========================================
+ Hits 297837 297863 +26
- Misses 53217 53245 +28
- Partials 17755 17758 +3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| /// Return the catalogs registered with this session. | ||
| fn catalog_list(&self) -> Arc<dyn CatalogProviderList>; |
There was a problem hiding this comment.
This new function on the Session trait is the main point of this PR. The other code is moved around to support the dependency requirements.
There was a problem hiding this comment.
Pull request overview
This PR refactors DataFusion’s catalog API surface by moving key catalog-related traits/types from datafusion-catalog into datafusion-session, enabling catalog access via &dyn Session (notably across the FFI boundary) without downcasting to SessionState.
Changes:
- Move catalog contract traits/types (catalog/schema/table/table-function interfaces) into
datafusion-sessionand re-export them fromdatafusion-catalog. - Extend the
Sessiontrait with a requiredcatalog_list()method and implement it for core/FFI/mock sessions. - Update FFI plumbing and tests to expose and validate catalog access/mutation across the FFI boundary; update the 55.0.0 upgrade guide accordingly.
Reviewed changes
Copilot reviewed 18 out of 19 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/source/library-user-guide/upgrading/55.0.0.md | Documents the trait moves and the new required Session::catalog_list method. |
| datafusion/session/src/table.rs | Introduces table-related traits/types (e.g., TableProvider, table functions) in datafusion-session. |
| datafusion/session/src/session.rs | Adds required catalog_list() to the Session trait. |
| datafusion/session/src/schema.rs | Introduces SchemaProvider in datafusion-session. |
| datafusion/session/src/lib.rs | Exposes new catalog, schema, and table modules and re-exports their public API. |
| datafusion/session/src/catalog.rs | Introduces catalog traits (CatalogProviderList, etc.) and EmptyCatalogProviderList plus unit tests. |
| datafusion/session/Cargo.toml | Adds arrow-schema dependency needed by moved traits/types. |
| datafusion/ffi/tests/ffi_integration.rs | Extends integration test coverage to validate catalog mutation visibility. |
| datafusion/ffi/src/tests/async_provider.rs | Updates async provider to use Session::catalog_list() and registers a catalog for FFI validation. |
| datafusion/ffi/src/session/mod.rs | Wires catalog_list through the FFI Session surface and stores it in ForeignSession. |
| datafusion/datasource/src/url.rs | Updates mock session in tests to implement the new catalog_list() requirement. |
| datafusion/datasource-arrow/src/file_format.rs | Updates mock session in tests to implement the new catalog_list() requirement. |
| datafusion/core/src/execution/session_state.rs | Implements Session::catalog_list() for SessionState and adds a unit test. |
| datafusion/core/src/datasource/listing_table_factory.rs | Updates mock session in tests to implement the new catalog_list() requirement. |
| datafusion/catalog/src/table.rs | Re-exports table traits/types from datafusion-session. |
| datafusion/catalog/src/schema.rs | Re-exports SchemaProvider from datafusion-session. |
| datafusion/catalog/src/lib.rs | Ensures Session remains re-exported from datafusion-session via datafusion-catalog. |
| datafusion/catalog/src/catalog.rs | Re-exports catalog traits from datafusion-session. |
| Cargo.lock | Records the new arrow-schema dependency for datafusion-session. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Fix schema.rs doc typo and unclosed code span, correct upgrade guide trait list (TableFunction is a struct), and add missing imports to the catalog_list example snippet. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Looks good to me -- thanks @timsaucer
It might be worthwhile updating these docs too to reflect that the interfaces are now in session: https://github.com/apache/datafusion/blob/f802ed1c51ad82cb1194d4f2bd3010ec451b2d38/datafusion/catalog/src/lib.rs#L28-L27
Also should we update the docs for session?
https://github.com/apache/datafusion/blob/b63ca3e09a6f1be95810ff86da06f4f4b0e930aa/datafusion/session/README.md#L24-L23
https://github.com/alamb/datafusion/blob/5f9bacddcd14c227935f62109facafcee7616f7c/datafusion/session/src/lib.rs#L20-L19
Specifically to mention that it also defines the APIs (not the implementations)
| @@ -0,0 +1,246 @@ | |||
| // Licensed to the Apache Software Foundation (ASF) under one | |||
There was a problem hiding this comment.
So the idea is now that session is defining the APIs and the datafusion-catalog has the implementations?
There was a problem hiding this comment.
Correct!
With this and the next PR that will build on this one, datafusion-session is becoming more of a datafusion-api where the interacting traits are all defined and then things like datafusion-catalog will have the concrete implementations.
AI Disclosure: This code was written in part by an AI agent.
AI Disclosure: This code was written in part by an AI agent.
|
woohoo |
## Which issue does this PR close? - Addresses a portion of apache#23678 - Follow on to apache#23703 ## Rationale for this change This PR unlocks using a query planner across the FFI boundary. ## What changes are included in this PR? Moves these traits to the `datafusion-session` crate: - `QueryPlanner` - `PhysicalPlanner` - `ExtensionPlanner` - `PhysicalOptimizerRule` - `PhysicalOptimizerContext` Additionally changed the method signatures from taking `&SessionState` to `&dyn Session`. Adds these methods on `Session` trait: - `fn query_planner(&self) -> Arc<dyn QueryPlanner + Send + Sync>` - `fn optimize(&self, plan: &LogicalPlan) -> Result<LogicalPlan>` - `fn physical_optimizers(&self) -> &[Arc<dyn PhysicalOptimizerRule + Send + Sync>]` - `fn statistics_registry(&self) -> Option<&StatisticsRegistry>` ## Are these changes tested? Unit tests are added for the new methods. ## Are there any user-facing changes? Yes, users must implement new methods for `query_planner` for their custom `Session`. This is not expected to impact many users, since it is most common to use the existing `SessionState`. --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
## Which issue does this PR close? - Unblocks apache/datafusion-python#1612 ## Rationale for this change This is the last in a series of PRs that would enable FFI `Session` to support a `QueryPlanner`. The prior work was in - apache#23649 - apache#23703 - apache#23842 With those changes in place we now have the dependencies correct that we can expose a `FFI_QueryPlanner` on a `FFI_Session`. With this we can enable foreign libraries such as `datafusion-distributed` and `ballista` to provide a query planner in Python and connect it directly to a `datafusion-python`'s `SessionContext`. ## What changes are included in this PR? Addition only. Adds these functions to `FFI_Session` and their supporting structures: - `query_planner()` - `optimize()` - `physical_optimizers()` ## Are these changes tested? Unit and integration tests are provided. ## Are there any user-facing changes? This is addition, but it does break the FFI ABI, which is already evolving in DF55. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
## Which issue does this PR close? - Addresses one portion of apache#23678. ## Rationale for this change This will enable users to create table views using table function over an existing table in the catalog via FFI. We are currently unable to access the catalog other than by downcasting a `Session` to `SessionState`, which is not effective over FFI. In order to support this, we need to move many trait definitions out of the `datafusion-catalog` crate and into the `datafusion-session` crate. There is more discussion in apache#23678 about why this is necessary. ## What changes are included in this PR? - Move a variety of catalog traits into `datafusion-session`. - Add `catalog_list()` function to `Session`. - Implement `catalog_list()` in `ForeignSession` for FFI users ## Are these changes tested? New tests are included. ## Are there any user-facing changes? User guide is updated. --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
## Which issue does this PR close? - Addresses a portion of apache#23678 - Follow on to apache#23703 ## Rationale for this change This PR unlocks using a query planner across the FFI boundary. ## What changes are included in this PR? Moves these traits to the `datafusion-session` crate: - `QueryPlanner` - `PhysicalPlanner` - `ExtensionPlanner` - `PhysicalOptimizerRule` - `PhysicalOptimizerContext` Additionally changed the method signatures from taking `&SessionState` to `&dyn Session`. Adds these methods on `Session` trait: - `fn query_planner(&self) -> Arc<dyn QueryPlanner + Send + Sync>` - `fn optimize(&self, plan: &LogicalPlan) -> Result<LogicalPlan>` - `fn physical_optimizers(&self) -> &[Arc<dyn PhysicalOptimizerRule + Send + Sync>]` - `fn statistics_registry(&self) -> Option<&StatisticsRegistry>` ## Are these changes tested? Unit tests are added for the new methods. ## Are there any user-facing changes? Yes, users must implement new methods for `query_planner` for their custom `Session`. This is not expected to impact many users, since it is most common to use the existing `SessionState`. --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
## Which issue does this PR close? - Unblocks apache/datafusion-python#1612 ## Rationale for this change This is the last in a series of PRs that would enable FFI `Session` to support a `QueryPlanner`. The prior work was in - apache#23649 - apache#23703 - apache#23842 With those changes in place we now have the dependencies correct that we can expose a `FFI_QueryPlanner` on a `FFI_Session`. With this we can enable foreign libraries such as `datafusion-distributed` and `ballista` to provide a query planner in Python and connect it directly to a `datafusion-python`'s `SessionContext`. ## What changes are included in this PR? Addition only. Adds these functions to `FFI_Session` and their supporting structures: - `query_planner()` - `optimize()` - `physical_optimizers()` ## Are these changes tested? Unit and integration tests are provided. ## Are there any user-facing changes? This is addition, but it does break the FFI ABI, which is already evolving in DF55. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Which issue does this PR close?
Rationale for this change
This will enable users to create table views using table function over an existing table in the catalog via FFI. We are currently unable to access the catalog other than by downcasting a
SessiontoSessionState, which is not effective over FFI.In order to support this, we need to move many trait definitions out of the
datafusion-catalogcrate and into thedatafusion-sessioncrate. There is more discussion in #23678 about why this is necessary.What changes are included in this PR?
datafusion-session.catalog_list()function toSession.catalog_list()inForeignSessionfor FFI usersAre these changes tested?
New tests are included.
Are there any user-facing changes?
User guide is updated.