Add drop table support #1266
Add drop table support #1266
Conversation
github-actions
bot
added
datafusion
datafusion/src/optimizer/utils.rs
Outdated
| @@ -228,6 +228,13 @@ pub fn from_plan( | |||
| name: name.clone(), | |||
| }) | |||
| } | |||
| LogicalPlan::DropMemoryTable { name, if_exist, .. } => { | |||
| Ok(LogicalPlan::DropMemoryTable { | |||
There was a problem hiding this comment.
I think this should handled the same as cases without input, like LogicalPlan::EmptyRelation
datafusion/src/logical_plan/plan.rs
Outdated
| @@ -212,6 +212,15 @@ pub enum LogicalPlan { | |||
| /// The logical plan | |||
| input: Arc<LogicalPlan>, | |||
| }, | |||
| /// Drops an in memory table. | |||
| DropMemoryTable { | |||
There was a problem hiding this comment.
I think it should be called DropTable as we are also dropping external tables.
| /// If the table exists | ||
| if_exist: bool, | ||
| /// Dummy schema | ||
| schema: DFSchemaRef, |
There was a problem hiding this comment.
i wonder if it's valuable to actually just attach the fetched table schema
There was a problem hiding this comment.
otherwise you can just remove this placeholder
There was a problem hiding this comment.
Why do we need a schema for dropping the table?
There was a problem hiding this comment.
yea, seems an empty schema is better here.
| object_type: ObjectType::Table, | ||
| if_exists, | ||
| names, | ||
| cascade: _, |
There was a problem hiding this comment.
is it possible to add cascade and purge to the statement to the logical plan, or otherwise have a warning here saying that they do not take any effect
There was a problem hiding this comment.
I've just added a comment here, is it okay?
| @@ -274,6 +283,7 @@ impl LogicalPlan { | |||
| LogicalPlan::Extension { node } => node.schema(), | |||
| LogicalPlan::Union { schema, .. } => schema, | |||
| LogicalPlan::CreateMemoryTable { input, .. } => input.schema(), | |||
| LogicalPlan::DropTable { schema, .. } => schema, | |||
There was a problem hiding this comment.
We could return an empty schema here instead of keeping it the plan node.
There was a problem hiding this comment.
I tried creating an empty schema before, but schema returns &DFSchemaRef, so rustc complains about a reference to dropped temporary object.
There was a problem hiding this comment.
yeah, that is unfortunate but I think the comments in the LogicalPlan struct are reasonable
There was a problem hiding this comment.
Hm. Another solution could be having / referencing a static object for an empty schema, but I'm ok with the current situation.
There was a problem hiding this comment.
Thank you @viirya
The comments on this PR got me thinking there are are really two types of LogicalPlan variants:
- That can actually be compiled / run as a
ExecutionPlan(e.g.LogicalPlan::Select) - That must effectively be "interpreted" in the
DataFrame(e.g.CreateTable,DropTable, etc)
It might be cool to split them up somehow.
Perhaps the SQLParser could produce something like
enum ParsedPlan {
Query(LogicalPlan),
DDL(DDLPlan)
}Where DDLPLan was like
enum DDLPlan {
CreateExternalTable {...}
CreateMemoryTable {..},
DropTable {..}
...
}| @@ -274,6 +283,7 @@ impl LogicalPlan { | |||
| LogicalPlan::Extension { node } => node.schema(), | |||
| LogicalPlan::Union { schema, .. } => schema, | |||
| LogicalPlan::CreateMemoryTable { input, .. } => input.schema(), | |||
| LogicalPlan::DropTable { schema, .. } => schema, | |||
There was a problem hiding this comment.
yeah, that is unfortunate but I think the comments in the LogicalPlan struct are reasonable
|
Thank you @viirya |
|
Thank you @Dandandan @alamb @jimexist |
|
thanks @viirya , just in time for the 6.0.0 release :D |
|
Filed #1281 to track the "split out the DDL commands" idea |
* Add drop table support. * Rename to DropTable. * Remove undeclared crate. * Fix clippy error. * For review comments.
Which issue does this PR close?
Closes #1252.
Rationale for this change
What changes are included in this PR?
Add drop table support.
Are there any user-facing changes?
User can drop table by using the new syntax.