Extract LogicalPlan::Create* DDL related plan structures into LogicalPlan::Ddl
#6121
Conversation
github-actions
bot
added
core
alamb
force-pushed
the
alamb/ddl_in_own_plan
branch
from
8a86fab to
2dfe8c7
Compare
| self.create_external_table(&cmd).await | ||
| } | ||
|
|
||
| LogicalPlan::CreateMemoryTable(CreateMemoryTable { |
There was a problem hiding this comment.
I moved this code into separate methods as this particular dispatch method was getting quite large
| @@ -628,6 +488,159 @@ impl SessionContext { | |||
| self.return_empty_dataframe() | |||
| } | |||
|
|
|||
| async fn create_memory_table(&self, cmd: CreateMemoryTable) -> Result<DataFrame> { | |||
| // There is no default plan for "CREATE EXTERNAL | ||
| // TABLE" -- it must be handled at a higher level (so | ||
| // that the appropriate table can be registered with | ||
| LogicalPlan::Ddl(ddl) => { |
There was a problem hiding this comment.
I think this reduction of repetition makes the intent of the code clearer
| Union, Unnest, UserDefinedLogicalNode, UserDefinedLogicalNodeCore, Values, Window, | ||
| WriteOp, | ||
| }; | ||
| pub use logical_plan::*; |
There was a problem hiding this comment.
this code was just re-exporting all the names in logical_plan anyways so I reduced the duplication
|
|
||
| /// Various types of DDL (CREATE / DROP) catalog manipulation | ||
| #[derive(Clone, PartialEq, Eq, Hash)] | ||
| pub enum DdlStatement { |
There was a problem hiding this comment.
This is the new wrapper enum
| @@ -1190,28 +1130,6 @@ pub enum JoinConstraint { | |||
| Using, | |||
| } | |||
|
|
|||
| /// Creates a catalog (aka "Database"). | |||
|
I wonder if you have any thoughts about this change @avantgardnerio @andygrove or @liukun4515 |
There was a problem hiding this comment.
I like it. I've always liked the idea of breaking up DML, DDL, etc - but the one deal breaker for me was that I would definitely like to keep them all as part of the same LogicalPlan so the same planning, optimizing and execution pipeline still works, so that placeholder replacement still works, etc.
This seems to achieve that, so 👍
|
I broke the build with this one :sad -- fix #6143 |
|
FWIW here is the follow on PR to move the |
Which issue does this PR close?
Part of #1281
Rationale for this change
While reviewing #6096 from @jaylmiller I noticed that the LogicalPlan was getting quite large for the various Catalog manipulation statements
What changes are included in this PR?
Thus I think it is time to finally implement #1281 and isolate these a bit more
Before I went and changed everything though I figured I would put up an RFC that moves the
CREATE*DDLs over. I think it looks much nicer.I will add the DROP variants (like
DropTable) as a follow on PRAre these changes tested?
Covered by existing tests
Are there any user-facing changes?
Yes, this is an API change -- some LogicalPlan variants are now wrapped up another layer of struct