[SPARK-30019][SQL] Add the owner property to v2 table by yaooqinn · Pull Request #27249 · apache/spark

yaooqinn · 2020-01-17T04:01:25Z

What changes were proposed in this pull request?

Add owner property to v2 table, it is reversed by TableCatalog, indicates the table's owner.

Why are the changes needed?

enhance ownership management of catalog API

Does this PR introduce any user-facing change?

yes, add 1 reserved property - owner , and it is not allowed to use in OPTIONS/TBLPROPERTIES anymore, only if legacy on

How was this patch tested?

add uts

...e/src/main/scala/org/apache/spark/sql/execution/datasources/v2/WriteToDataSourceV2Exec.scala

SparkQA · 2020-01-17T06:39:48Z

Test build #116892 has finished for PR 27249 at commit 30b91f5.

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

SparkQA · 2020-01-17T07:49:40Z

Test build #116898 has finished for PR 27249 at commit 0467b1e.

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

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

Test build #116914 has finished for PR 27249 at commit 4577e93.

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

SparkQA · 2020-01-17T11:35:15Z

Test build #116929 has finished for PR 27249 at commit 0b50979.

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

sql/hive/src/main/scala/org/apache/spark/sql/hive/client/HiveClientImpl.scala

cloud-fan · 2020-01-17T12:01:45Z

shall we also print owner name/type in DESC TABLE like DESC NAMESPACE?

yaooqinn · 2020-01-17T13:55:58Z

shall we also print owner name/type in DESC TABLE like DESC NAMESPACE?

The DescribeTableExec will show all reserved properties separately

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

Lines 52 to 56 in 16da714

    
           TableCatalog.RESERVED_PROPERTIES.asScala.toList.foreach(propKey => { 
        
             if (table.properties.containsKey(propKey)) { 
        
               rows += toCatalystRow(propKey.capitalize, table.properties.get(propKey), "") 
        
             } 
        
           })

cloud-fan · 2020-01-17T14:21:57Z

I mean, we should follow DESC NAMESPACE to implement it, e.g. display Owner Name: abc which looks nicer. Anyway we can do it in a followup.

cloud-fan · 2020-01-17T14:22:07Z

LGTM

SparkQA · 2020-01-17T14:26:26Z

Test build #116940 has finished for PR 27249 at commit 0fc2692.

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

SparkQA · 2020-01-17T17:57:12Z

Test build #116959 has finished for PR 27249 at commit 8e4f747.

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

SparkQA · 2020-01-17T20:04:14Z

Test build #116958 has finished for PR 27249 at commit f5512fb.

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

yaooqinn · 2020-01-19T02:27:25Z

retest this please

SparkQA · 2020-01-19T03:56:26Z

Test build #116986 has finished for PR 27249 at commit 8e4f747.

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

…r user

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

Test build #116991 has finished for PR 27249 at commit e618825.

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

yaooqinn · 2020-01-19T08:08:24Z

retest this please

SparkQA · 2020-01-19T10:21:31Z

Test build #116996 has finished for PR 27249 at commit e618825.

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

SparkQA · 2020-01-19T12:38:07Z

Test build #116999 has finished for PR 27249 at commit 46ae85b.

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

cloud-fan · 2020-01-20T08:25:17Z

docs/sql-migration-guide.md

+            owner
+          </td>
+          <td>
+            no


it's reserved for both table and namespace, or do I miss something?

nvm, we can update when revert ALTER NAMESPACE SET OWNER.

cloud-fan · 2020-01-20T08:26:32Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala

      case (PROP_LOCATION, _) => false
+      case (PROP_OWNER, _) if !legacyOn =>
+        throw new ParseException(s"$PROP_OWNER is a reserved table property , please use" +
+          " ALTER TABLE ... SET OWNER ... to specify it.", ctx)


it can't be changed after table is created.

cloud-fan · 2020-01-20T08:26:52Z

sql/core/src/main/scala/org/apache/spark/sql/execution/command/tables.scala

 import org.apache.spark.sql.catalyst.plans.DescribeTableSchema
 import org.apache.spark.sql.catalyst.plans.logical._
 import org.apache.spark.sql.catalyst.util.{escapeSingleQuotedString, quoteIdentifier, CaseInsensitiveMap}
+import org.apache.spark.sql.connector.catalog.TableCatalog


unnecessary change

cloud-fan · 2020-01-20T08:27:29Z

sql/core/src/test/scala/org/apache/spark/sql/ShowCreateTableSuite.scala


 import org.apache.spark.sql.catalyst.TableIdentifier
 import org.apache.spark.sql.catalyst.catalog.CatalogTable
+import org.apache.spark.sql.connector.catalog.TableCatalog


unnecessary

cloud-fan · 2020-01-20T08:29:01Z

sql/hive/src/main/scala/org/apache/spark/sql/hive/client/HiveClientImpl.scala

 import org.apache.spark.sql.catalyst.expressions.Expression
 import org.apache.spark.sql.catalyst.parser.{CatalystSqlParser, ParseException}
 import org.apache.spark.sql.connector.catalog.SupportsNamespaces._
+import org.apache.spark.sql.connector.catalog.TableCatalog


cloud-fan · 2020-01-20T08:30:27Z

can we add some tests to make sure CREATE/ALTER TABLE fails if setting owner property?

Also let's update the PR title. Now we are just adding an owner property to v2 table.

yaooqinn · 2020-01-20T09:17:11Z

can we add some tests to make sure CREATE/ALTER TABLE fails if setting owner property?

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

Lines 906 to 951 in 9bb78eb

    
           test("create/replace/alter table - reserved properties") { 
        
             import TableCatalog._ 
        
             withSQLConf((SQLConf.LEGACY_PROPERTY_NON_RESERVED.key, "false")) { 
        
               RESERVED_PROPERTIES.asScala.filterNot(_ == PROP_COMMENT).foreach { key => 
        
                 Seq("OPTIONS", "TBLPROPERTIES").foreach { clause => 
        
                   Seq("CREATE", "REPLACE").foreach { action => 
        
                     val e = intercept[ParseException] { 
        
                       sql(s"$action TABLE testcat.reservedTest (key int) USING foo $clause ('$key'='bar')") 
        
                     } 
        
                     assert(e.getMessage.contains(s"$key is a reserved table property")) 
        
                   } 
        
                 } 
        
                 val e1 = intercept[ParseException] { 
        
                   sql(s"ALTER TABLE testcat.reservedTest SET TBLPROPERTIES ('$key'='bar')") 
        
                 } 
        
                 assert(e1.getMessage.contains(s"$key is a reserved table property")) 
        
                 val e2 = intercept[ParseException] { 
        
                   sql(s"ALTER TABLE testcat.reservedTest UNSET TBLPROPERTIES ('$key')") 
        
                 } 
        
                 assert(e2.getMessage.contains(s"$key is a reserved table property")) 
        
               } 
        
             } 
        
             withSQLConf((SQLConf.LEGACY_PROPERTY_NON_RESERVED.key, "true")) { 
        
               RESERVED_PROPERTIES.asScala.filterNot(_ == PROP_COMMENT).foreach { key => 
        
                 Seq("OPTIONS", "TBLPROPERTIES").foreach { clause => 
        
                   withTable("testcat.reservedTest") { 
        
                     Seq("CREATE", "REPLACE").foreach { action => 
        
                       sql(s"$action TABLE testcat.reservedTest (key int) USING foo $clause ('$key'='bar')") 
        
                       val tableCatalog = catalog("testcat").asTableCatalog 
        
                       val identifier = Identifier.of(Array(), "reservedTest") 
        
                       val originValue = tableCatalog.loadTable(identifier).properties().get(key) 
        
                       assert(originValue != "bar", "reserved properties should not have side effects") 
        
                       sql(s"ALTER TABLE testcat.reservedTest SET TBLPROPERTIES ('$key'='newValue')") 
        
                       assert(tableCatalog.loadTable(identifier).properties().get(key) == originValue, 
        
                         "reserved properties should not have side effects") 
        
                       sql(s"ALTER TABLE testcat.reservedTest UNSET TBLPROPERTIES ('$key')") 
        
                       assert(tableCatalog.loadTable(identifier).properties().get(key) == originValue, 
        
                         "reserved properties should not have side effects") 
        
                     } 
        
                   } 
        
                 } 
        
               } 
        
             } 
        
           }

We have a general test for reserved ones

Also let's update the PR title. Now we are just adding an owner property to v2 table.

OK

SparkQA · 2020-01-20T13:37:11Z

Test build #117109 has finished for PR 27249 at commit 8e70ec1.

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

SparkQA · 2020-01-20T13:49:19Z

Test build #117106 has finished for PR 27249 at commit ce352a9.

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

cloud-fan · 2020-01-20T13:52:13Z

sql/core/src/test/scala/org/apache/spark/sql/execution/command/DDLSuite.scala

      } else {
        catalog.getTableMetadata(tableIdent).properties
      }
+      props -- TableCatalog.RESERVED_PROPERTIES.asScala


do we need this change?

cloud-fan · 2020-01-20T13:55:01Z

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

+      assert(sql(s"DESCRIBE TABLE EXTENDED spark_30019").where("col_name='Owner'")
+        .collect().head.getString(1) === Utils.getCurrentUserName())
+      val e2 = intercept[ParseException](
+        sql(s"CREATE TABLE spark_30019_2 WITH TBLPROPERTIES('owner'='spark_30019')"))


