Skip to content

[SPARK-36292][SQL] Refactor third set of 20 in QueryExecutionErrors to use error classes#33863

Closed
dgd-contributor wants to merge 9 commits intoapache:masterfrom
dgd-contributor:SPARK-36292-v2
Closed

[SPARK-36292][SQL] Refactor third set of 20 in QueryExecutionErrors to use error classes#33863
dgd-contributor wants to merge 9 commits intoapache:masterfrom
dgd-contributor:SPARK-36292-v2

Conversation

@dgd-contributor
Copy link

What changes were proposed in this pull request?

Refactor some exceptions in QueryExecutionErrors to use error classes.

cannotGenerateCodeForUnsupportedTypeError
cannotInterpolateClassIntoCodeBlockError
customCollectionClsNotResolvedError
classUnsupportedByMapObjectsError
nullAsMapKeyNotAllowedError
methodNotDeclaredError
constructorNotFoundError
primaryConstructorNotFoundError
unsupportedNaturalJoinTypeError
notExpectedUnresolvedEncoderError
unsupportedEncoderError
notOverrideExpectedMethodsError
failToConvertValueToJsonError
unexpectedOperatorInCorrelatedSubquery
unreachableError
unsupportedRoundingMode
resolveCannotHandleNestedSchema
inputExternalRowCannotBeNullError
fieldCannotBeNullMsg
fieldCannotBeNullError

Why are the changes needed?

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

Does this PR introduce any user-facing change?

No

How was this patch tested?

Existed UT Testcase

@AmplabJenkins
Copy link

Can one of the admins verify this patch?

@HyukjinKwon
Copy link
Member

cc @karenfeng FYI

Copy link
Contributor

