apache / apache/datafusion

Why do QueryPlanner and PhysicalPlanner exist as independent concepts?

Open
#13,943 1 comment 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?

I wrote a `LogicalPlan::Extension` and an `ExtensionPlanner` to convert it into a physical plan. Then I tried to figure out how to configure a `SessionContext` to use my extension planner. It took a little bit:

Step 1: `let planner = DefaultPhysicalPlanner::with_extension_planners(vec![Arc::new(GatherPlanner)])` to get an object that impls `PhysicalPlanner` that understands my extension.
Step 2: squint at `SessionStateBuilder` and surrounding environs looking for how to give it a custom `Arc`.
Step 3: eventually realize that `SessionStateBuilder` actually wants you to give it a `QueryPlanner` instead.
Step 4: `QueryPlanner` appears to be a strict subset of `PhysicalPlanner`? So I guess there should be some blanket impl or something that lets me pass in my `PhysicalPlanner` as a `QueryPlanner`?
Step 5: No apparently there is no blanket impl, and `DefaultQueryPlanner` is a private struct, and it's just a trivial wrapper around `DefaultPhysicalPlanner` but without any configurability.
Conclusion (?): Everyone who wants to extend planning is expected to copy/paste this code to make their own `dyn QueryPlanner` that wraps `DefaultPhysicalPlanner` but with their own configuration.

Is that... right? I keep feeling like I must be missing something.

### Describe the solution you'd like

I can think of a few options that would simplify:

- Merge `QueryPlanner` and `PhysicalPlanner` traits into a single trait
- Make `PhysicalPlanner` a sub-trait of `QueryPlanner`
- Provide a blanket `impl QueryPlanner for T`
- `impl QueryPlanner for DefaultPhysicalPlanner`

This is ordered with things that would have simplified my life as a new user the most on the top, and the least disruptive to the existing codebase on the bottom. I'm not sure what constraints led to the current design so I can't really judge which tradeoff is best.

### Describe alternatives you've considered

_No response_

### Additional context

_No response_

Contributor guide

Open the contributing guide

Research direction

Start with SessionStateBuilder, QueryPlanner, PhysicalPlanner, DefaultQueryPlanner, and DefaultPhysicalPlanner, then trace how ExtensionPlanner is configured for LogicalPlan::Extension. Compare the proposed trait relationships and document the design constraints; done means the relationship is clear and extension planners have an agreed configuration path.

Written by the indexing model from the issue text.

Assessment

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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.