[SPARK-36291][SQL] Refactor second set of 20 in QueryExecutionErrors to use error classes #33839

dgd-contributor · 2021-08-25T17:00:58Z

What changes were proposed in this pull request?

Refactor some exceptions in QueryExecutionErrors to use error classes.

inputTypeUnsupportedError
invalidFractionOfSecondError
overflowInSumOfDecimalError
overflowInIntegralDivideError
mapSizeExceedArraySizeWhenZipMapError
copyNullFieldNotAllowedError
literalTypeUnsupportedError
noDefaultForDataTypeError
doGenCodeOfAliasShouldNotBeCalledError
orderedOperationUnsupportedByDataTypeError
regexGroupIndexLessThanZeroError
regexGroupIndexExceedGroupCountError
invalidUrlError
dataTypeOperationUnsupportedError
mergeUnsupportedByWindowFunctionError
dataTypeUnexpectedError
typeUnsupportedError
negativeValueUnexpectedError
addNewFunctionMismatchedWithFunctionError
cannotGenerateCodeForUncomparableTypeError

Why are the changes needed?

There are currently ~350 exceptions in this file; so this PR only focuses on the second set of 20.

Does this PR introduce any user-facing change?

No

How was this patch tested?

Existed UT Testcase

AmplabJenkins · 2021-08-25T17:02:31Z

Can one of the admins verify this patch?

HyukjinKwon · 2021-08-26T00:53:57Z

cc @karenfeng FYI

Peng-Lei · 2021-08-27T07:30:21Z

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

  "UNABLE_TO_ACQUIRE_MEMORY" : {
    "message" : [ "Unable to acquire %s bytes of memory, got %s" ]
  },
