Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add CREATE DATABASE command to SQL #2094

Merged
merged 9 commits into from
Apr 6, 2022
Merged

Add CREATE DATABASE command to SQL #2094

merged 9 commits into from
Apr 6, 2022

Conversation

matthewmturner
Copy link
Contributor

Which issue does this PR close?

Second task from #1877

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

@github-actions github-actions bot added datafusion Changes in the datafusion crate sql SQL Planner labels Mar 25, 2022
alamb
alamb reviewed Mar 25, 2022
View reviewed changes
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Looking good from here

jychen7
jychen7 reviewed Mar 27, 2022
View reviewed changes
Copy link
Contributor

@jychen7 jychen7 left a comment

Choose a reason for hiding this comment

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

LGTM

@matthewmturner matthewmturner marked this pull request as ready for review March 29, 2022 14:04
@matthewmturner
Copy link
Contributor Author

i think i just need to rebase / resolve conflicts

@matthewmturner
Copy link
Contributor Author

Just need to add a test

@matthewmturner
Copy link
Contributor Author

Im a little confused why getting a ParserError - it seems i will need to look into this more on the sqlparser-rs side.

@matthewmturner
Copy link
Contributor Author

i think im going to need to add some logic for properly adding a schema to a catalog

@matthewmturner
Copy link
Contributor Author

Given there isnt a sql parameter in postgres for specifying the database for a schema (it uses the current active db - https://www.postgresql.org/docs/current/sql-createschema.html) i think we could do one or both of the following:

  • to align with postgres add the notion of active catalog and create schema within that. this will require adding a command to update the active catalog.
  • qualify the schema names. i.e. CREATE SCHEMA abc continues in current / active catalog but then we also allow CREATE SCHEMA my_catalog.abc which would create a schema in the specified catalog.

i personally think the second approach would align well with how catalogs / schemas are set using CREATE TABLE. Then we could always add more postgres like functionality as a second step.

@alamb @jychen7 what do you think?

@alamb
Copy link
Contributor

alamb commented Apr 4, 2022

i personally think the second approach would align well with how catalogs / schemas are set using CREATE TABLE. Then we could always add more postgres like functionality as a second step.

I agree the second approach seems reasonable 👍

@jychen7
Copy link
Contributor

jychen7 commented Apr 5, 2022

qualify the schema names. i.e. CREATE SCHEMA abc continues in current / active catalog but then we also allow CREATE SCHEMA my_catalog.abc which would create a schema in the specified catalog.

yeah, make sense

@matthewmturner
Copy link
Contributor Author

@alamb I believe this is ready

alamb
alamb approved these changes Apr 5, 2022
View reviewed changes
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

LGTM -- thanks @matthewmturner

datafusion/core/src/logical_plan/plan.rs Outdated
@@ -363,6 +374,8 @@ pub enum LogicalPlan {
CreateMemoryTable(CreateMemoryTable),
/// Creates a new catalog schema.
CreateCatalogSchema(CreateCatalogSchema),
/// Creates a new catalog schema.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// Creates a new catalog schema.
/// Creates a new catalog (aka "Database").

datafusion/core/src/logical_plan/plan.rs Outdated
pub schema: DFSchemaRef,
}

/// Creates a catalog.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// Creates a catalog.
/// Creates a catalog (aka "Database")

datafusion/core/src/execution/context.rs
ctx.sql("CREATE DATABASE test").await?.collect().await?;

// Create schema
ctx.sql("CREATE SCHEMA test.abc").await?.collect().await?;
Copy link
Contributor

Choose a reason for hiding this comment

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

this is cool 👍

@yjshen yjshen merged commit 8b09a5c into apache:master Apr 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jychen7 jychen7 jychen7 left review comments

@yjshen yjshen yjshen approved these changes

@alamb alamb alamb approved these changes

Assignees
No one assigned
Labels
datafusion Changes in the datafusion crate sql SQL Planner
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@matthewmturner @alamb @jychen7 @yjshen