From 3c960916f9f92f1bfe6ac42f74f5c0a419407601 Mon Sep 17 00:00:00 2001 From: Kent Yao Date: Fri, 6 Dec 2019 14:07:48 +0800 Subject: [PATCH 01/15] [SPARK-30018][SQL] Support ALTER DATABASE SET OWNER syntax --- .../spark/sql/catalyst/parser/SqlBase.g4 | 4 ++ .../connector/catalog/SupportsNamespaces.java | 3 +- .../catalyst/analysis/ResolveCatalogs.scala | 16 ++++++-- .../sql/catalyst/parser/AstBuilder.scala | 17 +++++++++ .../catalyst/plans/logical/statements.scala | 8 ++++ .../sql/catalyst/parser/DDLParserSuite.scala | 13 +++++++ .../analysis/ResolveSessionCatalog.scala | 7 ++++ .../spark/sql/execution/command/ddl.scala | 28 ++++++++++++-- .../sql/hive/execution/HiveDDLSuite.scala | 38 +++++++++++-------- 9 files changed, 111 insertions(+), 23 deletions(-) diff --git a/sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4 b/sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4 index abaaecf69f619..6968ae8ae67ff 100644 --- a/sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4 +++ b/sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4 @@ -97,6 +97,8 @@ statement SET (DBPROPERTIES | PROPERTIES) tablePropertyList #setNamespaceProperties | ALTER (database | NAMESPACE) multipartIdentifier SET locationSpec #setNamespaceLocation + | ALTER (database | NAMESPACE) multipartIdentifier + SET OWNER ownerType=(USER | ROLE | GROUP) IDENTIFIER #setNamespaceOwner | DROP (database | NAMESPACE) (IF EXISTS)? multipartIdentifier (RESTRICT | CASCADE)? #dropNamespace | SHOW (DATABASES | NAMESPACES) ((FROM | IN) multipartIdentifier)? @@ -1355,6 +1357,7 @@ nonReserved | OVERLAPS | OVERLAY | OVERWRITE + | OWNER | PARTITION | PARTITIONED | PARTITIONS @@ -1623,6 +1626,7 @@ OVER: 'OVER'; OVERLAPS: 'OVERLAPS'; OVERLAY: 'OVERLAY'; OVERWRITE: 'OVERWRITE'; +OWNER: 'OWNER'; PARTITION: 'PARTITION'; PARTITIONED: 'PARTITIONED'; PARTITIONS: 'PARTITIONS'; diff --git a/sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/SupportsNamespaces.java b/sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/SupportsNamespaces.java index 2e60487287f2d..4bc3553c1e140 100644 --- a/sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/SupportsNamespaces.java +++ b/sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/SupportsNamespaces.java @@ -66,7 +66,8 @@ public interface SupportsNamespaces extends CatalogPlugin { /** * The list of reserved namespace properties. */ - List RESERVED_PROPERTIES = Arrays.asList(PROP_COMMENT, PROP_LOCATION); + List RESERVED_PROPERTIES = + Arrays.asList(PROP_COMMENT, PROP_LOCATION, PROP_OWNER_NAME, PROP_OWNER_TYPE); /** * Return a default namespace for the catalog. diff --git a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveCatalogs.scala b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveCatalogs.scala index 896b2830d524e..36a3b6166030c 100644 --- a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveCatalogs.scala +++ b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveCatalogs.scala @@ -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,18 @@ class ResolveCatalogs(val catalogManager: CatalogManager) s"because view support in catalog has not been implemented yet") case AlterNamespaceSetPropertiesStatement(NonSessionCatalog(catalog, nameParts), properties) => - AlterNamespaceSetProperties(catalog.asNamespaceCatalog, nameParts, properties) + val availableProps = properties -- RESERVED_PROPERTIES.asScala + AlterNamespaceSetProperties(catalog.asNamespaceCatalog, nameParts, availableProps) case AlterNamespaceSetLocationStatement(NonSessionCatalog(catalog, nameParts), location) => AlterNamespaceSetProperties(catalog.asNamespaceCatalog, nameParts, - Map(SupportsNamespaces.PROP_LOCATION -> location)) + Map(PROP_LOCATION -> location)) + + case AlterNamespaceSetOwnerStatement(NonSessionCatalog(catalog, nameParts), name, typ) => + AlterNamespaceSetProperties( + catalog.asNamespaceCatalog, + nameParts, + Map(PROP_OWNER_NAME -> name, PROP_OWNER_TYPE -> typ)) case RenameTableStatement(NonSessionCatalog(catalog, oldName), newNameParts, isView) => if (isView) { diff --git a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala index 047cc22fe5641..3e9bb94147e5e 100644 --- a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala +++ b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala @@ -2579,6 +2579,23 @@ class AstBuilder(conf: SQLConf) extends SqlBaseBaseVisitor[AnyRef] with Logging } } + /** + * Create an [[AlterNamespaceSetOwnerStatement]] logical plan. + * + * For example: + * {{{ + * ALTER (DATABASE|SCHEMA|NAMESPACE) namespace SET OWNER (USER|ROLE|GROUP) identityName; + * }}} + */ + override def visitSetNamespaceOwner(ctx: SetNamespaceOwnerContext): LogicalPlan = { + withOrigin(ctx) { + AlterNamespaceSetOwnerStatement( + visitMultipartIdentifier(ctx.multipartIdentifier), + ctx.IDENTIFIER.getText, + ctx.ownerType.getText) + } + } + /** * Create a [[ShowNamespacesStatement]] command. */ diff --git a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/statements.scala b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/statements.scala index 5db099e1de631..8183093fe37ef 100644 --- a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/statements.scala +++ b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/statements.scala @@ -382,6 +382,14 @@ case class AlterNamespaceSetLocationStatement( namespace: Seq[String], location: String) extends ParsedStatement +/** + * ALTER (DATABASE|SCHEMA|NAMESPACE) ... SET OWNER command, as parsed from SQL. + */ +case class AlterNamespaceSetOwnerStatement( + namespace: Seq[String], + ownerName: String, + ownerType: String) extends ParsedStatement + /** * A SHOW NAMESPACES statement, as parsed from SQL. */ diff --git a/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/parser/DDLParserSuite.scala b/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/parser/DDLParserSuite.scala index aa2cde2e201b4..dc87064bed399 100644 --- a/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/parser/DDLParserSuite.scala +++ b/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/parser/DDLParserSuite.scala @@ -1215,6 +1215,19 @@ class DDLParserSuite extends AnalysisTest { AlterNamespaceSetLocationStatement(Seq("a", "b", "c"), "/home/user/db")) } + test("set namespace owner") { + comparePlans( + parsePlan("ALTER DATABASE a.b.c SET OWNER USER user1"), + AlterNamespaceSetOwnerStatement(Seq("a", "b", "c"), "user1", "USER")) + + comparePlans( + parsePlan("ALTER DATABASE a.b.c SET OWNER ROLE role1"), + AlterNamespaceSetOwnerStatement(Seq("a", "b", "c"), "role1", "ROLE")) + comparePlans( + parsePlan("ALTER DATABASE a.b.c SET OWNER GROUP group1"), + AlterNamespaceSetOwnerStatement(Seq("a", "b", "c"), "group1", "GROUP")) + } + test("show databases: basic") { comparePlans( parsePlan("SHOW DATABASES"), diff --git a/sql/core/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveSessionCatalog.scala b/sql/core/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveSessionCatalog.scala index 4cc701274a1f0..48ca4a48e0984 100644 --- a/sql/core/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveSessionCatalog.scala +++ b/sql/core/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveSessionCatalog.scala @@ -181,6 +181,13 @@ class ResolveSessionCatalog( } AlterDatabaseSetLocationCommand(nameParts.head, location) + case AlterNamespaceSetOwnerStatement(SessionCatalog(_, nameParts), ownerName, ownerType) => + if (nameParts.length != 1) { + throw new AnalysisException( + s"The database name is not valid: ${nameParts.quoted}") + } + AlterDatabaseSetOwnerCommand(nameParts.head, ownerName, ownerType) + case RenameTableStatement(SessionCatalog(_, oldName), newNameParts, isView) => AlterTableRenameCommand(oldName.asTableIdentifier, newNameParts.asTableIdentifier, isView) diff --git a/sql/core/src/main/scala/org/apache/spark/sql/execution/command/ddl.scala b/sql/core/src/main/scala/org/apache/spark/sql/execution/command/ddl.scala index bdba10eb488d7..5c0d589596ca0 100644 --- a/sql/core/src/main/scala/org/apache/spark/sql/execution/command/ddl.scala +++ b/sql/core/src/main/scala/org/apache/spark/sql/execution/command/ddl.scala @@ -72,12 +72,13 @@ case class CreateDatabaseCommand( override def run(sparkSession: SparkSession): Seq[Row] = { val catalog = sparkSession.sessionState.catalog + val availablePros = props -- RESERVED_PROPERTIES.asScala catalog.createDatabase( CatalogDatabase( databaseName, comment.getOrElse(""), path.map(CatalogUtils.stringToURI).getOrElse(catalog.getDefaultDBPath(databaseName)), - props), + availablePros), ifNotExists) Seq.empty[Row] } @@ -129,7 +130,8 @@ case class AlterDatabasePropertiesCommand( override def run(sparkSession: SparkSession): Seq[Row] = { val catalog = sparkSession.sessionState.catalog val db: CatalogDatabase = catalog.getDatabaseMetadata(databaseName) - catalog.alterDatabase(db.copy(properties = db.properties ++ props)) + val availableProps = props -- RESERVED_PROPERTIES.asScala + catalog.alterDatabase(db.copy(properties = db.properties ++ availableProps)) Seq.empty[Row] } @@ -156,6 +158,26 @@ case class AlterDatabaseSetLocationCommand(databaseName: String, location: Strin } } +/** + * A command for users to set ownership for a database + * If the database does not exist, an error message will be issued to indicate the database + * does not exist. + * The syntax of using this command in SQL is: + * {{{ + * ALTER (DATABASE|SCHEMA) database_name SET OWNER [USER|ROLE|GROUP] identityName + * }}} + */ +case class AlterDatabaseSetOwnerCommand(databaseName: String, ownerName: String, ownerType: String) + extends RunnableCommand { + override def run(sparkSession: SparkSession): Seq[Row] = { + val catalog = sparkSession.sessionState.catalog + val database = catalog.getDatabaseMetadata(databaseName) + val ownerships = Map(PROP_OWNER_NAME -> ownerName, PROP_OWNER_TYPE -> ownerType) + catalog.alterDatabase(database.copy(properties = database.properties ++ ownerships)) + Seq.empty[Row] + } +} + /** * A command for users to show the name of the database, its comment (if one has been set), and its * root location on the filesystem. When extended is true, it also shows the database's properties @@ -183,7 +205,7 @@ case class DescribeDatabaseCommand( Row("Owner Type", allDbProperties.getOrElse(PROP_OWNER_TYPE, "")) :: Nil if (extended) { - val properties = allDbProperties -- Seq(PROP_OWNER_NAME, PROP_OWNER_TYPE) + val properties = allDbProperties -- RESERVED_PROPERTIES.asScala val propertiesStr = if (properties.isEmpty) { "" diff --git a/sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveDDLSuite.scala b/sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveDDLSuite.scala index 0684d66558c61..fedd2ff2171fe 100644 --- a/sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveDDLSuite.scala +++ b/sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveDDLSuite.scala @@ -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,23 @@ 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()) - 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") + sql(s"ALTER DATABASE $db1 SET DBPROPERTIES ('a'='a', 'ownerName'='$owner'," + + s" 'ownerType'='XXX')") + checkOwner(db1, currentUser, "USER") + sql(s"ALTER DATABASE $db1 SET OWNER ROLE $owner") + checkOwner(db1, owner, "ROLE") + + sql(s"CREATE DATABASE $db2 WITH DBPROPERTIES('ownerName'='$owner', 'ownerType'='XXX')") + checkOwner(db2, currentUser, "USER") + sql(s"ALTER DATABASE $db2 SET DBPROPERTIES ('a'='a', 'ownerName'='$owner'," + + s" 'ownerType'='XXX')") + checkOwner(db2, currentUser, "USER") + sql(s"ALTER DATABASE $db2 SET OWNER GROUP $owner") + checkOwner(db2, owner, "GROUP") } finally { catalog.reset() } From 7188587f19f1e7d27dee70e9d1bb9f03dbef2023 Mon Sep 17 00:00:00 2001 From: Kent Yao Date: Fri, 6 Dec 2019 15:45:52 +0800 Subject: [PATCH 02/15] nit --- .../scala/org/apache/spark/sql/execution/command/ddl.scala | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/sql/core/src/main/scala/org/apache/spark/sql/execution/command/ddl.scala b/sql/core/src/main/scala/org/apache/spark/sql/execution/command/ddl.scala index 5c0d589596ca0..56a936ca9a748 100644 --- a/sql/core/src/main/scala/org/apache/spark/sql/execution/command/ddl.scala +++ b/sql/core/src/main/scala/org/apache/spark/sql/execution/command/ddl.scala @@ -72,13 +72,12 @@ case class CreateDatabaseCommand( override def run(sparkSession: SparkSession): Seq[Row] = { val catalog = sparkSession.sessionState.catalog - val availablePros = props -- RESERVED_PROPERTIES.asScala catalog.createDatabase( CatalogDatabase( databaseName, comment.getOrElse(""), path.map(CatalogUtils.stringToURI).getOrElse(catalog.getDefaultDBPath(databaseName)), - availablePros), + props), ifNotExists) Seq.empty[Row] } From 56ac955e0845809c9d7c2958344fe0149878e14e Mon Sep 17 00:00:00 2001 From: Kent Yao Date: Fri, 6 Dec 2019 23:45:20 +0800 Subject: [PATCH 03/15] refine --- docs/sql-keywords.md | 1 + .../spark/sql/catalyst/parser/SqlBase.g4 | 2 +- .../connector/catalog/SupportsNamespaces.java | 22 ++++++++++++++-- .../catalyst/analysis/ResolveCatalogs.scala | 22 ++++++++-------- .../sql/catalyst/parser/AstBuilder.scala | 6 ++--- .../catalyst/plans/logical/statements.scala | 2 +- .../sql/catalyst/parser/DDLParserSuite.scala | 6 ++--- .../analysis/ResolveSessionCatalog.scala | 22 ++++++++-------- .../spark/sql/execution/command/ddl.scala | 25 ++----------------- .../datasources/v2/DataSourceV2Strategy.scala | 3 ++- .../v2/DescribeNamespaceExec.scala | 12 +++++---- .../datasources/v2/V2SessionCatalog.scala | 16 +++--------- .../v2/V2SessionCatalogSuite.scala | 2 +- .../sql/hive/execution/HiveDDLSuite.scala | 18 ++++++++----- 14 files changed, 80 insertions(+), 79 deletions(-) diff --git a/docs/sql-keywords.md b/docs/sql-keywords.md index 3117ee40a8c9b..ae29a9ac3018d 100644 --- a/docs/sql-keywords.md +++ b/docs/sql-keywords.md @@ -203,6 +203,7 @@ Below is a list of all the keywords in Spark SQL. OVERLAPSreservednon-reservedreserved OVERLAYnon-reservednon-reservednon-reserved OVERWRITEnon-reservednon-reservednon-reserved + OWNERnon-reservednon-reservednon-reserved PARTITIONnon-reservednon-reservedreserved PARTITIONEDnon-reservednon-reservednon-reserved PARTITIONSnon-reservednon-reservednon-reserved diff --git a/sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4 b/sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4 index 6968ae8ae67ff..6f21dec7fb222 100644 --- a/sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4 +++ b/sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4 @@ -98,7 +98,7 @@ statement | ALTER (database | NAMESPACE) multipartIdentifier SET locationSpec #setNamespaceLocation | ALTER (database | NAMESPACE) multipartIdentifier - SET OWNER ownerType=(USER | ROLE | GROUP) IDENTIFIER #setNamespaceOwner + SET OWNER ownerType=(USER | ROLE | GROUP) identifier #setNamespaceOwner | DROP (database | NAMESPACE) (IF EXISTS)? multipartIdentifier (RESTRICT | CASCADE)? #dropNamespace | SHOW (DATABASES | NAMESPACES) ((FROM | IN) multipartIdentifier)? diff --git a/sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/SupportsNamespaces.java b/sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/SupportsNamespaces.java index 4bc3553c1e140..c158987a29feb 100644 --- a/sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/SupportsNamespaces.java +++ b/sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/SupportsNamespaces.java @@ -18,6 +18,7 @@ package org.apache.spark.sql.connector.catalog; import org.apache.spark.annotation.Experimental; +import org.apache.spark.sql.AnalysisException; import org.apache.spark.sql.catalyst.analysis.NamespaceAlreadyExistsException; import org.apache.spark.sql.catalyst.analysis.NoSuchNamespaceException; @@ -64,9 +65,26 @@ 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: + * + * {{ + * ALTER (DATABASE|SCHEMA|NAMESPACE) SET OWNER ... + * }} + */ + List OWNERSHIPS = Arrays.asList(PROP_OWNER_NAME, PROP_OWNER_TYPE); + + /** + * The list of immutable namespace properties, which can not be removed or changed directly by + * the syntax: + * {{ + * ALTER (DATABASE|SCHEMA|NAMESPACE) SET DBPROPERTIES(...) + * }} + * + * They need specific syntax to modify */ - List RESERVED_PROPERTIES = + List REVERSED_PROPERTIES = Arrays.asList(PROP_COMMENT, PROP_LOCATION, PROP_OWNER_NAME, PROP_OWNER_TYPE); /** diff --git a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveCatalogs.scala b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveCatalogs.scala index 36a3b6166030c..5dd2c3c20ab7c 100644 --- a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveCatalogs.scala +++ b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveCatalogs.scala @@ -97,17 +97,20 @@ class ResolveCatalogs(val catalogManager: CatalogManager) s"because view support in catalog has not been implemented yet") case AlterNamespaceSetPropertiesStatement(NonSessionCatalog(catalog, nameParts), properties) => - val availableProps = properties -- RESERVED_PROPERTIES.asScala - AlterNamespaceSetProperties(catalog.asNamespaceCatalog, nameParts, availableProps) + if (properties.keySet.intersect(REVERSED_PROPERTIES.asScala.toSet).nonEmpty) { + throw new AnalysisException(s"Cannot directly modify the reversed properties" + + s" ${REVERSED_PROPERTIES.asScala.mkString("[", ",", "]")}.") + } + AlterNamespaceSetProperties(catalog.asNamespaceCatalog, nameParts, properties) case AlterNamespaceSetLocationStatement(NonSessionCatalog(catalog, nameParts), location) => AlterNamespaceSetProperties(catalog.asNamespaceCatalog, nameParts, Map(PROP_LOCATION -> location)) - case AlterNamespaceSetOwnerStatement(NonSessionCatalog(catalog, nameParts), name, typ) => + case AlterNamespaceSetOwner(CatalogAndIdentifierParts(catalog, parts), name, typ) => AlterNamespaceSetProperties( catalog.asNamespaceCatalog, - nameParts, + parts, Map(PROP_OWNER_NAME -> name, PROP_OWNER_TYPE -> typ)) case RenameTableStatement(NonSessionCatalog(catalog, oldName), newNameParts, isView) => @@ -185,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) diff --git a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala index 3e9bb94147e5e..9e29f91073128 100644 --- a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala +++ b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala @@ -2580,7 +2580,7 @@ class AstBuilder(conf: SQLConf) extends SqlBaseBaseVisitor[AnyRef] with Logging } /** - * Create an [[AlterNamespaceSetOwnerStatement]] logical plan. + * Create an [[AlterNamespaceSetOwner]] logical plan. * * For example: * {{{ @@ -2589,9 +2589,9 @@ class AstBuilder(conf: SQLConf) extends SqlBaseBaseVisitor[AnyRef] with Logging */ override def visitSetNamespaceOwner(ctx: SetNamespaceOwnerContext): LogicalPlan = { withOrigin(ctx) { - AlterNamespaceSetOwnerStatement( + AlterNamespaceSetOwner( visitMultipartIdentifier(ctx.multipartIdentifier), - ctx.IDENTIFIER.getText, + ctx.identifier.getText, ctx.ownerType.getText) } } diff --git a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/statements.scala b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/statements.scala index 8183093fe37ef..950378f565997 100644 --- a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/statements.scala +++ b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/statements.scala @@ -385,7 +385,7 @@ case class AlterNamespaceSetLocationStatement( /** * ALTER (DATABASE|SCHEMA|NAMESPACE) ... SET OWNER command, as parsed from SQL. */ -case class AlterNamespaceSetOwnerStatement( +case class AlterNamespaceSetOwner( namespace: Seq[String], ownerName: String, ownerType: String) extends ParsedStatement diff --git a/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/parser/DDLParserSuite.scala b/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/parser/DDLParserSuite.scala index dc87064bed399..24aa0d0de8511 100644 --- a/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/parser/DDLParserSuite.scala +++ b/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/parser/DDLParserSuite.scala @@ -1218,14 +1218,14 @@ class DDLParserSuite extends AnalysisTest { test("set namespace owner") { comparePlans( parsePlan("ALTER DATABASE a.b.c SET OWNER USER user1"), - AlterNamespaceSetOwnerStatement(Seq("a", "b", "c"), "user1", "USER")) + AlterNamespaceSetOwner(Seq("a", "b", "c"), "user1", "USER")) comparePlans( parsePlan("ALTER DATABASE a.b.c SET OWNER ROLE role1"), - AlterNamespaceSetOwnerStatement(Seq("a", "b", "c"), "role1", "ROLE")) + AlterNamespaceSetOwner(Seq("a", "b", "c"), "role1", "ROLE")) comparePlans( parsePlan("ALTER DATABASE a.b.c SET OWNER GROUP group1"), - AlterNamespaceSetOwnerStatement(Seq("a", "b", "c"), "group1", "GROUP")) + AlterNamespaceSetOwner(Seq("a", "b", "c"), "group1", "GROUP")) } test("show databases: basic") { diff --git a/sql/core/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveSessionCatalog.scala b/sql/core/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveSessionCatalog.scala index 48ca4a48e0984..9bc2268e8e5a9 100644 --- a/sql/core/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveSessionCatalog.scala +++ b/sql/core/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveSessionCatalog.scala @@ -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" + + s" ${REVERSED_PROPERTIES.asScala.mkString("[", ",", "]")}.") + } AlterDatabasePropertiesCommand(nameParts.head, properties) case AlterNamespaceSetLocationStatement(SessionCatalog(_, nameParts), location) => @@ -181,13 +186,6 @@ class ResolveSessionCatalog( } AlterDatabaseSetLocationCommand(nameParts.head, location) - case AlterNamespaceSetOwnerStatement(SessionCatalog(_, nameParts), ownerName, ownerType) => - if (nameParts.length != 1) { - throw new AnalysisException( - s"The database name is not valid: ${nameParts.quoted}") - } - AlterDatabaseSetOwnerCommand(nameParts.head, ownerName, ownerType) - case RenameTableStatement(SessionCatalog(_, oldName), newNameParts, isView) => AlterTableRenameCommand(oldName.asTableIdentifier, newNameParts.asTableIdentifier, isView) @@ -309,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), _, _) => diff --git a/sql/core/src/main/scala/org/apache/spark/sql/execution/command/ddl.scala b/sql/core/src/main/scala/org/apache/spark/sql/execution/command/ddl.scala index 56a936ca9a748..bf278add5c399 100644 --- a/sql/core/src/main/scala/org/apache/spark/sql/execution/command/ddl.scala +++ b/sql/core/src/main/scala/org/apache/spark/sql/execution/command/ddl.scala @@ -129,8 +129,7 @@ case class AlterDatabasePropertiesCommand( override def run(sparkSession: SparkSession): Seq[Row] = { val catalog = sparkSession.sessionState.catalog val db: CatalogDatabase = catalog.getDatabaseMetadata(databaseName) - val availableProps = props -- RESERVED_PROPERTIES.asScala - catalog.alterDatabase(db.copy(properties = db.properties ++ availableProps)) + catalog.alterDatabase(db.copy(properties = db.properties ++ props)) Seq.empty[Row] } @@ -157,26 +156,6 @@ case class AlterDatabaseSetLocationCommand(databaseName: String, location: Strin } } -/** - * A command for users to set ownership for a database - * If the database does not exist, an error message will be issued to indicate the database - * does not exist. - * The syntax of using this command in SQL is: - * {{{ - * ALTER (DATABASE|SCHEMA) database_name SET OWNER [USER|ROLE|GROUP] identityName - * }}} - */ -case class AlterDatabaseSetOwnerCommand(databaseName: String, ownerName: String, ownerType: String) - extends RunnableCommand { - override def run(sparkSession: SparkSession): Seq[Row] = { - val catalog = sparkSession.sessionState.catalog - val database = catalog.getDatabaseMetadata(databaseName) - val ownerships = Map(PROP_OWNER_NAME -> ownerName, PROP_OWNER_TYPE -> ownerType) - catalog.alterDatabase(database.copy(properties = database.properties ++ ownerships)) - Seq.empty[Row] - } -} - /** * A command for users to show the name of the database, its comment (if one has been set), and its * root location on the filesystem. When extended is true, it also shows the database's properties @@ -204,7 +183,7 @@ case class DescribeDatabaseCommand( Row("Owner Type", allDbProperties.getOrElse(PROP_OWNER_TYPE, "")) :: Nil if (extended) { - val properties = allDbProperties -- RESERVED_PROPERTIES.asScala + val properties = allDbProperties -- REVERSED_PROPERTIES.asScala val propertiesStr = if (properties.isEmpty) { "" diff --git a/sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/DataSourceV2Strategy.scala b/sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/DataSourceV2Strategy.scala index a0d10f1d09e63..861921a6c079c 100644 --- a/sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/DataSourceV2Strategy.scala +++ b/sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/DataSourceV2Strategy.scala @@ -22,8 +22,9 @@ import scala.collection.JavaConverters._ import org.apache.spark.sql.{AnalysisException, Strategy} import org.apache.spark.sql.catalyst.expressions.{And, PredicateHelper, SubqueryExpression} import org.apache.spark.sql.catalyst.planning.PhysicalOperation -import org.apache.spark.sql.catalyst.plans.logical.{AlterNamespaceSetProperties, AlterTable, AppendData, CreateNamespace, CreateTableAsSelect, CreateV2Table, DeleteFromTable, DescribeNamespace, DescribeTable, DropNamespace, DropTable, LogicalPlan, OverwriteByExpression, OverwritePartitionsDynamic, RefreshTable, RenameTable, Repartition, ReplaceTable, ReplaceTableAsSelect, SetCatalogAndNamespace, ShowCurrentNamespace, ShowNamespaces, ShowTableProperties, ShowTables} +import org.apache.spark.sql.catalyst.plans.logical.{AlterNamespaceSetOwner, AlterNamespaceSetProperties, AlterTable, AppendData, CreateNamespace, CreateTableAsSelect, CreateV2Table, DeleteFromTable, DescribeNamespace, DescribeTable, DropNamespace, DropTable, LogicalPlan, OverwriteByExpression, OverwritePartitionsDynamic, RefreshTable, RenameTable, Repartition, ReplaceTable, ReplaceTableAsSelect, SetCatalogAndNamespace, ShowCurrentNamespace, ShowNamespaces, ShowTableProperties, ShowTables} import org.apache.spark.sql.connector.catalog.{StagingTableCatalog, TableCapability} +import org.apache.spark.sql.connector.catalog.SupportsNamespaces.{PROP_OWNER_NAME, PROP_OWNER_TYPE} import org.apache.spark.sql.connector.read.streaming.{ContinuousStream, MicroBatchStream} import org.apache.spark.sql.execution.{FilterExec, ProjectExec, SparkPlan} import org.apache.spark.sql.execution.datasources.DataSourceStrategy diff --git a/sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/DescribeNamespaceExec.scala b/sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/DescribeNamespaceExec.scala index 7ad872f10e847..e56bc2752d452 100644 --- a/sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/DescribeNamespaceExec.scala +++ b/sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/DescribeNamespaceExec.scala @@ -35,6 +35,7 @@ case class DescribeNamespaceExec( namespace: Seq[String], isExtended: Boolean) extends V2CommandExec { private val encoder = RowEncoder(StructType.fromAttributes(output)).resolveAndBind() + import SupportsNamespaces._ override protected def run(): Seq[InternalRow] = { val rows = new ArrayBuffer[InternalRow]() @@ -42,12 +43,13 @@ case class DescribeNamespaceExec( val metadata = catalog.loadNamespaceMetadata(ns) rows += toCatalystRow("Namespace Name", ns.last) - rows += toCatalystRow("Description", metadata.get(SupportsNamespaces.PROP_COMMENT)) - rows += toCatalystRow("Location", metadata.get(SupportsNamespaces.PROP_LOCATION)) + rows += toCatalystRow("Description", metadata.get(PROP_COMMENT)) + rows += toCatalystRow("Location", metadata.get(PROP_LOCATION)) + rows += toCatalystRow("Owner Name", metadata.get(PROP_OWNER_NAME)) + rows += toCatalystRow("Owner Type", metadata.get(PROP_OWNER_TYPE)) + if (isExtended) { - val properties = - metadata.asScala.toSeq.filter(p => - !SupportsNamespaces.RESERVED_PROPERTIES.contains(p._1)) + val properties = metadata.asScala -- REVERSED_PROPERTIES.asScala if (properties.nonEmpty) { rows += toCatalystRow("Properties", properties.mkString("(", ",", ")")) } diff --git a/sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/V2SessionCatalog.scala b/sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/V2SessionCatalog.scala index 8d9957fe898d6..b14b4b46876d5 100644 --- a/sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/V2SessionCatalog.scala +++ b/sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/V2SessionCatalog.scala @@ -225,15 +225,6 @@ class V2SessionCatalog(catalog: SessionCatalog, conf: SQLConf) override def alterNamespace(namespace: Array[String], changes: NamespaceChange*): Unit = { namespace match { case Array(db) => - // validate that this catalog's reserved properties are not removed - changes.foreach { - case remove: RemoveProperty - if SupportsNamespaces.RESERVED_PROPERTIES.contains(remove.property) => - throw new UnsupportedOperationException( - s"Cannot remove reserved property: ${remove.property}") - case _ => - } - val metadata = catalog.getDatabaseMetadata(db).toMetadata catalog.alterDatabase( toCatalogDatabase(db, CatalogV2Util.applyNamespaceChanges(metadata, changes))) @@ -263,6 +254,7 @@ class V2SessionCatalog(catalog: SessionCatalog, conf: SQLConf) } private[sql] object V2SessionCatalog { + import SupportsNamespaces._ /** * Convert v2 Transforms to v1 partition columns and an optional bucket spec. @@ -292,12 +284,12 @@ private[sql] object V2SessionCatalog { defaultLocation: Option[URI] = None): CatalogDatabase = { CatalogDatabase( name = db, - description = metadata.getOrDefault(SupportsNamespaces.PROP_COMMENT, ""), - locationUri = Option(metadata.get(SupportsNamespaces.PROP_LOCATION)) + description = metadata.getOrDefault(PROP_COMMENT, ""), + locationUri = Option(metadata.get(PROP_LOCATION)) .map(CatalogUtils.stringToURI) .orElse(defaultLocation) .getOrElse(throw new IllegalArgumentException("Missing database location")), - properties = metadata.asScala.toMap -- SupportsNamespaces.RESERVED_PROPERTIES.asScala) + properties = metadata.asScala.toMap -- Seq(PROP_COMMENT, PROP_LOCATION)) } private implicit class CatalogDatabaseHelper(catalogDatabase: CatalogDatabase) { diff --git a/sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/v2/V2SessionCatalogSuite.scala b/sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/v2/V2SessionCatalogSuite.scala index a02998856f789..24e2929f21c03 100644 --- a/sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/v2/V2SessionCatalogSuite.scala +++ b/sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/v2/V2SessionCatalogSuite.scala @@ -757,7 +757,7 @@ class V2SessionCatalogNamespaceSuite extends V2SessionCatalogBaseSuite { actual: scala.collection.Map[String, String]): Unit = { // remove location and comment that are automatically added by HMS unless they are expected val toRemove = - SupportsNamespaces.RESERVED_PROPERTIES.asScala.filter(expected.contains) + SupportsNamespaces.REVERSED_PROPERTIES.asScala.filter(expected.contains) assert(expected -- toRemove === actual) } diff --git a/sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveDDLSuite.scala b/sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveDDLSuite.scala index fedd2ff2171fe..9efadb4c85f47 100644 --- a/sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveDDLSuite.scala +++ b/sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveDDLSuite.scala @@ -394,19 +394,25 @@ class HiveCatalogedDDLSuite extends DDLSuite with TestHiveSingleton with BeforeA sql(s"CREATE DATABASE $db1") checkOwner(db1, currentUser, "USER") - sql(s"ALTER DATABASE $db1 SET DBPROPERTIES ('a'='a', 'ownerName'='$owner'," + - s" 'ownerType'='XXX')") + sql(s"ALTER DATABASE $db1 SET DBPROPERTIES ('a'='a')") checkOwner(db1, currentUser, "USER") + val e = intercept[AnalysisException](sql(s"ALTER DATABASE $db1 SET DBPROPERTIES ('a'='a'," + + s"'ownerName'='$owner','ownerType'='XXX')")) + assert(e.getMessage.contains("ownerName")) sql(s"ALTER DATABASE $db1 SET OWNER ROLE $owner") checkOwner(db1, owner, "ROLE") - sql(s"CREATE DATABASE $db2 WITH DBPROPERTIES('ownerName'='$owner', 'ownerType'='XXX')") - checkOwner(db2, currentUser, "USER") - sql(s"ALTER DATABASE $db2 SET DBPROPERTIES ('a'='a', 'ownerName'='$owner'," + - s" 'ownerType'='XXX')") + 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() } From 25cf2273ae57fed78ab2a30db15c4612b3d1ac99 Mon Sep 17 00:00:00 2001 From: Kent Yao Date: Sat, 7 Dec 2019 00:33:20 +0800 Subject: [PATCH 04/15] style --- .../apache/spark/sql/connector/catalog/SupportsNamespaces.java | 1 - 1 file changed, 1 deletion(-) diff --git a/sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/SupportsNamespaces.java b/sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/SupportsNamespaces.java index c158987a29feb..56cd43c54f016 100644 --- a/sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/SupportsNamespaces.java +++ b/sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/SupportsNamespaces.java @@ -18,7 +18,6 @@ package org.apache.spark.sql.connector.catalog; import org.apache.spark.annotation.Experimental; -import org.apache.spark.sql.AnalysisException; import org.apache.spark.sql.catalyst.analysis.NamespaceAlreadyExistsException; import org.apache.spark.sql.catalyst.analysis.NoSuchNamespaceException; From dd1d52bf3714fcfd37965b4fc54d1418b9e8be71 Mon Sep 17 00:00:00 2001 From: Kent Yao Date: Mon, 9 Dec 2019 13:28:15 +0800 Subject: [PATCH 05/15] Support define owner for datasource api --- .../sql/connector/catalog/PrincipalType.java | 10 +++++++ .../connector/catalog/SupportsNamespaces.java | 17 +++++++++--- .../sql/catalyst/parser/AstBuilder.scala | 17 ++++++++---- .../datasources/v2/CreateNamespaceExec.scala | 7 +++-- .../v2/DescribeNamespaceExec.scala | 6 ++--- .../datasources/v2/V2SessionCatalog.scala | 9 +++++++ .../sql/connector/DataSourceV2SQLSuite.scala | 27 +++++++++++++++++-- .../v2/V2SessionCatalogSuite.scala | 25 +++++------------ 8 files changed, 84 insertions(+), 34 deletions(-) create mode 100644 sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/PrincipalType.java diff --git a/sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/PrincipalType.java b/sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/PrincipalType.java new file mode 100644 index 0000000000000..8a054cab08140 --- /dev/null +++ b/sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/PrincipalType.java @@ -0,0 +1,10 @@ +package org.apache.spark.sql.connector.catalog; + +/** + * An enumeration support for Role-Based Access Control(RBAC) extensions. + */ +public enum PrincipalType { + USER, + GROUP, + ROLE +} diff --git a/sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/SupportsNamespaces.java b/sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/SupportsNamespaces.java index 56cd43c54f016..719dcebaca5b9 100644 --- a/sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/SupportsNamespaces.java +++ b/sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/SupportsNamespaces.java @@ -20,6 +20,7 @@ import org.apache.spark.annotation.Experimental; import org.apache.spark.sql.catalyst.analysis.NamespaceAlreadyExistsException; import org.apache.spark.sql.catalyst.analysis.NoSuchNamespaceException; +import org.apache.spark.util.Utils; import java.util.Arrays; import java.util.List; @@ -64,9 +65,7 @@ public interface SupportsNamespaces extends CatalogPlugin { String PROP_OWNER_TYPE = "ownerType"; /** - * The list of namespace ownership properties, cannot be used in `CREATE` syntax. - * - * Only support in: + * The list of namespace ownership properties, which can be used in `ALTER` syntax: * * {{ * ALTER (DATABASE|SCHEMA|NAMESPACE) SET OWNER ... @@ -86,6 +85,18 @@ public interface SupportsNamespaces extends CatalogPlugin { List REVERSED_PROPERTIES = Arrays.asList(PROP_COMMENT, PROP_LOCATION, PROP_OWNER_NAME, PROP_OWNER_TYPE); + /** + * Specify the default owner name for `CREATE` namespace. + * + */ + default String defaultOwner() { return Utils.getCurrentUserName(); } + + /** + * + * Specify the default owner type for `CREATE` namespace. + */ + default String defaultOwnerType() { return PrincipalType.USER.name(); } + /** * Return a default namespace for the catalog. *

