Extract Aggregate, Sort, and Join to struct from AggregatePlan #1326
Conversation
|
Thanks @matthewmturner ! |
matthewmturner
changed the title
datafusion/src/logical_plan/plan.rs
Outdated
| /// Aggregates its input based on a set of grouping and aggregate | ||
| /// expressions (e.g. SUM). | ||
| #[derive(Clone)] | ||
| pub struct AggregatePlan { |
There was a problem hiding this comment.
I think calling this Aggregate would be more consistent with what has been refactored in other PRs.
Let me know when this PR is ready @matthewmturner
Thanks!
datafusion/src/logical_plan/plan.rs
Outdated
| /// The schema description of the aggregate output | ||
| schema: DFSchemaRef, | ||
| }, | ||
| // Aggregate { |
There was a problem hiding this comment.
in the final version, we should remove these useless code
datafusion/src/logical_plan/plan.rs
Outdated
| @@ -289,7 +303,7 @@ impl LogicalPlan { | |||
| LogicalPlan::Projection { schema, .. } => schema, | |||
| LogicalPlan::Filter { input, .. } => input.schema(), | |||
| LogicalPlan::Window { schema, .. } => schema, | |||
| LogicalPlan::Aggregate { schema, .. } => schema, | |||
| LogicalPlan::Aggregate(aggregate) => &aggregate.schema, | |||
There was a problem hiding this comment.
@alamb I have question about the code style, before refactor, we use the style
::Aggregate {filed1, filed2, field3,....}
Someone may use the style after refactor
Aggregate(agg) =>
agg.field1
agg.field2
agg.field3
How about make consistent with before?
In My pull requests #1325 #1322 #1311, I keep consistent and use this style.
There was a problem hiding this comment.
@liukun4515 sorry I missed this question -- my preference would be for the resulting code to be consistent
For example, always use
LogicalPlan::Aggregate(Aggregate { ... })or
LogicalPlan::Aggregate(aggregate)I don't have any particular preference personally about which to use as long as we are consistent
datafusion/src/logical_plan/plan.rs
Outdated
| aggr_expr, | ||
| .. | ||
| } => group_expr.iter().chain(aggr_expr.iter()).cloned().collect(), | ||
| LogicalPlan::Aggregate(aggregate) => aggregate |
There was a problem hiding this comment.
Same opinion like above, if we keep consistent like before, we can make less changes.
There was a problem hiding this comment.
please align the code style using
LogicalPlan::Aggregate(aggregate {
field1，
field2
})
| @@ -20,8 +20,8 @@ | |||
| use crate::error::Result; | |||
| use crate::execution::context::ExecutionProps; | |||
| use crate::logical_plan::{ | |||
| col, DFField, DFSchema, Expr, ExprRewriter, ExpressionVisitor, LogicalPlan, | |||
| Recursion, RewriteRecursion, | |||
| col, plan::AggregatePlan, DFField, DFSchema, Expr, ExprRewriter, ExpressionVisitor, | |||
There was a problem hiding this comment.
we can make the struct AggregatePlan pub in the logical plan package mod file
|
I think this PR is now the last remaining in the set for #1228 -- we are close now. @matthewmturner any chance you can rebase this PR and we can wrap up this item? |
yes, sry for delay. i am working on it. i am going to align to style recommended by @liukun4515 |
If your pull request is ready, I will review it. |
|
@liukun4515 ready for review! |
There was a problem hiding this comment.
LGTM, if you address the code style comments.
@matthewmturner
|
LGTM, thanks @matthewmturner. You are the terminator of #1228 ! 🎉 |
There was a problem hiding this comment.
Looking great -- thank you @matthewmturner @liukun4515 and @xudong963 for driving this refactoring
Which issue does this PR close?
Closes #1303
Closes #1228
Rationale for this change
What changes are included in this PR?
Are there any user-facing changes?