@karenfeng karenfeng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

},
"CLASS_UNSUPPORTED_BY_MAP_OBJECTS" : {
"message" : [ "Class `%s` is not supported by `MapObjects` as resulting collection." ],
"sqlState" : "42000"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

0A000 may be a better fit here.

"FAILED_SET_ORIGINAL_PERMISSION_BACK" : {
"message" : [ "Failed to set original permission %s back to the created path: %s. Exception: %s" ]
},
"FAIL_TO_CONVERT_VALUE_TO_JSON" : {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For consistency: FAIL -> FAILED?

"sqlState" : "22023"
},
"RESOLVE_CANNOT_HANDLE_NESTED_SCHEMA" : {
"message" : [ "Can not handle nested schema yet... plan %s" ],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should avoid making promises about future support. Can you remove yet?

"message" : [ "Unable to acquire %s bytes of memory, got %s" ]
},
"UNEXPECTED_OPERATOR_IN_CORRELATED_SUBQUERY" : {
"message" : [ "Unexpected operator %s in correlated subquery%s" ],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: there should be a space between subquery and %s

"sqlState" : "42000"
},
"UNREACHABLE" : {
"message" : [ "This line should be unreachable%s" ],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: there should be a space between unreachable and %s

"sqlState" : "42000"
},
"UNSUPPORTED_ENCODER" : {
"message" : [ "Only expression encoders are supported for now." ],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should avoid making promises about future support. Can you remove for now?

},
"UNSUPPORTED_ENCODER" : {
"message" : [ "Only expression encoders are supported for now." ],
"sqlState" : "42000"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

0A000 may be a better fit for all "unsupported" errors.

},
"UNSUPPORTED_NATURAL_JOIN_TYPE" : {
"message" : [ "Unsupported natural join type %s" ],
"sqlState" : "42000"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

0A000 may be a better fit for all "unsupported" errors.

/**
* Unsupported operation exception thrown from Spark with an error class.
*/
class SparkUnsupportedOperationException(errorClass: String, messageParameters: Array[String])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should make these classes package-private, eg.

private[spark] class SparkSQLFeatureNotSupportedException(

Copy link
Contributor

@karenfeng karenfeng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

},
"FAILED_TO_CONVERT_VALUE_TO_JSON" : {
"message" : [ "Failed to convert value %s (class of %s) with the type of %s to JSON." ],
"sqlState" : "42000"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

22005 may be a better fit here.

@@ -55,7 +83,12 @@
"sqlState" : "22023"
},
"INVALID_JSON_SCHEMA_MAPTYPE" : {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While we're here, can you also update: INVALID_JSON_SCHEMA_MAPTYPE -> INVALID_JSON_MAP_KEY_TYPE? The original class name is not very descriptive.

"message" : [ "Input schema %s can only contain StringType as a key type for a MapType." ],
"sqlState" : "42000"
},
"METHOD_NOT_DECLARED" : {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is used? It's also already defined as MISSING_METHOD.

"message" : [ "PARTITION clause cannot contain a non-partition column name: %s" ],
"sqlState" : "42000"
},
"NOT_EXPECTED_UNRESOLVED_ENCODER" : {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NOT_EXPECTED_UNRESOLVED_ENCODER -> UNEXPECTED_UNRESOLVED_ENCODER

},
"NOT_EXPECTED_UNRESOLVED_ENCODER" : {
"message" : [ "Unresolved encoder expected, but %s was found." ],
"sqlState" : "42000"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

},
"RESOLVE_CANNOT_HANDLE_NESTED_SCHEMA" : {
"message" : [ "Can not handle nested schema... plan %s" ],
"sqlState" : "42000"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

"message" : [ "Failed to rename as %s was not found" ],
"sqlState" : "22023"
},
"RESOLVE_CANNOT_HANDLE_NESTED_SCHEMA" : {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RESOLVE_CANNOT_HANDLE_NESTED_SCHEMA -> CANNOT_RESOLVE_NESTED_SCHEMA

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bump

},
"UNREACHABLE" : {
"message" : [ "This line should be unreachable %s" ],
"sqlState" : "42000"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

"message" : [ "Unexpected operator %s in correlated subquery %s" ],
"sqlState" : "42000"
},
"UNREACHABLE" : {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

UNREACHABLE -> REACHED_UNREACHABLE_LINE - we should provide more detail

"message" : [ "Unsupported natural join type %s" ],
"sqlState" : "0A000"
},
"UNSUPPORTED_OPERATION_NOT_RESOLVED" : {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

UNSUPPORTED_OPERATION_NOT_RESOLVED -> NOT_RESOLVED

@dgd-contributor
Copy link
Author

cc @karenfeng, can you pls review it again?
Thank you.

Copy link
Contributor

@karenfeng karenfeng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left a few more comments. Thanks for working on this @dgd-contributor, please ping me when you've addressed them.

},
"CANNOT_GENERATE_CODE_FOR_UNSUPPORTED_TYPE" : {
"message" : [ "Cannot generate code for unsupported type: %s" ],
"sqlState" : "42000"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be 0A000 (unsupported)

},
"PRIMARY_CONSTRUCTOR_NOT_FOUND" : {
"message" : [ "Couldn't find a primary constructor on %s" ],
"sqlState" : "42000"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bump!

"message" : [ "Failed to rename as %s was not found" ],
"sqlState" : "22023"
},
"RESOLVE_CANNOT_HANDLE_NESTED_SCHEMA" : {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bump

"message" : [ "Unsupported natural join type %s" ],
"sqlState" : "0A000"
},
"UNSUPPORTED_ROUNDING" : {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

UNSUPPORTED_ROUNDING -> UNSUPPORTED_ROUNDING_MODE

"message" : [ "Unexpected operator %s in correlated subquery %s" ],
"sqlState" : "42000"
},
"UNEXPECTED_UNRESOLVED_ENCODER" : {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoops, I meant: UNEXPECTED_UNRESOLVED_ENCODER -> EXPECTED_UNRESOLVED_ENCODER

"message" : [ "PARTITION clause cannot contain a non-partition column name: %s" ],
"sqlState" : "42000"
},
"NOT_RESOLVED" : {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In retrospect, this is specific to custom collection class resolution. Can we make this class name more specific? Eg. CUSTOM_COLLECTION_CLASS_NOT_RESOLVED

"message" : [ "Failed to convert value %s (class of %s) with the type of %s to JSON." ],
"sqlState" : "22005"
},
"FIELD_CANNOT_BE_NULL" : {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's make this more specific: ROW_FIELD_CANNOT_BE_NULL

@dgd-contributor
Copy link
Author

Thank you for your review, @karenfeng. As we and @HyukjinKwon discussed contributing by sharing accounts, we will close this PR, and recreate this one with an individual account. We will also update the code with your comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants