From 4425aab435258c3e7c9b223ad6ff50f9d9a383bd Mon Sep 17 00:00:00 2001 From: Terry Kim Date: Wed, 8 Dec 2021 11:09:42 -0800 Subject: [PATCH 1/8] initial commit --- .../sql/catalyst/parser/DDLParserSuite.scala | 32 --------------- ...terNamespaceSetPropertiesParserSuite.scala | 40 +++++++++++++++++++ ...AlterNamespaceSetPropertiesSuiteBase.scala | 5 +++ 3 files changed, 45 insertions(+), 32 deletions(-) create mode 100644 sql/core/src/test/scala/org/apache/spark/sql/execution/command/AlterNamespaceSetPropertiesParserSuite.scala create mode 100644 sql/core/src/test/scala/org/apache/spark/sql/execution/command/AlterNamespaceSetPropertiesSuiteBase.scala 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 b92be01f9a3cc..9354614c41213 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 @@ -1770,38 +1770,6 @@ class DDLParserSuite extends AnalysisTest { "location" -> "/home/user/db"))) } - test("set namespace properties") { - comparePlans( - parsePlan("ALTER DATABASE a.b.c SET PROPERTIES ('a'='a', 'b'='b', 'c'='c')"), - SetNamespaceProperties( - UnresolvedNamespace(Seq("a", "b", "c")), Map("a" -> "a", "b" -> "b", "c" -> "c"))) - - comparePlans( - parsePlan("ALTER SCHEMA a.b.c SET PROPERTIES ('a'='a')"), - SetNamespaceProperties( - UnresolvedNamespace(Seq("a", "b", "c")), Map("a" -> "a"))) - - comparePlans( - parsePlan("ALTER NAMESPACE a.b.c SET PROPERTIES ('b'='b')"), - SetNamespaceProperties( - UnresolvedNamespace(Seq("a", "b", "c")), Map("b" -> "b"))) - - comparePlans( - parsePlan("ALTER DATABASE a.b.c SET DBPROPERTIES ('a'='a', 'b'='b', 'c'='c')"), - SetNamespaceProperties( - UnresolvedNamespace(Seq("a", "b", "c")), Map("a" -> "a", "b" -> "b", "c" -> "c"))) - - comparePlans( - parsePlan("ALTER SCHEMA a.b.c SET DBPROPERTIES ('a'='a')"), - SetNamespaceProperties( - UnresolvedNamespace(Seq("a", "b", "c")), Map("a" -> "a"))) - - comparePlans( - parsePlan("ALTER NAMESPACE a.b.c SET DBPROPERTIES ('b'='b')"), - SetNamespaceProperties( - UnresolvedNamespace(Seq("a", "b", "c")), Map("b" -> "b"))) - } - test("analyze table statistics") { comparePlans(parsePlan("analyze table a.b.c compute statistics"), AnalyzeTable( diff --git a/sql/core/src/test/scala/org/apache/spark/sql/execution/command/AlterNamespaceSetPropertiesParserSuite.scala b/sql/core/src/test/scala/org/apache/spark/sql/execution/command/AlterNamespaceSetPropertiesParserSuite.scala new file mode 100644 index 0000000000000..7b13830eb04bb --- /dev/null +++ b/sql/core/src/test/scala/org/apache/spark/sql/execution/command/AlterNamespaceSetPropertiesParserSuite.scala @@ -0,0 +1,40 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.spark.sql.execution.command + +import org.apache.spark.sql.catalyst.analysis.{AnalysisTest, UnresolvedNamespace} +import org.apache.spark.sql.catalyst.parser.CatalystSqlParser.parsePlan +import org.apache.spark.sql.catalyst.plans.logical.SetNamespaceProperties + +class AlterNamespaceSetPropertiesParserSuite extends AnalysisTest { + test("set namespace properties") { + Seq("DATABASE", "SCHEMA", "NAMESPACE").foreach { nsToken => + Seq("PROPERTIES", "DBPROPERTIES").foreach { propToken => + comparePlans( + parsePlan(s"ALTER $nsToken a.b.c SET $propToken ('a'='a', 'b'='b', 'c'='c')"), + SetNamespaceProperties( + UnresolvedNamespace(Seq("a", "b", "c")), Map("a" -> "a", "b" -> "b", "c" -> "c"))) + + comparePlans( + parsePlan(s"ALTER $nsToken a.b.c SET $propToken ('a'='a')"), + SetNamespaceProperties( + UnresolvedNamespace(Seq("a", "b", "c")), Map("a" -> "a"))) + } + } + } +} diff --git a/sql/core/src/test/scala/org/apache/spark/sql/execution/command/AlterNamespaceSetPropertiesSuiteBase.scala b/sql/core/src/test/scala/org/apache/spark/sql/execution/command/AlterNamespaceSetPropertiesSuiteBase.scala new file mode 100644 index 0000000000000..3307b24716eff --- /dev/null +++ b/sql/core/src/test/scala/org/apache/spark/sql/execution/command/AlterNamespaceSetPropertiesSuiteBase.scala @@ -0,0 +1,5 @@ +package org.apache.spark.sql.execution.command + +class AlterNamespaceSetPropertiesSuiteBase { + +} From ebba2b9d7881b7a830e2e01fc95015ea853a857b Mon Sep 17 00:00:00 2001 From: Terry Kim Date: Wed, 8 Dec 2021 11:51:23 -0800 Subject: [PATCH 2/8] more changes --- ...terNamespaceSetPropertiesParserSuite.scala | 6 ++ ...AlterNamespaceSetPropertiesSuiteBase.scala | 67 ++++++++++++++++++- .../execution/command/DDLParserSuite.scala | 6 -- .../sql/execution/command/DDLSuite.scala | 30 +-------- 4 files changed, 74 insertions(+), 35 deletions(-) diff --git a/sql/core/src/test/scala/org/apache/spark/sql/execution/command/AlterNamespaceSetPropertiesParserSuite.scala b/sql/core/src/test/scala/org/apache/spark/sql/execution/command/AlterNamespaceSetPropertiesParserSuite.scala index 7b13830eb04bb..4bac1627e67a8 100644 --- a/sql/core/src/test/scala/org/apache/spark/sql/execution/command/AlterNamespaceSetPropertiesParserSuite.scala +++ b/sql/core/src/test/scala/org/apache/spark/sql/execution/command/AlterNamespaceSetPropertiesParserSuite.scala @@ -37,4 +37,10 @@ class AlterNamespaceSetPropertiesParserSuite extends AnalysisTest { } } } + + test("property values must be set") { + assertUnsupported( + sql = "ALTER NAMESPACE my_db SET PROPERTIES('key_without_value', 'key_with_value'='x')", + containsThesePhrases = Seq("key_without_value")) + } } diff --git a/sql/core/src/test/scala/org/apache/spark/sql/execution/command/AlterNamespaceSetPropertiesSuiteBase.scala b/sql/core/src/test/scala/org/apache/spark/sql/execution/command/AlterNamespaceSetPropertiesSuiteBase.scala index 3307b24716eff..4e1a7c9cc3fe2 100644 --- a/sql/core/src/test/scala/org/apache/spark/sql/execution/command/AlterNamespaceSetPropertiesSuiteBase.scala +++ b/sql/core/src/test/scala/org/apache/spark/sql/execution/command/AlterNamespaceSetPropertiesSuiteBase.scala @@ -1,5 +1,70 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + package org.apache.spark.sql.execution.command -class AlterNamespaceSetPropertiesSuiteBase { +import org.apache.spark.sql.{AnalysisException, QueryTest} + +/** + * This base suite contains unified tests for the `ALTER NAMESPACE ... SET PROPERTIES` command that + * check V1 and V2 table catalogs. The tests that cannot run for all supported catalogs are located + * in more specific test suites: + * + * - V2 table catalog tests: + * `org.apache.spark.sql.execution.command.v2.AlterNamespaceSetPropertiesSuite` + * - V1 table catalog tests: + * `org.apache.spark.sql.execution.command.v1.AlterNamespaceSetPropertiesSuiteBase` + * - V1 In-Memory catalog: + * `org.apache.spark.sql.execution.command.v1.AlterNamespaceSetPropertiesSuite` + * - V1 Hive External catalog: + * `org.apache.spark.sql.hive.execution.command.AlterNamespaceSetPropertiesSuite` + */ +trait AlterNamespaceSetPropertiesSuiteBase extends QueryTest with DDLCommandTestUtils { + override val command = "ALTER NAMESPACE ... SET PROPERTIES" + + protected def namespace: String + + protected def notFoundMsgPrefix: String + + test("Namespace does not exist") { + val ns = "not_exist" + val message = intercept[AnalysisException] { + sql(s"ALTER DATABASE $catalog.$ns SET PROPERTIES ('d'='d')") + }.getMessage + assert(message.contains(s"$notFoundMsgPrefix '$ns' not found")) + } + + test("basic test") { + val ns = s"$catalog.$namespace" + withNamespace(ns) { + sql(s"CREATE NAMESPACE $ns") + sql(s"ALTER NAMESPACE $ns SET PROPERTIES ('a'='a', 'b'='b', 'c'='c')") + assert(getProperties(ns) === "((a,a), (b,b), (c,c))") + sql(s"ALTER DATABASE $ns SET PROPERTIES ('d'='d')") + assert(getProperties(ns) === "((a,a), (b,b), (c,c), (d,d))") + } + } + protected def getProperties(namespace: String): String = { + val locationRow = sql(s"DESCRIBE NAMESPACE EXTENDED $namespace") + .toDF("key", "value") + .where(s"key like 'Properties%'") + .collect() + assert(locationRow.length == 1) + locationRow(0).getString(1) + } } diff --git a/sql/core/src/test/scala/org/apache/spark/sql/execution/command/DDLParserSuite.scala b/sql/core/src/test/scala/org/apache/spark/sql/execution/command/DDLParserSuite.scala index b935c8b93a5b1..44d68584a0cba 100644 --- a/sql/core/src/test/scala/org/apache/spark/sql/execution/command/DDLParserSuite.scala +++ b/sql/core/src/test/scala/org/apache/spark/sql/execution/command/DDLParserSuite.scala @@ -59,12 +59,6 @@ class DDLParserSuite extends AnalysisTest with SharedSparkSession { ShowCurrentNamespaceCommand()) } - test("alter database - property values must be set") { - assertUnsupported( - sql = "ALTER DATABASE my_db SET DBPROPERTIES('key_without_value', 'key_with_value'='x')", - containsThesePhrases = Seq("key_without_value")) - } - test("insert overwrite directory") { val v1 = "INSERT OVERWRITE DIRECTORY '/tmp/file' USING parquet SELECT 1 as a" parser.parsePlan(v1) match { diff --git a/sql/core/src/test/scala/org/apache/spark/sql/execution/command/DDLSuite.scala b/sql/core/src/test/scala/org/apache/spark/sql/execution/command/DDLSuite.scala index 1cb7868a78197..56de04591992b 100644 --- a/sql/core/src/test/scala/org/apache/spark/sql/execution/command/DDLSuite.scala +++ b/sql/core/src/test/scala/org/apache/spark/sql/execution/command/DDLSuite.scala @@ -751,7 +751,7 @@ abstract class DDLSuite extends QueryTest with SQLTestUtils { } } - test("Alter/Describe Database") { + test("Describe Database") { val catalog = spark.sessionState.catalog val databaseNames = Seq("db1", "`database`") @@ -760,9 +760,7 @@ abstract class DDLSuite extends QueryTest with SQLTestUtils { val dbNameWithoutBackTicks = cleanIdentifier(dbName) val location = getDBPath(dbNameWithoutBackTicks) - sql(s"CREATE DATABASE $dbName") - - sql(s"ALTER DATABASE $dbName SET DBPROPERTIES ('a'='a', 'b'='b', 'c'='c')") + sql(s"CREATE DATABASE $dbName WITH PROPERTIES ('a'='a', 'b'='b', 'c'='c')") checkAnswer( sql(s"DESCRIBE DATABASE EXTENDED $dbName").toDF("key", "value") @@ -771,36 +769,12 @@ abstract class DDLSuite extends QueryTest with SQLTestUtils { Row("Comment", "") :: Row("Location", CatalogUtils.URIToString(location)) :: Row("Properties", "((a,a), (b,b), (c,c))") :: Nil) - - sql(s"ALTER DATABASE $dbName SET DBPROPERTIES ('d'='d')") - - checkAnswer( - sql(s"DESCRIBE DATABASE EXTENDED $dbName").toDF("key", "value") - .where("key not like 'Owner%'"), // filter for consistency with in-memory catalog - Row("Namespace Name", dbNameWithoutBackTicks) :: - Row("Comment", "") :: - Row("Location", CatalogUtils.URIToString(location)) :: - Row("Properties", "((a,a), (b,b), (c,c), (d,d))") :: Nil) } finally { catalog.reset() } } } - test("Alter Database - database does not exists") { - val databaseNames = Seq("db1", "`database`") - - databaseNames.foreach { dbName => - val dbNameWithoutBackTicks = cleanIdentifier(dbName) - assert(!spark.sessionState.catalog.databaseExists(dbNameWithoutBackTicks)) - - val message = intercept[AnalysisException] { - sql(s"ALTER DATABASE $dbName SET DBPROPERTIES ('d'='d')") - }.getMessage - assert(message.contains(s"Database '$dbNameWithoutBackTicks' not found")) - } - } - test("create table in default db") { val catalog = spark.sessionState.catalog val tableIdent1 = TableIdentifier("tab1", None) From dee8d6185f65d02db1d493f1b030746b5ba4988b Mon Sep 17 00:00:00 2001 From: Terry Kim Date: Wed, 8 Dec 2021 17:36:39 -0800 Subject: [PATCH 3/8] add missing files --- .../v1/AlterNamespaceSetPropertiesSuite.scala | 49 +++++++++++++++++++ .../v2/AlterNamespaceSetPropertiesSuite.scala | 34 +++++++++++++ 2 files changed, 83 insertions(+) create mode 100644 sql/core/src/test/scala/org/apache/spark/sql/execution/command/v1/AlterNamespaceSetPropertiesSuite.scala create mode 100644 sql/core/src/test/scala/org/apache/spark/sql/execution/command/v2/AlterNamespaceSetPropertiesSuite.scala diff --git a/sql/core/src/test/scala/org/apache/spark/sql/execution/command/v1/AlterNamespaceSetPropertiesSuite.scala b/sql/core/src/test/scala/org/apache/spark/sql/execution/command/v1/AlterNamespaceSetPropertiesSuite.scala new file mode 100644 index 0000000000000..81097314e0174 --- /dev/null +++ b/sql/core/src/test/scala/org/apache/spark/sql/execution/command/v1/AlterNamespaceSetPropertiesSuite.scala @@ -0,0 +1,49 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.spark.sql.execution.command.v1 + +import org.apache.spark.sql.execution.command + +/** + * This base suite contains unified tests for the `ALTER NAMESPACE ... SET PROPERTIES` command that + * checks V1 table catalogs. The tests that cannot run for all V1 catalogs are located in more + * specific test suites: + * + * - V1 In-Memory catalog: + * `org.apache.spark.sql.execution.command.v1.AlterNamespaceSetPropertiesSuite` + * - V1 Hive External catalog: + * `org.apache.spark.sql.hive.execution.command.AlterNamespaceSetPropertiesSuite` + */ +trait AlterNamespaceSetPropertiesSuiteBase extends command.AlterNamespaceSetPropertiesSuiteBase + with command.TestsV1AndV2Commands { + override def namespace: String = "db" + override def notFoundMsgPrefix: String = "Database" +} + +/** + * The class contains tests for the `ALTER NAMESPACE ... SET PROPERTIES` command to + * check V1 In-Memory table catalog. + */ +class AlterNamespaceSetPropertiesSuite extends AlterNamespaceSetPropertiesSuiteBase + with CommandSuiteBase { + override def commandVersion: String = super[AlterNamespaceSetPropertiesSuiteBase].commandVersion + + test("basic test") { + runBasicTest() + } +} diff --git a/sql/core/src/test/scala/org/apache/spark/sql/execution/command/v2/AlterNamespaceSetPropertiesSuite.scala b/sql/core/src/test/scala/org/apache/spark/sql/execution/command/v2/AlterNamespaceSetPropertiesSuite.scala new file mode 100644 index 0000000000000..e87dda9264dd6 --- /dev/null +++ b/sql/core/src/test/scala/org/apache/spark/sql/execution/command/v2/AlterNamespaceSetPropertiesSuite.scala @@ -0,0 +1,34 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.spark.sql.execution.command.v2 + +import org.apache.spark.sql.execution.command + +/** + * The class contains tests for the `ALTER NAMESPACE ... SET PROPERTIES` command to check V2 table + * catalogs. + */ +class AlterNamespaceSetPropertiesSuite extends command.AlterNamespaceSetPropertiesSuiteBase + with CommandSuiteBase { + override def namespace: String = "ns1.ns2" + override def notFoundMsgPrefix: String = "Namespace" + + test("basic test") { + runBasicTest() + } +} From 54d6f506e8be662f3bdb889aba074476edb48f83 Mon Sep 17 00:00:00 2001 From: Terry Kim Date: Wed, 8 Dec 2021 19:24:26 -0800 Subject: [PATCH 4/8] more tests --- .../sql/connector/DataSourceV2SQLSuite.scala | 48 ------------------- ...terNamespaceSetPropertiesParserSuite.scala | 9 ++-- ...AlterNamespaceSetPropertiesSuiteBase.scala | 48 +++++++++++++++++++ .../v1/AlterNamespaceSetPropertiesSuite.scala | 4 -- .../v2/AlterNamespaceSetPropertiesSuite.scala | 4 -- 5 files changed, 54 insertions(+), 59 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 44d79541d6c60..7187287d55efd 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 @@ -1224,54 +1224,6 @@ class DataSourceV2SQLSuite } } - test("ALTER NAMESPACE .. SET PROPERTIES using v2 catalog") { - withNamespace("testcat.ns1.ns2") { - sql("CREATE NAMESPACE IF NOT EXISTS testcat.ns1.ns2 COMMENT " + - "'test namespace' LOCATION '/tmp/ns_test' WITH PROPERTIES ('a'='a','b'='b','c'='c')") - sql("ALTER NAMESPACE testcat.ns1.ns2 SET PROPERTIES ('a'='b','b'='a')") - val descriptionDf = sql("DESCRIBE NAMESPACE EXTENDED testcat.ns1.ns2") - assert(descriptionDf.collect() === Seq( - Row("Namespace Name", "ns2"), - Row(SupportsNamespaces.PROP_COMMENT.capitalize, "test namespace"), - Row(SupportsNamespaces.PROP_LOCATION.capitalize, "file:/tmp/ns_test"), - Row(SupportsNamespaces.PROP_OWNER.capitalize, defaultUser), - Row("Properties", "((a,b), (b,a), (c,c))")) - ) - } - } - - test("ALTER NAMESPACE .. SET PROPERTIES reserved properties") { - import SupportsNamespaces._ - withSQLConf((SQLConf.LEGACY_PROPERTY_NON_RESERVED.key, "false")) { - CatalogV2Util.NAMESPACE_RESERVED_PROPERTIES.filterNot(_ == PROP_COMMENT).foreach { key => - withNamespace("testcat.reservedTest") { - sql("CREATE NAMESPACE testcat.reservedTest") - val exception = intercept[ParseException] { - sql(s"ALTER NAMESPACE testcat.reservedTest SET PROPERTIES ('$key'='dummyVal')") - } - assert(exception.getMessage.contains(s"$key is a reserved namespace property")) - } - } - } - withSQLConf((SQLConf.LEGACY_PROPERTY_NON_RESERVED.key, "true")) { - CatalogV2Util.NAMESPACE_RESERVED_PROPERTIES.filterNot(_ == PROP_COMMENT).foreach { key => - withNamespace("testcat.reservedTest") { - sql(s"CREATE NAMESPACE testcat.reservedTest") - sql(s"ALTER NAMESPACE testcat.reservedTest SET PROPERTIES ('$key'='foo')") - assert(sql("DESC NAMESPACE EXTENDED testcat.reservedTest") - .toDF("k", "v") - .where("k='Properties'") - .where("v=''") - .count == 1, s"$key is a reserved namespace property and ignored") - val meta = - catalog("testcat").asNamespaceCatalog.loadNamespaceMetadata(Array("reservedTest")) - assert(meta.get(key) == null || !meta.get(key).contains("foo"), - "reserved properties should not have side effects") - } - } - } - } - test("ALTER NAMESPACE .. SET LOCATION using v2 catalog") { withNamespace("testcat.ns1.ns2") { sql("CREATE NAMESPACE IF NOT EXISTS testcat.ns1.ns2 COMMENT " + diff --git a/sql/core/src/test/scala/org/apache/spark/sql/execution/command/AlterNamespaceSetPropertiesParserSuite.scala b/sql/core/src/test/scala/org/apache/spark/sql/execution/command/AlterNamespaceSetPropertiesParserSuite.scala index 4bac1627e67a8..868dc275b8a4d 100644 --- a/sql/core/src/test/scala/org/apache/spark/sql/execution/command/AlterNamespaceSetPropertiesParserSuite.scala +++ b/sql/core/src/test/scala/org/apache/spark/sql/execution/command/AlterNamespaceSetPropertiesParserSuite.scala @@ -19,6 +19,7 @@ package org.apache.spark.sql.execution.command import org.apache.spark.sql.catalyst.analysis.{AnalysisTest, UnresolvedNamespace} import org.apache.spark.sql.catalyst.parser.CatalystSqlParser.parsePlan +import org.apache.spark.sql.catalyst.parser.ParseException import org.apache.spark.sql.catalyst.plans.logical.SetNamespaceProperties class AlterNamespaceSetPropertiesParserSuite extends AnalysisTest { @@ -39,8 +40,10 @@ class AlterNamespaceSetPropertiesParserSuite extends AnalysisTest { } test("property values must be set") { - assertUnsupported( - sql = "ALTER NAMESPACE my_db SET PROPERTIES('key_without_value', 'key_with_value'='x')", - containsThesePhrases = Seq("key_without_value")) + val e = intercept[ParseException] { + parsePlan("ALTER NAMESPACE my_db SET PROPERTIES('key_without_value', 'key_with_value'='x')") + } + assert(e.getMessage.contains( + "Operation not allowed: Values must be specified for key(s): [key_without_value]")) } } diff --git a/sql/core/src/test/scala/org/apache/spark/sql/execution/command/AlterNamespaceSetPropertiesSuiteBase.scala b/sql/core/src/test/scala/org/apache/spark/sql/execution/command/AlterNamespaceSetPropertiesSuiteBase.scala index 4e1a7c9cc3fe2..5ceaf8ded3139 100644 --- a/sql/core/src/test/scala/org/apache/spark/sql/execution/command/AlterNamespaceSetPropertiesSuiteBase.scala +++ b/sql/core/src/test/scala/org/apache/spark/sql/execution/command/AlterNamespaceSetPropertiesSuiteBase.scala @@ -18,6 +18,9 @@ package org.apache.spark.sql.execution.command import org.apache.spark.sql.{AnalysisException, QueryTest} +import org.apache.spark.sql.catalyst.parser.ParseException +import org.apache.spark.sql.connector.catalog.{CatalogV2Util, SupportsNamespaces} +import org.apache.spark.sql.internal.SQLConf /** * This base suite contains unified tests for the `ALTER NAMESPACE ... SET PROPERTIES` command that @@ -34,6 +37,8 @@ import org.apache.spark.sql.{AnalysisException, QueryTest} * `org.apache.spark.sql.hive.execution.command.AlterNamespaceSetPropertiesSuite` */ trait AlterNamespaceSetPropertiesSuiteBase extends QueryTest with DDLCommandTestUtils { + import org.apache.spark.sql.connector.catalog.CatalogV2Implicits._ + override val command = "ALTER NAMESPACE ... SET PROPERTIES" protected def namespace: String @@ -59,6 +64,49 @@ trait AlterNamespaceSetPropertiesSuiteBase extends QueryTest with DDLCommandTest } } + test("test with properties set while creating namespace") { + val ns = s"$catalog.$namespace" + withNamespace(ns) { + sql(s"CREATE NAMESPACE $ns WITH PROPERTIES ('a'='a','b'='b','c'='c')") + assert(getProperties(ns) === "((a,a), (b,b), (c,c))") + sql(s"ALTER NAMESPACE $ns SET PROPERTIES ('a'='b', 'b'='a')") + assert(getProperties(ns) === "((a,b), (b,a), (c,c))") + } + } + + test("test reserved properties") { + val ns = s"$catalog.$namespace" + import SupportsNamespaces._ + withSQLConf((SQLConf.LEGACY_PROPERTY_NON_RESERVED.key, "false")) { + CatalogV2Util.NAMESPACE_RESERVED_PROPERTIES.filterNot(_ == PROP_COMMENT).foreach { key => + withNamespace(ns) { + sql(s"CREATE NAMESPACE $ns") + val exception = intercept[ParseException] { + sql(s"ALTER NAMESPACE $ns SET PROPERTIES ('$key'='dummyVal')") + } + assert(exception.getMessage.contains(s"$key is a reserved namespace property")) + } + } + } + withSQLConf((SQLConf.LEGACY_PROPERTY_NON_RESERVED.key, "true")) { + CatalogV2Util.NAMESPACE_RESERVED_PROPERTIES.filterNot(_ == PROP_COMMENT).foreach { key => + withNamespace(ns) { + sql(s"CREATE NAMESPACE $ns") + sql(s"ALTER NAMESPACE $ns SET PROPERTIES ('$key'='foo')") + assert(sql(s"DESC NAMESPACE EXTENDED $ns") + .toDF("k", "v") + .where("k='Properties'") + .where("v=''") + .count == 1, s"$key is a reserved namespace property and ignored") + val meta = spark.sessionState.catalogManager.catalog(catalog) + .asNamespaceCatalog.loadNamespaceMetadata(Array(namespace)) + assert(meta.get(key) == null || !meta.get(key).contains("foo"), + "reserved properties should not have side effects") + } + } + } + } + protected def getProperties(namespace: String): String = { val locationRow = sql(s"DESCRIBE NAMESPACE EXTENDED $namespace") .toDF("key", "value") diff --git a/sql/core/src/test/scala/org/apache/spark/sql/execution/command/v1/AlterNamespaceSetPropertiesSuite.scala b/sql/core/src/test/scala/org/apache/spark/sql/execution/command/v1/AlterNamespaceSetPropertiesSuite.scala index 81097314e0174..cb89db3181f31 100644 --- a/sql/core/src/test/scala/org/apache/spark/sql/execution/command/v1/AlterNamespaceSetPropertiesSuite.scala +++ b/sql/core/src/test/scala/org/apache/spark/sql/execution/command/v1/AlterNamespaceSetPropertiesSuite.scala @@ -42,8 +42,4 @@ trait AlterNamespaceSetPropertiesSuiteBase extends command.AlterNamespaceSetProp class AlterNamespaceSetPropertiesSuite extends AlterNamespaceSetPropertiesSuiteBase with CommandSuiteBase { override def commandVersion: String = super[AlterNamespaceSetPropertiesSuiteBase].commandVersion - - test("basic test") { - runBasicTest() - } } diff --git a/sql/core/src/test/scala/org/apache/spark/sql/execution/command/v2/AlterNamespaceSetPropertiesSuite.scala b/sql/core/src/test/scala/org/apache/spark/sql/execution/command/v2/AlterNamespaceSetPropertiesSuite.scala index e87dda9264dd6..b5b352a7e7c4c 100644 --- a/sql/core/src/test/scala/org/apache/spark/sql/execution/command/v2/AlterNamespaceSetPropertiesSuite.scala +++ b/sql/core/src/test/scala/org/apache/spark/sql/execution/command/v2/AlterNamespaceSetPropertiesSuite.scala @@ -27,8 +27,4 @@ class AlterNamespaceSetPropertiesSuite extends command.AlterNamespaceSetProperti with CommandSuiteBase { override def namespace: String = "ns1.ns2" override def notFoundMsgPrefix: String = "Namespace" - - test("basic test") { - runBasicTest() - } } From 4ef927ba3e19b57b4d014e9a431e47eaf05077a5 Mon Sep 17 00:00:00 2001 From: Terry Kim Date: Wed, 8 Dec 2021 19:37:11 -0800 Subject: [PATCH 5/8] add more testing --- .../command/AlterNamespaceSetPropertiesSuiteBase.scala | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/sql/core/src/test/scala/org/apache/spark/sql/execution/command/AlterNamespaceSetPropertiesSuiteBase.scala b/sql/core/src/test/scala/org/apache/spark/sql/execution/command/AlterNamespaceSetPropertiesSuiteBase.scala index 5ceaf8ded3139..56fd59eaaf86a 100644 --- a/sql/core/src/test/scala/org/apache/spark/sql/execution/command/AlterNamespaceSetPropertiesSuiteBase.scala +++ b/sql/core/src/test/scala/org/apache/spark/sql/execution/command/AlterNamespaceSetPropertiesSuiteBase.scala @@ -57,6 +57,7 @@ trait AlterNamespaceSetPropertiesSuiteBase extends QueryTest with DDLCommandTest val ns = s"$catalog.$namespace" withNamespace(ns) { sql(s"CREATE NAMESPACE $ns") + assert(getProperties(ns) === "") sql(s"ALTER NAMESPACE $ns SET PROPERTIES ('a'='a', 'b'='b', 'c'='c')") assert(getProperties(ns) === "((a,a), (b,b), (c,c))") sql(s"ALTER DATABASE $ns SET PROPERTIES ('d'='d')") @@ -92,14 +93,11 @@ trait AlterNamespaceSetPropertiesSuiteBase extends QueryTest with DDLCommandTest CatalogV2Util.NAMESPACE_RESERVED_PROPERTIES.filterNot(_ == PROP_COMMENT).foreach { key => withNamespace(ns) { sql(s"CREATE NAMESPACE $ns") + assert(getProperties(ns) === "") sql(s"ALTER NAMESPACE $ns SET PROPERTIES ('$key'='foo')") - assert(sql(s"DESC NAMESPACE EXTENDED $ns") - .toDF("k", "v") - .where("k='Properties'") - .where("v=''") - .count == 1, s"$key is a reserved namespace property and ignored") + assert(getProperties(ns) === "", s"$key is a reserved namespace property and ignored") val meta = spark.sessionState.catalogManager.catalog(catalog) - .asNamespaceCatalog.loadNamespaceMetadata(Array(namespace)) + .asNamespaceCatalog.loadNamespaceMetadata(Array(namespace)) assert(meta.get(key) == null || !meta.get(key).contains("foo"), "reserved properties should not have side effects") } From 5ac94f6737d414cba9528ebb87f6cb67400da540 Mon Sep 17 00:00:00 2001 From: Terry Kim Date: Wed, 8 Dec 2021 19:54:35 -0800 Subject: [PATCH 6/8] add hive test --- .../AlterNamespaceSetPropertiesSuite.scala | 29 +++++++++++++++++++ 1 file changed, 29 insertions(+) create mode 100644 sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/command/AlterNamespaceSetPropertiesSuite.scala diff --git a/sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/command/AlterNamespaceSetPropertiesSuite.scala b/sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/command/AlterNamespaceSetPropertiesSuite.scala new file mode 100644 index 0000000000000..3fffabdc17477 --- /dev/null +++ b/sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/command/AlterNamespaceSetPropertiesSuite.scala @@ -0,0 +1,29 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.spark.sql.hive.execution.command + +import org.apache.spark.sql.execution.command.v1 + +/** + * The class contains tests for the `ALTER NAMESPACE ... SET LOCATION` command to check + * V1 Hive external table catalog. + */ +class AlterNamespaceSetPropertiesSuite extends v1.AlterNamespaceSetPropertiesSuiteBase + with CommandSuiteBase { + override def commandVersion: String = super[AlterNamespaceSetPropertiesSuiteBase].commandVersion +} From bef81fa4970ed3f0bcbf1c387746461af0649877 Mon Sep 17 00:00:00 2001 From: Terry Kim Date: Wed, 8 Dec 2021 20:08:10 -0800 Subject: [PATCH 7/8] clean up --- .../AlterNamespaceSetPropertiesSuiteBase.scala | 13 ++++++------- .../command/AlterNamespaceSetPropertiesSuite.scala | 2 +- 2 files changed, 7 insertions(+), 8 deletions(-) diff --git a/sql/core/src/test/scala/org/apache/spark/sql/execution/command/AlterNamespaceSetPropertiesSuiteBase.scala b/sql/core/src/test/scala/org/apache/spark/sql/execution/command/AlterNamespaceSetPropertiesSuiteBase.scala index 56fd59eaaf86a..b1eb03268554e 100644 --- a/sql/core/src/test/scala/org/apache/spark/sql/execution/command/AlterNamespaceSetPropertiesSuiteBase.scala +++ b/sql/core/src/test/scala/org/apache/spark/sql/execution/command/AlterNamespaceSetPropertiesSuiteBase.scala @@ -37,8 +37,6 @@ import org.apache.spark.sql.internal.SQLConf * `org.apache.spark.sql.hive.execution.command.AlterNamespaceSetPropertiesSuite` */ trait AlterNamespaceSetPropertiesSuiteBase extends QueryTest with DDLCommandTestUtils { - import org.apache.spark.sql.connector.catalog.CatalogV2Implicits._ - override val command = "ALTER NAMESPACE ... SET PROPERTIES" protected def namespace: String @@ -76,8 +74,9 @@ trait AlterNamespaceSetPropertiesSuiteBase extends QueryTest with DDLCommandTest } test("test reserved properties") { - val ns = s"$catalog.$namespace" import SupportsNamespaces._ + import org.apache.spark.sql.connector.catalog.CatalogV2Implicits._ + val ns = s"$catalog.$namespace" withSQLConf((SQLConf.LEGACY_PROPERTY_NON_RESERVED.key, "false")) { CatalogV2Util.NAMESPACE_RESERVED_PROPERTIES.filterNot(_ == PROP_COMMENT).foreach { key => withNamespace(ns) { @@ -97,7 +96,7 @@ trait AlterNamespaceSetPropertiesSuiteBase extends QueryTest with DDLCommandTest sql(s"ALTER NAMESPACE $ns SET PROPERTIES ('$key'='foo')") assert(getProperties(ns) === "", s"$key is a reserved namespace property and ignored") val meta = spark.sessionState.catalogManager.catalog(catalog) - .asNamespaceCatalog.loadNamespaceMetadata(Array(namespace)) + .asNamespaceCatalog.loadNamespaceMetadata(namespace.split('.')) assert(meta.get(key) == null || !meta.get(key).contains("foo"), "reserved properties should not have side effects") } @@ -106,11 +105,11 @@ trait AlterNamespaceSetPropertiesSuiteBase extends QueryTest with DDLCommandTest } protected def getProperties(namespace: String): String = { - val locationRow = sql(s"DESCRIBE NAMESPACE EXTENDED $namespace") + val propsRow = sql(s"DESCRIBE NAMESPACE EXTENDED $namespace") .toDF("key", "value") .where(s"key like 'Properties%'") .collect() - assert(locationRow.length == 1) - locationRow(0).getString(1) + assert(propsRow.length == 1) + propsRow(0).getString(1) } } diff --git a/sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/command/AlterNamespaceSetPropertiesSuite.scala b/sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/command/AlterNamespaceSetPropertiesSuite.scala index 3fffabdc17477..77dd071c4475d 100644 --- a/sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/command/AlterNamespaceSetPropertiesSuite.scala +++ b/sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/command/AlterNamespaceSetPropertiesSuite.scala @@ -20,7 +20,7 @@ package org.apache.spark.sql.hive.execution.command import org.apache.spark.sql.execution.command.v1 /** - * The class contains tests for the `ALTER NAMESPACE ... SET LOCATION` command to check + * The class contains tests for the `ALTER NAMESPACE ... SET PROPERTIES` command to check * V1 Hive external table catalog. */ class AlterNamespaceSetPropertiesSuite extends v1.AlterNamespaceSetPropertiesSuiteBase From aaf0e5c22f9e159d19c484d07029a7b06745ee28 Mon Sep 17 00:00:00 2001 From: Terry Kim Date: Fri, 10 Dec 2021 11:26:13 -0800 Subject: [PATCH 8/8] add owner for in-memory catalog. --- .../apache/spark/sql/catalyst/catalog/InMemoryCatalog.scala | 6 +++++- .../command/AlterNamespaceSetPropertiesSuiteBase.scala | 6 ++++-- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/InMemoryCatalog.scala b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/InMemoryCatalog.scala index 7d29a9ed0ae79..c10e0bbfde213 100644 --- a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/InMemoryCatalog.scala +++ b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/InMemoryCatalog.scala @@ -30,8 +30,10 @@ import org.apache.spark.sql.catalyst.analysis._ import org.apache.spark.sql.catalyst.catalog.ExternalCatalogUtils._ import org.apache.spark.sql.catalyst.expressions.Expression import org.apache.spark.sql.catalyst.util.StringUtils +import org.apache.spark.sql.connector.catalog.SupportsNamespaces.PROP_OWNER import org.apache.spark.sql.errors.{QueryCompilationErrors, QueryExecutionErrors} import org.apache.spark.sql.types.StructType +import org.apache.spark.util.Utils /** * An in-memory (ephemeral) implementation of the system catalog. @@ -124,7 +126,9 @@ class InMemoryCatalog( throw QueryExecutionErrors.unableToCreateDatabaseAsFailedToCreateDirectoryError( dbDefinition, e) } - catalog.put(dbDefinition.name, new DatabaseDesc(dbDefinition)) + val newDb = dbDefinition.copy( + properties = dbDefinition.properties ++ Map(PROP_OWNER -> Utils.getCurrentUserName)) + catalog.put(dbDefinition.name, new DatabaseDesc(newDb)) } } diff --git a/sql/core/src/test/scala/org/apache/spark/sql/execution/command/AlterNamespaceSetPropertiesSuiteBase.scala b/sql/core/src/test/scala/org/apache/spark/sql/execution/command/AlterNamespaceSetPropertiesSuiteBase.scala index b1eb03268554e..c33795c836a1b 100644 --- a/sql/core/src/test/scala/org/apache/spark/sql/execution/command/AlterNamespaceSetPropertiesSuiteBase.scala +++ b/sql/core/src/test/scala/org/apache/spark/sql/execution/command/AlterNamespaceSetPropertiesSuiteBase.scala @@ -91,13 +91,15 @@ trait AlterNamespaceSetPropertiesSuiteBase extends QueryTest with DDLCommandTest withSQLConf((SQLConf.LEGACY_PROPERTY_NON_RESERVED.key, "true")) { CatalogV2Util.NAMESPACE_RESERVED_PROPERTIES.filterNot(_ == PROP_COMMENT).foreach { key => withNamespace(ns) { - sql(s"CREATE NAMESPACE $ns") + // Set the location explicitly because v2 catalog may not set the default location. + // Without this, `meta.get(key)` below may return null. + sql(s"CREATE NAMESPACE $ns LOCATION '/tmp'") assert(getProperties(ns) === "") sql(s"ALTER NAMESPACE $ns SET PROPERTIES ('$key'='foo')") assert(getProperties(ns) === "", s"$key is a reserved namespace property and ignored") val meta = spark.sessionState.catalogManager.catalog(catalog) .asNamespaceCatalog.loadNamespaceMetadata(namespace.split('.')) - assert(meta.get(key) == null || !meta.get(key).contains("foo"), + assert(!meta.get(key).contains("foo"), "reserved properties should not have side effects") } }