-
Notifications
You must be signed in to change notification settings - Fork 29k
[SPARK-29511][SQL] DataSourceV2: Support CREATE NAMESPACE #26166
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.
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
Conversation
|
cc: @cloud-fan / @rdblue |
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/CreateNamespaceExec.scala
Outdated
Show resolved
Hide resolved
| testShowNamespaces("SHOW NAMESPACES IN testcat", Seq("ns1")) | ||
| testShowNamespaces("SHOW NAMESPACES IN testcat.ns1", Seq("ns1.ns2")) | ||
|
|
||
| // TODO: Add tests for validating namespace metadata when DESCRIBE NAMESPACE is available. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will create a JIRA for this and follow up.
|
Test build #112284 has finished for PR 26166 at commit
|
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/v2Commands.scala
Outdated
Show resolved
Hide resolved
| ifNotExists: Boolean, | ||
| comment: Option[String], | ||
| locationSpec: Option[String], | ||
| properties: Map[String, String]) extends ParsedStatement |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cloud-fan, what is the behavior of IF NOT EXISTS with DBPROPERTIES? If the namespace exists, are properties updated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as table/database, if table/database doesn't exist, the command is a noop.
|
Test build #112341 has finished for PR 26166 at commit
|
|
Test build #112344 has finished for PR 26166 at commit
|
|
Test build #112345 has finished for PR 26166 at commit
|
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/statements.scala
Outdated
Show resolved
Hide resolved
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/CreateNamespaceExec.scala
Outdated
Show resolved
Hide resolved
|
Test build #112360 has finished for PR 26166 at commit
|
|
retest this please |
|
Test build #112371 has finished for PR 26166 at commit
|
|
retest this please |
|
Test build #112388 has finished for PR 26166 at commit
|
|
Can we retest this? |
|
retest this please |
|
@imback82 anyone can trigger retest by typing "retest this please" |
|
Test build #112456 has finished for PR 26166 at commit
|
| ((COMMENT comment=STRING) | | ||
| locationSpec | | ||
| (WITH DBPROPERTIES tablePropertyList))* #createDatabase | ||
| (WITH DBPROPERTIES tablePropertyList))* #createNamespace |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I missed this. Shall we use PROPERTIES instead of DBPROPERTIES? We can write DBPROPERTIES | PROPERTIES for backward compatibility
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for catching this. updated.
| * The logical plan of the CREATE NAMESPACE command that works for v2 catalogs. | ||
| */ | ||
| case class CreateNamespace( | ||
| catalog: SupportsNamespaces, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: 4 space identation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed.
| test("create namespace") { | ||
| val sql = | ||
| """ | ||
| |CREATE DATABASE IF NOT EXISTS database_name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we use a.b.c instead of database_name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated.
|
LGTM except several minor comments |
|
Test build #112465 has finished for PR 26166 at commit
|
|
Test build #112486 has finished for PR 26166 at commit
|
|
Test build #112487 has finished for PR 26166 at commit
|
|
Test build #112494 has finished for PR 26166 at commit
|
|
thanks, merging to master! |
What changes were proposed in this pull request?
This PR adds
CREATE NAMESPACEsupport for V2 catalogs.Why are the changes needed?
Currently, you cannot explicitly create namespaces for v2 catalogs.
Does this PR introduce any user-facing change?
The user can now perform the following:
to create a namespace
nsinsidemycatalogV2 catalog.How was this patch tested?
Added unit tests.