-
Notifications
You must be signed in to change notification settings - Fork 29.3k
[SPARK-30018][SQL] Support ALTER DATABASE SET OWNER syntax #26775
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
Changes from 4 commits
3c96091
7188587
56ac955
25cf227
dd1d52b
c1680a4
824cb6a
406c5c2
10e1bc3
d4017b2
42ec005
e5d695e
162fbf0
ae1ebec
bb2f919
c76e5b5
2182992
514b78a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -64,9 +64,27 @@ public interface SupportsNamespaces extends CatalogPlugin { | |
| String PROP_OWNER_TYPE = "ownerType"; | ||
|
|
||
| /** | ||
| * The list of reserved namespace properties. | ||
| * The list of namespace ownership properties, cannot be used in `CREATE` syntax. | ||
| * | ||
| * Only support in: | ||
| * | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How about just saying like this? We need to mention CREATE syntax here? |
||
| * {{ | ||
| * ALTER (DATABASE|SCHEMA|NAMESPACE) SET OWNER ... | ||
| * }} | ||
| */ | ||
| List<String> OWNERSHIPS = Arrays.asList(PROP_OWNER_NAME, PROP_OWNER_TYPE); | ||
|
|
||
| /** | ||
| * The list of immutable namespace properties, which can not be removed or changed directly by | ||
|
yaooqinn marked this conversation as resolved.
Outdated
|
||
| * the syntax: | ||
| * {{ | ||
| * ALTER (DATABASE|SCHEMA|NAMESPACE) SET DBPROPERTIES(...) | ||
|
yaooqinn marked this conversation as resolved.
Outdated
|
||
| * }} | ||
| * | ||
| * They need specific syntax to modify | ||
| */ | ||
| List<String> RESERVED_PROPERTIES = Arrays.asList(PROP_COMMENT, PROP_LOCATION); | ||
| List<String> REVERSED_PROPERTIES = | ||
| Arrays.asList(PROP_COMMENT, PROP_LOCATION, PROP_OWNER_NAME, PROP_OWNER_TYPE); | ||
|
|
||
| /** | ||
| * Return a default namespace for the catalog. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,10 +17,13 @@ | |
|
|
||
| package org.apache.spark.sql.catalyst.analysis | ||
|
|
||
| import scala.collection.JavaConverters._ | ||
|
|
||
| import org.apache.spark.sql.AnalysisException | ||
| import org.apache.spark.sql.catalyst.plans.logical._ | ||
| import org.apache.spark.sql.catalyst.rules.Rule | ||
| import org.apache.spark.sql.connector.catalog.{CatalogManager, CatalogPlugin, LookupCatalog, SupportsNamespaces, TableCatalog, TableChange} | ||
| import org.apache.spark.sql.connector.catalog.{CatalogManager, CatalogPlugin, LookupCatalog, TableCatalog, TableChange} | ||
| import org.apache.spark.sql.connector.catalog.SupportsNamespaces._ | ||
|
|
||
| /** | ||
| * Resolves catalogs from the multi-part identifiers in SQL statements, and convert the statements | ||
|
|
@@ -94,11 +97,21 @@ class ResolveCatalogs(val catalogManager: CatalogManager) | |
| s"because view support in catalog has not been implemented yet") | ||
|
|
||
| case AlterNamespaceSetPropertiesStatement(NonSessionCatalog(catalog, nameParts), properties) => | ||
| if (properties.keySet.intersect(REVERSED_PROPERTIES.asScala.toSet).nonEmpty) { | ||
| throw new AnalysisException(s"Cannot directly modify the reversed properties" + | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: drop
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this change related to this pr to support
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. to prohibit changing ownership ·SET PROPERTIES· |
||
| s" ${REVERSED_PROPERTIES.asScala.mkString("[", ",", "]")}.") | ||
| } | ||
| AlterNamespaceSetProperties(catalog.asNamespaceCatalog, nameParts, properties) | ||
|
|
||
| case AlterNamespaceSetLocationStatement(NonSessionCatalog(catalog, nameParts), location) => | ||
| AlterNamespaceSetProperties(catalog.asNamespaceCatalog, nameParts, | ||
| Map(SupportsNamespaces.PROP_LOCATION -> location)) | ||
| Map(PROP_LOCATION -> location)) | ||
|
|
||
| case AlterNamespaceSetOwner(CatalogAndIdentifierParts(catalog, parts), name, typ) => | ||
| AlterNamespaceSetProperties( | ||
| catalog.asNamespaceCatalog, | ||
| parts, | ||
| Map(PROP_OWNER_NAME -> name, PROP_OWNER_TYPE -> typ)) | ||
|
|
||
| case RenameTableStatement(NonSessionCatalog(catalog, oldName), newNameParts, isView) => | ||
| if (isView) { | ||
|
|
@@ -175,12 +188,11 @@ class ResolveCatalogs(val catalogManager: CatalogManager) | |
| s"Can not specify catalog `${catalog.name}` for view ${viewName.quoted} " + | ||
| s"because view support in catalog has not been implemented yet") | ||
|
|
||
| case c @ CreateNamespaceStatement(NonSessionCatalog(catalog, nameParts), _, _) => | ||
| CreateNamespace( | ||
| catalog.asNamespaceCatalog, | ||
| nameParts, | ||
| c.ifNotExists, | ||
| c.properties) | ||
| case c @ CreateNamespaceStatement(NonSessionCatalog(catalog, nameParts), _, properties) => | ||
| if (properties.keySet.intersect(OWNERSHIPS.asScala.toSet).nonEmpty) { | ||
| throw new AnalysisException("Cannot specify the ownership in CREATE NAMESPACE.") | ||
| } | ||
| CreateNamespace(catalog.asNamespaceCatalog, nameParts, c.ifNotExists, properties) | ||
|
|
||
| case DropNamespaceStatement(NonSessionCatalog(catalog, nameParts), ifExists, cascade) => | ||
| DropNamespace(catalog, nameParts, ifExists, cascade) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -25,6 +25,7 @@ import org.apache.spark.sql.catalyst.catalog.{BucketSpec, CatalogTable, CatalogT | |
| import org.apache.spark.sql.catalyst.plans.logical._ | ||
| import org.apache.spark.sql.catalyst.rules.Rule | ||
| import org.apache.spark.sql.connector.catalog.{CatalogManager, CatalogPlugin, LookupCatalog, SupportsNamespaces, Table, TableCatalog, TableChange, V1Table} | ||
| import org.apache.spark.sql.connector.catalog.SupportsNamespaces._ | ||
| import org.apache.spark.sql.connector.expressions.Transform | ||
| import org.apache.spark.sql.execution.command._ | ||
| import org.apache.spark.sql.execution.datasources.{CreateTable, DataSource, RefreshTable} | ||
|
|
@@ -172,6 +173,10 @@ class ResolveSessionCatalog( | |
| throw new AnalysisException( | ||
| s"The database name is not valid: ${nameParts.quoted}") | ||
| } | ||
| if (properties.keySet.intersect(REVERSED_PROPERTIES.asScala.toSet).nonEmpty) { | ||
| throw new AnalysisException(s"Cannot directly modify the reversed properties" + | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: drop |
||
| s" ${REVERSED_PROPERTIES.asScala.mkString("[", ",", "]")}.") | ||
| } | ||
| AlterDatabasePropertiesCommand(nameParts.head, properties) | ||
|
|
||
| case AlterNamespaceSetLocationStatement(SessionCatalog(_, nameParts), location) => | ||
|
|
@@ -302,10 +307,12 @@ class ResolveSessionCatalog( | |
| throw new AnalysisException( | ||
| s"The database name is not valid: ${nameParts.quoted}") | ||
| } | ||
|
|
||
| val comment = c.properties.get(SupportsNamespaces.PROP_COMMENT) | ||
| val location = c.properties.get(SupportsNamespaces.PROP_LOCATION) | ||
| val newProperties = c.properties -- SupportsNamespaces.RESERVED_PROPERTIES.asScala | ||
| if (c.properties.keySet.intersect(OWNERSHIPS.asScala.toSet).nonEmpty) { | ||
| throw new AnalysisException("Cannot specify the ownership in CREATE DATABASE.") | ||
| } | ||
| val comment = c.properties.get(PROP_COMMENT) | ||
| val location = c.properties.get(PROP_LOCATION) | ||
| val newProperties = c.properties -- REVERSED_PROPERTIES.asScala | ||
| CreateDatabaseCommand(nameParts.head, c.ifNotExists, location, comment, newProperties) | ||
|
|
||
| case d @ DropNamespaceStatement(SessionCatalog(_, nameParts), _, _) => | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -374,11 +374,14 @@ class HiveCatalogedDDLSuite extends DDLSuite with TestHiveSingleton with BeforeA | |
| } | ||
| } | ||
|
|
||
| private def checkOwner(db: String, expected: String): Unit = { | ||
| val owner = sql(s"DESCRIBE DATABASE EXTENDED $db") | ||
| .where("database_description_item='Owner Name'") | ||
| private def checkOwner(db: String, expectedOwnerName: String, expectedOwnerType: String): Unit = { | ||
| val df = sql(s"DESCRIBE DATABASE EXTENDED $db") | ||
| val owner = df.where("database_description_item='Owner Name'") | ||
| .collect().head.getString(1) | ||
| assert(owner === expected) | ||
| val typ = df.where("database_description_item='Owner Type'") | ||
| .collect().head.getString(1) | ||
| assert(owner === expectedOwnerName) | ||
| assert(typ === expectedOwnerType) | ||
| } | ||
|
|
||
| test("Database Ownership") { | ||
|
|
@@ -387,20 +390,29 @@ class HiveCatalogedDDLSuite extends DDLSuite with TestHiveSingleton with BeforeA | |
| val db1 = "spark_29425_1" | ||
| val db2 = "spark_29425_2" | ||
| val owner = "spark_29425" | ||
| val currentUser = Utils.getCurrentUserName() | ||
|
|
||
| sql(s"CREATE DATABASE $db1") | ||
| checkOwner(db1, Utils.getCurrentUserName()) | ||
| checkOwner(db1, currentUser, "USER") | ||
| sql(s"ALTER DATABASE $db1 SET DBPROPERTIES ('a'='a')") | ||
| checkOwner(db1, Utils.getCurrentUserName()) | ||
|
|
||
| // TODO: Specify ownership should be forbidden after we implement `SET OWNER` syntax | ||
| sql(s"CREATE DATABASE $db2 WITH DBPROPERTIES('ownerName'='$owner')") | ||
| checkOwner(db2, owner) | ||
| sql(s"ALTER DATABASE $db2 SET DBPROPERTIES ('a'='a')") | ||
| checkOwner(db2, owner) | ||
| // TODO: Changing ownership should be forbidden after we implement `SET OWNER` syntax | ||
| sql(s"ALTER DATABASE $db2 SET DBPROPERTIES ('ownerName'='a')") | ||
| checkOwner(db2, "a") | ||
| checkOwner(db1, currentUser, "USER") | ||
| val e = intercept[AnalysisException](sql(s"ALTER DATABASE $db1 SET DBPROPERTIES ('a'='a'," | ||
| + s"'ownerName'='$owner','ownerType'='XXX')")) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. does case sensitivity matter for reserved properties? what if users specify
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ok maybe it's fine to treat
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It does not matter but I guess we should mention this
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| assert(e.getMessage.contains("ownerName")) | ||
| sql(s"ALTER DATABASE $db1 SET OWNER ROLE $owner") | ||
| checkOwner(db1, owner, "ROLE") | ||
|
|
||
| val e2 = intercept[AnalysisException]( | ||
| sql(s"CREATE DATABASE $db2 WITH DBPROPERTIES('ownerName'='$owner', 'ownerType'='XXX')")) | ||
| assert(e2.getMessage.contains("ownership")) | ||
| sql(s"CREATE DATABASE $db2 WITH DBPROPERTIES('comment'='$owner')") | ||
| checkOwner(db2, currentUser, "USER") | ||
| sql(s"ALTER DATABASE $db2 SET OWNER GROUP $owner") | ||
| checkOwner(db2, owner, "GROUP") | ||
| sql(s"ALTER DATABASE $db2 SET OWNER GROUP `$owner`") | ||
| checkOwner(db2, owner, "GROUP") | ||
| sql(s"ALTER DATABASE $db2 SET OWNER GROUP OWNER") | ||
| checkOwner(db2, "OWNER", "GROUP") | ||
| } finally { | ||
| catalog.reset() | ||
| } | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.