From d5d1f9bf5af29a37471771fdb45110d1bd546ced Mon Sep 17 00:00:00 2001 From: panbingkun Date: Wed, 26 Jun 2024 15:52:39 +0800 Subject: [PATCH 1/6] [SPARK-48720][SQL] Fix `IF EXISTS` ignored for `ALTER TABLE ... UNSET TBLPROPERTIES ...` in v2 --- .../sql/connector/catalog/TableChange.java | 15 +- .../plans/logical/v2AlterTableCommands.scala | 2 +- .../sql/connector/catalog/CatalogV2Util.scala | 17 ++- .../sql/errors/QueryCompilationErrors.scala | 4 +- .../sql/catalyst/parser/DDLParserSuite.scala | 19 --- .../sql/connector/catalog/CatalogSuite.scala | 4 +- .../catalog/InMemoryTableCatalog.scala | 2 +- .../spark/sql/execution/command/ddl.scala | 2 +- .../datasources/v2/V2SessionCatalog.scala | 3 +- ...SourceV2DataFrameSessionCatalogSuite.scala | 3 +- .../AlterTableSetTblPropertiesSuiteBase.scala | 78 ++++++++-- ...erTableUnsetTblPropertiesParserSuite.scala | 63 ++++++++ ...lterTableUnsetTblPropertiesSuiteBase.scala | 144 ++++++++++++++++++ .../execution/command/DDLParserSuite.scala | 12 -- .../sql/execution/command/DDLSuite.scala | 50 ------ .../v1/AlterTableSetTblPropertiesSuite.scala | 4 + .../AlterTableUnsetTblPropertiesSuite.scala | 65 ++++++++ .../v2/AlterTableSetTblPropertiesSuite.scala | 4 + .../AlterTableUnsetTblPropertiesSuite.scala | 54 +++++++ .../v2/V2SessionCatalogSuite.scala | 4 +- .../sql/hive/execution/HiveDDLSuite.scala | 4 - .../AlterTableUnsetTblPropertiesSuite.scala | 27 ++++ 22 files changed, 470 insertions(+), 110 deletions(-) create mode 100644 sql/core/src/test/scala/org/apache/spark/sql/execution/command/AlterTableUnsetTblPropertiesParserSuite.scala create mode 100644 sql/core/src/test/scala/org/apache/spark/sql/execution/command/AlterTableUnsetTblPropertiesSuiteBase.scala create mode 100644 sql/core/src/test/scala/org/apache/spark/sql/execution/command/v1/AlterTableUnsetTblPropertiesSuite.scala create mode 100644 sql/core/src/test/scala/org/apache/spark/sql/execution/command/v2/AlterTableUnsetTblPropertiesSuite.scala create mode 100644 sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/command/AlterTableUnsetTblPropertiesSuite.scala diff --git a/sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/TableChange.java b/sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/TableChange.java index ebecb6f507e6a..32e9b176017b1 100644 --- a/sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/TableChange.java +++ b/sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/TableChange.java @@ -61,10 +61,11 @@ static TableChange setProperty(String property, String value) { * If the property does not exist, the change will succeed. * * @param property the property name + * @param ifExists silence the error if property doesn't exist during unset * @return a TableChange for the addition */ - static TableChange removeProperty(String property) { - return new RemoveProperty(property); + static TableChange removeProperty(String property, boolean ifExists) { + return new RemoveProperty(property, ifExists); } /** @@ -292,21 +293,27 @@ public int hashCode() { */ final class RemoveProperty implements TableChange { private final String property; + private final boolean ifExists; - private RemoveProperty(String property) { + private RemoveProperty(String property, boolean ifExists) { this.property = property; + this.ifExists = ifExists; } public String property() { return property; } + public boolean ifExists() { + return ifExists; + } + @Override public boolean equals(Object o) { if (this == o) return true; if (o == null || getClass() != o.getClass()) return false; RemoveProperty that = (RemoveProperty) o; - return property.equals(that.property); + return property.equals(that.property) && that.ifExists() == this.ifExists(); } @Override diff --git a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/v2AlterTableCommands.scala b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/v2AlterTableCommands.scala index 9c66e68d686d5..ed2153d82edd1 100644 --- a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/v2AlterTableCommands.scala +++ b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/v2AlterTableCommands.scala @@ -91,7 +91,7 @@ case class UnsetTableProperties( propertyKeys: Seq[String], ifExists: Boolean) extends AlterTableCommand { override def changes: Seq[TableChange] = { - propertyKeys.map(key => TableChange.removeProperty(key)) + propertyKeys.map(key => TableChange.removeProperty(key, ifExists)) } override protected def withNewChildInternal(newChild: LogicalPlan): LogicalPlan = copy(table = newChild) diff --git a/sql/catalyst/src/main/scala/org/apache/spark/sql/connector/catalog/CatalogV2Util.scala b/sql/catalyst/src/main/scala/org/apache/spark/sql/connector/catalog/CatalogV2Util.scala index f36310e8ad899..f6af4626b708b 100644 --- a/sql/catalyst/src/main/scala/org/apache/spark/sql/connector/catalog/CatalogV2Util.scala +++ b/sql/catalyst/src/main/scala/org/apache/spark/sql/connector/catalog/CatalogV2Util.scala @@ -20,6 +20,7 @@ package org.apache.spark.sql.connector.catalog import java.util import java.util.Collections +import scala.collection.mutable import scala.jdk.CollectionConverters._ import org.apache.spark.SparkIllegalArgumentException @@ -33,6 +34,7 @@ import org.apache.spark.sql.catalyst.util.ResolveDefaultColumns._ import org.apache.spark.sql.connector.catalog.TableChange._ import org.apache.spark.sql.connector.catalog.functions.UnboundFunction import org.apache.spark.sql.connector.expressions.LiteralValue +import org.apache.spark.sql.errors.QueryCompilationErrors import org.apache.spark.sql.execution.datasources.v2.DataSourceV2Relation import org.apache.spark.sql.types.{ArrayType, MapType, Metadata, MetadataBuilder, StructField, StructType} import org.apache.spark.sql.util.CaseInsensitiveStringMap @@ -107,30 +109,43 @@ private[sql] object CatalogV2Util { * Apply properties changes to a map and return the result. */ def applyPropertiesChanges( + catalogName: String, + ident: Identifier, properties: Map[String, String], changes: Seq[TableChange]): Map[String, String] = { - applyPropertiesChanges(properties.asJava, changes).asScala.toMap + applyPropertiesChanges(catalogName, ident, properties.asJava, changes).asScala.toMap } /** * Apply properties changes to a Java map and return the result. */ def applyPropertiesChanges( + catalogName: String, + ident: Identifier, properties: util.Map[String, String], changes: Seq[TableChange]): util.Map[String, String] = { val newProperties = new util.HashMap[String, String](properties) + val nonexistentKeys = mutable.ArrayBuilder.make[String] changes.foreach { case set: SetProperty => newProperties.put(set.property, set.value) case unset: RemoveProperty => + if (!unset.ifExists && !properties.containsKey(unset.property)) { + nonexistentKeys += unset.property + } newProperties.remove(unset.property) case _ => // ignore non-property changes } + if (!nonexistentKeys.result().isEmpty) { + throw QueryCompilationErrors.unsetNonExistentPropertiesError( + nonexistentKeys.result().toSeq, catalogName +: ident.asMultipartIdentifier) + } + Collections.unmodifiableMap(newProperties) } diff --git a/sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryCompilationErrors.scala b/sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryCompilationErrors.scala index d3bd265d0459e..04b94691a0516 100644 --- a/sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryCompilationErrors.scala +++ b/sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryCompilationErrors.scala @@ -2659,12 +2659,12 @@ private[sql] object QueryCompilationErrors extends QueryErrorsBase with Compilat } def unsetNonExistentPropertiesError( - properties: Seq[String], table: TableIdentifier): Throwable = { + properties: Seq[String], nameParts: Seq[String]): Throwable = { new AnalysisException( errorClass = "UNSET_NONEXISTENT_PROPERTIES", messageParameters = Map( "properties" -> properties.map(toSQLId).mkString(", "), - "table" -> toSQLId(table.nameParts)) + "table" -> toSQLId(nameParts)) ) } diff --git a/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/parser/DDLParserSuite.scala b/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/parser/DDLParserSuite.scala index 8612a6e9c50ff..f88c516f0019d 100644 --- a/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/parser/DDLParserSuite.scala +++ b/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/parser/DDLParserSuite.scala @@ -1075,25 +1075,6 @@ class DDLParserSuite extends AnalysisTest { ifExists = true)) } - // ALTER TABLE table_name UNSET TBLPROPERTIES [IF EXISTS] ('comment', 'key'); - test("alter table: alter table properties") { - val sql2_table = "ALTER TABLE table_name UNSET TBLPROPERTIES ('comment', 'test')" - val sql3_table = "ALTER TABLE table_name UNSET TBLPROPERTIES IF EXISTS ('comment', 'test')" - - comparePlans( - parsePlan(sql2_table), - UnsetTableProperties( - UnresolvedTable(Seq("table_name"), "ALTER TABLE ... UNSET TBLPROPERTIES", true), - Seq("comment", "test"), - ifExists = false)) - comparePlans( - parsePlan(sql3_table), - UnsetTableProperties( - UnresolvedTable(Seq("table_name"), "ALTER TABLE ... UNSET TBLPROPERTIES", true), - Seq("comment", "test"), - ifExists = true)) - } - test("alter table: add column") { comparePlans( parsePlan("ALTER TABLE table_name ADD COLUMN x int"), diff --git a/sql/catalyst/src/test/scala/org/apache/spark/sql/connector/catalog/CatalogSuite.scala b/sql/catalyst/src/test/scala/org/apache/spark/sql/connector/catalog/CatalogSuite.scala index e20dfd4f60512..b6474549f0f94 100644 --- a/sql/catalyst/src/test/scala/org/apache/spark/sql/connector/catalog/CatalogSuite.scala +++ b/sql/catalyst/src/test/scala/org/apache/spark/sql/connector/catalog/CatalogSuite.scala @@ -269,7 +269,7 @@ class CatalogSuite extends SparkFunSuite { assert(table.properties.asScala == Map("prop-1" -> "1")) - val updated = catalog.alterTable(testIdent, TableChange.removeProperty("prop-1")) + val updated = catalog.alterTable(testIdent, TableChange.removeProperty("prop-1", false)) assert(updated.properties.asScala == Map()) val loaded = catalog.loadTable(testIdent) @@ -285,7 +285,7 @@ class CatalogSuite extends SparkFunSuite { assert(table.properties.asScala == Map()) - val updated = catalog.alterTable(testIdent, TableChange.removeProperty("prop-1")) + val updated = catalog.alterTable(testIdent, TableChange.removeProperty("prop-1", true)) assert(updated.properties.asScala == Map()) val loaded = catalog.loadTable(testIdent) diff --git a/sql/catalyst/src/test/scala/org/apache/spark/sql/connector/catalog/InMemoryTableCatalog.scala b/sql/catalyst/src/test/scala/org/apache/spark/sql/connector/catalog/InMemoryTableCatalog.scala index d511477ef5d33..0232446c08fa4 100644 --- a/sql/catalyst/src/test/scala/org/apache/spark/sql/connector/catalog/InMemoryTableCatalog.scala +++ b/sql/catalyst/src/test/scala/org/apache/spark/sql/connector/catalog/InMemoryTableCatalog.scala @@ -131,7 +131,7 @@ class BasicInMemoryTableCatalog extends TableCatalog { override def alterTable(ident: Identifier, changes: TableChange*): Table = { val table = loadTable(ident).asInstanceOf[InMemoryTable] - val properties = CatalogV2Util.applyPropertiesChanges(table.properties, changes) + val properties = CatalogV2Util.applyPropertiesChanges(name, ident, table.properties, changes) val schema = CatalogV2Util.applySchemaChanges(table.schema, changes, None, "ALTER TABLE") // fail if the last column in the schema was dropped diff --git a/sql/core/src/main/scala/org/apache/spark/sql/execution/command/ddl.scala b/sql/core/src/main/scala/org/apache/spark/sql/execution/command/ddl.scala index 6f402188910e0..5f639bc5665ae 100644 --- a/sql/core/src/main/scala/org/apache/spark/sql/execution/command/ddl.scala +++ b/sql/core/src/main/scala/org/apache/spark/sql/execution/command/ddl.scala @@ -340,7 +340,7 @@ case class AlterTableUnsetPropertiesCommand( && key != TableCatalog.PROP_COMMENT) if (nonexistentKeys.nonEmpty) { throw QueryCompilationErrors.unsetNonExistentPropertiesError( - nonexistentKeys, table.identifier) + nonexistentKeys, table.identifier.nameParts) } } // If comment is in the table property, we reset it to None diff --git a/sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/V2SessionCatalog.scala b/sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/V2SessionCatalog.scala index 3d6de985a62f5..bc70868bb8f86 100644 --- a/sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/V2SessionCatalog.scala +++ b/sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/V2SessionCatalog.scala @@ -272,7 +272,8 @@ class V2SessionCatalog(catalog: SessionCatalog) throw QueryCompilationErrors.noSuchTableError(ident) } - val properties = CatalogV2Util.applyPropertiesChanges(catalogTable.properties, changes) + val properties = CatalogV2Util.applyPropertiesChanges( + name, ident, catalogTable.properties, changes) val schema = CatalogV2Util.applySchemaChanges( catalogTable.schema, changes, catalogTable.provider, "ALTER TABLE") val comment = properties.get(TableCatalog.PROP_COMMENT) diff --git a/sql/core/src/test/scala/org/apache/spark/sql/connector/DataSourceV2DataFrameSessionCatalogSuite.scala b/sql/core/src/test/scala/org/apache/spark/sql/connector/DataSourceV2DataFrameSessionCatalogSuite.scala index 7bbb6485c273f..e8f6acfe98ffc 100644 --- a/sql/core/src/test/scala/org/apache/spark/sql/connector/DataSourceV2DataFrameSessionCatalogSuite.scala +++ b/sql/core/src/test/scala/org/apache/spark/sql/connector/DataSourceV2DataFrameSessionCatalogSuite.scala @@ -109,7 +109,8 @@ class InMemoryTableSessionCatalog extends TestV2SessionCatalogBase[InMemoryTable override def alterTable(ident: Identifier, changes: TableChange*): Table = { Option(tables.get(ident)) match { case Some(table) => - val properties = CatalogV2Util.applyPropertiesChanges(table.properties, changes) + val properties = CatalogV2Util.applyPropertiesChanges( + name, ident, table.properties, changes) val provider = Option(properties.get("provider")) val schema = CatalogV2Util.applySchemaChanges( diff --git a/sql/core/src/test/scala/org/apache/spark/sql/execution/command/AlterTableSetTblPropertiesSuiteBase.scala b/sql/core/src/test/scala/org/apache/spark/sql/execution/command/AlterTableSetTblPropertiesSuiteBase.scala index 64b70d709b93f..f1fda2d22bf89 100644 --- a/sql/core/src/test/scala/org/apache/spark/sql/execution/command/AlterTableSetTblPropertiesSuiteBase.scala +++ b/sql/core/src/test/scala/org/apache/spark/sql/execution/command/AlterTableSetTblPropertiesSuiteBase.scala @@ -19,6 +19,10 @@ package org.apache.spark.sql.execution.command import org.apache.spark.sql.{AnalysisException, QueryTest} import org.apache.spark.sql.catalyst.TableIdentifier +import org.apache.spark.sql.catalyst.parser.ParseException +import org.apache.spark.sql.connector.catalog.{CatalogV2Util, TableCatalog} +import org.apache.spark.sql.errors.DataTypeErrors.toSQLId +import org.apache.spark.sql.internal.SQLConf /** * This base suite contains unified tests for the `ALTER TABLE .. SET TBLPROPERTIES` @@ -39,6 +43,25 @@ trait AlterTableSetTblPropertiesSuiteBase extends QueryTest with DDLCommandTestU def checkTblProps(tableIdent: TableIdentifier, expectedTblProps: Map[String, String]): Unit + def getTblPropertyValue(tableIdent: TableIdentifier, key: String): String + + test("table to alter does not exist") { + withNamespaceAndTable("ns", "does_not_exist") { t => + val sqlText = s"ALTER TABLE $t SET TBLPROPERTIES ('k1' = 'v1')" + checkError( + exception = intercept[AnalysisException] { + sql(sqlText) + }, + errorClass = "TABLE_OR_VIEW_NOT_FOUND", + parameters = Map("relationName" -> toSQLId(t)), + context = ExpectedContext( + fragment = t, + start = 12, + stop = 11 + t.length) + ) + } + } + test("alter table set tblproperties") { withNamespaceAndTable("ns", "tbl") { t => sql(s"CREATE TABLE $t (col1 int, col2 string, a int, b int) $defaultUsing") @@ -54,16 +77,53 @@ trait AlterTableSetTblPropertiesSuiteBase extends QueryTest with DDLCommandTestU sql(s"ALTER TABLE $t SET TBLPROPERTIES ('k1' = 'v1', 'k2' = 'v8')") checkTblProps(tableIdent, Map("k1" -> "v1", "k2" -> "v8", "k3" -> "v3")) + } + } - // table to alter does not exist - checkError( - exception = intercept[AnalysisException] { - sql("ALTER TABLE does_not_exist SET TBLPROPERTIES ('winner' = 'loser')") - }, - errorClass = "TABLE_OR_VIEW_NOT_FOUND", - parameters = Map("relationName" -> "`does_not_exist`"), - context = ExpectedContext(fragment = "does_not_exist", start = 12, stop = 25) - ) + test("test reserved properties") { + import TableCatalog._ + val keyParameters = Map[String, String]( + PROP_PROVIDER -> "please use the USING clause to specify it", + PROP_LOCATION -> "please use the LOCATION clause to specify it", + PROP_OWNER -> "it will be set to the current user", + PROP_EXTERNAL -> "please use CREATE EXTERNAL TABLE" + ) + withSQLConf((SQLConf.LEGACY_PROPERTY_NON_RESERVED.key, "false")) { + CatalogV2Util.TABLE_RESERVED_PROPERTIES.filterNot(_ == PROP_COMMENT).foreach { key => + withNamespaceAndTable("ns", "tbl") { t => + val sqlText = s"ALTER TABLE $t SET TBLPROPERTIES ('$key'='bar')" + checkError( + exception = intercept[ParseException] { + sql(sqlText) + }, + errorClass = "UNSUPPORTED_FEATURE.SET_TABLE_PROPERTY", + parameters = Map( + "property" -> key, + "msg" -> keyParameters.getOrElse( + key, "please remove it from the TBLPROPERTIES list.")), + context = ExpectedContext( + fragment = sqlText, + start = 0, + stop = 40 + t.length + key.length)) + } + } + } + withSQLConf((SQLConf.LEGACY_PROPERTY_NON_RESERVED.key, "true")) { + CatalogV2Util.TABLE_RESERVED_PROPERTIES.filterNot(_ == PROP_COMMENT).foreach { key => + Seq("OPTIONS", "TBLPROPERTIES").foreach { clause => + withNamespaceAndTable("ns", "tbl") { t => + sql(s"CREATE TABLE $t (key int) USING parquet $clause ('$key'='bar')") + val tableIdent = TableIdentifier("tbl", Some("ns"), Some(catalog)) + + val originValue = getTblPropertyValue(tableIdent, key) + assert(originValue != "bar", "reserved properties should not have side effects") + + sql(s"ALTER TABLE $t SET TBLPROPERTIES ('$key'='newValue')") + assert(getTblPropertyValue(tableIdent, key) == originValue, + "reserved properties should not have side effects") + } + } + } } } } diff --git a/sql/core/src/test/scala/org/apache/spark/sql/execution/command/AlterTableUnsetTblPropertiesParserSuite.scala b/sql/core/src/test/scala/org/apache/spark/sql/execution/command/AlterTableUnsetTblPropertiesParserSuite.scala new file mode 100644 index 0000000000000..bbf7ab0ff59c0 --- /dev/null +++ b/sql/core/src/test/scala/org/apache/spark/sql/execution/command/AlterTableUnsetTblPropertiesParserSuite.scala @@ -0,0 +1,63 @@ +/* + * 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.SparkThrowable +import org.apache.spark.sql.catalyst.analysis.{AnalysisTest, UnresolvedTable} +import org.apache.spark.sql.catalyst.parser.CatalystSqlParser.parsePlan +import org.apache.spark.sql.catalyst.parser.ParseException +import org.apache.spark.sql.catalyst.plans.logical.UnsetTableProperties +import org.apache.spark.sql.test.SharedSparkSession + +class AlterTableUnsetTblPropertiesParserSuite extends AnalysisTest with SharedSparkSession { + + private def parseException(sqlText: String): SparkThrowable = { + intercept[ParseException](sql(sqlText).collect()) + } + + // ALTER TABLE table_name UNSET TBLPROPERTIES [IF EXISTS] ('comment', 'key'); + test("alter table: alter table properties") { + val sql1 = "ALTER TABLE table_name UNSET TBLPROPERTIES ('comment', 'test')" + val sql2 = "ALTER TABLE table_name UNSET TBLPROPERTIES IF EXISTS ('comment', 'test')" + + comparePlans( + parsePlan(sql1), + UnsetTableProperties( + UnresolvedTable(Seq("table_name"), "ALTER TABLE ... UNSET TBLPROPERTIES", true), + Seq("comment", "test"), + ifExists = false)) + comparePlans( + parsePlan(sql2), + UnsetTableProperties( + UnresolvedTable(Seq("table_name"), "ALTER TABLE ... UNSET TBLPROPERTIES", true), + Seq("comment", "test"), + ifExists = true)) + } + + test("alter table unset properties - property values must NOT be set") { + val sql = "ALTER TABLE my_tab UNSET TBLPROPERTIES('key_without_value', 'key_with_value'='x')" + checkError( + exception = parseException(sql), + errorClass = "_LEGACY_ERROR_TEMP_0035", + parameters = Map("message" -> "Values should not be specified for key(s): [key_with_value]"), + context = ExpectedContext( + fragment = sql, + start = 0, + stop = 80)) + } +} diff --git a/sql/core/src/test/scala/org/apache/spark/sql/execution/command/AlterTableUnsetTblPropertiesSuiteBase.scala b/sql/core/src/test/scala/org/apache/spark/sql/execution/command/AlterTableUnsetTblPropertiesSuiteBase.scala new file mode 100644 index 0000000000000..1efc3a1fdd81e --- /dev/null +++ b/sql/core/src/test/scala/org/apache/spark/sql/execution/command/AlterTableUnsetTblPropertiesSuiteBase.scala @@ -0,0 +1,144 @@ +/* + * 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.TableIdentifier +import org.apache.spark.sql.catalyst.parser.ParseException +import org.apache.spark.sql.connector.catalog.{CatalogV2Util, TableCatalog} +import org.apache.spark.sql.errors.DataTypeErrors.toSQLId +import org.apache.spark.sql.internal.SQLConf + +/** + * This base suite contains unified tests for the `ALTER TABLE .. UNSET TBLPROPERTIES` + * 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.AlterTableUnsetTblPropertiesSuite` + * - V1 table catalog tests: + * `org.apache.spark.sql.execution.command.v1.AlterTableUnsetTblPropertiesSuiteBase` + * - V1 In-Memory catalog: + * `org.apache.spark.sql.execution.command.v1.AlterTableUnsetTblPropertiesSuite` + * - V1 Hive External catalog: + * `org.apache.spark.sql.hive.execution.command.AlterTableUnsetTblPropertiesSuite` + */ +trait AlterTableUnsetTblPropertiesSuiteBase extends QueryTest with DDLCommandTestUtils { + override val command = "ALTER TABLE .. UNSET TBLPROPERTIES" + + def checkTblProps(tableIdent: TableIdentifier, expectedTblProps: Map[String, String]): Unit + + def getTblPropertyValue(tableIdent: TableIdentifier, key: String): String + + test("table to alter does not exist") { + withNamespaceAndTable("ns", "does_not_exist") { t => + val sqlText = s"ALTER TABLE $t UNSET TBLPROPERTIES ('k1')" + checkError( + exception = intercept[AnalysisException] { + sql(sqlText) + }, + errorClass = "TABLE_OR_VIEW_NOT_FOUND", + parameters = Map("relationName" -> toSQLId(t)), + context = ExpectedContext( + fragment = t, + start = 12, + stop = 11 + t.length) + ) + } + } + + test("alter table unset tblproperties") { + withNamespaceAndTable("ns", "tbl") { t => + sql(s"CREATE TABLE $t (col1 int, col2 string, a int, b int) $defaultUsing") + val tableIdent = TableIdentifier("tbl", Some("ns"), Some(catalog)) + checkTblProps(tableIdent, Map.empty[String, String]) + + sql(s"ALTER TABLE $t SET TBLPROPERTIES ('k1' = 'v1', 'k2' = 'v2', 'k3' = 'v3', 'k4' = 'v4')") + checkTblProps(tableIdent, Map("k1" -> "v1", "k2" -> "v2", "k3" -> "v3", "k4" -> "v4")) + + // unset table properties + sql(s"ALTER TABLE $t UNSET TBLPROPERTIES ('k1')") + checkTblProps(tableIdent, Map("k2" -> "v2", "k3" -> "v3", "k4" -> "v4")) + + // unset table properties without explicitly specifying database + sql(s"USE $catalog.ns") + sql(s"ALTER TABLE tbl UNSET TBLPROPERTIES ('k2')") + checkTblProps(tableIdent, Map("k3" -> "v3", "k4" -> "v4")) + + // property to unset does not exist + checkError( + exception = intercept[AnalysisException] { + sql(s"ALTER TABLE $t UNSET TBLPROPERTIES ('k3', 'k5')") + }, + errorClass = "UNSET_NONEXISTENT_PROPERTIES", + parameters = Map("properties" -> "`k5`", "table" -> toSQLId(tableIdent.nameParts)) + ) + + // property to unset does not exist, but "IF EXISTS" is specified + sql(s"ALTER TABLE $t UNSET TBLPROPERTIES IF EXISTS ('k3', 'k5')") + checkTblProps(tableIdent, Map("k4" -> "v4")) + } + } + + test("test reserved properties") { + import TableCatalog._ + val keyParameters = Map[String, String]( + PROP_PROVIDER -> "please use the USING clause to specify it", + PROP_LOCATION -> "please use the LOCATION clause to specify it", + PROP_OWNER -> "it will be set to the current user", + PROP_EXTERNAL -> "please use CREATE EXTERNAL TABLE" + ) + withSQLConf((SQLConf.LEGACY_PROPERTY_NON_RESERVED.key, "false")) { + CatalogV2Util.TABLE_RESERVED_PROPERTIES.filterNot(_ == PROP_COMMENT).foreach { key => + withNamespaceAndTable("ns", "tbl") { t => + val sqlText = s"ALTER TABLE $t UNSET TBLPROPERTIES ('$key')" + checkError( + exception = intercept[ParseException] { + sql(sqlText) + }, + errorClass = "UNSUPPORTED_FEATURE.SET_TABLE_PROPERTY", + parameters = Map( + "property" -> key, + "msg" -> keyParameters.getOrElse( + key, "please remove it from the TBLPROPERTIES list.")), + context = ExpectedContext( + fragment = sqlText, + start = 0, + stop = 36 + t.length + key.length)) + } + } + } + withSQLConf((SQLConf.LEGACY_PROPERTY_NON_RESERVED.key, "true")) { + CatalogV2Util.TABLE_RESERVED_PROPERTIES.filterNot(_ == PROP_COMMENT).foreach { key => + Seq("OPTIONS", "TBLPROPERTIES").foreach { clause => + withNamespaceAndTable("ns", "tbl") { t => + sql(s"CREATE TABLE $t (key int) USING parquet $clause ('$key'='bar')") + val tableIdent = TableIdentifier("tbl", Some("ns"), Some(catalog)) + + val originValue = getTblPropertyValue(tableIdent, key) + assert(originValue != "bar", "reserved properties should not have side effects") + + sql(s"ALTER TABLE $t UNSET TBLPROPERTIES ('$key')") + assert(getTblPropertyValue(tableIdent, key) == originValue, + "reserved properties should not have side effects") + } + } + } + } + } +} diff --git a/sql/core/src/test/scala/org/apache/spark/sql/execution/command/DDLParserSuite.scala b/sql/core/src/test/scala/org/apache/spark/sql/execution/command/DDLParserSuite.scala index 505f0b4bdea62..70276051defa9 100644 --- a/sql/core/src/test/scala/org/apache/spark/sql/execution/command/DDLParserSuite.scala +++ b/sql/core/src/test/scala/org/apache/spark/sql/execution/command/DDLParserSuite.scala @@ -107,18 +107,6 @@ class DDLParserSuite extends AnalysisTest with SharedSparkSession { stop = 98)) } - test("alter table unset properties - property values must NOT be set") { - val sql = "ALTER TABLE my_tab UNSET TBLPROPERTIES('key_without_value', 'key_with_value'='x')" - checkError( - exception = parseException(sql), - errorClass = "_LEGACY_ERROR_TEMP_0035", - parameters = Map("message" -> "Values should not be specified for key(s): [key_with_value]"), - context = ExpectedContext( - fragment = sql, - start = 0, - stop = 80)) - } - test("alter table: exchange partition (not supported)") { val sql = """ALTER TABLE table_name_1 EXCHANGE PARTITION diff --git a/sql/core/src/test/scala/org/apache/spark/sql/execution/command/DDLSuite.scala b/sql/core/src/test/scala/org/apache/spark/sql/execution/command/DDLSuite.scala index 553b68bec52fe..bc592e4600203 100644 --- a/sql/core/src/test/scala/org/apache/spark/sql/execution/command/DDLSuite.scala +++ b/sql/core/src/test/scala/org/apache/spark/sql/execution/command/DDLSuite.scala @@ -327,10 +327,6 @@ abstract class DDLSuite extends QueryTest with DDLSuiteBase { protected val reversedProperties = Seq(PROP_OWNER) - test("alter table: unset properties (datasource table)") { - testUnsetProperties(isDatasourceTable = true) - } - test("ALTER TABLE UNSET nonexistent property should throw an exception") { val tableName = "test_table" withTable(tableName) { @@ -1113,52 +1109,6 @@ abstract class DDLSuite extends QueryTest with DDLSuiteBase { ) } - protected def testUnsetProperties(isDatasourceTable: Boolean): Unit = { - if (!isUsingHiveMetastore) { - assert(isDatasourceTable, "InMemoryCatalog only supports data source tables") - } - val catalog = spark.sessionState.catalog - val tableIdent = TableIdentifier("tab1", Some("dbx")) - createDatabase(catalog, "dbx") - createTable(catalog, tableIdent, isDatasourceTable) - def getProps: Map[String, String] = { - if (isUsingHiveMetastore) { - normalizeCatalogTable(catalog.getTableMetadata(tableIdent)).properties - } else { - catalog.getTableMetadata(tableIdent).properties - } - } - // unset table properties - sql("ALTER TABLE dbx.tab1 SET TBLPROPERTIES ('j' = 'am', 'p' = 'an', 'c' = 'lan', 'x' = 'y')") - sql("ALTER TABLE dbx.tab1 UNSET TBLPROPERTIES ('j')") - assert(getProps == Map("p" -> "an", "c" -> "lan", "x" -> "y")) - // unset table properties without explicitly specifying database - catalog.setCurrentDatabase("dbx") - sql("ALTER TABLE tab1 UNSET TBLPROPERTIES ('p')") - assert(getProps == Map("c" -> "lan", "x" -> "y")) - // table to alter does not exist - val sql1 = "ALTER TABLE does_not_exist UNSET TBLPROPERTIES ('c' = 'lan')" - checkError( - exception = intercept[ParseException] { - sql(sql1) - }, - errorClass = "_LEGACY_ERROR_TEMP_0035", - parameters = Map("message" -> "Values should not be specified for key(s): [c]"), - context = ExpectedContext(fragment = sql1, start = 0, stop = 59) - ) - // property to unset does not exist - checkError( - exception = intercept[AnalysisException] { - sql("ALTER TABLE tab1 UNSET TBLPROPERTIES ('c', 'xyz')") - }, - errorClass = "UNSET_NONEXISTENT_PROPERTIES", - parameters = Map("properties" -> "`xyz`", "table" -> "`spark_catalog`.`dbx`.`tab1`") - ) - // property to unset does not exist, but "IF EXISTS" is specified - sql("ALTER TABLE tab1 UNSET TBLPROPERTIES IF EXISTS ('c', 'xyz')") - assert(getProps == Map("x" -> "y")) - } - protected def testChangeColumn(isDatasourceTable: Boolean): Unit = { if (!isUsingHiveMetastore) { assert(isDatasourceTable, "InMemoryCatalog only supports data source tables") diff --git a/sql/core/src/test/scala/org/apache/spark/sql/execution/command/v1/AlterTableSetTblPropertiesSuite.scala b/sql/core/src/test/scala/org/apache/spark/sql/execution/command/v1/AlterTableSetTblPropertiesSuite.scala index e74e5d4fc9ea5..cc59ee237daf3 100644 --- a/sql/core/src/test/scala/org/apache/spark/sql/execution/command/v1/AlterTableSetTblPropertiesSuite.scala +++ b/sql/core/src/test/scala/org/apache/spark/sql/execution/command/v1/AlterTableSetTblPropertiesSuite.scala @@ -56,6 +56,10 @@ trait AlterTableSetTblPropertiesSuiteBase extends command.AlterTableSetTblProper assert(actualTblProps == expectedTblProps) } } + + override def getTblPropertyValue(tableIdent: TableIdentifier, key: String): String = { + getTableProperties(tableIdent).getOrElse(key, null) + } } class AlterTableSetTblPropertiesSuite diff --git a/sql/core/src/test/scala/org/apache/spark/sql/execution/command/v1/AlterTableUnsetTblPropertiesSuite.scala b/sql/core/src/test/scala/org/apache/spark/sql/execution/command/v1/AlterTableUnsetTblPropertiesSuite.scala new file mode 100644 index 0000000000000..0f7b4a0aad62e --- /dev/null +++ b/sql/core/src/test/scala/org/apache/spark/sql/execution/command/v1/AlterTableUnsetTblPropertiesSuite.scala @@ -0,0 +1,65 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.spark.sql.execution.command.v1 + +import org.apache.spark.sql.catalyst.TableIdentifier +import org.apache.spark.sql.execution.command +import org.apache.spark.sql.internal.StaticSQLConf.CATALOG_IMPLEMENTATION + +/** + * This base suite contains unified tests for the `ALTER TABLE .. UNSET TBLPROPERTIES` + * command that check V1 table catalogs. The tests that cannot run for all V1 catalogs + * are located in more specific test suites: + * + * - V1 In-Memory catalog: + * `org.apache.spark.sql.execution.command.v1.AlterTableUnsetTblPropertiesSuite` + * - V1 Hive External catalog: + * `org.apache.spark.sql.hive.execution.command.AlterTableUnsetTblPropertiesSuite` + */ +trait AlterTableUnsetTblPropertiesSuiteBase extends command.AlterTableUnsetTblPropertiesSuiteBase { + private[sql] lazy val sessionCatalog = spark.sessionState.catalog + + private def isUsingHiveMetastore: Boolean = { + spark.sparkContext.conf.get(CATALOG_IMPLEMENTATION) == "hive" + } + + private def normalizeTblProps(props: Map[String, String]): Map[String, String] = { + props.filterNot(p => Seq("transient_lastDdlTime").contains(p._1)) + } + + private def getTableProperties(tableIdent: TableIdentifier): Map[String, String] = { + sessionCatalog.getTableMetadata(tableIdent).properties + } + + override def checkTblProps(tableIdent: TableIdentifier, + expectedTblProps: Map[String, String]): Unit = { + val actualTblProps = getTableProperties(tableIdent) + if (isUsingHiveMetastore) { + assert(normalizeTblProps(actualTblProps) == expectedTblProps) + } else { + assert(actualTblProps == expectedTblProps) + } + } + + override def getTblPropertyValue(tableIdent: TableIdentifier, key: String): String = { + getTableProperties(tableIdent).getOrElse(key, null) + } +} + +class AlterTableUnsetTblPropertiesSuite + extends AlterTableUnsetTblPropertiesSuiteBase with CommandSuiteBase diff --git a/sql/core/src/test/scala/org/apache/spark/sql/execution/command/v2/AlterTableSetTblPropertiesSuite.scala b/sql/core/src/test/scala/org/apache/spark/sql/execution/command/v2/AlterTableSetTblPropertiesSuite.scala index 7d7b2ad8686ee..22e19216d06f4 100644 --- a/sql/core/src/test/scala/org/apache/spark/sql/execution/command/v2/AlterTableSetTblPropertiesSuite.scala +++ b/sql/core/src/test/scala/org/apache/spark/sql/execution/command/v2/AlterTableSetTblPropertiesSuite.scala @@ -47,4 +47,8 @@ class AlterTableSetTblPropertiesSuite val actualTblProps = getTableMetadata(tableIdent).properties.asScala.toMap assert(normalizeTblProps(actualTblProps) === expectedTblProps) } + + override def getTblPropertyValue(tableIdent: TableIdentifier, key: String): String = { + getTableMetadata(tableIdent).properties.asScala.toMap.getOrElse(key, null) + } } diff --git a/sql/core/src/test/scala/org/apache/spark/sql/execution/command/v2/AlterTableUnsetTblPropertiesSuite.scala b/sql/core/src/test/scala/org/apache/spark/sql/execution/command/v2/AlterTableUnsetTblPropertiesSuite.scala new file mode 100644 index 0000000000000..699d8185ee6f0 --- /dev/null +++ b/sql/core/src/test/scala/org/apache/spark/sql/execution/command/v2/AlterTableUnsetTblPropertiesSuite.scala @@ -0,0 +1,54 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.spark.sql.execution.command.v2 + +import scala.jdk.CollectionConverters._ + +import org.apache.spark.sql.catalyst.TableIdentifier +import org.apache.spark.sql.connector.catalog.{Identifier, Table} +import org.apache.spark.sql.connector.catalog.CatalogV2Implicits.CatalogHelper +import org.apache.spark.sql.execution.command + +/** + * The class contains tests for the `ALTER TABLE .. UNSET TBLPROPERTIES` command to + * check V2 table catalogs. + */ +class AlterTableUnsetTblPropertiesSuite + extends command.AlterTableUnsetTblPropertiesSuiteBase with CommandSuiteBase { + + private def normalizeTblProps(props: Map[String, String]): Map[String, String] = { + props.filterNot(p => Seq("provider", "owner").contains(p._1)) + } + + private def getTableMetadata(tableIndent: TableIdentifier): Table = { + val nameParts = tableIndent.nameParts + val v2Catalog = spark.sessionState.catalogManager.catalog(nameParts.head).asTableCatalog + val namespace = nameParts.drop(1).init.toArray + v2Catalog.loadTable(Identifier.of(namespace, nameParts.last)) + } + + override def checkTblProps(tableIdent: TableIdentifier, + expectedTblProps: Map[String, String]): Unit = { + val actualTblProps = getTableMetadata(tableIdent).properties.asScala.toMap + assert(normalizeTblProps(actualTblProps) === expectedTblProps) + } + + override def getTblPropertyValue(tableIdent: TableIdentifier, key: String): String = { + getTableMetadata(tableIdent).properties.asScala.toMap.getOrElse(key, null) + } +} diff --git a/sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/v2/V2SessionCatalogSuite.scala b/sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/v2/V2SessionCatalogSuite.scala index 4de74af250006..a725a8fea4adf 100644 --- a/sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/v2/V2SessionCatalogSuite.scala +++ b/sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/v2/V2SessionCatalogSuite.scala @@ -323,7 +323,7 @@ class V2SessionCatalogTableSuite extends V2SessionCatalogBaseSuite { assert(filterV2TableProperties(table.properties) == Map("prop-1" -> "1")) - catalog.alterTable(testIdent, TableChange.removeProperty("prop-1")) + catalog.alterTable(testIdent, TableChange.removeProperty("prop-1", false)) val updated = catalog.loadTable(testIdent) assert(filterV2TableProperties(updated.properties) == Map()) @@ -341,7 +341,7 @@ class V2SessionCatalogTableSuite extends V2SessionCatalogBaseSuite { assert(filterV2TableProperties(table.properties) == Map()) - catalog.alterTable(testIdent, TableChange.removeProperty("prop-1")) + catalog.alterTable(testIdent, TableChange.removeProperty("prop-1", true)) val updated = catalog.loadTable(testIdent) assert(filterV2TableProperties(updated.properties) == Map()) diff --git a/sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveDDLSuite.scala b/sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveDDLSuite.scala index 65b70ad8bcaeb..ff269cba8a67f 100644 --- a/sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveDDLSuite.scala +++ b/sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveDDLSuite.scala @@ -154,10 +154,6 @@ class HiveDDLSuite fs.exists(filesystemPath) } - test("alter table: unset properties") { - testUnsetProperties(isDatasourceTable = false) - } - test("alter table: change column") { testChangeColumn(isDatasourceTable = false) } diff --git a/sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/command/AlterTableUnsetTblPropertiesSuite.scala b/sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/command/AlterTableUnsetTblPropertiesSuite.scala new file mode 100644 index 0000000000000..d4e029eef2e02 --- /dev/null +++ b/sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/command/AlterTableUnsetTblPropertiesSuite.scala @@ -0,0 +1,27 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.spark.sql.hive.execution.command + +import org.apache.spark.sql.execution.command.v1 + +/** + * The class contains tests for the `ALTER TABLE .. UNSET TBLPROPERTIES` command to check + * V1 Hive external table catalog. + */ +class AlterTableUnsetTblPropertiesSuite + extends v1.AlterTableUnsetTblPropertiesSuiteBase with CommandSuiteBase From 0de8ddfb6c53adcd6ee49309dfc0934e9367b996 Mon Sep 17 00:00:00 2001 From: panbingkun Date: Tue, 2 Jul 2024 11:32:00 +0800 Subject: [PATCH 2/6] update --- .../sql/connector/catalog/TableChange.java | 15 +++-------- .../plans/logical/v2AlterTableCommands.scala | 2 +- .../sql/connector/catalog/CatalogV2Util.scala | 17 +------------ .../sql/errors/QueryCompilationErrors.scala | 4 +-- .../sql/connector/catalog/CatalogSuite.scala | 4 +-- .../catalog/InMemoryTableCatalog.scala | 2 +- .../spark/sql/execution/command/ddl.scala | 2 +- .../datasources/v2/V2SessionCatalog.scala | 3 +-- ...SourceV2DataFrameSessionCatalogSuite.scala | 3 +-- .../AlterTableSetTblPropertiesSuiteBase.scala | 4 +-- ...erTableUnsetTblPropertiesParserSuite.scala | 6 +++-- ...lterTableUnsetTblPropertiesSuiteBase.scala | 17 ++----------- .../AlterTableUnsetTblPropertiesSuite.scala | 25 +++++++++++++++++++ .../AlterTableUnsetTblPropertiesSuite.scala | 20 +++++++++++++++ .../v2/V2SessionCatalogSuite.scala | 4 +-- 15 files changed, 69 insertions(+), 59 deletions(-) diff --git a/sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/TableChange.java b/sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/TableChange.java index 32e9b176017b1..ebecb6f507e6a 100644 --- a/sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/TableChange.java +++ b/sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/TableChange.java @@ -61,11 +61,10 @@ static TableChange setProperty(String property, String value) { * If the property does not exist, the change will succeed. * * @param property the property name - * @param ifExists silence the error if property doesn't exist during unset * @return a TableChange for the addition */ - static TableChange removeProperty(String property, boolean ifExists) { - return new RemoveProperty(property, ifExists); + static TableChange removeProperty(String property) { + return new RemoveProperty(property); } /** @@ -293,27 +292,21 @@ public int hashCode() { */ final class RemoveProperty implements TableChange { private final String property; - private final boolean ifExists; - private RemoveProperty(String property, boolean ifExists) { + private RemoveProperty(String property) { this.property = property; - this.ifExists = ifExists; } public String property() { return property; } - public boolean ifExists() { - return ifExists; - } - @Override public boolean equals(Object o) { if (this == o) return true; if (o == null || getClass() != o.getClass()) return false; RemoveProperty that = (RemoveProperty) o; - return property.equals(that.property) && that.ifExists() == this.ifExists(); + return property.equals(that.property); } @Override diff --git a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/v2AlterTableCommands.scala b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/v2AlterTableCommands.scala index ed2153d82edd1..9c66e68d686d5 100644 --- a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/v2AlterTableCommands.scala +++ b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/v2AlterTableCommands.scala @@ -91,7 +91,7 @@ case class UnsetTableProperties( propertyKeys: Seq[String], ifExists: Boolean) extends AlterTableCommand { override def changes: Seq[TableChange] = { - propertyKeys.map(key => TableChange.removeProperty(key, ifExists)) + propertyKeys.map(key => TableChange.removeProperty(key)) } override protected def withNewChildInternal(newChild: LogicalPlan): LogicalPlan = copy(table = newChild) diff --git a/sql/catalyst/src/main/scala/org/apache/spark/sql/connector/catalog/CatalogV2Util.scala b/sql/catalyst/src/main/scala/org/apache/spark/sql/connector/catalog/CatalogV2Util.scala index f6af4626b708b..f36310e8ad899 100644 --- a/sql/catalyst/src/main/scala/org/apache/spark/sql/connector/catalog/CatalogV2Util.scala +++ b/sql/catalyst/src/main/scala/org/apache/spark/sql/connector/catalog/CatalogV2Util.scala @@ -20,7 +20,6 @@ package org.apache.spark.sql.connector.catalog import java.util import java.util.Collections -import scala.collection.mutable import scala.jdk.CollectionConverters._ import org.apache.spark.SparkIllegalArgumentException @@ -34,7 +33,6 @@ import org.apache.spark.sql.catalyst.util.ResolveDefaultColumns._ import org.apache.spark.sql.connector.catalog.TableChange._ import org.apache.spark.sql.connector.catalog.functions.UnboundFunction import org.apache.spark.sql.connector.expressions.LiteralValue -import org.apache.spark.sql.errors.QueryCompilationErrors import org.apache.spark.sql.execution.datasources.v2.DataSourceV2Relation import org.apache.spark.sql.types.{ArrayType, MapType, Metadata, MetadataBuilder, StructField, StructType} import org.apache.spark.sql.util.CaseInsensitiveStringMap @@ -109,43 +107,30 @@ private[sql] object CatalogV2Util { * Apply properties changes to a map and return the result. */ def applyPropertiesChanges( - catalogName: String, - ident: Identifier, properties: Map[String, String], changes: Seq[TableChange]): Map[String, String] = { - applyPropertiesChanges(catalogName, ident, properties.asJava, changes).asScala.toMap + applyPropertiesChanges(properties.asJava, changes).asScala.toMap } /** * Apply properties changes to a Java map and return the result. */ def applyPropertiesChanges( - catalogName: String, - ident: Identifier, properties: util.Map[String, String], changes: Seq[TableChange]): util.Map[String, String] = { val newProperties = new util.HashMap[String, String](properties) - val nonexistentKeys = mutable.ArrayBuilder.make[String] changes.foreach { case set: SetProperty => newProperties.put(set.property, set.value) case unset: RemoveProperty => - if (!unset.ifExists && !properties.containsKey(unset.property)) { - nonexistentKeys += unset.property - } newProperties.remove(unset.property) case _ => // ignore non-property changes } - if (!nonexistentKeys.result().isEmpty) { - throw QueryCompilationErrors.unsetNonExistentPropertiesError( - nonexistentKeys.result().toSeq, catalogName +: ident.asMultipartIdentifier) - } - Collections.unmodifiableMap(newProperties) } diff --git a/sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryCompilationErrors.scala b/sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryCompilationErrors.scala index 04b94691a0516..d3bd265d0459e 100644 --- a/sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryCompilationErrors.scala +++ b/sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryCompilationErrors.scala @@ -2659,12 +2659,12 @@ private[sql] object QueryCompilationErrors extends QueryErrorsBase with Compilat } def unsetNonExistentPropertiesError( - properties: Seq[String], nameParts: Seq[String]): Throwable = { + properties: Seq[String], table: TableIdentifier): Throwable = { new AnalysisException( errorClass = "UNSET_NONEXISTENT_PROPERTIES", messageParameters = Map( "properties" -> properties.map(toSQLId).mkString(", "), - "table" -> toSQLId(nameParts)) + "table" -> toSQLId(table.nameParts)) ) } diff --git a/sql/catalyst/src/test/scala/org/apache/spark/sql/connector/catalog/CatalogSuite.scala b/sql/catalyst/src/test/scala/org/apache/spark/sql/connector/catalog/CatalogSuite.scala index b6474549f0f94..e20dfd4f60512 100644 --- a/sql/catalyst/src/test/scala/org/apache/spark/sql/connector/catalog/CatalogSuite.scala +++ b/sql/catalyst/src/test/scala/org/apache/spark/sql/connector/catalog/CatalogSuite.scala @@ -269,7 +269,7 @@ class CatalogSuite extends SparkFunSuite { assert(table.properties.asScala == Map("prop-1" -> "1")) - val updated = catalog.alterTable(testIdent, TableChange.removeProperty("prop-1", false)) + val updated = catalog.alterTable(testIdent, TableChange.removeProperty("prop-1")) assert(updated.properties.asScala == Map()) val loaded = catalog.loadTable(testIdent) @@ -285,7 +285,7 @@ class CatalogSuite extends SparkFunSuite { assert(table.properties.asScala == Map()) - val updated = catalog.alterTable(testIdent, TableChange.removeProperty("prop-1", true)) + val updated = catalog.alterTable(testIdent, TableChange.removeProperty("prop-1")) assert(updated.properties.asScala == Map()) val loaded = catalog.loadTable(testIdent) diff --git a/sql/catalyst/src/test/scala/org/apache/spark/sql/connector/catalog/InMemoryTableCatalog.scala b/sql/catalyst/src/test/scala/org/apache/spark/sql/connector/catalog/InMemoryTableCatalog.scala index 0232446c08fa4..d511477ef5d33 100644 --- a/sql/catalyst/src/test/scala/org/apache/spark/sql/connector/catalog/InMemoryTableCatalog.scala +++ b/sql/catalyst/src/test/scala/org/apache/spark/sql/connector/catalog/InMemoryTableCatalog.scala @@ -131,7 +131,7 @@ class BasicInMemoryTableCatalog extends TableCatalog { override def alterTable(ident: Identifier, changes: TableChange*): Table = { val table = loadTable(ident).asInstanceOf[InMemoryTable] - val properties = CatalogV2Util.applyPropertiesChanges(name, ident, table.properties, changes) + val properties = CatalogV2Util.applyPropertiesChanges(table.properties, changes) val schema = CatalogV2Util.applySchemaChanges(table.schema, changes, None, "ALTER TABLE") // fail if the last column in the schema was dropped diff --git a/sql/core/src/main/scala/org/apache/spark/sql/execution/command/ddl.scala b/sql/core/src/main/scala/org/apache/spark/sql/execution/command/ddl.scala index 5f639bc5665ae..6f402188910e0 100644 --- a/sql/core/src/main/scala/org/apache/spark/sql/execution/command/ddl.scala +++ b/sql/core/src/main/scala/org/apache/spark/sql/execution/command/ddl.scala @@ -340,7 +340,7 @@ case class AlterTableUnsetPropertiesCommand( && key != TableCatalog.PROP_COMMENT) if (nonexistentKeys.nonEmpty) { throw QueryCompilationErrors.unsetNonExistentPropertiesError( - nonexistentKeys, table.identifier.nameParts) + nonexistentKeys, table.identifier) } } // If comment is in the table property, we reset it to None diff --git a/sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/V2SessionCatalog.scala b/sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/V2SessionCatalog.scala index bc70868bb8f86..3d6de985a62f5 100644 --- a/sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/V2SessionCatalog.scala +++ b/sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/V2SessionCatalog.scala @@ -272,8 +272,7 @@ class V2SessionCatalog(catalog: SessionCatalog) throw QueryCompilationErrors.noSuchTableError(ident) } - val properties = CatalogV2Util.applyPropertiesChanges( - name, ident, catalogTable.properties, changes) + val properties = CatalogV2Util.applyPropertiesChanges(catalogTable.properties, changes) val schema = CatalogV2Util.applySchemaChanges( catalogTable.schema, changes, catalogTable.provider, "ALTER TABLE") val comment = properties.get(TableCatalog.PROP_COMMENT) diff --git a/sql/core/src/test/scala/org/apache/spark/sql/connector/DataSourceV2DataFrameSessionCatalogSuite.scala b/sql/core/src/test/scala/org/apache/spark/sql/connector/DataSourceV2DataFrameSessionCatalogSuite.scala index e8f6acfe98ffc..7bbb6485c273f 100644 --- a/sql/core/src/test/scala/org/apache/spark/sql/connector/DataSourceV2DataFrameSessionCatalogSuite.scala +++ b/sql/core/src/test/scala/org/apache/spark/sql/connector/DataSourceV2DataFrameSessionCatalogSuite.scala @@ -109,8 +109,7 @@ class InMemoryTableSessionCatalog extends TestV2SessionCatalogBase[InMemoryTable override def alterTable(ident: Identifier, changes: TableChange*): Table = { Option(tables.get(ident)) match { case Some(table) => - val properties = CatalogV2Util.applyPropertiesChanges( - name, ident, table.properties, changes) + val properties = CatalogV2Util.applyPropertiesChanges(table.properties, changes) val provider = Option(properties.get("provider")) val schema = CatalogV2Util.applySchemaChanges( diff --git a/sql/core/src/test/scala/org/apache/spark/sql/execution/command/AlterTableSetTblPropertiesSuiteBase.scala b/sql/core/src/test/scala/org/apache/spark/sql/execution/command/AlterTableSetTblPropertiesSuiteBase.scala index f1fda2d22bf89..ac3c84dff718c 100644 --- a/sql/core/src/test/scala/org/apache/spark/sql/execution/command/AlterTableSetTblPropertiesSuiteBase.scala +++ b/sql/core/src/test/scala/org/apache/spark/sql/execution/command/AlterTableSetTblPropertiesSuiteBase.scala @@ -62,7 +62,7 @@ trait AlterTableSetTblPropertiesSuiteBase extends QueryTest with DDLCommandTestU } } - test("alter table set tblproperties") { + test("alter table set properties") { withNamespaceAndTable("ns", "tbl") { t => sql(s"CREATE TABLE $t (col1 int, col2 string, a int, b int) $defaultUsing") val tableIdent = TableIdentifier("tbl", Some("ns"), Some(catalog)) @@ -80,7 +80,7 @@ trait AlterTableSetTblPropertiesSuiteBase extends QueryTest with DDLCommandTestU } } - test("test reserved properties") { + test("alter table set reserved properties") { import TableCatalog._ val keyParameters = Map[String, String]( PROP_PROVIDER -> "please use the USING clause to specify it", diff --git a/sql/core/src/test/scala/org/apache/spark/sql/execution/command/AlterTableUnsetTblPropertiesParserSuite.scala b/sql/core/src/test/scala/org/apache/spark/sql/execution/command/AlterTableUnsetTblPropertiesParserSuite.scala index bbf7ab0ff59c0..1e675a64f2235 100644 --- a/sql/core/src/test/scala/org/apache/spark/sql/execution/command/AlterTableUnsetTblPropertiesParserSuite.scala +++ b/sql/core/src/test/scala/org/apache/spark/sql/execution/command/AlterTableUnsetTblPropertiesParserSuite.scala @@ -38,13 +38,15 @@ class AlterTableUnsetTblPropertiesParserSuite extends AnalysisTest with SharedSp comparePlans( parsePlan(sql1), UnsetTableProperties( - UnresolvedTable(Seq("table_name"), "ALTER TABLE ... UNSET TBLPROPERTIES", true), + UnresolvedTable(Seq("table_name"), "ALTER TABLE ... UNSET TBLPROPERTIES", + suggestAlternative = true), Seq("comment", "test"), ifExists = false)) comparePlans( parsePlan(sql2), UnsetTableProperties( - UnresolvedTable(Seq("table_name"), "ALTER TABLE ... UNSET TBLPROPERTIES", true), + UnresolvedTable(Seq("table_name"), "ALTER TABLE ... UNSET TBLPROPERTIES", + suggestAlternative = true), Seq("comment", "test"), ifExists = true)) } diff --git a/sql/core/src/test/scala/org/apache/spark/sql/execution/command/AlterTableUnsetTblPropertiesSuiteBase.scala b/sql/core/src/test/scala/org/apache/spark/sql/execution/command/AlterTableUnsetTblPropertiesSuiteBase.scala index 1efc3a1fdd81e..7292019c6df54 100644 --- a/sql/core/src/test/scala/org/apache/spark/sql/execution/command/AlterTableUnsetTblPropertiesSuiteBase.scala +++ b/sql/core/src/test/scala/org/apache/spark/sql/execution/command/AlterTableUnsetTblPropertiesSuiteBase.scala @@ -62,7 +62,7 @@ trait AlterTableUnsetTblPropertiesSuiteBase extends QueryTest with DDLCommandTes } } - test("alter table unset tblproperties") { + test("alter table unset properties") { withNamespaceAndTable("ns", "tbl") { t => sql(s"CREATE TABLE $t (col1 int, col2 string, a int, b int) $defaultUsing") val tableIdent = TableIdentifier("tbl", Some("ns"), Some(catalog)) @@ -79,23 +79,10 @@ trait AlterTableUnsetTblPropertiesSuiteBase extends QueryTest with DDLCommandTes sql(s"USE $catalog.ns") sql(s"ALTER TABLE tbl UNSET TBLPROPERTIES ('k2')") checkTblProps(tableIdent, Map("k3" -> "v3", "k4" -> "v4")) - - // property to unset does not exist - checkError( - exception = intercept[AnalysisException] { - sql(s"ALTER TABLE $t UNSET TBLPROPERTIES ('k3', 'k5')") - }, - errorClass = "UNSET_NONEXISTENT_PROPERTIES", - parameters = Map("properties" -> "`k5`", "table" -> toSQLId(tableIdent.nameParts)) - ) - - // property to unset does not exist, but "IF EXISTS" is specified - sql(s"ALTER TABLE $t UNSET TBLPROPERTIES IF EXISTS ('k3', 'k5')") - checkTblProps(tableIdent, Map("k4" -> "v4")) } } - test("test reserved properties") { + test("alter table unset reserved properties") { import TableCatalog._ val keyParameters = Map[String, String]( PROP_PROVIDER -> "please use the USING clause to specify it", diff --git a/sql/core/src/test/scala/org/apache/spark/sql/execution/command/v1/AlterTableUnsetTblPropertiesSuite.scala b/sql/core/src/test/scala/org/apache/spark/sql/execution/command/v1/AlterTableUnsetTblPropertiesSuite.scala index 0f7b4a0aad62e..18f19abb21256 100644 --- a/sql/core/src/test/scala/org/apache/spark/sql/execution/command/v1/AlterTableUnsetTblPropertiesSuite.scala +++ b/sql/core/src/test/scala/org/apache/spark/sql/execution/command/v1/AlterTableUnsetTblPropertiesSuite.scala @@ -17,7 +17,9 @@ package org.apache.spark.sql.execution.command.v1 +import org.apache.spark.sql.AnalysisException import org.apache.spark.sql.catalyst.TableIdentifier +import org.apache.spark.sql.errors.DataTypeErrors.toSQLId import org.apache.spark.sql.execution.command import org.apache.spark.sql.internal.StaticSQLConf.CATALOG_IMPLEMENTATION @@ -59,6 +61,29 @@ trait AlterTableUnsetTblPropertiesSuiteBase extends command.AlterTableUnsetTblPr override def getTblPropertyValue(tableIdent: TableIdentifier, key: String): String = { getTableProperties(tableIdent).getOrElse(key, null) } + + test("alter table unset non-existent properties") { + withNamespaceAndTable("ns", "tbl") { t => + sql(s"CREATE TABLE $t (col1 int, col2 string, a int, b int) $defaultUsing") + val tableIdent = TableIdentifier("tbl", Some("ns"), Some(catalog)) + + sql(s"ALTER TABLE $t SET TBLPROPERTIES ('k1' = 'v1', 'k2' = 'v2', 'k3' = 'v3')") + checkTblProps(tableIdent, Map("k1" -> "v1", "k2" -> "v2", "k3" -> "v3")) + + // property to unset does not exist + checkError( + exception = intercept[AnalysisException] { + sql(s"ALTER TABLE $t UNSET TBLPROPERTIES ('k3', 'k4')") + }, + errorClass = "UNSET_NONEXISTENT_PROPERTIES", + parameters = Map("properties" -> "`k4`", "table" -> toSQLId(tableIdent.nameParts)) + ) + + // property to unset does not exist, but "IF EXISTS" is specified + sql(s"ALTER TABLE $t UNSET TBLPROPERTIES IF EXISTS ('k3', 'k4')") + checkTblProps(tableIdent, Map("k1" -> "v1", "k2" -> "v2")) + } + } } class AlterTableUnsetTblPropertiesSuite diff --git a/sql/core/src/test/scala/org/apache/spark/sql/execution/command/v2/AlterTableUnsetTblPropertiesSuite.scala b/sql/core/src/test/scala/org/apache/spark/sql/execution/command/v2/AlterTableUnsetTblPropertiesSuite.scala index 699d8185ee6f0..b70dd7094fc4a 100644 --- a/sql/core/src/test/scala/org/apache/spark/sql/execution/command/v2/AlterTableUnsetTblPropertiesSuite.scala +++ b/sql/core/src/test/scala/org/apache/spark/sql/execution/command/v2/AlterTableUnsetTblPropertiesSuite.scala @@ -51,4 +51,24 @@ class AlterTableUnsetTblPropertiesSuite override def getTblPropertyValue(tableIdent: TableIdentifier, key: String): String = { getTableMetadata(tableIdent).properties.asScala.toMap.getOrElse(key, null) } + + test("alter table unset non-existent properties") { + withNamespaceAndTable("ns", "tbl") { t => + sql(s"CREATE TABLE $t (col1 int, col2 string, a int, b int) $defaultUsing") + val tableIdent = TableIdentifier("tbl", Some("ns"), Some(catalog)) + + sql(s"ALTER TABLE $t SET TBLPROPERTIES ('k1' = 'v1', 'k2' = 'v2', 'k3' = 'v3')") + checkTblProps(tableIdent, Map("k1" -> "v1", "k2" -> "v2", "k3" -> "v3")) + + // when using the v2 command, when unsetting `non-existent` properties, + // The command will ignore `non-existent` properties and finally succeed + // property to unset does not exist + sql(s"ALTER TABLE $t UNSET TBLPROPERTIES ('k3', 'k4')") + checkTblProps(tableIdent, Map("k1" -> "v1", "k2" -> "v2")) + + // property to unset does not exist, but "IF EXISTS" is specified + sql(s"ALTER TABLE $t UNSET TBLPROPERTIES IF EXISTS ('k2', 'k3')") + checkTblProps(tableIdent, Map("k1" -> "v1")) + } + } } diff --git a/sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/v2/V2SessionCatalogSuite.scala b/sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/v2/V2SessionCatalogSuite.scala index a725a8fea4adf..4de74af250006 100644 --- a/sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/v2/V2SessionCatalogSuite.scala +++ b/sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/v2/V2SessionCatalogSuite.scala @@ -323,7 +323,7 @@ class V2SessionCatalogTableSuite extends V2SessionCatalogBaseSuite { assert(filterV2TableProperties(table.properties) == Map("prop-1" -> "1")) - catalog.alterTable(testIdent, TableChange.removeProperty("prop-1", false)) + catalog.alterTable(testIdent, TableChange.removeProperty("prop-1")) val updated = catalog.loadTable(testIdent) assert(filterV2TableProperties(updated.properties) == Map()) @@ -341,7 +341,7 @@ class V2SessionCatalogTableSuite extends V2SessionCatalogBaseSuite { assert(filterV2TableProperties(table.properties) == Map()) - catalog.alterTable(testIdent, TableChange.removeProperty("prop-1", true)) + catalog.alterTable(testIdent, TableChange.removeProperty("prop-1")) val updated = catalog.loadTable(testIdent) assert(filterV2TableProperties(updated.properties) == Map()) From 5733fbbfc225b4795a0220cf23d6bc9d5fe79fe1 Mon Sep 17 00:00:00 2001 From: panbingkun Date: Tue, 2 Jul 2024 14:51:07 +0800 Subject: [PATCH 3/6] update --- docs/sql-ref-syntax-ddl-alter-table.md | 20 ++++++++++++++----- .../AlterTableUnsetTblPropertiesSuite.scala | 4 ++++ .../AlterTableUnsetTblPropertiesSuite.scala | 6 ++++-- 3 files changed, 23 insertions(+), 7 deletions(-) diff --git a/docs/sql-ref-syntax-ddl-alter-table.md b/docs/sql-ref-syntax-ddl-alter-table.md index 566e73da21513..97720bbf734f6 100644 --- a/docs/sql-ref-syntax-ddl-alter-table.md +++ b/docs/sql-ref-syntax-ddl-alter-table.md @@ -236,20 +236,30 @@ ALTER TABLE table_identifier DROP [ IF EXISTS ] partition_spec [PURGE] ### SET AND UNSET -#### SET TABLE PROPERTIES +#### SET PROPERTIES `ALTER TABLE SET` command is used for setting the table properties. If a particular property was already set, this overrides the old value with the new one. -`ALTER TABLE UNSET` is used to drop the table property. - ##### Syntax ```sql --- Set Table Properties +-- Set Properties ALTER TABLE table_identifier SET TBLPROPERTIES ( key1 = val1, key2 = val2, ... ) +``` + +#### UNSET PROPERTIES + +`ALTER TABLE UNSET` command is used to drop the table property. --- Unset Table Properties +**Note:** If the specified property key does not exist, when you use the v1 command and do not specify `IF EXISTS`, +it will throw the error-condition `UNSET_NONEXISTENT_PROPERTIES` and finally `failed`, +however regardless of whether `IF EXISTS` is set or not, the v2 command will ignore it and finally `succeed`. + +##### Syntax + +```sql +-- Unset Properties ALTER TABLE table_identifier UNSET TBLPROPERTIES [ IF EXISTS ] ( key1, key2, ... ) ``` diff --git a/sql/core/src/test/scala/org/apache/spark/sql/execution/command/v1/AlterTableUnsetTblPropertiesSuite.scala b/sql/core/src/test/scala/org/apache/spark/sql/execution/command/v1/AlterTableUnsetTblPropertiesSuite.scala index 18f19abb21256..59a21d54aa1cf 100644 --- a/sql/core/src/test/scala/org/apache/spark/sql/execution/command/v1/AlterTableUnsetTblPropertiesSuite.scala +++ b/sql/core/src/test/scala/org/apache/spark/sql/execution/command/v1/AlterTableUnsetTblPropertiesSuite.scala @@ -62,6 +62,10 @@ trait AlterTableUnsetTblPropertiesSuiteBase extends command.AlterTableUnsetTblPr getTableProperties(tableIdent).getOrElse(key, null) } + /** + * When using the v1 command to unset `non-existent` properties, the command will + * throw the error-condition `"UNSET_NONEXISTENT_PROPERTIES` and finally `failed` + */ test("alter table unset non-existent properties") { withNamespaceAndTable("ns", "tbl") { t => sql(s"CREATE TABLE $t (col1 int, col2 string, a int, b int) $defaultUsing") diff --git a/sql/core/src/test/scala/org/apache/spark/sql/execution/command/v2/AlterTableUnsetTblPropertiesSuite.scala b/sql/core/src/test/scala/org/apache/spark/sql/execution/command/v2/AlterTableUnsetTblPropertiesSuite.scala index b70dd7094fc4a..04e7a8f10385d 100644 --- a/sql/core/src/test/scala/org/apache/spark/sql/execution/command/v2/AlterTableUnsetTblPropertiesSuite.scala +++ b/sql/core/src/test/scala/org/apache/spark/sql/execution/command/v2/AlterTableUnsetTblPropertiesSuite.scala @@ -52,6 +52,10 @@ class AlterTableUnsetTblPropertiesSuite getTableMetadata(tableIdent).properties.asScala.toMap.getOrElse(key, null) } + /** + * When using the v2 command to unset `non-existent` properties, + * the command will ignore `non-existent` properties and finally succeed + */ test("alter table unset non-existent properties") { withNamespaceAndTable("ns", "tbl") { t => sql(s"CREATE TABLE $t (col1 int, col2 string, a int, b int) $defaultUsing") @@ -60,8 +64,6 @@ class AlterTableUnsetTblPropertiesSuite sql(s"ALTER TABLE $t SET TBLPROPERTIES ('k1' = 'v1', 'k2' = 'v2', 'k3' = 'v3')") checkTblProps(tableIdent, Map("k1" -> "v1", "k2" -> "v2", "k3" -> "v3")) - // when using the v2 command, when unsetting `non-existent` properties, - // The command will ignore `non-existent` properties and finally succeed // property to unset does not exist sql(s"ALTER TABLE $t UNSET TBLPROPERTIES ('k3', 'k4')") checkTblProps(tableIdent, Map("k1" -> "v1", "k2" -> "v2")) From 9a8dcf38d7b7cd47c75f6a3bba5fd035217e2040 Mon Sep 17 00:00:00 2001 From: panbingkun Date: Wed, 3 Jul 2024 09:20:48 +0800 Subject: [PATCH 4/6] remove ifExists logical --- docs/sql-ref-syntax-ddl-alter-table.md | 4 +-- .../spark/sql/execution/command/ddl.scala | 8 ----- ...lterTableUnsetTblPropertiesSuiteBase.scala | 18 ++++++++++++ .../AlterTableUnsetTblPropertiesSuite.scala | 29 ------------------- .../AlterTableUnsetTblPropertiesSuite.scala | 22 -------------- 5 files changed, 19 insertions(+), 62 deletions(-) diff --git a/docs/sql-ref-syntax-ddl-alter-table.md b/docs/sql-ref-syntax-ddl-alter-table.md index 97720bbf734f6..8cb89ace8be98 100644 --- a/docs/sql-ref-syntax-ddl-alter-table.md +++ b/docs/sql-ref-syntax-ddl-alter-table.md @@ -252,9 +252,7 @@ ALTER TABLE table_identifier SET TBLPROPERTIES ( key1 = val1, key2 = val2, ... ) `ALTER TABLE UNSET` command is used to drop the table property. -**Note:** If the specified property key does not exist, when you use the v1 command and do not specify `IF EXISTS`, -it will throw the error-condition `UNSET_NONEXISTENT_PROPERTIES` and finally `failed`, -however regardless of whether `IF EXISTS` is set or not, the v2 command will ignore it and finally `succeed`. +**Note:** If the specified property key does not exist, whether specify `IF EXISTS` or not, the command will ignore it and finally succeed. ##### Syntax diff --git a/sql/core/src/main/scala/org/apache/spark/sql/execution/command/ddl.scala b/sql/core/src/main/scala/org/apache/spark/sql/execution/command/ddl.scala index 6f402188910e0..3f221bfa53051 100644 --- a/sql/core/src/main/scala/org/apache/spark/sql/execution/command/ddl.scala +++ b/sql/core/src/main/scala/org/apache/spark/sql/execution/command/ddl.scala @@ -335,14 +335,6 @@ case class AlterTableUnsetPropertiesCommand( override def run(sparkSession: SparkSession): Seq[Row] = { val catalog = sparkSession.sessionState.catalog val table = catalog.getTableRawMetadata(tableName) - if (!ifExists) { - val nonexistentKeys = propKeys.filter(key => !table.properties.contains(key) - && key != TableCatalog.PROP_COMMENT) - if (nonexistentKeys.nonEmpty) { - throw QueryCompilationErrors.unsetNonExistentPropertiesError( - nonexistentKeys, table.identifier) - } - } // If comment is in the table property, we reset it to None val tableComment = if (propKeys.contains(TableCatalog.PROP_COMMENT)) None else table.comment val newProperties = table.properties.filter { case (k, _) => !propKeys.contains(k) } diff --git a/sql/core/src/test/scala/org/apache/spark/sql/execution/command/AlterTableUnsetTblPropertiesSuiteBase.scala b/sql/core/src/test/scala/org/apache/spark/sql/execution/command/AlterTableUnsetTblPropertiesSuiteBase.scala index 7292019c6df54..be8d85d2ef670 100644 --- a/sql/core/src/test/scala/org/apache/spark/sql/execution/command/AlterTableUnsetTblPropertiesSuiteBase.scala +++ b/sql/core/src/test/scala/org/apache/spark/sql/execution/command/AlterTableUnsetTblPropertiesSuiteBase.scala @@ -82,6 +82,24 @@ trait AlterTableUnsetTblPropertiesSuiteBase extends QueryTest with DDLCommandTes } } + test("alter table unset non-existent properties") { + withNamespaceAndTable("ns", "tbl") { t => + sql(s"CREATE TABLE $t (col1 int, col2 string, a int, b int) $defaultUsing") + val tableIdent = TableIdentifier("tbl", Some("ns"), Some(catalog)) + + sql(s"ALTER TABLE $t SET TBLPROPERTIES ('k1' = 'v1', 'k2' = 'v2', 'k3' = 'v3')") + checkTblProps(tableIdent, Map("k1" -> "v1", "k2" -> "v2", "k3" -> "v3")) + + // property to unset does not exist + sql(s"ALTER TABLE $t UNSET TBLPROPERTIES ('k3', 'k4')") + checkTblProps(tableIdent, Map("k1" -> "v1", "k2" -> "v2")) + + // property to unset does not exist, but "IF EXISTS" is specified + sql(s"ALTER TABLE $t UNSET TBLPROPERTIES IF EXISTS ('k2', 'k3')") + checkTblProps(tableIdent, Map("k1" -> "v1")) + } + } + test("alter table unset reserved properties") { import TableCatalog._ val keyParameters = Map[String, String]( diff --git a/sql/core/src/test/scala/org/apache/spark/sql/execution/command/v1/AlterTableUnsetTblPropertiesSuite.scala b/sql/core/src/test/scala/org/apache/spark/sql/execution/command/v1/AlterTableUnsetTblPropertiesSuite.scala index 59a21d54aa1cf..0f7b4a0aad62e 100644 --- a/sql/core/src/test/scala/org/apache/spark/sql/execution/command/v1/AlterTableUnsetTblPropertiesSuite.scala +++ b/sql/core/src/test/scala/org/apache/spark/sql/execution/command/v1/AlterTableUnsetTblPropertiesSuite.scala @@ -17,9 +17,7 @@ package org.apache.spark.sql.execution.command.v1 -import org.apache.spark.sql.AnalysisException import org.apache.spark.sql.catalyst.TableIdentifier -import org.apache.spark.sql.errors.DataTypeErrors.toSQLId import org.apache.spark.sql.execution.command import org.apache.spark.sql.internal.StaticSQLConf.CATALOG_IMPLEMENTATION @@ -61,33 +59,6 @@ trait AlterTableUnsetTblPropertiesSuiteBase extends command.AlterTableUnsetTblPr override def getTblPropertyValue(tableIdent: TableIdentifier, key: String): String = { getTableProperties(tableIdent).getOrElse(key, null) } - - /** - * When using the v1 command to unset `non-existent` properties, the command will - * throw the error-condition `"UNSET_NONEXISTENT_PROPERTIES` and finally `failed` - */ - test("alter table unset non-existent properties") { - withNamespaceAndTable("ns", "tbl") { t => - sql(s"CREATE TABLE $t (col1 int, col2 string, a int, b int) $defaultUsing") - val tableIdent = TableIdentifier("tbl", Some("ns"), Some(catalog)) - - sql(s"ALTER TABLE $t SET TBLPROPERTIES ('k1' = 'v1', 'k2' = 'v2', 'k3' = 'v3')") - checkTblProps(tableIdent, Map("k1" -> "v1", "k2" -> "v2", "k3" -> "v3")) - - // property to unset does not exist - checkError( - exception = intercept[AnalysisException] { - sql(s"ALTER TABLE $t UNSET TBLPROPERTIES ('k3', 'k4')") - }, - errorClass = "UNSET_NONEXISTENT_PROPERTIES", - parameters = Map("properties" -> "`k4`", "table" -> toSQLId(tableIdent.nameParts)) - ) - - // property to unset does not exist, but "IF EXISTS" is specified - sql(s"ALTER TABLE $t UNSET TBLPROPERTIES IF EXISTS ('k3', 'k4')") - checkTblProps(tableIdent, Map("k1" -> "v1", "k2" -> "v2")) - } - } } class AlterTableUnsetTblPropertiesSuite diff --git a/sql/core/src/test/scala/org/apache/spark/sql/execution/command/v2/AlterTableUnsetTblPropertiesSuite.scala b/sql/core/src/test/scala/org/apache/spark/sql/execution/command/v2/AlterTableUnsetTblPropertiesSuite.scala index 04e7a8f10385d..699d8185ee6f0 100644 --- a/sql/core/src/test/scala/org/apache/spark/sql/execution/command/v2/AlterTableUnsetTblPropertiesSuite.scala +++ b/sql/core/src/test/scala/org/apache/spark/sql/execution/command/v2/AlterTableUnsetTblPropertiesSuite.scala @@ -51,26 +51,4 @@ class AlterTableUnsetTblPropertiesSuite override def getTblPropertyValue(tableIdent: TableIdentifier, key: String): String = { getTableMetadata(tableIdent).properties.asScala.toMap.getOrElse(key, null) } - - /** - * When using the v2 command to unset `non-existent` properties, - * the command will ignore `non-existent` properties and finally succeed - */ - test("alter table unset non-existent properties") { - withNamespaceAndTable("ns", "tbl") { t => - sql(s"CREATE TABLE $t (col1 int, col2 string, a int, b int) $defaultUsing") - val tableIdent = TableIdentifier("tbl", Some("ns"), Some(catalog)) - - sql(s"ALTER TABLE $t SET TBLPROPERTIES ('k1' = 'v1', 'k2' = 'v2', 'k3' = 'v3')") - checkTblProps(tableIdent, Map("k1" -> "v1", "k2" -> "v2", "k3" -> "v3")) - - // property to unset does not exist - sql(s"ALTER TABLE $t UNSET TBLPROPERTIES ('k3', 'k4')") - checkTblProps(tableIdent, Map("k1" -> "v1", "k2" -> "v2")) - - // property to unset does not exist, but "IF EXISTS" is specified - sql(s"ALTER TABLE $t UNSET TBLPROPERTIES IF EXISTS ('k2', 'k3')") - checkTblProps(tableIdent, Map("k1" -> "v1")) - } - } } From fe674fe9cb97d08a81846982350ebacf9fc168d0 Mon Sep 17 00:00:00 2001 From: panbingkun Date: Wed, 3 Jul 2024 14:25:32 +0800 Subject: [PATCH 5/6] fix --- .../resources/error/error-conditions.json | 6 ----- .../sql/errors/QueryCompilationErrors.scala | 10 --------- .../sql/execution/command/DDLSuite.scala | 17 -------------- .../sql/hive/execution/HiveDDLSuite.scala | 22 +++---------------- 4 files changed, 3 insertions(+), 52 deletions(-) diff --git a/common/utils/src/main/resources/error/error-conditions.json b/common/utils/src/main/resources/error/error-conditions.json index 78269d29fa2cd..025a4ece8d1aa 100644 --- a/common/utils/src/main/resources/error/error-conditions.json +++ b/common/utils/src/main/resources/error/error-conditions.json @@ -4252,12 +4252,6 @@ ], "sqlState" : "42883" }, - "UNSET_NONEXISTENT_PROPERTIES" : { - "message" : [ - "Attempted to unset non-existent properties [] in table ." - ], - "sqlState" : "42K0J" - }, "UNSUPPORTED_ADD_FILE" : { "message" : [ "Don't support add file." diff --git a/sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryCompilationErrors.scala b/sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryCompilationErrors.scala index d3bd265d0459e..85556881184aa 100644 --- a/sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryCompilationErrors.scala +++ b/sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryCompilationErrors.scala @@ -2658,16 +2658,6 @@ private[sql] object QueryCompilationErrors extends QueryErrorsBase with Compilat "config" -> SQLConf.ALLOW_NON_EMPTY_LOCATION_IN_CTAS.key)) } - def unsetNonExistentPropertiesError( - properties: Seq[String], table: TableIdentifier): Throwable = { - new AnalysisException( - errorClass = "UNSET_NONEXISTENT_PROPERTIES", - messageParameters = Map( - "properties" -> properties.map(toSQLId).mkString(", "), - "table" -> toSQLId(table.nameParts)) - ) - } - def alterTableChangeColumnNotSupportedForColumnTypeError( tableName: String, originColumn: StructField, diff --git a/sql/core/src/test/scala/org/apache/spark/sql/execution/command/DDLSuite.scala b/sql/core/src/test/scala/org/apache/spark/sql/execution/command/DDLSuite.scala index bc592e4600203..994c420feae1f 100644 --- a/sql/core/src/test/scala/org/apache/spark/sql/execution/command/DDLSuite.scala +++ b/sql/core/src/test/scala/org/apache/spark/sql/execution/command/DDLSuite.scala @@ -327,23 +327,6 @@ abstract class DDLSuite extends QueryTest with DDLSuiteBase { protected val reversedProperties = Seq(PROP_OWNER) - test("ALTER TABLE UNSET nonexistent property should throw an exception") { - val tableName = "test_table" - withTable(tableName) { - sql(s"CREATE TABLE $tableName (a STRING, b INT) USING parquet") - - checkError( - exception = intercept[AnalysisException] { - sql(s"ALTER TABLE $tableName UNSET TBLPROPERTIES ('test_prop1', 'test_prop2', 'comment')") - }, - errorClass = "UNSET_NONEXISTENT_PROPERTIES", - parameters = Map( - "properties" -> "`test_prop1`, `test_prop2`", - "table" -> "`spark_catalog`.`default`.`test_table`") - ) - } - } - test("alter table: change column (datasource table)") { testChangeColumn(isDatasourceTable = true) } diff --git a/sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveDDLSuite.scala b/sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveDDLSuite.scala index ff269cba8a67f..fd437e7dc954f 100644 --- a/sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveDDLSuite.scala +++ b/sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveDDLSuite.scala @@ -763,15 +763,9 @@ class HiveDDLSuite sql(s"ALTER VIEW $viewName UNSET TBLPROPERTIES ('p')") checkProperties(Map()) - checkError( - exception = intercept[AnalysisException] { - sql(s"ALTER VIEW $viewName UNSET TBLPROPERTIES ('p')") - }, - errorClass = "UNSET_NONEXISTENT_PROPERTIES", - parameters = Map( - "properties" -> "`p`", - "table" -> s"`$SESSION_CATALOG_NAME`.`default`.`view1`") - ) + // unset non-existent properties + sql(s"ALTER VIEW $viewName UNSET TBLPROPERTIES ('p')") + checkProperties(Map()) } } } @@ -1880,16 +1874,6 @@ class HiveDDLSuite "tableName" -> "spark_catalog.default.tbl", "invalidKeys" -> s"[${forbiddenPrefix}foo]") ) - checkError( - exception = intercept[AnalysisException] { - sql(s"ALTER TABLE tbl UNSET TBLPROPERTIES ('${forbiddenPrefix}foo')") - }, - errorClass = "UNSET_NONEXISTENT_PROPERTIES", - parameters = Map( - "properties" -> (s"${(forbiddenPrefix.split("\\.") :+ "foo"). - map(part => s"`$part`").mkString(".")}"), - "table" -> "`spark_catalog`.`default`.`tbl`") - ) checkError( exception = intercept[AnalysisException] { sql(s"CREATE TABLE tbl2 (a INT) TBLPROPERTIES ('${forbiddenPrefix}foo'='anything')") From 1c0c2601f1ffa387f0066859d5dbfcbb2490a6f2 Mon Sep 17 00:00:00 2001 From: panbingkun Date: Wed, 3 Jul 2024 16:00:09 +0800 Subject: [PATCH 6/6] update docs/sql-ref-syntax-ddl-alter-table.md --- docs/sql-ref-syntax-ddl-alter-table.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/sql-ref-syntax-ddl-alter-table.md b/docs/sql-ref-syntax-ddl-alter-table.md index 8cb89ace8be98..31eaf659b5c7a 100644 --- a/docs/sql-ref-syntax-ddl-alter-table.md +++ b/docs/sql-ref-syntax-ddl-alter-table.md @@ -258,7 +258,7 @@ ALTER TABLE table_identifier SET TBLPROPERTIES ( key1 = val1, key2 = val2, ... ) ```sql -- Unset Properties -ALTER TABLE table_identifier UNSET TBLPROPERTIES [ IF EXISTS ] ( key1, key2, ... ) +ALTER TABLE table_identifier UNSET TBLPROPERTIES ( key1, key2, ... ) ``` #### SET SERDE