[SPARK-30018][SQL] Support ALTER DATABASE SET OWNER syntax #26775

yaooqinn · 2019-12-06T06:43:21Z

What changes were proposed in this pull request?

In this pull request, we are going to support SET OWNER syntax for databases and namespaces,

ALTER (DATABASE|SCHEME|NAMESPACE) database_name SET OWNER [USER|ROLE|GROUP] user_or_role_group;

Before this commit 332e252, we didn't care much about ownerships for the catalog objects. In 332e252, we determined to use properties to store ownership staff, and temporarily used alter database ... set dbproperties ... to support switch ownership of a database. This PR aims to use the formal syntax to replace it.

In hive, ownerName/Type are fields of the database objects, also they can be normal properties.

create schema test1 with dbproperties('ownerName'='yaooqinn')

The create/alter database syntax will not change the owner to yaooqinn but store it in parameters. e.g.

+----------+----------+---------------------------------------------------------------+-------------+-------------+-----------------------+--+
| db_name  | comment  |                           location                            | owner_name  | owner_type  |      parameters       |
+----------+----------+---------------------------------------------------------------+-------------+-------------+-----------------------+--+
| test1    |          | hdfs://quickstart.cloudera:8020/user/hive/warehouse/test1.db  | anonymous   | USER        | {ownerName=yaooqinn}  |
+----------+----------+---------------------------------------------------------------+-------------+-------------+-----------------------+--+

In this pull request, because we let the ownerName become reversed, so it will neither change the owner nor store in dbproperties, just be omitted silently.

Why are the changes needed?

Formal syntax support for changing database ownership

Does this PR introduce any user-facing change?

yes, add a new syntax

How was this patch tested?

add unit tests

SparkQA · 2019-12-06T08:05:01Z

Test build #114928 has finished for PR 26775 at commit 3c96091.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class AlterNamespaceSetOwnerStatement(
case class AlterDatabaseSetOwnerCommand(databaseName: String, ownerName: String, ownerType: String)

SparkQA · 2019-12-06T08:05:01Z

Test build #114930 has finished for PR 26775 at commit 7188587.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

yaooqinn · 2019-12-06T08:25:46Z

retest this please

yaooqinn · 2019-12-06T08:26:34Z

cc @cloud-fan @maropu @wangyum, thanks for reviewing.

sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4

cloud-fan · 2019-12-06T08:56:39Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveCatalogs.scala

@@ -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


I think it's better to fail if reserved properties are being set/altered.

ok, I will go this way.

I notice that the RESERVED_PROPERTIES is unremovable but not reversed.. the comment and location is not exactly same with ownerName/Type. the name RESERVED_PROPERTIES here is deceptive.

can Hive alter table location via ALTER TABLE SET TBLPROPERTIES?

Seems like Hive can only do it for table comment: https://cwiki.apache.org/confluence/display/Hive/LanguageManual+DDL#LanguageManualDDL-AlterTableComment

Hive will not change Database field members via properties, if we set dbproperites('location'='xxx'), the location here is just a property inside properties, meaningless to the catalog

OK so we can't follow Hive here. It seems wrong to me to change location and owner with SET TBLPROPERTIES or SET DBPROPERTIES, but comment should be fine.

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/statements.scala

SparkQA · 2019-12-06T12:32:31Z

Test build #114934 has finished for PR 26775 at commit 7188587.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

yaooqinn · 2019-12-06T14:44:00Z

Code path for now:

[[CreateNamespaceStatement]]
    users can specify `comment` `location`
    users can not sepcify `ownerName` `ownerType`

    -> [[CreateDatabaseCommand]]
    We shall throw exception when meet user specified `ownerName` `ownerType`
    [[ResolveSessionCatalog]] will convert `comment` `location` to db fields
    [[HiveClientImpl]] will gather `ownerName` `ownerType`
    
    -> [[CreateNamespace]]
    we shall throw exception when meet user specified `ownerName` `ownerType`
       -> [[CreateNamespaceExec]]
       all propeties get here, no `ownerName` `ownerType` guaranteed
       if users use built in catalog
          [[V2SessionCatalog]] will convert `comment` `location` to db fields
          [[HiveClientImpl]] will gather `ownerName` `ownerType`
       if users use a external [[DelegatingCatalogExtension]]
          users need to care how to create `ownerName` `ownerType` or do we use spark user here?

