[SPARK-40975][SQL] Rename the error class _LEGACY_ERROR_TEMP_0021 to UNSUPPORTED_TYPED_LITERAL
#38448
Conversation
MaxGekk
changed the title
_LEGACY_ERROR_TEMP_0021 to UNSUPPORTED_TYPED_LITERAL_LEGACY_ERROR_TEMP_0021 to UNSUPPORTED_TYPED_LITERAL
MaxGekk
changed the title
_LEGACY_ERROR_TEMP_0021 to UNSUPPORTED_TYPED_LITERAL_LEGACY_ERROR_TEMP_0021 to UNSUPPORTED_TYPED_LITERAL
|
The failure is not related to my changes: The same finished successfully on my laptop. |
|
@cloud-fan @itholic @srielau @LuciferYang @panbingkun Please, review this PR. |
| throw QueryParsingErrors.literalValueTypeUnsupportedError( | ||
| unsupportedType = other, | ||
| supportedTypes = | ||
| Seq("DATE", "TIMESTAMP_NTZ", "TIMESTAMP_LTZ", "TIMESTAMP", "INTERVAL", "X"), |
There was a problem hiding this comment.
Maybe a lowercase hexadecimal would be less confusing?
There was a problem hiding this comment.
Here, I pointed out the exact keyword that can be used in the typed literals, see
The syntax is:
There was a problem hiding this comment.
We could give a hint to user like X hexadecimal
There was a problem hiding this comment.
Do you mean "X" -> "X hexadecimal" ?
There was a problem hiding this comment.
Yep, but I would like it as is. Inconsistency confuses always. If we point out supported "types" in typed literals, we should show supported words only.
There was a problem hiding this comment.
Agree with you, "X hexadecimal" looks a little strange
itholic
left a comment
itholic
left a comment
There was a problem hiding this comment.
LGTM when the test passing
| "errorClass" : "UNSUPPORTED_TYPED_LITERAL", | ||
| "messageParameters" : { | ||
| "valueType" : "GEO" | ||
| "supportedTypes" : "\"DATE\", \"TIMESTAMP_NTZ\", \"TIMESTAMP_LTZ\", \"TIMESTAMP\", \"INTERVAL\", \"X\"", |
There was a problem hiding this comment.
isn't it a constant list? does it need to be a parameter?
There was a problem hiding this comment.
It depends on implementation, and we might support more in the future. Having the list in the source code can give the following benefits:
- The list of parameters might be formatted in different ways ( as a foldable or drop-down list) by frontend tools.
- The error classes can be re-used from another places with different list.
- More likely, devs will not forget to update the list in source code.
- If the list will be in the JSON file, tech editors might forget to edit it or accidentally modify it.
There was a problem hiding this comment.
I think it should be a parameter, unless UNSUPPORTED_TYPED_LITERAL will only be used in this scenario
|
Merging to master. Thank you, @cloud-fan @LuciferYang @itholic @srielau for review. |
…o `UNSUPPORTED_TYPED_LITERAL` ### What changes were proposed in this pull request? In the PR, I propose to assign the proper name `UNSUPPORTED_TYPED_LITERAL ` to the legacy error class `_LEGACY_ERROR_TEMP_0021 `, and modify test suite to use `checkError()` which checks the error class name, context and etc. ### Why are the changes needed? Proper name improves user experience w/ Spark SQL. ### Does this PR introduce _any_ user-facing change? Yes, the PR changes an user-facing error message. ### How was this patch tested? By running the modified test suites: ``` $ PYSPARK_PYTHON=python3 build/sbt "sql/testOnly org.apache.spark.sql.SQLQueryTestSuite" $ build/sbt "test:testOnly *ExpressionParserSuite" $ build/sbt "test:testOnly *AnalysisSuite" ``` Closes apache#38448 from MaxGekk/error-class-typed-literal. Authored-by: Max Gekk <[email protected]> Signed-off-by: Max Gekk <[email protected]>
5 participants
What changes were proposed in this pull request?
In the PR, I propose to assign the proper name
UNSUPPORTED_TYPED_LITERALto the legacy error class_LEGACY_ERROR_TEMP_0021, and modify test suite to usecheckError()which checks the error class name, context and etc.Why are the changes needed?
Proper name improves user experience w/ Spark SQL.
Does this PR introduce any user-facing change?
Yes, the PR changes an user-facing error message.
How was this patch tested?
By running the modified test suites: