apache / apache/datafusion

[Discuss] Different implementation style between Expr, LogicalPlan and ExecutionPlan

Open
#2,175 32 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? Please describe what you are trying to do.**

In current DataFusion code base, `Expr`, `LogicalPlan` and `ExecutionPlan` are represented in different ways. Actually they are all tree based structures. `Expr` and `LogicalPlan` are Enums, `ExecutionPlan` is a Trait.
The LogicalPlan enum wrapped the different logical operator structs like Projection and Filter, etc.

````
pub enum LogicalPlan {
Projection(Projection),
Filter(Filter),
............
}
````

But the Expr enum doesn't wrap the expression structs and define the different expressions directly in the enum.

````
pub enum Expr {
Alias(Box, String),
Column(Column),
............
AggregateFunction {
fun: aggregate_function::AggregateFunction,
args: Vec,
distinct: bool,
},
WindowFunction {
fun: window_function::WindowFunction,
args: Vec,
partition_by: Vec,
order_by: Vec,
window_frame: Option,
},
............
}
````

I think we should unify the coding style, at least the Expr and LogicalPlan representations should follow the same style. And for physical ExecutionPlan, it is Trait/Trait Objects, I would prefer to use Enum also. Want to hear thoughts from other members.

**Describe the solution you'd like**
A clear and concise description of what you want to happen.

**Describe alternatives you've considered**
A clear and concise description of any alternative solutions or features you've considered.

**Additional context**
Add any other context or screenshots about the feature request here.

Contributor guide

Open the contributing guide

Research direction

Start by comparing the Expr, LogicalPlan, and ExecutionPlan definitions described in the issue and review the surrounding tree representations. The issue is a design discussion rather than a bounded implementation task; done would require agreement on a unified representation and a defined migration scope.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust
Domain
data-engineering
Issue type
Refactor
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Needs clarification
Newbie friendliness
20/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.