do we need it? I think we already have CREATE/REPLACE/ALTER TABLE tests for reserved properties.

you mean this whole test or this line?

cloud-fan · 2020-01-20T13:58:58Z

LGTM except a few minor comments

SparkQA · 2020-01-20T15:04:36Z

Test build #117100 has finished for PR 27249 at commit 9bb78eb.

This patch fails from timeout after a configured wait of 400m.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-01-20T18:31:29Z

Test build #117120 has finished for PR 27249 at commit 97f07b8.

This patch passes all tests.
This patch does not merge cleanly.
This patch adds no public classes.

SparkQA · 2020-01-20T20:00:15Z

Test build #117127 has finished for PR 27249 at commit 6b48061.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class UnresolvedTableWithViewExists(view: ResolvedView) extends LeafNode
case class ResolvedView(identifier: Identifier, isTempView: Boolean) extends LeafNode
abstract class AlterTable extends Command
case class AlterTableAddColumns(
case class AlterTableAlterColumn(
case class AlterTableRenameColumn(
case class AlterTableDropColumns(
case class AlterTableSetProperties(
case class AlterTableUnsetProperties(
case class AlterTableSetLocation(

cloud-fan · 2020-01-21T02:38:05Z

thanks, merging to master!

yaooqinn added 3 commits January 16, 2020 23:32

tmp

6708db6

[SPARK-30019][SQL] Add ALTER TABLE SET OWNER syntax

30b91f5

nit

e9ade41

yaooqinn requested a review from cloud-fan January 17, 2020 05:39

doc

0467b1e

cloud-fan reviewed Jan 17, 2020

View reviewed changes

...e/src/main/scala/org/apache/spark/sql/execution/datasources/v2/WriteToDataSourceV2Exec.scala Outdated Show resolved Hide resolved

fix tests

4577e93

use planner to decide ownership

0b50979

fix DataFrameWriterV2Suite

0fc2692

cloud-fan reviewed Jan 17, 2020

View reviewed changes

sql/hive/src/main/scala/org/apache/spark/sql/hive/client/HiveClientImpl.scala Outdated Show resolved Hide resolved

yaooqinn added 2 commits January 18, 2020 01:01

restore ownerType for show create table

f5512fb

nit

8e4f747

if owner is empty, let it be sparkUser instead of hive's authenticato…

e618825

…r user

nit

46ae85b

yaooqinn added 2 commits January 20, 2020 15:20

Merge branch 'master' into SPARK-30019

c04082b

revert alter table set owner

9bb78eb

cloud-fan reviewed Jan 20, 2020

View reviewed changes

nit

ce352a9

yaooqinn changed the title ~~[SPARK-30019][SQL] Add ALTER TABLE SET OWNER syntax~~ [SPARK-30019][SQL] Add the owner property to v2 table Jan 20, 2020

nit

8e70ec1

cloud-fan reviewed Jan 20, 2020

View reviewed changes

nit

97f07b8

Merge branch 'master' into SPARK-30019

6b48061

cloud-fan closed this in 24efa43 Jan 21, 2020

dongjoon-hyun added the SQL label Feb 5, 2020

Comments

Conversation

yaooqinn commented Jan 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

Uh oh!

SparkQA commented Jan 17, 2020

Uh oh!

SparkQA commented Jan 17, 2020

Uh oh!

SparkQA commented Jan 17, 2020

Uh oh!

SparkQA commented Jan 17, 2020

Uh oh!

Uh oh!

cloud-fan commented Jan 17, 2020

Uh oh!

yaooqinn commented Jan 17, 2020

Uh oh!

cloud-fan commented Jan 17, 2020

Uh oh!

cloud-fan commented Jan 17, 2020

Uh oh!

SparkQA commented Jan 17, 2020

Uh oh!

SparkQA commented Jan 17, 2020

Uh oh!

SparkQA commented Jan 17, 2020

Uh oh!

yaooqinn commented Jan 19, 2020

Uh oh!

SparkQA commented Jan 19, 2020

Uh oh!

SparkQA commented Jan 19, 2020

Uh oh!

yaooqinn commented Jan 19, 2020

Uh oh!

SparkQA commented Jan 19, 2020

Uh oh!

SparkQA commented Jan 19, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cloud-fan commented Jan 20, 2020

Uh oh!

yaooqinn commented Jan 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SparkQA commented Jan 20, 2020

Uh oh!

SparkQA commented Jan 20, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cloud-fan Jan 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cloud-fan commented Jan 20, 2020

Uh oh!

SparkQA commented Jan 20, 2020

Uh oh!

SparkQA commented Jan 20, 2020

Uh oh!

yaooqinn commented Jan 17, 2020 •

edited

Loading

yaooqinn commented Jan 20, 2020 •

edited

Loading

cloud-fan Jan 20, 2020 •

edited

Loading