diff --git a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala index 9e29f91073128..45a1948f7159f 100644 --- a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala +++ b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala @@ -39,7 +39,7 @@ import org.apache.spark.sql.catalyst.plans.logical._ import org.apache.spark.sql.catalyst.util.DateTimeUtils.{getZoneId, stringToDate, stringToTimestamp} import org.apache.spark.sql.catalyst.util.IntervalUtils import org.apache.spark.sql.catalyst.util.IntervalUtils.IntervalUnit -import org.apache.spark.sql.connector.catalog.SupportsNamespaces +import org.apache.spark.sql.connector.catalog.{PrincipalType, SupportsNamespaces} import org.apache.spark.sql.connector.expressions.{ApplyTransform, BucketTransform, DaysTransform, Expression => V2Expression, FieldReference, HoursTransform, IdentityTransform, LiteralValue, MonthsTransform, Transform, YearsTransform} import org.apache.spark.sql.internal.SQLConf import org.apache.spark.sql.types._ @@ -2589,10 +2589,17 @@ class AstBuilder(conf: SQLConf) extends SqlBaseBaseVisitor[AnyRef] with Logging */ override def visitSetNamespaceOwner(ctx: SetNamespaceOwnerContext): LogicalPlan = { withOrigin(ctx) { - AlterNamespaceSetOwner( - visitMultipartIdentifier(ctx.multipartIdentifier), - ctx.identifier.getText, - ctx.ownerType.getText) + val ownerType = ctx.ownerType.getText + try { + AlterNamespaceSetOwner( + visitMultipartIdentifier(ctx.multipartIdentifier), + ctx.identifier.getText, + PrincipalType.valueOf(ownerType).name) + } catch { + case _: IllegalArgumentException => + throw new ParseException(s"$ownerType is not supported, need to be one of" + + s" ${PrincipalType.values().mkString("[", ",", "]")}", ctx) + } } } diff --git a/sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/CreateNamespaceExec.scala b/sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/CreateNamespaceExec.scala index 0f69f85dd8376..49a50b6273bc6 100644 --- a/sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/CreateNamespaceExec.scala +++ b/sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/CreateNamespaceExec.scala @@ -22,7 +22,7 @@ import scala.collection.JavaConverters.mapAsJavaMapConverter import org.apache.spark.sql.catalyst.InternalRow import org.apache.spark.sql.catalyst.analysis.NamespaceAlreadyExistsException import org.apache.spark.sql.catalyst.expressions.Attribute -import org.apache.spark.sql.connector.catalog.SupportsNamespaces +import org.apache.spark.sql.connector.catalog.{PrincipalType, SupportsNamespaces} /** * Physical plan node for creating a namespace. @@ -35,11 +35,14 @@ case class CreateNamespaceExec( extends V2CommandExec { override protected def run(): Seq[InternalRow] = { import org.apache.spark.sql.connector.catalog.CatalogV2Implicits._ + import org.apache.spark.sql.connector.catalog.SupportsNamespaces._ val ns = namespace.toArray if (!catalog.namespaceExists(ns)) { try { - catalog.createNamespace(ns, properties.asJava) + val ownership = Map(PROP_OWNER_NAME -> catalog.defaultOwner(), + PROP_OWNER_TYPE -> PrincipalType.valueOf(catalog.defaultOwnerType).name()) + catalog.createNamespace(ns, (properties ++ ownership).asJava) } catch { case _: NamespaceAlreadyExistsException if ifNotExists => logWarning(s"Namespace ${namespace.quoted} was created concurrently. Ignoring.") diff --git a/sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/DescribeNamespaceExec.scala b/sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/DescribeNamespaceExec.scala index e56bc2752d452..bfbd70e1f580e 100644 --- a/sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/DescribeNamespaceExec.scala +++ b/sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/DescribeNamespaceExec.scala @@ -45,13 +45,13 @@ case class DescribeNamespaceExec( rows += toCatalystRow("Namespace Name", ns.last) rows += toCatalystRow("Description", metadata.get(PROP_COMMENT)) rows += toCatalystRow("Location", metadata.get(PROP_LOCATION)) - rows += toCatalystRow("Owner Name", metadata.get(PROP_OWNER_NAME)) - rows += toCatalystRow("Owner Type", metadata.get(PROP_OWNER_TYPE)) + rows += toCatalystRow("Owner Name", metadata.getOrDefault(PROP_OWNER_NAME, "")) + rows += toCatalystRow("Owner Type", metadata.getOrDefault(PROP_OWNER_TYPE, "")) if (isExtended) { val properties = metadata.asScala -- REVERSED_PROPERTIES.asScala if (properties.nonEmpty) { - rows += toCatalystRow("Properties", properties.mkString("(", ",", ")")) + rows += toCatalystRow("Properties", properties.toSeq.mkString("(", ",", ")")) } } rows diff --git a/sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/V2SessionCatalog.scala b/sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/V2SessionCatalog.scala index b14b4b46876d5..56ee9f83af459 100644 --- a/sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/V2SessionCatalog.scala +++ b/sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/V2SessionCatalog.scala @@ -226,6 +226,15 @@ class V2SessionCatalog(catalog: SessionCatalog, conf: SQLConf) namespace match { case Array(db) => val metadata = catalog.getDatabaseMetadata(db).toMetadata + // validate that this catalog's reserved properties are not removed + changes.foreach { + case remove: RemoveProperty + if SupportsNamespaces.REVERSED_PROPERTIES.contains(remove.property) => + throw new UnsupportedOperationException( + s"Cannot remove reserved property: ${remove.property}") + case _ => + } + catalog.alterDatabase( toCatalogDatabase(db, CatalogV2Util.applyNamespaceChanges(metadata, changes))) diff --git a/sql/core/src/test/scala/org/apache/spark/sql/connector/DataSourceV2SQLSuite.scala b/sql/core/src/test/scala/org/apache/spark/sql/connector/DataSourceV2SQLSuite.scala index 629fd28414c54..b0b80fe050e49 100644 --- a/sql/core/src/test/scala/org/apache/spark/sql/connector/DataSourceV2SQLSuite.scala +++ b/sql/core/src/test/scala/org/apache/spark/sql/connector/DataSourceV2SQLSuite.scala @@ -29,6 +29,7 @@ import org.apache.spark.sql.internal.SQLConf.V2_SESSION_CATALOG_IMPLEMENTATION import org.apache.spark.sql.sources.SimpleScanSource import org.apache.spark.sql.types.{BooleanType, LongType, StringType, StructType} import org.apache.spark.sql.util.CaseInsensitiveStringMap +import org.apache.spark.util.Utils class DataSourceV2SQLSuite extends InsertIntoTests(supportsDynamicOverwrite = true, includeSQLOnlyTests = true) @@ -933,7 +934,9 @@ class DataSourceV2SQLSuite assert(description === Seq( Row("Namespace Name", "ns2"), Row("Description", "test namespace"), - Row("Location", "/tmp/ns_test") + Row("Location", "/tmp/ns_test"), + Row("Owner Name", Utils.getCurrentUserName()), + Row("Owner Type", PrincipalType.USER.name()) )) } } @@ -948,6 +951,8 @@ class DataSourceV2SQLSuite Row("Namespace Name", "ns2"), Row("Description", "test namespace"), Row("Location", "/tmp/ns_test"), + Row("Owner Name", Utils.getCurrentUserName()), + Row("Owner Type", PrincipalType.USER.name()), Row("Properties", "((a,b),(b,a),(c,c))") )) } @@ -962,7 +967,25 @@ class DataSourceV2SQLSuite assert(descriptionDf.collect() === Seq( Row("Namespace Name", "ns2"), Row("Description", "test namespace"), - Row("Location", "/tmp/ns_test_2") + Row("Location", "/tmp/ns_test_2"), + Row("Owner Name", Utils.getCurrentUserName()), + Row("Owner Type", PrincipalType.USER.name()) + )) + } + } + + test("AlterNamespaceSetOwner using v2 catalog") { + withNamespace("testcat.ns1.ns2") { + sql("CREATE NAMESPACE IF NOT EXISTS testcat.ns1.ns2 COMMENT " + + "'test namespace' LOCATION '/tmp/ns_test_3'") + sql("ALTER NAMESPACE testcat.ns1.ns2 SET OWNER ROLE adminRole") + val descriptionDf = sql("DESCRIBE NAMESPACE EXTENDED testcat.ns1.ns2") + assert(descriptionDf.collect() === Seq( + Row("Namespace Name", "ns2"), + Row("Description", "test namespace"), + Row("Location", "/tmp/ns_test_3"), + Row("Owner Name", "adminRole"), + Row("Owner Type", PrincipalType.ROLE.name()) )) } } diff --git a/sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/v2/V2SessionCatalogSuite.scala b/sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/v2/V2SessionCatalogSuite.scala index 24e2929f21c03..bb328e26f1c39 100644 --- a/sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/v2/V2SessionCatalogSuite.scala +++ b/sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/v2/V2SessionCatalogSuite.scala @@ -1010,31 +1010,18 @@ class V2SessionCatalogNamespaceSuite extends V2SessionCatalogBaseSuite { assert(exc.getMessage.contains(testNs.quoted)) } - test("alterNamespace: fail to remove location") { + test("alterNamespace: fail to remove reversed properties") { val catalog = newCatalog() catalog.createNamespace(testNs, emptyProps) - val exc = intercept[UnsupportedOperationException] { - catalog.alterNamespace(testNs, NamespaceChange.removeProperty("location")) - } - - assert(exc.getMessage.contains("Cannot remove reserved property: location")) - - catalog.dropNamespace(testNs) - } + SupportsNamespaces.REVERSED_PROPERTIES.asScala.foreach { p => + val exc = intercept[UnsupportedOperationException] { + catalog.alterNamespace(testNs, NamespaceChange.removeProperty(p)) + } + assert(exc.getMessage.contains(s"Cannot remove reserved property: $p")) - test("alterNamespace: fail to remove comment") { - val catalog = newCatalog() - - catalog.createNamespace(testNs, Map("comment" -> "test db").asJava) - - val exc = intercept[UnsupportedOperationException] { - catalog.alterNamespace(testNs, NamespaceChange.removeProperty("comment")) } - - assert(exc.getMessage.contains("Cannot remove reserved property: comment")) - catalog.dropNamespace(testNs) } } From c1680a437542f4e2ac111da62ab5eceffb5f17f2 Mon Sep 17 00:00:00 2001 From: Kent Yao Date: Mon, 9 Dec 2019 17:08:51 +0800 Subject: [PATCH 06/15] later expose ownership api --- .../sql/connector/catalog/SupportsNamespaces.java | 14 +------------- .../sql/catalyst/analysis/ResolveCatalogs.scala | 6 +++--- .../catalyst/analysis/ResolveSessionCatalog.scala | 8 ++++---- .../apache/spark/sql/execution/command/ddl.scala | 2 +- .../datasources/v2/CreateNamespaceExec.scala | 5 +++-- .../datasources/v2/DescribeNamespaceExec.scala | 2 +- .../datasources/v2/V2SessionCatalog.scala | 2 +- .../datasources/v2/V2SessionCatalogSuite.scala | 4 ++-- 8 files changed, 16 insertions(+), 27 deletions(-) diff --git a/sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/SupportsNamespaces.java b/sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/SupportsNamespaces.java index 719dcebaca5b9..d00d83e11514b 100644 --- a/sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/SupportsNamespaces.java +++ b/sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/SupportsNamespaces.java @@ -82,21 +82,9 @@ public interface SupportsNamespaces extends CatalogPlugin { * * They need specific syntax to modify */ - List REVERSED_PROPERTIES = + List RESERVED_PROPERTIES = Arrays.asList(PROP_COMMENT, PROP_LOCATION, PROP_OWNER_NAME, PROP_OWNER_TYPE); - /** - * Specify the default owner name for `CREATE` namespace. - * - */ - default String defaultOwner() { return Utils.getCurrentUserName(); } - - /** - * - * Specify the default owner type for `CREATE` namespace. - */ - default String defaultOwnerType() { return PrincipalType.USER.name(); } - /** * Return a default namespace for the catalog. *

diff --git a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveCatalogs.scala b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveCatalogs.scala index 5dd2c3c20ab7c..4d76aba00227b 100644 --- a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveCatalogs.scala +++ b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveCatalogs.scala @@ -97,9 +97,9 @@ 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" + - s" ${REVERSED_PROPERTIES.asScala.mkString("[", ",", "]")}.") + if (properties.keySet.intersect(RESERVED_PROPERTIES.asScala.toSet).nonEmpty) { + throw new AnalysisException("Cannot directly modify the reserved properties" + + s" ${RESERVED_PROPERTIES.asScala.mkString("[", ",", "]")}.") } AlterNamespaceSetProperties(catalog.asNamespaceCatalog, nameParts, properties) diff --git a/sql/core/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveSessionCatalog.scala b/sql/core/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveSessionCatalog.scala index 9bc2268e8e5a9..fbe4bba72cefe 100644 --- a/sql/core/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveSessionCatalog.scala +++ b/sql/core/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveSessionCatalog.scala @@ -173,9 +173,9 @@ 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" + - s" ${REVERSED_PROPERTIES.asScala.mkString("[", ",", "]")}.") + if (properties.keySet.intersect(RESERVED_PROPERTIES.asScala.toSet).nonEmpty) { + throw new AnalysisException("Cannot directly modify the reserved properties" + + s" ${RESERVED_PROPERTIES.asScala.mkString("[", ",", "]")}.") } AlterDatabasePropertiesCommand(nameParts.head, properties) @@ -312,7 +312,7 @@ class ResolveSessionCatalog( } val comment = c.properties.get(PROP_COMMENT) val location = c.properties.get(PROP_LOCATION) - val newProperties = c.properties -- REVERSED_PROPERTIES.asScala + val newProperties = c.properties -- RESERVED_PROPERTIES.asScala CreateDatabaseCommand(nameParts.head, c.ifNotExists, location, comment, newProperties) case d @ DropNamespaceStatement(SessionCatalog(_, nameParts), _, _) => diff --git a/sql/core/src/main/scala/org/apache/spark/sql/execution/command/ddl.scala b/sql/core/src/main/scala/org/apache/spark/sql/execution/command/ddl.scala index bf278add5c399..e9d77db52a30c 100644 --- a/sql/core/src/main/scala/org/apache/spark/sql/execution/command/ddl.scala +++ b/sql/core/src/main/scala/org/apache/spark/sql/execution/command/ddl.scala @@ -183,7 +183,7 @@ case class DescribeDatabaseCommand( Row("Owner Type", allDbProperties.getOrElse(PROP_OWNER_TYPE, "")) :: Nil if (extended) { - val properties = allDbProperties -- REVERSED_PROPERTIES.asScala + val properties = allDbProperties -- RESERVED_PROPERTIES.asScala val propertiesStr = if (properties.isEmpty) { "" diff --git a/sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/CreateNamespaceExec.scala b/sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/CreateNamespaceExec.scala index 49a50b6273bc6..dbe9bf8f21fff 100644 --- a/sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/CreateNamespaceExec.scala +++ b/sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/CreateNamespaceExec.scala @@ -23,6 +23,7 @@ import org.apache.spark.sql.catalyst.InternalRow import org.apache.spark.sql.catalyst.analysis.NamespaceAlreadyExistsException import org.apache.spark.sql.catalyst.expressions.Attribute import org.apache.spark.sql.connector.catalog.{PrincipalType, SupportsNamespaces} +import org.apache.spark.util.Utils /** * Physical plan node for creating a namespace. @@ -40,8 +41,8 @@ case class CreateNamespaceExec( val ns = namespace.toArray if (!catalog.namespaceExists(ns)) { try { - val ownership = Map(PROP_OWNER_NAME -> catalog.defaultOwner(), - PROP_OWNER_TYPE -> PrincipalType.valueOf(catalog.defaultOwnerType).name()) + val ownership = Map(PROP_OWNER_NAME -> Utils.getCurrentUserName(), + PROP_OWNER_TYPE -> PrincipalType.USER.name()) catalog.createNamespace(ns, (properties ++ ownership).asJava) } catch { case _: NamespaceAlreadyExistsException if ifNotExists => diff --git a/sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/DescribeNamespaceExec.scala b/sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/DescribeNamespaceExec.scala index bfbd70e1f580e..7fbc6c71d78a9 100644 --- a/sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/DescribeNamespaceExec.scala +++ b/sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/DescribeNamespaceExec.scala @@ -49,7 +49,7 @@ case class DescribeNamespaceExec( rows += toCatalystRow("Owner Type", metadata.getOrDefault(PROP_OWNER_TYPE, "")) if (isExtended) { - val properties = metadata.asScala -- REVERSED_PROPERTIES.asScala + val properties = metadata.asScala -- RESERVED_PROPERTIES.asScala if (properties.nonEmpty) { rows += toCatalystRow("Properties", properties.toSeq.mkString("(", ",", ")")) } diff --git a/sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/V2SessionCatalog.scala b/sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/V2SessionCatalog.scala index 56ee9f83af459..1a7c1fa0c8d2a 100644 --- a/sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/V2SessionCatalog.scala +++ b/sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/V2SessionCatalog.scala @@ -229,7 +229,7 @@ class V2SessionCatalog(catalog: SessionCatalog, conf: SQLConf) // validate that this catalog's reserved properties are not removed changes.foreach { case remove: RemoveProperty - if SupportsNamespaces.REVERSED_PROPERTIES.contains(remove.property) => + if SupportsNamespaces.RESERVED_PROPERTIES.contains(remove.property) => throw new UnsupportedOperationException( s"Cannot remove reserved property: ${remove.property}") case _ => diff --git a/sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/v2/V2SessionCatalogSuite.scala b/sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/v2/V2SessionCatalogSuite.scala index bb328e26f1c39..d3c81e8a2cc1e 100644 --- a/sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/v2/V2SessionCatalogSuite.scala +++ b/sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/v2/V2SessionCatalogSuite.scala @@ -757,7 +757,7 @@ class V2SessionCatalogNamespaceSuite extends V2SessionCatalogBaseSuite { actual: scala.collection.Map[String, String]): Unit = { // remove location and comment that are automatically added by HMS unless they are expected val toRemove = - SupportsNamespaces.REVERSED_PROPERTIES.asScala.filter(expected.contains) + SupportsNamespaces.RESERVED_PROPERTIES.asScala.filter(expected.contains) assert(expected -- toRemove === actual) } @@ -1015,7 +1015,7 @@ class V2SessionCatalogNamespaceSuite extends V2SessionCatalogBaseSuite { catalog.createNamespace(testNs, emptyProps) - SupportsNamespaces.REVERSED_PROPERTIES.asScala.foreach { p => + SupportsNamespaces.RESERVED_PROPERTIES.asScala.foreach { p => val exc = intercept[UnsupportedOperationException] { catalog.alterNamespace(testNs, NamespaceChange.removeProperty(p)) } From 824cb6a8f7c0954cea559707bdf89aa5872a9557 Mon Sep 17 00:00:00 2001 From: Kent Yao Date: Mon, 9 Dec 2019 17:40:32 +0800 Subject: [PATCH 07/15] style fix --- .../apache/spark/sql/connector/catalog/SupportsNamespaces.java | 1 - 1 file changed, 1 deletion(-) diff --git a/sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/SupportsNamespaces.java b/sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/SupportsNamespaces.java index d00d83e11514b..2ac05dc834e5f 100644 --- a/sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/SupportsNamespaces.java +++ b/sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/SupportsNamespaces.java @@ -20,7 +20,6 @@ import org.apache.spark.annotation.Experimental; import org.apache.spark.sql.catalyst.analysis.NamespaceAlreadyExistsException; import org.apache.spark.sql.catalyst.analysis.NoSuchNamespaceException; -import org.apache.spark.util.Utils; import java.util.Arrays; import java.util.List; From 406c5c25b7db170cea556c72f8d6fc8570daf282 Mon Sep 17 00:00:00 2001 From: Kent Yao Date: Wed, 11 Dec 2019 00:21:56 +0800 Subject: [PATCH 08/15] rm PrincipalType --- .../sql/connector/catalog/PrincipalType.java | 10 ---------- .../spark/sql/catalyst/parser/AstBuilder.scala | 17 +++++------------ .../datasources/v2/CreateNamespaceExec.scala | 6 +++--- .../sql/connector/DataSourceV2SQLSuite.scala | 8 ++++---- 4 files changed, 12 insertions(+), 29 deletions(-) delete mode 100644 sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/PrincipalType.java diff --git a/sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/PrincipalType.java b/sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/PrincipalType.java deleted file mode 100644 index 8a054cab08140..0000000000000 --- a/sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/PrincipalType.java +++ /dev/null @@ -1,10 +0,0 @@ -package org.apache.spark.sql.connector.catalog; - -/** - * An enumeration support for Role-Based Access Control(RBAC) extensions. - */ -public enum PrincipalType { - USER, - GROUP, - ROLE -} diff --git a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala index 45a1948f7159f..9e29f91073128 100644 --- a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala +++ b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala @@ -39,7 +39,7 @@ import org.apache.spark.sql.catalyst.plans.logical._ import org.apache.spark.sql.catalyst.util.DateTimeUtils.{getZoneId, stringToDate, stringToTimestamp} import org.apache.spark.sql.catalyst.util.IntervalUtils import org.apache.spark.sql.catalyst.util.IntervalUtils.IntervalUnit -import org.apache.spark.sql.connector.catalog.{PrincipalType, SupportsNamespaces} +import org.apache.spark.sql.connector.catalog.SupportsNamespaces import org.apache.spark.sql.connector.expressions.{ApplyTransform, BucketTransform, DaysTransform, Expression => V2Expression, FieldReference, HoursTransform, IdentityTransform, LiteralValue, MonthsTransform, Transform, YearsTransform} import org.apache.spark.sql.internal.SQLConf import org.apache.spark.sql.types._ @@ -2589,17 +2589,10 @@ class AstBuilder(conf: SQLConf) extends SqlBaseBaseVisitor[AnyRef] with Logging */ override def visitSetNamespaceOwner(ctx: SetNamespaceOwnerContext): LogicalPlan = { withOrigin(ctx) { - val ownerType = ctx.ownerType.getText - try { - AlterNamespaceSetOwner( - visitMultipartIdentifier(ctx.multipartIdentifier), - ctx.identifier.getText, - PrincipalType.valueOf(ownerType).name) - } catch { - case _: IllegalArgumentException => - throw new ParseException(s"$ownerType is not supported, need to be one of" + - s" ${PrincipalType.values().mkString("[", ",", "]")}", ctx) - } + AlterNamespaceSetOwner( + visitMultipartIdentifier(ctx.multipartIdentifier), + ctx.identifier.getText, + ctx.ownerType.getText) } } diff --git a/sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/CreateNamespaceExec.scala b/sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/CreateNamespaceExec.scala index dbe9bf8f21fff..5af592cfd5f5a 100644 --- a/sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/CreateNamespaceExec.scala +++ b/sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/CreateNamespaceExec.scala @@ -22,7 +22,7 @@ import scala.collection.JavaConverters.mapAsJavaMapConverter import org.apache.spark.sql.catalyst.InternalRow import org.apache.spark.sql.catalyst.analysis.NamespaceAlreadyExistsException import org.apache.spark.sql.catalyst.expressions.Attribute -import org.apache.spark.sql.connector.catalog.{PrincipalType, SupportsNamespaces} +import org.apache.spark.sql.connector.catalog.SupportsNamespaces import org.apache.spark.util.Utils /** @@ -41,8 +41,8 @@ case class CreateNamespaceExec( val ns = namespace.toArray if (!catalog.namespaceExists(ns)) { try { - val ownership = Map(PROP_OWNER_NAME -> Utils.getCurrentUserName(), - PROP_OWNER_TYPE -> PrincipalType.USER.name()) + val ownership = + Map(PROP_OWNER_NAME -> Utils.getCurrentUserName(), PROP_OWNER_TYPE -> "USER") catalog.createNamespace(ns, (properties ++ ownership).asJava) } catch { case _: NamespaceAlreadyExistsException if ifNotExists => diff --git a/sql/core/src/test/scala/org/apache/spark/sql/connector/DataSourceV2SQLSuite.scala b/sql/core/src/test/scala/org/apache/spark/sql/connector/DataSourceV2SQLSuite.scala index b0b80fe050e49..8af2913e3147c 100644 --- a/sql/core/src/test/scala/org/apache/spark/sql/connector/DataSourceV2SQLSuite.scala +++ b/sql/core/src/test/scala/org/apache/spark/sql/connector/DataSourceV2SQLSuite.scala @@ -936,7 +936,7 @@ class DataSourceV2SQLSuite Row("Description", "test namespace"), Row("Location", "/tmp/ns_test"), Row("Owner Name", Utils.getCurrentUserName()), - Row("Owner Type", PrincipalType.USER.name()) + Row("Owner Type", "USER") )) } } @@ -952,7 +952,7 @@ class DataSourceV2SQLSuite Row("Description", "test namespace"), Row("Location", "/tmp/ns_test"), Row("Owner Name", Utils.getCurrentUserName()), - Row("Owner Type", PrincipalType.USER.name()), + Row("Owner Type", "USER"), Row("Properties", "((a,b),(b,a),(c,c))") )) } @@ -969,7 +969,7 @@ class DataSourceV2SQLSuite Row("Description", "test namespace"), Row("Location", "/tmp/ns_test_2"), Row("Owner Name", Utils.getCurrentUserName()), - Row("Owner Type", PrincipalType.USER.name()) + Row("Owner Type", "USER") )) } } @@ -985,7 +985,7 @@ class DataSourceV2SQLSuite Row("Description", "test namespace"), Row("Location", "/tmp/ns_test_3"), Row("Owner Name", "adminRole"), - Row("Owner Type", PrincipalType.ROLE.name()) + Row("Owner Type", "ROLE") )) } } From 10e1bc3168a1a9435983cdc13a50dc773b1cee1c Mon Sep 17 00:00:00 2001 From: Kent Yao Date: Wed, 11 Dec 2019 17:23:21 +0800 Subject: [PATCH 09/15] mv to v2 commands --- .../spark/sql/catalyst/plans/logical/statements.scala | 8 -------- .../spark/sql/catalyst/plans/logical/v2Commands.scala | 8 ++++++++ 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/statements.scala b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/statements.scala index 950378f565997..5db099e1de631 100644 --- a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/statements.scala +++ b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/statements.scala @@ -382,14 +382,6 @@ case class AlterNamespaceSetLocationStatement( namespace: Seq[String], location: String) extends ParsedStatement -/** - * ALTER (DATABASE|SCHEMA|NAMESPACE) ... SET OWNER command, as parsed from SQL. - */ -case class AlterNamespaceSetOwner( - namespace: Seq[String], - ownerName: String, - ownerType: String) extends ParsedStatement - /** * A SHOW NAMESPACES statement, as parsed from SQL. */ diff --git a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/v2Commands.scala b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/v2Commands.scala index d87758a7df7b6..e842f1f78e9f7 100644 --- a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/v2Commands.scala +++ b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/v2Commands.scala @@ -279,6 +279,14 @@ case class AlterNamespaceSetProperties( namespace: Seq[String], properties: Map[String, String]) extends Command +/** + * ALTER (DATABASE|SCHEMA|NAMESPACE) ... SET OWNER command, as parsed from SQL. + */ +case class AlterNamespaceSetOwner( + namespace: Seq[String], + ownerName: String, + ownerType: String) extends Command + /** * The logical plan of the SHOW NAMESPACES command that works for v2 catalogs. */ From e5d695eb63b77d605906e260781276ed20f009be Mon Sep 17 00:00:00 2001 From: Kent Yao Date: Mon, 6 Jan 2020 11:07:54 +0800 Subject: [PATCH 10/15] rm unused lines --- .../spark/sql/connector/catalog/SupportsNamespaces.java | 9 --------- 1 file changed, 9 deletions(-) diff --git a/sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/SupportsNamespaces.java b/sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/SupportsNamespaces.java index 2ac05dc834e5f..232736b8f1e37 100644 --- a/sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/SupportsNamespaces.java +++ b/sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/SupportsNamespaces.java @@ -63,15 +63,6 @@ public interface SupportsNamespaces extends CatalogPlugin { */ String PROP_OWNER_TYPE = "ownerType"; - /** - * The list of namespace ownership properties, which can be used in `ALTER` syntax: - * - * {{ - * ALTER (DATABASE|SCHEMA|NAMESPACE) SET OWNER ... - * }} - */ - List OWNERSHIPS = Arrays.asList(PROP_OWNER_NAME, PROP_OWNER_TYPE); - /** * The list of immutable namespace properties, which can not be removed or changed directly by * the syntax: From ae1ebec24d5fca1a6c418275a1500f56de70ff54 Mon Sep 17 00:00:00 2001 From: Kent Yao Date: Thu, 9 Jan 2020 14:14:31 +0800 Subject: [PATCH 11/15] improve err msg --- .../org/apache/spark/sql/catalyst/parser/AstBuilder.scala | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala index 61b5ac31a088c..92e10dcf67ca8 100644 --- a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala +++ b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala @@ -2533,7 +2533,8 @@ class AstBuilder(conf: SQLConf) extends SqlBaseBaseVisitor[AnyRef] with Logging throw new ParseException(s"$PROP_COMMENT is a reserved namespace property, please use" + s" the COMMENT clause to specify it.", ctx) case (ownership, _) if ownership == PROP_OWNER_NAME || ownership == PROP_OWNER_TYPE => - throw new ParseException(s"$ownership is a reserved namespace property", ctx) + throw new ParseException(s"$ownership is a reserved namespace property , please use" + + " ALTER NAMESPACE ... SET OWNER ... to specify it.", ctx) case _ => } properties From bb2f919c2cb68c4fa3ace59355514d61088ac089 Mon Sep 17 00:00:00 2001 From: Kent Yao Date: Thu, 9 Jan 2020 16:39:54 +0800 Subject: [PATCH 12/15] nit --- .../antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4 | 1 + .../spark/sql/connector/catalog/SupportsNamespaces.java | 4 ++-- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4 b/sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4 index 23f904bd6cdad..78b76efcddc74 100644 --- a/sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4 +++ b/sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4 @@ -1097,6 +1097,7 @@ ansiNonReserved | OVER | OVERLAY | OVERWRITE + | OWNER | PARTITION | PARTITIONED | PARTITIONS diff --git a/sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/SupportsNamespaces.java b/sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/SupportsNamespaces.java index 232736b8f1e37..d58fe611b2704 100644 --- a/sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/SupportsNamespaces.java +++ b/sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/SupportsNamespaces.java @@ -64,10 +64,10 @@ public interface SupportsNamespaces extends CatalogPlugin { String PROP_OWNER_TYPE = "ownerType"; /** - * The list of immutable namespace properties, which can not be removed or changed directly by + * The list of reserved namespace properties, which can not be removed or changed directly by * the syntax: * {{ - * ALTER (DATABASE|SCHEMA|NAMESPACE) SET DBPROPERTIES(...) + * ALTER NAMESPACE ... SET PROPERTIES ... * }} * * They need specific syntax to modify From c76e5b5d951081a69616fabf0a4be53c125814c6 Mon Sep 17 00:00:00 2001 From: Kent Yao Date: Thu, 9 Jan 2020 16:53:02 +0800 Subject: [PATCH 13/15] nit --- .../org/apache/spark/sql/connector/DataSourceV2SQLSuite.scala | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/sql/core/src/test/scala/org/apache/spark/sql/connector/DataSourceV2SQLSuite.scala b/sql/core/src/test/scala/org/apache/spark/sql/connector/DataSourceV2SQLSuite.scala index 7b8e62c9e1ea6..593da7e812652 100644 --- a/sql/core/src/test/scala/org/apache/spark/sql/connector/DataSourceV2SQLSuite.scala +++ b/sql/core/src/test/scala/org/apache/spark/sql/connector/DataSourceV2SQLSuite.scala @@ -885,7 +885,7 @@ class DataSourceV2SQLSuite .isEmpty, s"$key is a reserved namespace property and ignored") val meta = catalog("testcat").asNamespaceCatalog.loadNamespaceMetadata(Array("reservedTest")) - assert(!Option(meta.get(key)).exists(_.contains("foo")), + assert(meta.get(key) == null || !meta.get(key).contains("foo"), "reserved properties should not have side effects") } } @@ -1016,7 +1016,7 @@ class DataSourceV2SQLSuite .isEmpty, s"$key is a reserved namespace property and ignored") val meta = catalog("testcat").asNamespaceCatalog.loadNamespaceMetadata(Array("reservedTest")) - assert(!Option(meta.get(key)).exists(_.contains("foo")), + assert(meta.get(key) == null || !meta.get(key).contains("foo"), "reserved properties should not have side effects") } } From 218299212e586f05748ccd4ddb8c9518cc1626bf Mon Sep 17 00:00:00 2001 From: Kent Yao Date: Thu, 9 Jan 2020 16:57:46 +0800 Subject: [PATCH 14/15] don't show reserved properties if none --- .../datasources/v2/DescribeNamespaceExec.scala | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/DescribeNamespaceExec.scala b/sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/DescribeNamespaceExec.scala index 7fbc6c71d78a9..979d740efa8ff 100644 --- a/sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/DescribeNamespaceExec.scala +++ b/sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/DescribeNamespaceExec.scala @@ -43,10 +43,18 @@ case class DescribeNamespaceExec( val metadata = catalog.loadNamespaceMetadata(ns) rows += toCatalystRow("Namespace Name", ns.last) - rows += toCatalystRow("Description", metadata.get(PROP_COMMENT)) - rows += toCatalystRow("Location", metadata.get(PROP_LOCATION)) - rows += toCatalystRow("Owner Name", metadata.getOrDefault(PROP_OWNER_NAME, "")) - rows += toCatalystRow("Owner Type", metadata.getOrDefault(PROP_OWNER_TYPE, "")) + Option(metadata.get(PROP_COMMENT)).foreach { + rows += toCatalystRow("Description", _) + } + Option(metadata.get(PROP_LOCATION)).foreach { + rows += toCatalystRow("Location", _) + } + Option(metadata.get(PROP_OWNER_NAME)).foreach { + rows += toCatalystRow("Owner Name", _) + } + Option(metadata.get(PROP_OWNER_TYPE)).foreach { + rows += toCatalystRow("Owner Type", _) + } if (isExtended) { val properties = metadata.asScala -- RESERVED_PROPERTIES.asScala From 514b78a4d574ea48736a31770136d6b4eae6a25e Mon Sep 17 00:00:00 2001 From: Kent Yao Date: Thu, 9 Jan 2020 19:12:23 +0800 Subject: [PATCH 15/15] fix test --- .../apache/spark/sql/hive/execution/HiveDDLSuite.scala | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveDDLSuite.scala b/sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveDDLSuite.scala index 67c6840c09f87..b3f7fc4d0557e 100644 --- a/sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveDDLSuite.scala +++ b/sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveDDLSuite.scala @@ -397,16 +397,16 @@ class HiveCatalogedDDLSuite extends DDLSuite with TestHiveSingleton with BeforeA checkOwner(db1, currentUser, "USER") sql(s"ALTER DATABASE $db1 SET DBPROPERTIES ('a'='a')") checkOwner(db1, currentUser, "USER") - val e = intercept[AnalysisException](sql(s"ALTER DATABASE $db1 SET DBPROPERTIES ('a'='a'," + val e = intercept[ParseException](sql(s"ALTER DATABASE $db1 SET DBPROPERTIES ('a'='a'," + s"'ownerName'='$owner','ownerType'='XXX')")) 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')") + val e2 = intercept[ParseException]( + sql(s"CREATE DATABASE $db2 WITH DBPROPERTIES('ownerName'='$owner')")) + assert(e2.getMessage.contains("ownerName")) + sql(s"CREATE DATABASE $db2") checkOwner(db2, currentUser, "USER") sql(s"ALTER DATABASE $db2 SET OWNER GROUP $owner") checkOwner(db2, owner, "GROUP")