You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "MaxGekk (via GitHub)" <gi...@apache.org> on 2023/10/09 17:28:40 UTC
Re: [PR] [SPARK-44837][SQL] Improve ALTER TABLE ALTER PARTITION column error message [spark]
MaxGekk commented on code in PR #42524:
URL: https://github.com/apache/spark/pull/42524#discussion_r1350588319
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/command/ddl.scala:
##########
@@ -415,13 +417,21 @@ case class AlterTableChangeColumnCommand(
Seq.empty[Row]
}
- // Find the origin column from schema by column name, throw an AnalysisException if the column
- // reference is invalid.
+ // Find the origin column from schema by column name
private def findColumnByName(
- schema: StructType, name: String, resolver: Resolver): StructField = {
+ schema: StructType, name: String, resolver: Resolver): Option[StructField] = {
schema.fields.collectFirst {
case field if resolver(field.name, name) => field
- }.getOrElse(throw QueryCompilationErrors.cannotFindColumnError(name, schema.fieldNames))
+ }
+ }
+
+ private def verifyNonPartitionColumn(
+ table: CatalogTable, columnName: String, resolver: Resolver): Unit = {
+ findColumnByName(table.partitionSchema, columnName, resolver) match {
+ case Some(_) =>
+ throw QueryCompilationErrors.cannotAlterPartitionColumn(table.qualifiedName, columnName)
+ case None =>
+ }
Review Comment:
Could you make it shorter:
```suggestion
if (findColumnByName(table.dataSchema, columnName, resolver).isDefined) {
throw QueryCompilationErrors.cannotAlterPartitionColumn(table.qualifiedName, columnName)
}
```
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/command/ddl.scala:
##########
@@ -415,13 +417,21 @@ case class AlterTableChangeColumnCommand(
Seq.empty[Row]
}
- // Find the origin column from schema by column name, throw an AnalysisException if the column
- // reference is invalid.
+ // Find the origin column from schema by column name
private def findColumnByName(
- schema: StructType, name: String, resolver: Resolver): StructField = {
+ schema: StructType, name: String, resolver: Resolver): Option[StructField] = {
schema.fields.collectFirst {
case field if resolver(field.name, name) => field
- }.getOrElse(throw QueryCompilationErrors.cannotFindColumnError(name, schema.fieldNames))
+ }
+ }
+
+ private def verifyNonPartitionColumn(
Review Comment:
If it is used only once, let's inline it.
##########
common/utils/src/main/resources/error/error-classes.json:
##########
@@ -97,6 +97,13 @@
"The method <methodName> can not be called on streaming Dataset/DataFrame."
]
},
+ "CANNOT_ALTER_PARTITION_COLUMN" : {
+ "message" : [
+ "ALTER TABLE (ALTER|CHANGE) COLUMN is not supported for partition columns.",
+ "<columnName> is a partition column in table <tableName>."
Review Comment:
Let's follow other message in `error-classes.json`:
```suggestion
"ALTER TABLE (ALTER|CHANGE) COLUMN is not supported for partition columns, but found the partition column <columnName> in the table <tableName>."
```
##########
common/utils/src/main/resources/error/error-classes.json:
##########
@@ -97,6 +97,13 @@
"The method <methodName> can not be called on streaming Dataset/DataFrame."
]
},
+ "CANNOT_ALTER_PARTITION_COLUMN" : {
+ "message" : [
+ "ALTER TABLE (ALTER|CHANGE) COLUMN is not supported for partition columns.",
+ "<columnName> is a partition column in table <tableName>."
Review Comment:
nit:
```suggestion
"<columnName> is a partition column in the table <tableName>."
```
--
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