[GitHub] [spark] MaxGekk commented on a diff in pull request #37887: [SPARK-40360] ALREADY_EXISTS and NOT_FOUND exceptions
MaxGekk commented on code in PR #37887: URL: https://github.com/apache/spark/pull/37887#discussion_r1014297862 ## sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/AlreadyExistException.scala: ## @@ -20,66 +20,112 @@ package org.apache.spark.sql.catalyst.analysis import org.apache.spark.sql.AnalysisException import org.apache.spark.sql.catalyst.InternalRow import org.apache.spark.sql.catalyst.catalog.CatalogTypes.TablePartitionSpec -import org.apache.spark.sql.connector.catalog.CatalogV2Implicits._ -import org.apache.spark.sql.connector.catalog.Identifier +import org.apache.spark.sql.catalyst.util.{quoteIdentifier, quoteNameParts } import org.apache.spark.sql.types.StructType /** * Thrown by a catalog when an item already exists. The analyzer will rethrow the exception * as an [[org.apache.spark.sql.AnalysisException]] with the correct position information. */ class DatabaseAlreadyExistsException(db: String) - extends NamespaceAlreadyExistsException(s"Database '$db' already exists") + extends NamespaceAlreadyExistsException(Array(db)) -class NamespaceAlreadyExistsException(message: String) - extends AnalysisException( -message, -errorClass = Some("_LEGACY_ERROR_TEMP_1118"), -messageParameters = Map("msg" -> message)) { + +class NamespaceAlreadyExistsException(errorClass: String, messageParameters: Map[String, String]) + extends AnalysisException(errorClass, messageParameters) { def this(namespace: Array[String]) = { -this(s"Namespace '${namespace.quoted}' already exists") +this(errorClass = "SCHEMA_ALREADY_EXISTS", + Map("schemaName" -> quoteNameParts(namespace))) } } -class TableAlreadyExistsException(message: String, cause: Option[Throwable] = None) - extends AnalysisException( -message, -errorClass = Some("_LEGACY_ERROR_TEMP_1116"), -messageParameters = Map("msg" -> message), -cause = cause) { + +class TableAlreadyExistsException(errorClass: String, messageParameters: Map[String, String], + cause: Option[Throwable] = None) + extends AnalysisException(errorClass, messageParameters, cause = cause) { def this(db: String, table: String) = { -this(s"Table or view '$table' already exists in database '$db'") +this(errorClass = "TABLE_OR_VIEW_ALREADY_EXISTS", + messageParameters = Map("relationName" -> +(quoteIdentifier(db) + "." + quoteIdentifier(table + } + + def this(table: String) = { +this(errorClass = "TABLE_OR_VIEW_ALREADY_EXISTS", + messageParameters = Map("relationName" -> +quoteNameParts(UnresolvedAttribute.parseAttributeName(table } - def this(tableIdent: Identifier) = { -this(s"Table ${tableIdent.quoted} already exists") + def this(table: Seq[String]) = { +this(errorClass = "TABLE_OR_VIEW_ALREADY_EXISTS", + messageParameters = Map("relationName" -> quoteNameParts(table))) + } +} + +class TempTableAlreadyExistsException(errorClass: String, messageParameters: Map[String, String], + cause: Option[Throwable] = None) + extends AnalysisException(errorClass, messageParameters, cause = cause) { + def this(table: String) = { +this(errorClass = "TEMP_TABLE_OR_VIEW_ALREADY_EXISTS", + messageParameters = Map("relationName" +-> quoteNameParts(UnresolvedAttribute.parseAttributeName(table } } -class TempTableAlreadyExistsException(table: String) - extends TableAlreadyExistsException(s"Temporary view '$table' already exists") +class PartitionAlreadyExistsException(errorClass: String, messageParameters: Map[String, String]) + extends AnalysisException(errorClass, messageParameters) { + def this(db: String, table: String, spec: TablePartitionSpec) = { +this(errorClass = "PARTITIONS_ALREADY_EXIST", + Map("partitionList" -> ("PARTITION (" + +spec.map( kv => quoteIdentifier(kv._1) + s" = ${kv._2}").mkString(", ") + ")"), +"tableName" -> (quoteIdentifier(db) + "." + quoteIdentifier(table + } + + def this(tableName: String, partitionIdent: InternalRow, partitionSchema: StructType) = { +this(errorClass = "PARTITIONS_ALREADY_EXIST", + Map("partitionList" -> +("PARTITION (" + partitionIdent.toSeq(partitionSchema).zip(partitionSchema.map(_.name)) +.map( kv => quoteIdentifier(s"${kv._2}") + s" = ${kv._1}").mkString(", ") + ")"), +"tableName" -> quoteNameParts(UnresolvedAttribute.parseAttributeName(tableName + } +} -class PartitionsAlreadyExistException(message: String) extends AnalysisException(message) { +class PartitionsAlreadyExistException(errorClass: String, messageParameters: Map[String, String]) + extends AnalysisException(errorClass, messageParameters) { def this(db: String, table: String, specs: Seq[TablePartitionSpec]) = { -this(s"The following partitions already exist in table '$table' database '$db':\n" - + specs.mkString("\n===\n")) +this(errorClass = "PARTITIONS_AL
[GitHub] [spark] MaxGekk commented on a diff in pull request #37887: [SPARK-40360] ALREADY_EXISTS and NOT_FOUND exceptions
MaxGekk commented on code in PR #37887: URL: https://github.com/apache/spark/pull/37887#discussion_r992561099 ## core/src/main/resources/error/error-classes.json: ## @@ -346,6 +346,18 @@ } } }, + "INDEX_ALREADY_EXISTS" : { +"message" : [ + "Cannot create the index because it already exists. ." +], +"sqlState" : "42000" + }, + "INDEX_NOT_FOUND" : { +"message" : [ + "Cannot find the index. ." Review Comment: > Can we do this later? Yep, sure. -- 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-unsubscr...@spark.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] MaxGekk commented on a diff in pull request #37887: [SPARK-40360] ALREADY_EXISTS and NOT_FOUND exceptions
MaxGekk commented on code in PR #37887: URL: https://github.com/apache/spark/pull/37887#discussion_r991513440 ## sql/core/src/test/scala/org/apache/spark/sql/connector/AlterTableTests.scala: ## @@ -45,14 +47,15 @@ trait AlterTableTests extends SharedSparkSession { test("AlterTable: table does not exist") { val t2 = s"${catalogAndNamespace}fake_table" +val quoted = UnresolvedAttribute.parseAttributeName(s"${catalogAndNamespace}table_name") + .map(part => quoteIdentifier(part)).mkString(".") Review Comment: Does `toSQLId` do the same, no? ## core/src/main/resources/error/error-classes.json: ## @@ -536,12 +563,71 @@ "Failed to set original permission back to the created path: . Exception: " ] }, + "ROUTINE_ALREADY_EXISTS" : { +"message" : [ + "Cannot create the function because it already exists.", + "Choose a different name, drop or replace the existing function, or add the IF NOT EXISTS clause to tolerate a pre-existing function." +], +"sqlState" : "42000" + }, + "ROUTINE_NOT_FOUND" : { +"message" : [ + "The function cannot be found. Verify the spelling and correctness of the schema and catalog.", + "If you did not qualify the name with a schema and catalog, verify the current_schema() output, or qualify the name with the correct schema and catalog.", + "To tolerate the error on drop use DROP FUNCTION IF EXISTS." +], +"sqlState" : "42000" + }, + "SCHEMA_ALREADY_EXISTS" : { +"message" : [ + "Cannot create schema because it already exists.", + "Choose a different name, drop the existing schema, or add the IF NOT EXISTS clause to tolerate pre-existing schema." +], +"sqlState" : "42000" + }, + "SCHEMA_NOT_EMPTY" : { +"message" : [ + "Cannot drop a schema because it contains objects.", + "Use DROP SCHEMA ... CASCADE to drop the schema and all its objects." +], +"sqlState" : "42000" + }, + "SCHEMA_NOT_FOUND" : { +"message" : [ + "The schema cannot be found. Verify the spelling and correctness of the schema and catalog.", + "If you did not qualify the name with a catalog, verify the current_schema() output, or qualify the name with the correct catalog.", + "To tolerate the error on drop use DROP SCHEMA IF EXISTS." +], +"sqlState" : "42000" + }, "SECOND_FUNCTION_ARGUMENT_NOT_INTEGER" : { "message" : [ "The second argument of function needs to be an integer." ], "sqlState" : "22023" }, + "TABLE_OR_VIEW_ALREADY_EXISTS" : { +"message" : [ + "Cannot create table or view because it already exists.", + "Choose a different name, drop or replace the existing object, or add the IF NOT EXISTS clause to tolerate pre-existing objects." +], +"sqlState" : "42000" + }, + "TABLE_OR_VIEW_NOT_FOUND" : { +"message" : [ + "The table or view cannot be found. Verify the spelling and correctness of the schema and catalog.", + "If you did not qualify the name with a schema, verify the current_schema() output, or qualify the name with the correct schema and catalog.", + "To tolerate the error on drop use DROP VIEW IF EXISTS or DROP TABLE IF EXISTS." +], +"sqlState" : "42000" + }, + "TEMP_TABLE_OR_VIEW_ALREADY_EXISTS" : { +"message" : [ + "Cannot create the temporary view because it already exists.", Review Comment: Why not to create separate error classes? Or main error class like `ALREADY_EXISTS` + sub-classes per every relation type. ## core/src/main/resources/error/error-classes.json: ## @@ -536,12 +563,71 @@ "Failed to set original permission back to the created path: . Exception: " ] }, + "ROUTINE_ALREADY_EXISTS" : { +"message" : [ + "Cannot create the function because it already exists.", + "Choose a different name, drop or replace the existing function, or add the IF NOT EXISTS clause to tolerate a pre-existing function." +], +"sqlState" : "42000" + }, + "ROUTINE_NOT_FOUND" : { +"message" : [ + "The function cannot be found. Verify the spelling and correctness of the schema and catalog.", + "If you did not qualify the name with a schema and catalog, verify the current_schema() output, or qualify the name with the correct schema and catalog.", + "To tolerate the error on drop use DROP FUNCTION IF EXISTS." +], +"sqlState" : "42000" + }, + "SCHEMA_ALREADY_EXISTS" : { +"message" : [ + "Cannot create schema because it already exists.", + "Choose a different name, drop the existing schema, or add the IF NOT EXISTS clause to tolerate pre-existing schema." +], +"sqlState" : "42000" + }, + "SCHEMA_NOT_EMPTY" : { +"message" : [ + "Cannot drop a schema because it contains objects.", + "Use DROP SCHEMA ... CASCADE to drop the schema and all its objects." +], +"sqlState" : "42000"
[GitHub] [spark] MaxGekk commented on a diff in pull request #37887: [SPARK-40360] ALREADY_EXISTS and NOT_FOUND exceptions
MaxGekk commented on code in PR #37887: URL: https://github.com/apache/spark/pull/37887#discussion_r989885531 ## core/src/main/resources/error/error-classes.json: ## @@ -536,12 +563,71 @@ "Failed to set original permission back to the created path: . Exception: " ] }, + "ROUTINE_ALREADY_EXISTS" : { +"message" : [ + "Cannot create the function because it already exists.", + "Choose a different name, drop or replace the existing function, or add the IF NOT EXISTS clause to tolerate a pre-existing function." +], +"sqlState" : "42000" + }, + "ROUTINE_NOT_FOUND" : { Review Comment: Why do you name it `ROUTINE...` if SQL key word is `FUNCTION...`. The last one is even used in the error message. Could you rename to `FUNCTION...` for consistency, otherwise, please, explain the difference. ## core/src/main/resources/error/error-classes.json: ## @@ -346,6 +346,18 @@ } } }, + "INDEX_ALREADY_EXISTS" : { Review Comment: Where is it used? ## core/src/main/resources/error/error-classes.json: ## @@ -536,12 +563,71 @@ "Failed to set original permission back to the created path: . Exception: " ] }, + "ROUTINE_ALREADY_EXISTS" : { +"message" : [ + "Cannot create the function because it already exists.", + "Choose a different name, drop or replace the existing function, or add the IF NOT EXISTS clause to tolerate a pre-existing function." +], +"sqlState" : "42000" + }, + "ROUTINE_NOT_FOUND" : { +"message" : [ + "The function cannot be found. Verify the spelling and correctness of the schema and catalog.", + "If you did not qualify the name with a schema and catalog, verify the current_schema() output, or qualify the name with the correct schema and catalog.", + "To tolerate the error on drop use DROP FUNCTION IF EXISTS." +], +"sqlState" : "42000" + }, + "SCHEMA_ALREADY_EXISTS" : { +"message" : [ + "Cannot create schema because it already exists.", + "Choose a different name, drop the existing schema, or add the IF NOT EXISTS clause to tolerate pre-existing schema." +], +"sqlState" : "42000" + }, + "SCHEMA_NOT_EMPTY" : { +"message" : [ + "Cannot drop a schema because it contains objects.", + "Use DROP SCHEMA ... CASCADE to drop the schema and all its objects." +], +"sqlState" : "42000" + }, + "SCHEMA_NOT_FOUND" : { +"message" : [ + "The schema cannot be found. Verify the spelling and correctness of the schema and catalog.", + "If you did not qualify the name with a catalog, verify the current_schema() output, or qualify the name with the correct catalog.", + "To tolerate the error on drop use DROP SCHEMA IF EXISTS." +], +"sqlState" : "42000" + }, "SECOND_FUNCTION_ARGUMENT_NOT_INTEGER" : { "message" : [ "The second argument of function needs to be an integer." ], "sqlState" : "22023" }, + "TABLE_OR_VIEW_ALREADY_EXISTS" : { +"message" : [ + "Cannot create table or view because it already exists.", + "Choose a different name, drop or replace the existing object, or add the IF NOT EXISTS clause to tolerate pre-existing objects." +], +"sqlState" : "42000" + }, + "TABLE_OR_VIEW_NOT_FOUND" : { +"message" : [ + "The table or view cannot be found. Verify the spelling and correctness of the schema and catalog.", + "If you did not qualify the name with a schema, verify the current_schema() output, or qualify the name with the correct schema and catalog.", + "To tolerate the error on drop use DROP VIEW IF EXISTS or DROP TABLE IF EXISTS." +], +"sqlState" : "42000" + }, + "TEMP_TABLE_OR_VIEW_ALREADY_EXISTS" : { +"message" : [ + "Cannot create the temporary view because it already exists.", Review Comment: The name is about temp **table** or view, but the error message says only about view. ## core/src/test/scala/org/apache/spark/SparkFunSuite.scala: ## @@ -374,6 +382,19 @@ abstract class SparkFunSuite checkError(exception, errorClass, sqlState, parameters, matchPVals = true, Array(context)) + protected def checkErrorTableNotFound( Review Comment: Here is the core, right? which is not aware of any tables. Please, move it from here. ## core/src/main/resources/error/error-classes.json: ## @@ -536,12 +563,71 @@ "Failed to set original permission back to the created path: . Exception: " ] }, + "ROUTINE_ALREADY_EXISTS" : { +"message" : [ + "Cannot create the function because it already exists.", + "Choose a different name, drop or replace the existing function, or add the IF NOT EXISTS clause to tolerate a pre-existing function." +], +"sqlState" : "42000" + }, + "ROUTINE_NOT_FOUND" : { +"message" : [ + "The function cannot be found. Verify the spelling