Extract EmptyRelation, Limit, Values from LogicalPlan
#1325
Conversation
liukun4515
force-pushed
the
datafusion_1305
branch
from
f66b2ce
to
b807f23
Compare
|
@alamb PTAL |
| /// The logical plan | ||
| input: Arc<LogicalPlan>, | ||
| }, | ||
| Limit(Limit), |
There was a problem hiding this comment.
Using a different name will be better, such as LimitPlan, keep the same as others.
There was a problem hiding this comment.
I actually think the way this PR is written, Limit(Limit) is more consistent. The only one that seems to follow a different pattern is TableScanPlan (which we should perhaps rename to TableScan?)
| @@ -107,7 +107,7 @@ impl OptimizerRule for ConstantFolding { | |||
|
|
|||
| utils::from_plan(plan, &expr, &new_inputs) | |||
| } | |||
| LogicalPlan::TableScan { .. } | LogicalPlan::EmptyRelation { .. } => { | |||
| LogicalPlan::TableScan { .. } | LogicalPlan::EmptyRelation(_) => { | |||
There was a problem hiding this comment.
nit: keep consistent. { .. } (_)
There was a problem hiding this comment.
perhaps we can do this in a follow on pr. Good spot @xudong963
There was a problem hiding this comment.
@xudong963 @alamb
I might miss this point when write the code.
After all the extracting logical plan, i will file a followup pull request to keep consistent.
There was a problem hiding this comment.
(I merged the PR in before you had a chance to respond to this comment -- so it was my fault :) )
|
Merged this one in to minimize conflicts with other outstanding PRs |
alamb
changed the title
EmptyRelation, Limit, Values from LogicalPlan
Which issue does this PR close?
Closes #1305
Rationale for this change
What changes are included in this PR?
Are there any user-facing changes?