alamb commented on code in PR #3267:
URL: https://github.com/apache/arrow-datafusion/pull/3267#discussion_r962128665
##########
datafusion/expr/src/logical_plan/plan.rs:
##########
@@ -1019,6 +1030,17 @@ pub struct DropTable {
pub schema: DFSchemaRef,
}
+/// Drops a view.
Review Comment:
Not for this PR, but it seems to me like requiring new `LogicalPlan` nodes
for each type of DDL (`CREATE`, `DROP`, etc) is somewhat cumbersome. Maybe we
can eventually refactor type of thing into `LogicalPlan::DDL(DDL)` and keep all
the DDL related structs in their own structure. I'll file a follow on ticket
for this
##########
datafusion/sql/src/planner.rs:
##########
@@ -245,20 +245,29 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {
schema: Arc::new(DFSchema::empty()),
})),
Statement::Drop {
- object_type: ObjectType::Table,
+ object_type,
if_exists,
names,
cascade: _,
purge: _,
- } =>
- // We don't support cascade and purge for now.
- {
- Ok(LogicalPlan::DropTable(DropTable {
+ // We don't support cascade and purge for now.
+ // nor do we support multiple object names
+ } => match object_type {
+ ObjectType::Table => Ok(LogicalPlan::DropTable(DropTable {
name: names.get(0).unwrap().to_string(),
if_exists,
schema: DFSchemaRef::new(DFSchema::empty()),
- }))
- }
+ })),
+ ObjectType::View => Ok(LogicalPlan::DropView(DropView {
+ name: names.get(0).unwrap().to_string(),
+ if_exists,
+ schema: DFSchemaRef::new(DFSchema::empty()),
+ })),
+ _ => Err(DataFusionError::NotImplemented(
+ "Only `DROP TABLE/VIEW ...` statement is supported
currently"
Review Comment:
👍
##########
datafusion/core/tests/sql/create_drop.rs:
##########
@@ -112,6 +112,104 @@ async fn drop_table() -> Result<()> {
Ok(())
}
+#[tokio::test]
+async fn drop_view() -> Result<()> {
+ let ctx =
+
SessionContext::with_config(SessionConfig::new().with_information_schema(true));
+ plan_and_collect(&ctx, "CREATE VIEW v AS SELECT 1").await?;
+ let rb = plan_and_collect(
+ &ctx,
+ "select * from information_schema.tables where table_name = 'v' and
table_type = 'VIEW'",
+ )
+ .await?;
+ assert_eq!(rb[0].num_rows(), 1);
+
+ plan_and_collect(&ctx, "DROP VIEW v").await?;
+ let rb = plan_and_collect(
+ &ctx,
+ "select * from information_schema.tables where table_name = 'v' and
table_type = 'VIEW'",
+ )
+ .await?;
+ assert!(rb.is_empty());
+ Ok(())
+}
+
+#[tokio::test]
+#[should_panic(expected = "doesn't exist")]
+async fn drop_view_nonexistent() {
+ let ctx = SessionContext::new();
+ ctx.sql("DROP VIEW non_existent_view")
+ .await
+ .unwrap()
+ .collect()
+ .await
+ .unwrap();
+}
+
+#[tokio::test]
+#[should_panic(expected = "doesn't exist")]
+async fn drop_view_cant_drop_table() {
Review Comment:
👍 nice
##########
datafusion/core/src/execution/context.rs:
##########
@@ -313,10 +310,7 @@ impl SessionContext {
let table = self.table(name.as_str());
match (if_not_exists, or_replace, table) {
- (true, false, Ok(_)) => {
- let plan = LogicalPlanBuilder::empty(false).build()?;
- Ok(Arc::new(DataFrame::new(self.state.clone(), &plan)))
- }
+ (true, false, Ok(_)) => self.return_empty_dataframe(),
Review Comment:
This change to refactor and reduce duplicated code, is a very nice example
of leaving the code better than when you found it. It is very much appreciated
@kmitchener
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