[[AlterNamespaceSetPropertiesStatement]]
we shall throw exception when meet user specified `ownerName` `ownerType` `comment`(?) `location` before transform to
    -> [[AlterNamespaceSetProperties]]
    -> [[AlterDatabasePropertiesCommand]]

then, if users want to change these, use [[AlterNamespaceSetLocationStatement]] and [[AlterNamespaceSetOwner]]

Shall we expose a api in [[SupportsNamespace]] to deal with ownership staff? Then we need not to care what the underlying catalog delegation it is. We handle these in [[HiveClientImpl]] for now, it seems not a good solution.

SparkQA · 2019-12-06T16:01:50Z

Test build #114951 has finished for PR 26775 at commit 56ac955.

This patch fails Java style tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class AlterNamespaceSetOwner(

SparkQA · 2019-12-06T17:55:27Z

Test build #114957 has finished for PR 26775 at commit 25cf227.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

maropu · 2019-12-09T00:50:49Z

How do we use ROLE/GROUP? I read the hive doc though. Probably, we need to describe how-to-use in the doc (we already have somewhere?). cc: @dongjoon-hyun

maropu · 2019-12-09T00:53:18Z

sql/core/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveSessionCatalog.scala

@@ -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" +


nit: drop s in the head.

maropu · 2019-12-09T01:00:55Z

sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/SupportsNamespaces.java

+   * The list of namespace ownership properties, cannot be used in `CREATE` syntax.
+   *
+   * Only support in:
+   *


How about just saying like this? We need to mention CREATE syntax here?

* The list of namespace ownership properties, which can be used in `ALTER` syntax: * * {{ * ALTER (DATABASE|SCHEMA|NAMESPACE) SET OWNER ... * }}

maropu · 2019-12-09T01:04:46Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveCatalogs.scala

@@ -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" +


nit: drop s

Is this change related to this pr to support SET OWNER?

to prohibit changing ownership ·SET PROPERTIES·

cloud-fan · 2019-12-09T05:23:12Z

For data source API, we do want to use properties to set owner/location/comment to make the API flexible. The problem is about end-user API: how do we expect end-users to set them?

What I expect:

for CREATE TABLE/NAMESPACE, location should be set by LOCATION clause, comment should be set by COMMENT or properties. I'm not sure about owner: does hive support setting owner during CREATE TABLE/DATABASE
for ALTER TABLE/NAMESPACE, should be the same. location/owner should be set by special syntax instead of via properties.

yaooqinn · 2019-12-09T05:43:31Z

For DataSource API developers:
Exposes defaultOwner/OwnerType in SupportsNamespaces to defined the catalog default ownership. By default, we use our sparkUser as default owner and USER as default ownerType. This should be considered as how the developers define authentication. A bit similar with Hive's HiveAuthenticationProvider . Currently, we use this only in CreateNamespaceExec to define the default ownership. When we support authorization, these can be used in privilege checking for queries, commands etc..

For end users:
We don't allow user to set ownership or location or comment in create syntax, the ownership should be inherited from the catalog impl, and the comment and location should from the specific clauses.

Also modifying these properties need specific commands.

cloud-fan · 2019-12-09T05:54:02Z

hmm, isn't it better to let Spark decide the default owner instead of catalog implementations?

yaooqinn · 2019-12-09T06:03:45Z

E.g. we currently don't support multi tenancy in Spark's ThriftServer, but if can use such a flexible implementation, we may can implement defaultOwner = session.getUser to get the actual user from client side, and do authentication/authorization for it. In the current implementation of Spark's ThriftServer, the sparkUser can only works as a super user, then at least we can achieve metadata security.

cloud-fan · 2019-12-09T06:46:12Z

OK let's discuss it later. For now let's focus on ALTER TABLE/NAMESPACE SET OWNER.

SparkQA · 2019-12-09T08:05:02Z

Test build #115012 has finished for PR 26775 at commit dd1d52b.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

yaooqinn · 2019-12-09T08:06:05Z

retest this please

cloud-fan · 2019-12-09T08:55:49Z

sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/SupportsNamespaces.java

+   * Specify the default owner name for `CREATE` namespace.
+   *
+   */
+  default String defaultOwner() { return Utils.getCurrentUserName(); }


Can we do it later? I don't think there is a consensus yet. For now let's let spark decide the default owner, as how we do it for tables in hive metastore.

SparkQA · 2020-01-06T05:04:52Z

Test build #116124 has finished for PR 26775 at commit e5d695e.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4

sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/SupportsNamespaces.java

sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/CreateNamespaceExec.scala

cloud-fan · 2020-01-09T07:49:18Z

sql/core/src/test/scala/org/apache/spark/sql/connector/DataSourceV2SQLSuite.scala

@@ -884,7 +885,8 @@ class DataSourceV2SQLSuite
            .isEmpty, s"$key is a reserved namespace property and ignored")
          val meta =
            catalog("testcat").asNamespaceCatalog.loadNamespaceMetadata(Array("reservedTest"))
-          assert(meta.get(key) === null, "reserved properties should not have side effects")
+          assert(!Option(meta.get(key)).exists(_.contains("foo")),


what's wrong with meta.get(key) === null?

this can be fixed if we revert the change in Create syntax

is it simply meta.get(key) != "foo"?

'location' might change the path from relative to absolute

meta.get(key) != "foo" still works, right?

yes, but once if the location goes wrong and have side effects != 'foo' is not able to check that.

how about meta.get(key) == null || !meta.get(key).contains("foo")?

cloud-fan · 2020-01-09T07:50:18Z

...ore/src/main/scala/org/apache/spark/sql/execution/datasources/v2/DescribeNamespaceExec.scala

-    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.getOrDefault(PROP_OWNER_NAME, ""))


we should skip printing owner if the properties don't exist

we should do the same for other fields.

yes, this fields are optional across different catalog implementations

SparkQA · 2020-01-09T08:05:02Z

Test build #116350 has finished for PR 26775 at commit 162fbf0.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-01-09T08:05:02Z

Test build #116351 has finished for PR 26775 at commit ae1ebec.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

cloud-fan · 2020-01-09T09:14:06Z

sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/SupportsNamespaces.java

+   * The list of reserved namespace properties, which can not be removed or changed directly by
+   * the syntax:
+   * {{
+   *   ALTER NAMESPACE ... SET PROPERTIES ...


do we support UNSET?

we have no ALTER NAMESPACE UNSET PROPERTIES yet

cloud-fan · 2020-01-09T09:16:24Z

sql/core/src/test/scala/org/apache/spark/sql/connector/DataSourceV2SQLSuite.scala

+  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'")


not related to this PR, does ANSI SQL define a OWNER clause for CREATE TABLE/NAMESPACE?

I checked the Standard and did not find such a clause for CREATE TABLE/NAMESPACE syntaxes

cloud-fan · 2020-01-09T09:18:30Z

sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveDDLSuite.scala

-      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')"))


does case sensitivity matter for reserved properties? what if users specify Ownername?

ok maybe it's fine to treat Ownername as a normal property.

It does not matter but I guess we should mention this

spark-sql> create namespace abcde with properties('LOCATION'= 'b'); 20/01/09 17:21:54 INFO HiveMetaStore: 0: get_database: global_temp 20/01/09 17:21:54 INFO audit: ugi=kentyao ip=unknown-ip-addr cmd=get_database: global_temp 20/01/09 17:21:54 WARN ObjectStore: Failed to get database global_temp, returning NoSuchObjectException 20/01/09 17:21:54 INFO HiveMetaStore: 0: create_database: Database(name:abcde, description:, locationUri:file:/Users/kentyao/Downloads/spark/spark-3.0.0-SNAPSHOT-bin-20200103/spark-warehouse/abcde.db, parameters:{LOCATION=b}, ownerName:kentyao, ownerType:USER) 20/01/09 17:21:54 INFO audit: ugi=kentyao ip=unknown-ip-addr cmd=create_database: Database(name:abcde, description:, locationUri:file:/Users/kentyao/Downloads/spark/spark-3.0.0-SNAPSHOT-bin-20200103/spark-warehouse/abcde.db, parameters:{LOCATION=b}, ownerName:kentyao, ownerType:USER) 20/01/09 17:21:54 WARN ObjectStore: Failed to get database abcde, returning NoSuchObjectException 20/01/09 17:21:54 INFO FileUtils: Creating directory if it doesn't exist: file:/Users/kentyao/Downloads/spark/spark-3.0.0-SNAPSHOT-bin-20200103/spark-warehouse/abcde.db Time taken: 1.891 seconds

spark-sql> create namespace abcdef with properties('location'= 'b'); 20/01/09 17:22:52 INFO HiveMetaStore: 0: create_database: Database(name:abcdef, description:, locationUri:file:/Users/kentyao/Downloads/spark/spark-3.0.0-SNAPSHOT-bin-20200103/b, parameters:{}, ownerName:kentyao, ownerType:USER) 20/01/09 17:22:52 INFO audit: ugi=kentyao ip=unknown-ip-addr cmd=create_database: Database(name:abcdef, description:, locationUri:file:/Users/kentyao/Downloads/spark/spark-3.0.0-SNAPSHOT-bin-20200103/b, parameters:{}, ownerName:kentyao, ownerType:USER) 20/01/09 17:22:52 WARN ObjectStore: Failed to get database abcdef, returning NoSuchObjectException 20/01/09 17:22:52 INFO FileUtils: Creating directory if it doesn't exist: file:/Users/kentyao/Downloads/spark/spark-3.0.0-SNAPSHOT-bin-20200103/b

spark-sql> desc namespace extended abcde; 20/01/09 17:24:14 INFO HiveMetaStore: 0: get_database: abcde 20/01/09 17:24:14 INFO audit: ugi=kentyao ip=unknown-ip-addr cmd=get_database: abcde 20/01/09 17:24:14 INFO HiveMetaStore: 0: get_database: abcde 20/01/09 17:24:14 INFO audit: ugi=kentyao ip=unknown-ip-addr cmd=get_database: abcde Database Name abcde Description Location file:/Users/kentyao/Downloads/spark/spark-3.0.0-SNAPSHOT-bin-20200103/spark-warehouse/abcde.db Owner Name kentyao Owner Type USER Properties ((LOCATION,b)) Time taken: 0.048 seconds, Fetched 6 row(s) 20/01/09 17:24:14 INFO SparkSQLCLIDriver: Time taken: 0.048 seconds, Fetched 6 row(s) spark-sql> desc namespace extended abcdef; 20/01/09 17:24:21 INFO HiveMetaStore: 0: get_database: abcdef 20/01/09 17:24:21 INFO audit: ugi=kentyao ip=unknown-ip-addr cmd=get_database: abcdef 20/01/09 17:24:21 INFO HiveMetaStore: 0: get_database: abcdef 20/01/09 17:24:21 INFO audit: ugi=kentyao ip=unknown-ip-addr cmd=get_database: abcdef Database Name abcdef Description Location file:/Users/kentyao/Downloads/spark/spark-3.0.0-SNAPSHOT-bin-20200103/b Owner Name kentyao Owner Type USER Properties Time taken: 0.016 seconds, Fetched 6 row(s)

SparkQA · 2020-01-09T10:16:33Z

Test build #116376 has finished for PR 26775 at commit bb2f919.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-01-09T12:02:49Z

Test build #116378 has finished for PR 26775 at commit 2182992.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-01-09T15:41:04Z

Test build #116388 has finished for PR 26775 at commit 514b78a.

This patch fails SparkR unit tests.
This patch merges cleanly.
This patch adds no public classes.

cloud-fan · 2020-01-09T15:47:21Z

retest this please

SparkQA · 2020-01-09T20:09:07Z

Test build #116406 has finished for PR 26775 at commit 514b78a.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-01-09T20:21:45Z

Test build #116404 has finished for PR 26775 at commit 514b78a.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

cloud-fan · 2020-01-10T08:47:24Z

thanks, merging to master!

yaooqinn added 2 commits December 6, 2019 14:07

[SPARK-30018][SQL] Support ALTER DATABASE SET OWNER syntax

3c96091

nit

7188587

cloud-fan reviewed Dec 6, 2019

View reviewed changes

sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4 Show resolved Hide resolved

cloud-fan reviewed Dec 6, 2019

View reviewed changes

sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4 Outdated Show resolved Hide resolved

cloud-fan reviewed Dec 6, 2019

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/statements.scala Outdated Show resolved Hide resolved

refine

56ac955

style

25cf227

dongjoon-hyun added the SQL label Dec 6, 2019

maropu reviewed Dec 9, 2019

View reviewed changes

Support define owner for datasource api

dd1d52b

cloud-fan reviewed Dec 9, 2019

View reviewed changes

rm unused lines

e5d695e

yaooqinn added 2 commits January 9, 2020 14:08

Merge branch 'master' into SPARK-30018

162fbf0

improve err msg

ae1ebec