Extract EmptyRelation, Limit, Values from LogicalPlan#1325
Extract EmptyRelation, Limit, Values from LogicalPlan#1325alamb merged 2 commits intoapache:masterfrom
EmptyRelation, Limit, Values from LogicalPlan#1325Conversation
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?)
| 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 :) )
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
I think it looks good. Thanks @liukun4515 !
|
Merged this one in to minimize conflicts with other outstanding PRs |
alamb
changed the title
EmptyRelation, Limit, Values from LogicalPlan
…ache#1325) * fall back to Spark when hashing decimals with precision > 18 * murmur3 checks * refactor * fix * address feedback
4 participants
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?