Support DROP SCHEMA statements #6096

jaylmiller · 2023-04-22T19:04:13Z

Which issue does this PR close?

Closes #6027

Rationale for this change

Being able to drop schemas is generally just a good feature to have: exists in most databases.

What changes are included in this PR?

Add optional method deregister_schema to CatalogProvider trait (and an impl for it in MemoryCatalogProvider)
Add a new LogicalPlan node DropCatalogSchema
Modify parser to be able to parse LogicalPlan::DropCatalogSchema
Implement dropping schema in the datafusion context

Are these changes tested?

Unit tests and sql integration tests

Are there any user-facing changes?

this is a user-facing feature but everything is backwards compatible.

datafusion/expr/src/logical_plan/plan.rs

datafusion/common/src/table_reference.rs

comphead · 2023-04-24T17:43:46Z

datafusion/core/src/execution/context.rs

+                    (false, None, false) => Err(DataFusionError::Execution(format!(
+                        "Schema '{name}' doesn't exist."
+                    ))),
+                    // no schema found but dont return error


is it possible scenario?

This scenario would occur for a statement like DROP SCHEMA does_not_exist

The is like DROP SCHEMA foo IF EXISTS

This PR I think is consistent with what postgres does in this case

postgres=# drop schema if exists foo; NOTICE: schema "foo" does not exist, skipping DROP SCHEMA

comphead · 2023-04-24T17:48:03Z

datafusion/core/tests/sql/create_drop.rs

+    );
+    ctx.sql("DROP SCHEMA test_schema CASCADE").await?;
+
+    assert!(


alamb

Thank you @jaylmiller and @viirya and @comphead for the reviews. I left some structure comments that I think are worth considering but I don't think they are required to merge this PR as:

This PR is well tested
Some of the comments are to clean up the code structure rather than anything fundamental

alamb · 2023-04-25T15:17:20Z

datafusion/common/src/table_reference.rs

@@ -450,3 +450,39 @@ mod tests {
        );
    }
 }
+
+#[derive(Debug, Clone, PartialEq, Eq, Hash, PartialOrd, Ord)]


This is somewhat strange / non standard to have code below a mod test -- maybe it would be cleaner in its own module (e.g. datafusion/common/src/schema_reference.rs) perhaps

Sure that sounds good to me

alamb · 2023-04-25T15:18:13Z

datafusion/core/src/catalog/catalog.rs

