apache / apache/datafusion

make `ExecutionPlan` self contained. (allow for computed properties `PlanProperties`)

Open
#10,018 3 comments 2 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?

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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.