You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GitBox <gi...@apache.org> on 2021/12/09 04:15:02 UTC
[GitHub] [spark] imback82 opened a new pull request #34842: [SPARK-37590][SQL][TEST] Unify v1 and v2 ALTER TABLE .. SET PROPERTIES tests
imback82 opened a new pull request #34842:
URL: https://github.com/apache/spark/pull/34842
<!--
Thanks for sending a pull request! Here are some tips for you:
1. If this is your first time, please read our contributor guidelines: https://spark.apache.org/contributing.html
2. Ensure you have added or run the appropriate tests for your PR: https://spark.apache.org/developer-tools.html
3. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP][SPARK-XXXX] Your PR title ...'.
4. Be sure to keep the PR description updated to reflect all changes.
5. Please write your PR title to summarize what this PR proposes.
6. If possible, provide a concise example to reproduce the issue for a faster review.
7. If you want to add a new configuration, please read the guideline first for naming configurations in
'core/src/main/scala/org/apache/spark/internal/config/ConfigEntry.scala'.
8. If you want to add or modify an error type or message, please read the guideline first in
'core/src/main/resources/error/README.md'.
-->
### What changes were proposed in this pull request?
<!--
Please clarify what changes you are proposing. The purpose of this section is to outline the changes and how this PR fixes the issue.
If possible, please consider writing useful notes for better and faster reviews in your PR. See the examples below.
1. If you refactor some codes with changing classes, showing the class hierarchy will help reviewers.
2. If you fix some SQL features, you can provide some references of other DBMSes.
3. If there is design documentation, please add the link.
4. If there is a discussion in the mailing list, please add the link.
-->
1. Move `ALTER NAMESPACE ... SET PROPERTIES` parsing tests to `AlterNamespaceSetPropertiesParserSuite`.
2. Put common `ALTER NAMESPACE ... SET PROPERTIES` tests into one trait `org.apache.spark.sql.execution.command.AlterNamespaceSetPropertiesSuiteBase`, and put datasource specific tests to the `v1.AlterNamespaceSetPropertiesSuite` and `v2.AlterNamespaceSetPropertiesSuite`.
The changes follow the approach of #30287.
### Why are the changes needed?
<!--
Please clarify why the changes are needed. For instance,
1. If you propose a new API, clarify the use case for a new API.
2. If you fix a bug, you can clarify why it is a bug.
-->
1. The unification will allow to run common `ALTER NAMESPACE ... SET PROPERTIES` tests for both DSv1/Hive DSv1 and DSv2
2. We can detect missing features and differences between DSv1 and DSv2 implementations.
### Does this PR introduce _any_ user-facing change?
<!--
Note that it means *any* user-facing change including all aspects such as the documentation fix.
If yes, please clarify the previous behavior and the change this PR proposes - provide the console output, description and/or an example to show the behavior difference if possible.
If possible, please also clarify if this is a user-facing change compared to the released Spark versions or within the unreleased branches such as master.
If no, write 'No'.
-->
No
### How was this patch tested?
<!--
If tests were added, say they were added here. Please make sure to add some test cases that check the changes thoroughly including negative and positive cases if possible.
If it was tested in a way different from regular unit tests, please clarify how you tested step by step, ideally copy and paste-able, so that other reviewers can test and check, and descendants can verify in the future.
If tests were not added, please describe why they were not added and/or why it was difficult to add.
-->
Existing unit tests and new tests.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #34842: [SPARK-37590][SQL][TEST] Unify v1 and v2 ALTER TABLE .. SET PROPERTIES tests
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #34842:
URL: https://github.com/apache/spark/pull/34842#issuecomment-991258539
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] imback82 commented on a change in pull request #34842: [SPARK-37590][SQL][TEST] Unify v1 and v2 ALTER TABLE .. SET PROPERTIES tests
Posted by GitBox <gi...@apache.org>.
imback82 commented on a change in pull request #34842:
URL: https://github.com/apache/spark/pull/34842#discussion_r766928173
##########
File path: sql/core/src/test/scala/org/apache/spark/sql/execution/command/AlterNamespaceSetPropertiesSuiteBase.scala
##########
@@ -0,0 +1,115 @@
+/*
+ * 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.{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
+ * 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")
+ 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')")
+ assert(getProperties(ns) === "((a,a), (b,b), (c,c), (d,d))")
+ }
+ }
+
+ 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") {
+ 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) {
+ 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")
+ 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"),
Review comment:
I added an owner while creating a database in `InMemoryCatalog`, explicitly added a location while creating a namespace in the test (to handle the difference for v2 catalog), and removed the `meta.get(key) == null` check.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #34842: [SPARK-37590][SQL][TEST] Unify v1 and v2 ALTER TABLE .. SET PROPERTIES tests
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #34842:
URL: https://github.com/apache/spark/pull/34842#issuecomment-991334048
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/50553/
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #34842: [SPARK-37590][SQL][TEST] Unify v1 and v2 ALTER TABLE .. SET PROPERTIES tests
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #34842:
URL: https://github.com/apache/spark/pull/34842#issuecomment-989567260
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/50501/
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on pull request #34842: [SPARK-37590][SQL][TEST] Unify v1 and v2 ALTER TABLE .. SET PROPERTIES tests
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on pull request #34842:
URL: https://github.com/apache/spark/pull/34842#issuecomment-992107252
thanks, merging to master!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #34842: [SPARK-37590][SQL][TEST] Unify v1 and v2 ALTER TABLE .. SET PROPERTIES tests
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #34842:
URL: https://github.com/apache/spark/pull/34842#issuecomment-991334048
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/50553/
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #34842: [SPARK-37590][SQL][TEST] Unify v1 and v2 ALTER TABLE .. SET PROPERTIES tests
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #34842:
URL: https://github.com/apache/spark/pull/34842#discussion_r765456740
##########
File path: sql/core/src/test/scala/org/apache/spark/sql/execution/command/AlterNamespaceSetPropertiesSuiteBase.scala
##########
@@ -0,0 +1,115 @@
+/*
+ * 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.{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
+ * 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")
+ 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')")
+ assert(getProperties(ns) === "((a,a), (b,b), (c,c), (d,d))")
Review comment:
I think this is a mistake and we should fix it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] imback82 commented on a change in pull request #34842: [SPARK-37590][SQL][TEST] Unify v1 and v2 ALTER TABLE .. SET PROPERTIES tests
Posted by GitBox <gi...@apache.org>.
imback82 commented on a change in pull request #34842:
URL: https://github.com/apache/spark/pull/34842#discussion_r765491479
##########
File path: sql/core/src/test/scala/org/apache/spark/sql/execution/command/AlterNamespaceSetPropertiesSuiteBase.scala
##########
@@ -0,0 +1,115 @@
+/*
+ * 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.{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
+ * 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")
+ 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')")
+ assert(getProperties(ns) === "((a,a), (b,b), (c,c), (d,d))")
+ }
+ }
+
+ 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") {
+ 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) {
+ 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")
+ 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"),
Review comment:
This was taken from `DataSourceV2SQLSuite.scala`, but further looking into this, there seems to be a difference.
In the above loop, the `key` will be either `location` or `owner`, and `loadNamespaceMetadata` returns the following:
| key | v1 catalog | v2 catalog |
|----|----------|-----------|
| location | non-null | null |
| owner | null | non-null |
I think the `null` case is interesting.
1. v2 catalog returns `null` for `location` because `CREATE NAMESPACE` doesn't create a default location if not specified, where as [v1 catalog creates a default location](https://github.com/apache/spark/blob/8f6e439068281633acefb895f8c4bd9203868c24/sql/core/src/main/scala/org/apache/spark/sql/execution/command/ddl.scala#L82). This is expected for v2 catalog, right?
2. v1 catalog returns `null` for `owner` since the following doesn't set `owner` to metadata: https://github.com/apache/spark/blob/8f6e439068281633acefb895f8c4bd9203868c24/sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/V2SessionCatalog.scala#L347-L355. Do you know if this was intentional?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #34842: [SPARK-37590][SQL][TEST] Unify v1 and v2 ALTER TABLE .. SET PROPERTIES tests
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #34842:
URL: https://github.com/apache/spark/pull/34842#discussion_r765457219
##########
File path: sql/core/src/test/scala/org/apache/spark/sql/execution/command/AlterNamespaceSetPropertiesSuiteBase.scala
##########
@@ -0,0 +1,115 @@
+/*
+ * 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.{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
+ * 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")
+ 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')")
+ assert(getProperties(ns) === "((a,a), (b,b), (c,c), (d,d))")
+ }
+ }
+
+ 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") {
+ 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) {
+ 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")
+ 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"),
Review comment:
is this a behavior difference between v1 and v2? null vs empty string
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #34842: [SPARK-37590][SQL][TEST] Unify v1 and v2 ALTER TABLE .. SET PROPERTIES tests
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #34842:
URL: https://github.com/apache/spark/pull/34842#issuecomment-991398815
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/146078/
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #34842: [SPARK-37590][SQL][TEST] Unify v1 and v2 ALTER TABLE .. SET PROPERTIES tests
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #34842:
URL: https://github.com/apache/spark/pull/34842#issuecomment-991328195
Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/50553/
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #34842: [SPARK-37590][SQL][TEST] Unify v1 and v2 ALTER TABLE .. SET PROPERTIES tests
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #34842:
URL: https://github.com/apache/spark/pull/34842#issuecomment-989689763
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/146025/
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] yaooqinn commented on a change in pull request #34842: [SPARK-37590][SQL][TEST] Unify v1 and v2 ALTER TABLE .. SET PROPERTIES tests
Posted by GitBox <gi...@apache.org>.
yaooqinn commented on a change in pull request #34842:
URL: https://github.com/apache/spark/pull/34842#discussion_r766497682
##########
File path: sql/core/src/test/scala/org/apache/spark/sql/execution/command/AlterNamespaceSetPropertiesSuiteBase.scala
##########
@@ -0,0 +1,115 @@
+/*
+ * 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.{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
+ * 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")
+ 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')")
+ assert(getProperties(ns) === "((a,a), (b,b), (c,c), (d,d))")
+ }
+ }
+
+ 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") {
+ 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) {
+ 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")
+ 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"),
Review comment:
for v1 and v2 database and table creation, we both respect the `sparkUser` now
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #34842: [SPARK-37590][SQL][TEST] Unify v1 and v2 ALTER TABLE .. SET PROPERTIES tests
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #34842:
URL: https://github.com/apache/spark/pull/34842#issuecomment-991284139
Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/50553/
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #34842: [SPARK-37590][SQL][TEST] Unify v1 and v2 ALTER TABLE .. SET PROPERTIES tests
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #34842:
URL: https://github.com/apache/spark/pull/34842#issuecomment-991334048
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #34842: [SPARK-37590][SQL][TEST] Unify v1 and v2 ALTER TABLE .. SET PROPERTIES tests
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #34842:
URL: https://github.com/apache/spark/pull/34842#issuecomment-989567260
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/50501/
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] imback82 commented on a change in pull request #34842: [SPARK-37590][SQL][TEST] Unify v1 and v2 ALTER TABLE .. SET PROPERTIES tests
Posted by GitBox <gi...@apache.org>.
imback82 commented on a change in pull request #34842:
URL: https://github.com/apache/spark/pull/34842#discussion_r765422131
##########
File path: sql/core/src/test/scala/org/apache/spark/sql/execution/command/AlterNamespaceSetPropertiesSuiteBase.scala
##########
@@ -0,0 +1,115 @@
+/*
+ * 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.{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
+ * 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")
+ 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')")
+ assert(getProperties(ns) === "((a,a), (b,b), (c,c), (d,d))")
Review comment:
@cloud-fan, for v2 catalog, this is consistent with v1 catalog behavior because of our test catalog implementation (add existing metadata): https://github.com/apache/spark/blob/5edd9592dda6a540b398e6f6a662ea603f0e8ce2/sql/catalyst/src/test/scala/org/apache/spark/sql/connector/catalog/InMemoryTableCatalog.scala#L212,
not because of the v2 command (just passes whatever is specified):
https://github.com/apache/spark/blob/5edd9592dda6a540b398e6f6a662ea603f0e8ce2/sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/AlterNamespaceSetPropertiesExec.scala#L32
FYI, v1 command is responsible for adding the existing metadata, not the catalog implementation:
https://github.com/apache/spark/blob/5edd9592dda6a540b398e6f6a662ea603f0e8ce2/sql/core/src/main/scala/org/apache/spark/sql/execution/command/ddl.scala#L135
Is this by design?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #34842: [SPARK-37590][SQL][TEST] Unify v1 and v2 ALTER TABLE .. SET PROPERTIES tests
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #34842:
URL: https://github.com/apache/spark/pull/34842#issuecomment-989513137
**[Test build #146025 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/146025/testReport)** for PR 34842 at commit [`bef81fa`](https://github.com/apache/spark/commit/bef81fa4970ed3f0bcbf1c387746461af0649877).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #34842: [SPARK-37590][SQL][TEST] Unify v1 and v2 ALTER TABLE .. SET PROPERTIES tests
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #34842:
URL: https://github.com/apache/spark/pull/34842#issuecomment-989549464
Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/50501/
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #34842: [SPARK-37590][SQL][TEST] Unify v1 and v2 ALTER TABLE .. SET PROPERTIES tests
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #34842:
URL: https://github.com/apache/spark/pull/34842#issuecomment-989531707
Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/50501/
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #34842: [SPARK-37590][SQL][TEST] Unify v1 and v2 ALTER TABLE .. SET PROPERTIES tests
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #34842:
URL: https://github.com/apache/spark/pull/34842#issuecomment-991398382
**[Test build #146078 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/146078/testReport)** for PR 34842 at commit [`aaf0e5c`](https://github.com/apache/spark/commit/aaf0e5c22f9e159d19c484d07029a7b06745ee28).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA removed a comment on pull request #34842: [SPARK-37590][SQL][TEST] Unify v1 and v2 ALTER TABLE .. SET PROPERTIES tests
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #34842:
URL: https://github.com/apache/spark/pull/34842#issuecomment-991258539
**[Test build #146078 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/146078/testReport)** for PR 34842 at commit [`aaf0e5c`](https://github.com/apache/spark/commit/aaf0e5c22f9e159d19c484d07029a7b06745ee28).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #34842: [SPARK-37590][SQL][TEST] Unify v1 and v2 ALTER TABLE .. SET PROPERTIES tests
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #34842:
URL: https://github.com/apache/spark/pull/34842#issuecomment-991334048
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] imback82 commented on a change in pull request #34842: [SPARK-37590][SQL][TEST] Unify v1 and v2 ALTER TABLE .. SET PROPERTIES tests
Posted by GitBox <gi...@apache.org>.
imback82 commented on a change in pull request #34842:
URL: https://github.com/apache/spark/pull/34842#discussion_r766087819
##########
File path: sql/core/src/test/scala/org/apache/spark/sql/execution/command/AlterNamespaceSetPropertiesSuiteBase.scala
##########
@@ -0,0 +1,115 @@
+/*
+ * 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.{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
+ * 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")
+ 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')")
+ assert(getProperties(ns) === "((a,a), (b,b), (c,c), (d,d))")
+ }
+ }
+
+ 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") {
+ 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) {
+ 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")
+ 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"),
Review comment:
For the owner, the difference comes because v2 command always adds the owner when the namespace is created:
https://github.com/apache/spark/blob/7b000116575cdc8e42653a694d9ad5273346fa61/sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/CreateNamespaceExec.scala#L45-L47
, whereas for v1 command doesn't add the owner when the database is created.
Instead, for v1 Hive catalog, the user property is inserted when the database is retrieved:
https://github.com/apache/spark/blob/7b000116575cdc8e42653a694d9ad5273346fa61/sql/hive/src/main/scala/org/apache/spark/sql/hive/client/HiveClientImpl.scala#L370-L373
Meanwhile, the v1 in-memory catalog implementation doesn't add the owner when the database is retrieved, so we see `null` owner above. (and the owner is a part of the property, so updating `V2SessionCatalog` doesn't really address the issue).
One thing we can do is to update the v1 in-memory catalog to add the owner when the database is created or retrieved, but it is still not consistent since adding the owner is a responsibility of the command in v2, but a responsibility of the catalog in v1. Any thoughts?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #34842: [SPARK-37590][SQL][TEST] Unify v1 and v2 ALTER TABLE .. SET PROPERTIES tests
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #34842:
URL: https://github.com/apache/spark/pull/34842#discussion_r766489751
##########
File path: sql/core/src/test/scala/org/apache/spark/sql/execution/command/AlterNamespaceSetPropertiesSuiteBase.scala
##########
@@ -0,0 +1,115 @@
+/*
+ * 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.{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
+ * 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")
+ 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')")
+ assert(getProperties(ns) === "((a,a), (b,b), (c,c), (d,d))")
+ }
+ }
+
+ 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") {
+ 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) {
+ 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")
+ 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"),
Review comment:
It may be too risky to change the v1 behavior now (Hive metastore fills the owner field). Let's just update the v1 in-memory catalog to fill the owner field as well.
@yaooqinn which one do you think should set the owner field? Spark or the underlying catalog?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #34842: [SPARK-37590][SQL][TEST] Unify v1 and v2 ALTER TABLE .. SET PROPERTIES tests
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #34842:
URL: https://github.com/apache/spark/pull/34842#issuecomment-989683249
**[Test build #146025 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/146025/testReport)** for PR 34842 at commit [`bef81fa`](https://github.com/apache/spark/commit/bef81fa4970ed3f0bcbf1c387746461af0649877).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] imback82 commented on a change in pull request #34842: [SPARK-37590][SQL][TEST] Unify v1 and v2 ALTER TABLE .. SET PROPERTIES tests
Posted by GitBox <gi...@apache.org>.
imback82 commented on a change in pull request #34842:
URL: https://github.com/apache/spark/pull/34842#discussion_r770761242
##########
File path: sql/core/src/test/scala/org/apache/spark/sql/execution/command/AlterNamespaceSetPropertiesSuiteBase.scala
##########
@@ -0,0 +1,117 @@
+/*
+ * 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.{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
+ * 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")
+ 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')")
+ assert(getProperties(ns) === "((a,a), (b,b), (c,c), (d,d))")
+ }
+ }
+
+ 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") {
+ 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) {
+ 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) {
+ // 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'")
Review comment:
Yes, will do. Thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #34842: [SPARK-37590][SQL][TEST] Unify v1 and v2 ALTER TABLE .. SET PROPERTIES tests
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #34842:
URL: https://github.com/apache/spark/pull/34842#discussion_r770222484
##########
File path: sql/core/src/test/scala/org/apache/spark/sql/execution/command/AlterNamespaceSetPropertiesSuiteBase.scala
##########
@@ -0,0 +1,117 @@
+/*
+ * 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.{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
+ * 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")
+ 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')")
+ assert(getProperties(ns) === "((a,a), (b,b), (c,c), (d,d))")
+ }
+ }
+
+ 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") {
+ 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) {
+ 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) {
+ // 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'")
Review comment:
Not all test environments can access `/tmp` because it's a directory under root. I think we should change it to `tmp` which will be qualified with the warehouse path.
@imback82 can you make a followup to fix this? Thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] imback82 commented on a change in pull request #34842: [SPARK-37590][SQL][TEST] Unify v1 and v2 ALTER TABLE .. SET PROPERTIES tests
Posted by GitBox <gi...@apache.org>.
imback82 commented on a change in pull request #34842:
URL: https://github.com/apache/spark/pull/34842#discussion_r765482510
##########
File path: sql/core/src/test/scala/org/apache/spark/sql/execution/command/AlterNamespaceSetPropertiesSuiteBase.scala
##########
@@ -0,0 +1,115 @@
+/*
+ * 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.{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
+ * 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")
+ 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')")
+ assert(getProperties(ns) === "((a,a), (b,b), (c,c), (d,d))")
Review comment:
OK, will fix this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA removed a comment on pull request #34842: [SPARK-37590][SQL][TEST] Unify v1 and v2 ALTER TABLE .. SET PROPERTIES tests
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #34842:
URL: https://github.com/apache/spark/pull/34842#issuecomment-989513137
**[Test build #146025 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/146025/testReport)** for PR 34842 at commit [`bef81fa`](https://github.com/apache/spark/commit/bef81fa4970ed3f0bcbf1c387746461af0649877).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #34842: [SPARK-37590][SQL][TEST] Unify v1 and v2 ALTER TABLE .. SET PROPERTIES tests
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #34842:
URL: https://github.com/apache/spark/pull/34842#issuecomment-991258539
**[Test build #146078 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/146078/testReport)** for PR 34842 at commit [`aaf0e5c`](https://github.com/apache/spark/commit/aaf0e5c22f9e159d19c484d07029a7b06745ee28).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan closed pull request #34842: [SPARK-37590][SQL][TEST] Unify v1 and v2 ALTER TABLE .. SET PROPERTIES tests
Posted by GitBox <gi...@apache.org>.
cloud-fan closed pull request #34842:
URL: https://github.com/apache/spark/pull/34842
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #34842: [SPARK-37590][SQL][TEST] Unify v1 and v2 ALTER TABLE .. SET PROPERTIES tests
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #34842:
URL: https://github.com/apache/spark/pull/34842#issuecomment-989689763
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/146025/
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA removed a comment on pull request #34842: [SPARK-37590][SQL][TEST] Unify v1 and v2 ALTER TABLE .. SET PROPERTIES tests
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #34842:
URL: https://github.com/apache/spark/pull/34842#issuecomment-991258539
**[Test build #146078 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/146078/testReport)** for PR 34842 at commit [`aaf0e5c`](https://github.com/apache/spark/commit/aaf0e5c22f9e159d19c484d07029a7b06745ee28).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #34842: [SPARK-37590][SQL][TEST] Unify v1 and v2 ALTER TABLE .. SET PROPERTIES tests
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #34842:
URL: https://github.com/apache/spark/pull/34842#discussion_r765730896
##########
File path: sql/core/src/test/scala/org/apache/spark/sql/execution/command/AlterNamespaceSetPropertiesSuiteBase.scala
##########
@@ -0,0 +1,115 @@
+/*
+ * 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.{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
+ * 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")
+ 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')")
+ assert(getProperties(ns) === "((a,a), (b,b), (c,c), (d,d))")
+ }
+ }
+
+ 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") {
+ 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) {
+ 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")
+ 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"),
Review comment:
For the location, I think it's OK. The catalog implementation should decide the default location (or even no location if the source is not file-based). We should accept this difference.
For the owner, it seems a bug that `V2SessionCatalog` does not propagate the owner field.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] imback82 commented on a change in pull request #34842: [SPARK-37590][SQL][TEST] Unify v1 and v2 ALTER TABLE .. SET PROPERTIES tests
Posted by GitBox <gi...@apache.org>.
imback82 commented on a change in pull request #34842:
URL: https://github.com/apache/spark/pull/34842#discussion_r766928173
##########
File path: sql/core/src/test/scala/org/apache/spark/sql/execution/command/AlterNamespaceSetPropertiesSuiteBase.scala
##########
@@ -0,0 +1,115 @@
+/*
+ * 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.{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
+ * 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")
+ 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')")
+ assert(getProperties(ns) === "((a,a), (b,b), (c,c), (d,d))")
+ }
+ }
+
+ 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") {
+ 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) {
+ 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")
+ 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"),
Review comment:
I added an owner while creating a database in `InMemoryCatalog`, explicitly added a location while creating a namespace in the test (to handle the difference for v2 catalog), and removed the `meta.get(key) == null` check.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #34842: [SPARK-37590][SQL][TEST] Unify v1 and v2 ALTER TABLE .. SET PROPERTIES tests
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #34842:
URL: https://github.com/apache/spark/pull/34842#issuecomment-991398815
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/146078/
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org