[SPARK-36302][SQL] Refactor thirteenth set of 20 query execution errors to use error classes #33805

dgd-contributor · 2021-08-23T01:20:03Z

What changes were proposed in this pull request?

Adds error classes to some of the exceptions in QueryExecutionErrors.

Why are the changes needed?

Improves auditing for developers and adds useful fields for users (error class and SQLSTATE).

Does this PR introduce any user-facing change?

Fills in missing error class and SQLSTATE fields.

How was this patch tested?

Existing tests

HyukjinKwon · 2021-08-23T01:31:08Z

cc @karenfeng FYI

AmplabJenkins · 2021-08-23T01:41:55Z

Can one of the admins verify this patch?

Peng-Lei · 2021-08-23T06:33:04Z

Please see #PR. I try to define the new exception to mix SparkThrowable into a base Exception type. It can help us reduce conflicts in parallel development

karenfeng

Thanks for working on this @dgd-contributor! I left some comments.

karenfeng · 2021-09-07T22:00:31Z

core/src/main/resources/error/error-classes.json

I believe this error message is meant to be a single string without any newlines. Given how complex this is, however, we can turn this into an array of strings which will be joined with newlines for the user. In this case, it would look like:

"message" : [ "Caught Hive MetaException attempting to get partition metadata by filter from Hive.", "You can set the Spark configuration setting %s to true to work around this problem, however this will result in degraded performance.", "Please report a bug: https://issues.apache.org/jira/browse/SPARK" ]

karenfeng · 2021-09-07T22:01:49Z

core/src/main/resources/error/error-classes.json

I'm not sure about 0D000; would 42000 be a better fit?

karenfeng · 2021-09-07T22:40:48Z

core/src/main/resources/error/error-classes.json

Given that this is an error class, you can remove the _ERROR suffix to reduce redundancy and clutter.

karenfeng · 2021-09-07T22:41:38Z

core/src/main/resources/error/error-classes.json

0A000 may fit better here

karenfeng · 2021-09-07T22:42:03Z

core/src/main/resources/error/error-classes.json

0A000 may be a better fit

karenfeng · 2021-09-07T22:43:01Z

core/src/main/resources/error/error-classes.json

You can also split this one.

karenfeng · 2021-09-07T22:43:42Z

core/src/main/resources/error/error-classes.json

This may be 0A000

karenfeng · 2021-09-07T22:43:55Z

core/src/main/resources/error/error-classes.json

I'm not sure if this is 42000; maybe we can leave this empty?

karenfeng · 2021-09-07T22:44:59Z

sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryExecutionErrors.scala

Do you meant to use the RENAME_PATH_AS_EXISTS_PATH_ERROR error class?

karenfeng · 2021-09-07T22:49:15Z

core/src/main/resources/error/error-classes.json

This doesn't make much sense grammatically; maybe RENAME_OVERWRITES_EXISTING_PATH? We can also consolidate this with RENAME_PATH_AS_EXISTS_PATH_ERROR, and have everyone throw this error class.

…rs to use error classes

dgd-contributor · 2021-09-24T06:53:09Z

CC @karenfeng. Could you take another look when you have time ? Thanks!

karenfeng

Thanks for working on this @dgd-contributor! I left some more comments.

karenfeng · 2021-09-28T03:38:10Z

