Extract Explain, Analyze, Extension in LogicalPlan as independent struct #1317
Conversation
github-actions
bot
added
ballista
datafusion
| @@ -973,24 +973,24 @@ impl TryInto<protobuf::LogicalPlanNode> for &LogicalPlan { | |||
| )), | |||
| }) | |||
| } | |||
| LogicalPlan::Analyze { verbose, input, .. } => { | |||
| let input: protobuf::LogicalPlanNode = input.as_ref().try_into()?; | |||
| LogicalPlan::Analyze(a) => { | |||
There was a problem hiding this comment.
It's better to import the Analyze struct and use this code style
LogicalPlan::Analyze(Aanlyze{
verbose, input,....
})
There was a problem hiding this comment.
In fact, I don't have a strong desire which is better. When the struct has many variables, I'd like to use StructVar(s) => {s.v1, s.v2 ...} to keep code concise. If there are a few variables, it doesn't matter, both styles make sense to me.
There was a problem hiding this comment.
I agree either style seems fine to me
| } = plan | ||
| { | ||
| let mut stringified_plans = stringified_plans.clone(); | ||
| if let LogicalPlan::Explain(e) = plan { |
There was a problem hiding this comment.
same comments like above.
| @@ -696,21 +698,21 @@ impl LogicalPlanBuilder { | |||
| let schema = schema.to_dfschema_ref()?; | |||
|
|
|||
| if analyze { | |||
| Ok(Self::from(LogicalPlan::Analyze { | |||
| Ok(Self::from(LogicalPlan::Analyze(AnalyzePlan { | |||
There was a problem hiding this comment.
I think this code style is better than above style.
This is same with this pull request #1290
There was a problem hiding this comment.
LGTM -- thanks @xudong963 and @liukun4515
I also merged apache/master into this branch locally to test for logical conflicts and things look good 👍
Which issue does this PR close?
Closes #1307
Rationale for this change
What changes are included in this PR?
Are there any user-facing changes?