Extract Projection, Filter, Window in LogicalPlan as independent struct #1309
Conversation
|
@ic4y Thanks for your contribution. You need to fix the lint error, after fixing it, I'll start reviewing! |
|
@xudong963 |
| @@ -777,9 +778,9 @@ impl TryInto<protobuf::LogicalPlanNode> for &LogicalPlan { | |||
| ))) | |||
| } | |||
| } | |||
| LogicalPlan::Projection { | |||
| LogicalPlan::Projection(ProjectionPlan { | |||
There was a problem hiding this comment.
I think we can use the style, LogicalPlan::Projection(p) => {p.var1, p.var2, ...}.
FYI, #1228 (comment)
| @@ -794,7 +795,7 @@ impl TryInto<protobuf::LogicalPlanNode> for &LogicalPlan { | |||
| }, | |||
| ))), | |||
| }), | |||
| LogicalPlan::Filter { predicate, input } => { | |||
| LogicalPlan::Filter(FilterPlan { predicate, input }) => { | |||
There was a problem hiding this comment.
ditto, you can also check other places.
datafusion/src/logical_plan/plan.rs
Outdated
| /// The schema description of the window output | ||
| schema: DFSchemaRef, | ||
| }, | ||
| /// the ProjectionPlan |
There was a problem hiding this comment.
I think we don't need annotation here. cc @alamb
nit: It's good to start with an upper case, such as The projection plan.
datafusion/src/logical_plan/plan.rs
Outdated
| Projection(ProjectionPlan), | ||
| /// the FilterPlan | ||
| Filter(FilterPlan), | ||
| /// the WindowPlan |
|
BTW, you need to solve conflicts. |
|
@xudong963 Thanks, I will do it today |
# Conflicts: # datafusion/src/execution/context.rs # datafusion/src/logical_plan/builder.rs # datafusion/src/logical_plan/plan.rs # datafusion/src/optimizer/filter_push_down.rs # datafusion/src/optimizer/limit_push_down.rs # datafusion/src/optimizer/projection_push_down.rs # datafusion/src/optimizer/utils.rs # datafusion/src/physical_plan/planner.rs # datafusion/tests/custom_sources.rs
|
@ic4y After you finish it, ping me and I'll take a look |
|
@xudong963 Thanks, I have finished. |
|
LGTM!
You can edit here correctly. |
datafusion/src/logical_plan/plan.rs
Outdated
|
|
||
| /// Window its input based on a set of window spec and window function (e.g. SUM or RANK) | ||
| #[derive(Clone)] | ||
| pub struct WindowPlan { |
There was a problem hiding this comment.
Thank you @ic4y
I feel like these names are not consistent with the other structs in this file
How about renaming
ProjectionPlan --> Projection
FilterPlan --> Filter
WindowPlan --> Window
To be consistent with the naming of the model of CrossJoin (not CrossJoinPlan), Repartition (not RepartitonPlan), etc)?
There was a problem hiding this comment.
OK, thanks @alamb I will modify according to your suggestion.
| LogicalPlan::Projection { | ||
| expr, input, alias, .. | ||
| } => Ok(protobuf::LogicalPlanNode { | ||
| LogicalPlan::Projection(projection_plan) => Ok(protobuf::LogicalPlanNode { |
There was a problem hiding this comment.
Same comment in the pull request
@xudong963 how about make consistent like before?
If the changes is suffering for multi contributors, we can fill a follow up pull request to make consistent.
There was a problem hiding this comment.
@liukun4515 My thoughts are the same as you, @xudong963 What you think？
I submit it first, and then modify it if necessary.
There was a problem hiding this comment.
The modification looks good to me.
# Conflicts: # ballista/rust/core/src/serde/logical_plan/to_proto.rs # datafusion/src/logical_plan/builder.rs # datafusion/src/logical_plan/plan.rs # datafusion/src/optimizer/limit_push_down.rs # datafusion/src/optimizer/utils.rs # datafusion/src/physical_plan/planner.rs
There was a problem hiding this comment.
Thanks @ic4y @liukun4515 and @xudong963 . Really appreciate your help and progress here
Closes #1302
Which issue does this PR close?
Related to #1228
Rationale for this change
What changes are included in this PR?
Are there any user-facing changes?