-
Notifications
You must be signed in to change notification settings - Fork 1.5k
refactor: move PREPARE/EXECUTE into LogicalPlan::Statement
#13311
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Conversation
@@ -311,6 +311,7 @@ fn optimize_projections( | |||
| LogicalPlan::Explain(_) | |||
| LogicalPlan::Analyze(_) | |||
| LogicalPlan::Subquery(_) | |||
| LogicalPlan::Statement(_) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a slt test for this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great -- thank you @jonahgao
} | ||
|
||
// Manual implementation needed because of `schema` field. Comparison excludes this field. | ||
impl PartialOrd for TransactionStart { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a nice cleanup (not to have a schema
as a field 👌 nice
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is a neat cleanup! Thanks @jonahgao 👍
…13311) * refactor: move PREPARE/EXECUTE into `LogicalPlan::Statement` * Fix cargo doc * Restore TableScan * Fix cargo doc
Which issue does this PR close?
See #13242 (comment)
Rationale for this change
Move
LogicalPlan::Prepare
andLogicalPlan::Execute
intoLogicalPlan::Statement
.They are more suitable for being implemented as statements. This can also reduce the variants of
LogicalPlan
.What changes are included in this PR?
LogicalPlan::Statement
PartialOrd
Are these changes tested?
Yes. By existing tests.
Are there any user-facing changes?
Yes