apache / apache/datafusion

Expand Session trait to parity with SessionState

Open
#23,678 2 comments 0 reactions 0 assignees View on GitHub
enhancement
Dominant language
Rust
Stars
9.3k
Forks
2.4k
Avg merge
3d 7h
Merged PRs (30d)
344

Description

### Is your feature request related to a problem or challenge?

At a high level we are trying to expand our FFI crate to support both `datafusion-python` and `datafusion-java` with more extension points. The problem we run into when using these other languages with multiple libraries is that we cannot perform any kind of downcasting of a `&dyn Session` into the underlying `&Session`. There is a discussion of why this is the case on the [datafusion-python online documentation](https://datafusion.apache.org/python/contributor-guide/ffi.html).

What we want to do is to expose more of the functions currently in `SessionState` to be usable by anyone with access to a `&dyn Session`.

One use case: Users wish to be able to access the `CatalogListProvider` so that a table function could create a new view of an existing table. We cannot currently do this with `&Session` because the `catalog_list()` function does not exist on it.

Second use case: Users wish to create a query planner to replace the build in query planner. The problem with this is that the method within `QueryPlanner`, `create_physical_plan()` takes a `&SessionState` parameter. For us to be able to expose a `FFI_QueryPlanner` we need this method to take `&dyn Session`, which in turn needs `PhysicalPlanner` to have a similar change. If we want to create method to expose *or set* a query planner on the `Session` we need to resolve the fact that these traits are defined in `datafusion-core` so not accessible to `Session`.

### Describe the solution you'd like

I would like to reorganize our trait definitions. Specifically it seems like we need to either hoist these trait definitions into either `datafusion-session` or the `datafusion-expr` crates. Before doing any work, I think we should review the dependency graph and consider if we should have some thin crates that only expose surface API surfaces or not. On the one hand, that could be cleaner than doing something along the lines of adding `QueryPlanner`, `PhysicalPlanner`, `CatalogProviderList`, `TableProvider`, etc into either `datafusion-session` or `datafusion-expr` crates. The down side of adding additional crates is that we're now adding even more crates to our already pretty large dependency graph.

I think the first task should be to review and propose a solution for where the trait definitions should sit.

### Describe alternatives you've considered

There is another path where we use type erasure and then downcasting so that we don't have dependency graph problems. Personally I find this to be a hacky way to get around the dependency issue.

### Additional context

Related issues:

- https://github.com/apache/datafusion/issues/23348
- https://github.com/apache/datafusion/issues/15095

Contributor guide

Open the contributing guide

Research direction

Start by reviewing the dependency graph among datafusion-core, datafusion-session, and datafusion-expr, along with the trait definitions for Session, SessionState, QueryPlanner, PhysicalPlanner, CatalogProviderList, and TableProvider. Compare hoisting the traits with introducing thin crates, and consider the related issues 23348 and 15095. Done means a documented proposal for where these trait definitions should live and how the choice supports the FFI use cases without unnecessary dependency growth.

Written by the indexing model from the issue text.

Assessment

Tech stack
java, python, rust
Domain
api, backend-api-design
Issue type
Feature
Difficulty
5/5
Estimated time
Over a week
Activity status
Quiet
Clarity
Needs clarification
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.