Conversation
datafusion/src/optimizer/utils.rs
Outdated
| }) | ||
| } | ||
| 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
| 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?
| 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 {..}
...
}| 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.
5 participants
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.