+  "UNEXPECTED_DATATYPE" : {


Could we try to merge them?

can we merge them like

"UNEXPECTED" : { "message" : [ "Unexpected %s" ], "sqlState" : "22000"

I think it makes sense to just keep one or the other; maybe UNEXPECTED_TYPE?

I also think 42000 may be a better fit.

Peng-Lei · 2021-08-27T07:33:59Z

There is no suitable SQLSTATE for all error class added newly ?

dgd-contributor · 2021-08-30T07:00:49Z

There is no suitable SQLSTATE for all error class added newly ?

No I haven't working on adding sqlstate, I will add them soon

karenfeng

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

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

karenfeng · 2021-09-07T21:42:06Z

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

  "UNABLE_TO_ACQUIRE_MEMORY" : {
    "message" : [ "Unable to acquire %s bytes of memory, got %s" ]
  },
+  "UNEXPECTED_DATATYPE" : {


I think it makes sense to just keep one or the other; maybe UNEXPECTED_TYPE?

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

karenfeng · 2021-09-07T21:54:22Z

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

  "UNABLE_TO_ACQUIRE_MEMORY" : {
    "message" : [ "Unable to acquire %s bytes of memory, got %s" ]
  },
+  "UNEXPECTED_DATATYPE" : {


I also think 42000 may be a better fit.

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

core/src/main/scala/org/apache/spark/SparkException.scala

karenfeng

Thanks for working on this @dgd-contributor! I left some comments, please tag me when you address them.

karenfeng · 2021-09-16T22:45:51Z

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

@@ -14,13 +21,20 @@
    "message" : [ "Found duplicate keys '%s'" ],
    "sqlState" : "23000"
  },
+  "EXCEED_ARRAY_SIZE_WHEN_ZIP_MAP" : {


This doesn't make much sense grammatically. How about ZIPPED_MAPS_EXCEED_SIZE_LIMIT?

karenfeng · 2021-09-16T22:46:48Z

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

@@ -46,6 +68,10 @@
    "message" : [ "Index %s must be between 0 and the length of the ArrayData." ],
    "sqlState" : "22023"
  },
+  "INTEGRAL_DIVIDE_OVERFLOW" : {
+    "message" : [ "Overflow in integral divide." ],
+    "sqlState" : "22015"


I think 22003 is a better fit here.

karenfeng · 2021-09-16T22:48:15Z

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

+  },
+  "GROUP_INDEX_LESS_THAN_ZERO" : {
+    "message" : [ "The specified group index cannot be less than zero" ],
+    "sqlState" : "07009"


I think 42000 is a better fit here as well.

karenfeng · 2021-09-16T22:49:31Z

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

  "FAILED_RENAME_PATH" : {
    "message" : [ "Failed to rename %s to %s as destination already exists" ],
    "sqlState" : "22023"
  },
  "FAILED_SET_ORIGINAL_PERMISSION_BACK" : {
    "message" : [ "Failed to set original permission %s back to the created path: %s. Exception: %s" ]
  },
+  "FOUND_NEGATIVE_VALUES" : {


I think UNEXPECTED_NEGATIVE_VALUES is a better descriptor.

karenfeng · 2021-09-16T22:50:07Z

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

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


I think COPY_NULL_FIELD_NOT_ALLOWED is a better descriptor.

karenfeng · 2021-09-16T22:51:12Z

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

@@ -77,6 +111,14 @@
    "message" : [ "PARTITION clause cannot contain a non-partition column name: %s" ],
    "sqlState" : "42000"
  },
+  "NOT_MATCH_FUNCTION_NAME" : {
+    "message" : [ "%s is not matched at addNewFunction" ],
+    "sqlState" : "42000"


This is an internal error, let's remove the SQLSTATE.

karenfeng · 2021-09-16T22:52:38Z

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

@@ -77,6 +111,14 @@
    "message" : [ "PARTITION clause cannot contain a non-partition column name: %s" ],
    "sqlState" : "42000"
  },
+  "NOT_MATCH_FUNCTION_NAME" : {


This doesn't make much sense grammatically. How about ADD_NEW_FUNCTION_NAME_MISMATCHED?

karenfeng · 2021-09-16T22:55:40Z

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

  "UNSUPPORTED_LITERAL_TYPE" : {
    "message" : [ "Unsupported literal type %s %s" ],
    "sqlState" : "0A000"
  },
+  "UNSUPPORTED_ORDERED_OPERATIONS" : {


This should be more specific, like ORDERED_OPERATIONS_UNSUPPORTED_BY_DATA_TYPE

karenfeng · 2021-09-16T22:56:26Z

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

  "UNRECOGNIZED_SQL_TYPE" : {
    "message" : [ "Unrecognized SQL type %s" ],
    "sqlState" : "42000"
  },
+  "UNSUPPORTED_DATATYPE" : {


Nit: DATATYPE -> DATA_TYPE.

…o use error classes

dgd-contributor · 2021-09-21T04:02:16Z

@karenfeng Can you re-review, I've resolve conflict and what you suggested

karenfeng

LGTM overall - I only have one small typo nit. @HyukjinKwon, can you take a pass once that's addressed?

karenfeng · 2021-09-21T21:32:12Z

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

@@ -60,6 +70,14 @@
  "GROUPING_SIZE_LIMIT_EXCEEDED" : {
    "message" : [ "Grouping sets size cannot be greater than %s" ]
  },
+  "GROUP_INDEX_EXCEED_GROUP_COUNT" : {


Nit: EXCEED -> EXCEEDS

karenfeng · 2021-09-21T22:09:51Z

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

  }

  def invalidFractionOfSecondError(): DateTimeException = {
-    new SparkDateTimeException(errorClass = "INVALID_FRACTION_OF_SECOND", Array.empty)


Why was this changed?

sorry, just a clean up

HyukjinKwon · 2021-09-23T01:47:40Z

@dgd-contributor, BTW, I think we shouldn't use one account shared to many people because:

it is ambiguous who reviewers communicate w/, e.g.) reviewers asked one thing to fix in a PR but other PRs still have the same mistakes
it is unclear who's the main contributor in the change

I sent an email to the address in your GitHub profile but I haven't got a reply yet. Would you mind contacting me via gurwls223@apache.org?

HyukjinKwon · 2021-09-26T03:15:44Z

@dgd-contributor, please contact me or private@spark.apache.org. As I shared in the email, the submissions from the specific shared account will not be accepted for now.

dgd-contributor · 2021-09-27T01:03:58Z

@dgd-contributor, please contact me or private@spark.apache.org. As I shared in the email, the submissions from the specific shared account will not be accepted for now.

Could you please check mail from ncthanh.work@gmail.com? Sorry for the inconvenience because the email from your side has been blocked by our mail system.

github-actions · 2022-01-06T00:12:50Z

We're closing this PR because it hasn't been updated in a while. This isn't a judgement on the merit of the PR in any way. It's just a way of keeping the PR queue manageable.
If you'd like to revive this PR, please reopen it and ask a committer to remove the Stale tag!

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

HyukjinKwon changed the title ~~[SPARK-36291][SQL] refactor second set of 20 in QueryExecutionErrors to use error classes~~ [SPARK-36291][SQL] Refactor second set of 20 in QueryExecutionErrors to use error classes Aug 26, 2021

dgd-contributor changed the title ~~[SPARK-36291][SQL] Refactor second set of 20 in QueryExecutionErrors to use error classes~~ [WIP][SPARK-36291][SQL] Refactor second set of 20 in QueryExecutionErrors to use error classes Aug 26, 2021

Peng-Lei reviewed Aug 27, 2021

View reviewed changes

karenfeng reviewed Sep 7, 2021

View reviewed changes

dgd-contributor force-pushed the SPARK-36291_RefactorSecondSet branch from bfb37b8 to f0da69d Compare September 10, 2021 03:49

karenfeng reviewed Sep 16, 2021

View reviewed changes

dchvn added 9 commits September 21, 2021 10:19

Add needed classes from pr 36107

32185f7

[SPARK-36291][SQL] Refactor second set of 20 query execution errors t…

2a120c0

…o use error classes

Alphabet order

9768954

invalid msg

d05e08b

resolve fail test

6aed077

Add sqlState

ee0ae67

Resolve suggestions

c857943

merge unexpected datatype w type

1c953d2

Resolve suggested

4ce22c0

dgd-contributor force-pushed the SPARK-36291_RefactorSecondSet branch from f0da69d to 4ce22c0 Compare September 21, 2021 04:01

update

94dccbc

dgd-contributor changed the title ~~[WIP][SPARK-36291][SQL] Refactor second set of 20 in QueryExecutionErrors to use error classes~~ [SPARK-36291][SQL] Refactor second set of 20 in QueryExecutionErrors to use error classes Sep 21, 2021

karenfeng approved these changes Sep 21, 2021

View reviewed changes

karenfeng reviewed Sep 21, 2021

View reviewed changes

error-classes fix

a780a6d

EXCEED to EXCEEDS

37a2a0d

github-actions bot added the Stale label Jan 6, 2022

github-actions bot closed this Jan 7, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-36291][SQL] Refactor second set of 20 in QueryExecutionErrors to use error classes #33839

[SPARK-36291][SQL] Refactor second set of 20 in QueryExecutionErrors to use error classes #33839

dgd-contributor commented Aug 25, 2021

AmplabJenkins commented Aug 25, 2021

HyukjinKwon commented Aug 26, 2021

Peng-Lei Aug 27, 2021

dgd-contributor Aug 31, 2021

karenfeng Sep 7, 2021

karenfeng Sep 7, 2021

Peng-Lei commented Aug 27, 2021

dgd-contributor commented Aug 30, 2021 •

edited

karenfeng left a comment

karenfeng Sep 7, 2021

karenfeng Sep 7, 2021

karenfeng left a comment

karenfeng Sep 16, 2021

karenfeng Sep 16, 2021

karenfeng Sep 16, 2021

karenfeng Sep 16, 2021

karenfeng Sep 16, 2021

karenfeng Sep 16, 2021

karenfeng Sep 16, 2021

karenfeng Sep 16, 2021

karenfeng Sep 16, 2021

dgd-contributor commented Sep 21, 2021

karenfeng left a comment

karenfeng Sep 21, 2021

karenfeng Sep 21, 2021

dgd-contributor Sep 23, 2021

HyukjinKwon commented Sep 23, 2021 •

edited

HyukjinKwon commented Sep 26, 2021

dgd-contributor commented Sep 27, 2021

github-actions bot commented Jan 6, 2022

[SPARK-36291][SQL] Refactor second set of 20 in QueryExecutionErrors to use error classes #33839

[SPARK-36291][SQL] Refactor second set of 20 in QueryExecutionErrors to use error classes #33839

Conversation

dgd-contributor commented Aug 25, 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?

AmplabJenkins commented Aug 25, 2021

HyukjinKwon commented Aug 26, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Peng-Lei commented Aug 27, 2021

dgd-contributor commented Aug 30, 2021 • edited

karenfeng left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karenfeng left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgd-contributor commented Sep 21, 2021

karenfeng left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HyukjinKwon commented Sep 23, 2021 • edited

HyukjinKwon commented Sep 26, 2021

dgd-contributor commented Sep 27, 2021

github-actions bot commented Jan 6, 2022

dgd-contributor commented Aug 30, 2021 •

edited

HyukjinKwon commented Sep 23, 2021 •

edited