make `ExecutionPlan` self contained. (allow for computed properties `PlanProperties`)
- 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?
if you have some structs that you want to loosely couple to datafusion, it is now impossible.
for example with <37
```rs
#[derive(Debug, Clone)]
pub struct MyStruct {
projection: Arc,
predicate_projection: Arc,
// ...
}
```
and then you can just implement the trait as one would expect
```rs
impl ExecutionPlan for MyStruct {
fn output_partitioning(&self) -> Partitioning {
Partitioning::UnknownPartitioning(1)
}
fn output_ordering(&self) -> Option<&[datafusion::physical_expr::PhysicalSortExpr]> {
None
}
// ...
}
```
But with datafusion 37, the datafusion specific components are now leaked into the outer struct, and force the user to modify the struct to contain `PlanProperties`.
```rs
impl ExecutionPlan for MyStruct {
fn properties(&self) -> &PlanProperties {
// no possible way to create `&PlanProperties`.
// the only option is to add it as a field on `MyStruct`
// which breaks the encapsulation of `ExecutionPlan`
}
}
```
This is especially amplified if you want to feature flag datafusion specific functionality.
## datafusion <37
```rs
#[derive(Debug, Clone)]
pub struct MyStruct {
// ...
}
impl MyStruct {
pub fn new() -> Self {
Self {
// ..
}
}
}
#[cfg(feature = "datafusion")]
impl ExecutionPlan for MyStruct {
// ...
}
```
## datafusion 37
```rs
#[derive(Debug, Clone)]
pub struct MyStruct {
// ...
#[cfg(feature = "datafusion")]
plan_properties: PlanProperties
}
impl MyStruct {
#[cfg(feature = "datafusion")]
pub fn new() -> Self {
let properties = make_properties_somehow():
Self {
// ..
plan_properties: properties
}
}
#[cfg(not(feature = "datafusion"))]
pub fn new() -> Self {
Self {
// ..
}
}
}
#[cfg(feature = "datafusion)]
impl ExecutionPlan for MyStruct {
// ...
}
```
### Describe the solution you'd like
modify `ExecutionPlan` to not have any methods that return a reference. Specifically `ExecutionPlan::properties`
If a reference is wanted for performance reasons, we should instead use `Cow`.
### Describe alternatives you've considered
NA
### Additional context
_No response_
Contributor guide
Research direction
Start with the ExecutionPlan::properties method and the PlanProperties type discussed in the issue, then inspect how existing execution plans construct and expose their properties. Compare the requested self-contained behavior with the Cow alternative. Done means the interface no longer forces implementors to store DataFusion-specific properties externally, with existing execution-plan behavior preserved.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- rust
- Domain
- data-engineering
- Issue type
- Feature
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 35/100