core/src/main/resources/error/error-classes.json

 {
+  "ALTER_TABLE_WITH_DROP_PARTITION_AND_PURGE_UNSUPPORTED" : {
+    "message" : [ "ALTER TABLE ... DROP PARTITION ... PURGE" ],
+    "sqlState" : "42000"


0A000 may be a better fit for these unsupported-type errors

karenfeng · 2021-09-28T03:38:32Z

core/src/main/resources/error/error-classes.json

  "CANNOT_EVALUATE_EXPRESSION" : {
    "message" : [ "Cannot evaluate expression: %s" ]
  },
+  "CANNOT_FETCH_TABLES_OF_DATABASE" : {


We can simplify this: CANNOT_FETCH_TABLES_OF_DATABASE -> CANNOT_FETCH DATABASE_TABLES

karenfeng · 2021-09-28T03:40:10Z

core/src/main/resources/error/error-classes.json

  },
+  "CANNOT_FETCH_TABLES_OF_DATABASE" : {
+    "message" : [ "Unable to fetch tables of db %s" ],
+    "sqlState" : "42000"


This seems to be a catch-all type exception. I wouldn't group this as a syntax/semantic error (42000); maybe we can leave the SQLSTATE field empty.

karenfeng · 2021-09-28T03:43:48Z

core/src/main/resources/error/error-classes.json

  "CONCURRENT_QUERY" : {
    "message" : [ "Another instance of this query was just started by a concurrent session." ]
  },
+  "CONVERT_HIVE_TABLE_TO_CATALOG_TABLE" : {


CONVERT_HIVE_TABLE_TO_CATALOG_TABLE does not capture the error here; CANNOT_CONVERT_HIVE_TABLE_TO_CATALOG_TABLE would better encompass it

karenfeng · 2021-09-28T03:44:07Z

core/src/main/resources/error/error-classes.json

  },
+  "CONVERT_HIVE_TABLE_TO_CATALOG_TABLE" : {
+    "message" : [ "%s, db: %s, table: %s" ],
+    "sqlState" : "0A000"


This isn't an unsupported operation issue; I'd leave this empty

karenfeng · 2021-09-28T03:46:10Z

core/src/main/resources/error/error-classes.json

  },
+  "FAILED_RENAME_TEMP_FILE" : {
+    "message" : [ "Failed to rename temp file %s to %s as rename returned false" ],
+    "sqlState" : "42000"


This isn't necessarily a syntax error. I don't have a good idea for which SQLSTATE this should be though. I'd leave this empty.

karenfeng · 2021-09-28T03:48:38Z

core/src/main/resources/error/error-classes.json

+  "LEGACY_METADATA_PATH_EXISTS" : {
+    "message" : [ "Error: we detected a possible problem with the location of your \"_spark_metadata\" directory and you likely need to move it before restarting this query.", "Earlier version of Spark incorrectly escaped paths when writing out the \"_spark_metadata\" directory for structured streaming.", "While this was corrected in Spark 3.0, it appears that your query was started using an earlier version that incorrectly handled the \"_spark_metadata\" path.", "Correct \"_spark_metadata\" Directory: %s. Incorrect \"_spark_metadata\" Directory: %s", "Please move the data from the incorrect directory to the correct one, delete the incorrect directory, and then restart this query.", "If you believe you are receiving this message in error, you can disable it with the SQL conf %s." ]
+  },
+  "LOAD_HIVE_CLIENT_CAUSES_NO_CLASS_DEF_FOUND" : {


DEF -> DEFINITION

karenfeng · 2021-09-28T03:49:44Z

core/src/main/resources/error/error-classes.json

+  },
+  "LOAD_HIVE_CLIENT_CAUSES_NO_CLASS_DEF_FOUND" : {
+    "message" : [ "%s when creating Hive client using classpath: %s", "Please make sure that jars for your version of hive and hadoop are included in the paths passed to %s." ],
+    "sqlState" : "42000"


I'm not sure if this is syntax/semantic error either. I'd remove this.

karenfeng · 2021-09-28T03:50:18Z

core/src/main/resources/error/error-classes.json

    "message" : [ "PARTITION clause cannot contain a non-partition column name: %s" ],
    "sqlState" : "42000"
  },
+  "PARTITION_COLUMN_NOT_FOUND_IN_SCHEMA" : {


Rename to match MISSING_COLUMN: MISSING_PARTITION_COLUMN

karenfeng · 2021-09-28T03:50:51Z

core/src/main/resources/error/error-classes.json

    "sqlState" : "0A000"
  },
+  "UNSUPPORTED_HIVE_METASTORE_VERSION" : {
+    "message" : [ "Unsupported Hive Metastore version (%s). Please set %s with a valid version." ]


I think this is a 0A000 SQLSTATE.

…s.json

dgd-contributor · 2021-09-28T09:35:30Z

Thanks for your suggestion, @karenfeng, I have updated this PR. Please take another look when you have time, thanks!

karenfeng

LGTM overall! Thanks @dgd-contributor; I left one small nit to address.

karenfeng · 2021-09-30T19:08:01Z

core/src/main/resources/error/error-classes.json

  "FAILED_SET_ORIGINAL_PERMISSION_BACK" : {
    "message" : [ "Failed to set original permission %s back to the created path: %s. Exception: %s" ]
  },
+  "GET_PARTITION_METADATA_BY_FILTER" : {


The naming of this isn't obviously an error. Can we name this something like CANNOT_GET_PARTITION_METADATA_BY_FILTER?

dgd-contributor · 2021-10-04T02:32:41Z

Thanks for all your suggestions in recent our PRs, @karenfeng. dgd-contributes is a team, so it may be inconvenient for you when you help a member to resolve this problem and other members don't know anything about it. You can see the same problem in another PR so we are sorry about it. Now, a member of dgd team will work on all issues involved in refactoring query execution errors to use error classes. We will close all recent PR and a member will make PRs for those issues, so when you have time, please help us again.
If you get uncomfortable when helping us, we are sorry again.

github-actions bot added CORE SQL labels Aug 23, 2021

HyukjinKwon changed the title ~~[SPARK-36302][SQL] :Refactor thirteenth set of 20 query execution errors to use error classes~~ [SPARK-36302][SQL] Refactor thirteenth set of 20 query execution errors to use error classes Aug 23, 2021

karenfeng reviewed Sep 7, 2021

View reviewed changes

dgd-contributor force-pushed the SPARK-36302 branch 2 times, most recently from c3de099 to 0a4b38e Compare September 24, 2021 04:37

[SPARK-36302][SQL] Refactor thirteenth set of 20 query execution erro…

0a4b38e

…rs to use error classes

karenfeng reviewed Sep 28, 2021

View reviewed changes

[SPARK-36302]: Update(1)-fix SQLState and wrong names in error-classe…

9bf5085

…s.json

karenfeng approved these changes Sep 30, 2021

View reviewed changes

dgd-contributor closed this Oct 4, 2021

[SPARK-36302][SQL] Refactor thirteenth set of 20 query execution errors to use error classes #33805

[SPARK-36302][SQL] Refactor thirteenth set of 20 query execution errors to use error classes #33805

Uh oh!

Conversation

dgd-contributor commented Aug 23, 2021

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

HyukjinKwon commented Aug 23, 2021

Uh oh!

AmplabJenkins commented Aug 23, 2021

Uh oh!

Peng-Lei commented Aug 23, 2021

Uh oh!

karenfeng left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dgd-contributor commented Sep 24, 2021

Uh oh!

karenfeng left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dgd-contributor commented Sep 28, 2021

Uh oh!

karenfeng left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dgd-contributor commented Oct 4, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!