+    ///
+    /// By default returns a "Not Implemented" error
+    fn deregister_schema(&self, name: &str) -> Result<Option<Arc<dyn SchemaProvider>>> {
+        // use variables to avoid unused variable warnings


An alternative pattern is to name the variable with a prefix. Like_

fn deregister_schema(&self, _name: &str) -> Result<Option<Arc<dyn SchemaProvider>>> {

alamb · 2023-04-25T15:25:58Z

datafusion/core/src/execution/context.rs

+                    (false, None, false) => Err(DataFusionError::Execution(format!(
+                        "Schema '{name}' doesn't exist."
+                    ))),
+                    // no schema found but dont return error


The is like DROP SCHEMA foo IF EXISTS

This PR I think is consistent with what postgres does in this case

postgres=# drop schema if exists foo; NOTICE: schema "foo" does not exist, skipping DROP SCHEMA

alamb · 2023-04-25T15:27:25Z

datafusion/core/tests/sqllogictests/test_files/information_schema.slt

@@ -375,7 +375,7 @@ SHOW CREATE TABLE test.xyz
 ----
 datafusion test xyz CREATE VIEW test.xyz AS SELECT * FROM abc

-statement error DataFusion error: This feature is not implemented: Only `DROP TABLE/VIEW
+statement error DataFusion error: Execution error: Cannot drop schema test because other tables depend on it: xyz


alamb · 2023-04-25T17:39:40Z

datafusion/core/src/execution/context.rs

+    /// Attempts to find a schema and deregister it. Returns a tuple of the schema and a
+    /// flag indicating whether dereg was performed (e.g if schema is found but has tables
+    /// then `cascade` must be set)
+    fn find_and_deregister_schema<'a>(


I wonder if different catalogs might want to implement the "is the schema empty" check differently 🤔

If so, it might make sense to put logic for "is the schema empty so can we drop it" in the CatalogProvider

Perhaps if CatalogProvider had a signature like:

fn deregister_schema(&self, name: &str, cascade: bool) -> Result<Option<Arc<dyn SchemaProvider>>> {

I think this code would get significantly simpler as well as the debug_asserts to reassert what had just been validated would be unnecessary.

That being said, we could also do such a change as a follow on PR

Hmm that is a good point. I was trying to keep the signature as similar as possible to the other Catalog's deregister methods. But you are right that it could make sense to do it like this. The code within the context provider is a bit complex at the moment

alamb · 2023-04-25T17:40:55Z

datafusion/core/src/execution/context.rs

@@ -2617,4 +2690,96 @@ mod tests {
                .unwrap()
        }
    }
+
+    /// helper for the following drop schema tests


I think we could use sqllogic tests https://github.com/apache/arrow-datafusion/blob/main/datafusion/core/tests/sqllogictests/ to write this more concisely

For example, perhaps we could extend:

https://github.com/apache/arrow-datafusion/blob/main/datafusion/core/tests/sqllogictests/test_files/ddl.slt

alamb · 2023-04-25T17:41:20Z

datafusion/core/tests/sql/create_drop.rs

@@ -73,3 +73,40 @@ async fn create_external_table_with_ddl() -> Result<()> {

    Ok(())
 }
+
+#[tokio::test]


I recommend putting these tests in https://github.com/apache/arrow-datafusion/blob/main/datafusion/core/tests/sqllogictests/test_files/ddl.slt rather than a rs test

jaylmiller · 2023-04-25T18:27:54Z

@alamb Thanks for the thorough review! I will modify this PR accordingly in the next day or so.

alamb · 2023-04-27T20:47:40Z

FYI I think #6144 will likely also cause conflicts with this PR

jaylmiller · 2023-04-27T21:54:06Z

FYI I think #6144 will likely also cause conflicts with this PR

@alamb No worries! I've actually been following along with your work there--thanks for CCing me.

I was planning on updating this PR according to your review in the next few days anyways--so conflicts not a big deal. Thanks for the heads up.

jaylmiller · 2023-04-29T14:51:35Z

@alamb Thanks for the suggestions: managed to clean this PR up ALOT following them.

I think this should be ready to review/merge, but let me know what you think!

alamb

I think this looks really nice -- thank you @jaylmiller

alamb · 2023-04-30T15:59:59Z

datafusion/core/src/catalog/catalog.rs

@@ -124,6 +124,26 @@ pub trait CatalogProvider: Sync + Send {
            "Registering new schemas is not supported".to_string(),
        ))
    }
+
+    /// Removes a schema from this catalog. Implementations of this method should return
+    /// errors if the schema exists but cannot be dropped. For example, in DataFusion's


❤️ --nice documentation

alamb · 2023-04-30T16:02:51Z

datafusion/expr/src/logical_plan/ddl.rs

+pub struct DropCatalogSchema {
+    /// The schema name
+    pub name: OwnedSchemaReference,
+    /// If the schema exists


Suggested change

/// If the schema exists

/// If true, do not error if the schema does not exist

github-actions bot added core Core DataFusion crate logical-expr Logical plan and expressions optimizer Optimizer rules sql SQL Planner labels Apr 22, 2023

jaylmiller changed the title ~~Add schema dropping~~ Support DROP SCHEMA statements Apr 22, 2023

jaylmiller marked this pull request as ready for review April 22, 2023 19:35

jaylmiller mentioned this pull request Apr 22, 2023

Support DROP SCHEMA statements #6027

Closed

viirya reviewed Apr 22, 2023

View reviewed changes

datafusion/expr/src/logical_plan/plan.rs Outdated Show resolved Hide resolved

datafusion/expr/src/logical_plan/plan.rs Outdated Show resolved Hide resolved

datafusion/common/src/table_reference.rs Outdated Show resolved Hide resolved

github-actions bot added the sqllogictest SQL Logic Tests (.slt) label Apr 22, 2023

comphead reviewed Apr 24, 2023

View reviewed changes

datafusion/core/tests/sql/create_drop.rs Outdated

);

ctx.sql("DROP SCHEMA test_schema CASCADE").await?;

assert!(

Copy link

Contributor

comphead Apr 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

alamb approved these changes Apr 25, 2023

View reviewed changes

alamb mentioned this pull request Apr 25, 2023

Extract LogicalPlan::Create* DDL related plan structures into LogicalPlan::Ddl #6121

Merged

scsmithr mentioned this pull request Apr 27, 2023

fix: Object name casing for views and schemas GlareDB/glaredb#927

Merged

alamb mentioned this pull request Apr 27, 2023

Complete Extracting LogicalPlan::Drop* DDL related plan structures into LogicalPlan::Ddl #6144

Merged

jaylmiller force-pushed the drop-schema branch from 280f40f to 45dcf3c Compare April 29, 2023 14:23

github-actions bot removed the optimizer Optimizer rules label Apr 29, 2023

jaylmiller force-pushed the drop-schema branch 2 times, most recently from fa552de to be19d7b Compare April 29, 2023 14:51

jaylmiller requested a review from alamb April 29, 2023 14:52

jaylmiller force-pushed the drop-schema branch from be19d7b to 336c501 Compare April 29, 2023 15:09

drop schema refactored

b8da32f

jaylmiller force-pushed the drop-schema branch from 336c501 to b8da32f Compare April 29, 2023 22:19

alamb approved these changes Apr 30, 2023

View reviewed changes

alamb merged commit a015798 into apache:main Apr 30, 2023

andygrove added the enhancement New feature or request label May 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support DROP SCHEMA statements #6096

Support DROP SCHEMA statements #6096

jaylmiller commented Apr 22, 2023 •

edited

Loading

comphead Apr 24, 2023

jaylmiller Apr 24, 2023 •

edited

Loading

alamb Apr 25, 2023

comphead Apr 24, 2023

alamb left a comment

alamb Apr 25, 2023

jaylmiller Apr 25, 2023

alamb Apr 25, 2023

alamb Apr 25, 2023

alamb Apr 25, 2023

alamb Apr 25, 2023

jaylmiller Apr 25, 2023 •

edited

Loading

alamb Apr 25, 2023

alamb Apr 25, 2023

jaylmiller commented Apr 25, 2023

alamb commented Apr 27, 2023

jaylmiller commented Apr 27, 2023

jaylmiller commented Apr 29, 2023

alamb left a comment

alamb Apr 30, 2023

alamb Apr 30, 2023

	/// If the schema exists
	/// If true, do not error if the schema does not exist

Support DROP SCHEMA statements #6096

Support DROP SCHEMA statements #6096

Conversation

jaylmiller commented Apr 22, 2023 • edited Loading

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Choose a reason for hiding this comment

jaylmiller Apr 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaylmiller Apr 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaylmiller commented Apr 25, 2023

alamb commented Apr 27, 2023

jaylmiller commented Apr 27, 2023

jaylmiller commented Apr 29, 2023

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaylmiller commented Apr 22, 2023 •

edited

Loading

jaylmiller Apr 24, 2023 •

edited

Loading

jaylmiller Apr 25, 2023 •

edited

Loading