MaxGekk commented on code in PR #45407:
URL: https://github.com/apache/spark/pull/45407#discussion_r1515965942
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/command/DDLParserSuite.scala:
##########
@@ -455,19 +455,6 @@ class DDLParserSuite extends AnalysisTest with
SharedSparkSession {
fragment = fragment3,
start = 16,
stop = 40))
-
- val sql4 = "select interval '.1111111111' second"
Review Comment:
Could you explain why did you remove the check, please.
##########
common/utils/src/main/resources/error/error-classes.json:
##########
@@ -2060,6 +2085,12 @@
},
"sqlState" : "42000"
},
+ "INVALID_INTERVAL_FORMAT" : {
+ "message" : [
+ "Error parsing '<input>' to interval, <msg>. Please ensure that the
value provided is in a valid format for defining an interval. You can reference
the documentation for the correct format. If the issue persists, please double
check that the input value is not null or empty and try again."
Review Comment:
We should avoid embedding a text in English. Could you create error
sub-classes instead of passing `msg`. A reason is to have entire error message
format in `error-classes.json`, so, it can be translated to other languages
like Serbian, and tech writers won't need to modified the source code.
##########
common/utils/src/main/resources/error/error-classes.json:
##########
@@ -1770,6 +1777,12 @@
],
"sqlState" : "22P03"
},
+ "INVALID_CHARACTER_IN_DATETIME_PATTERN" : {
Review Comment:
Could you convert it to sub-class of `INVALID_DATETIME_PATTERN`
##########
common/utils/src/main/resources/error/error-classes.json:
##########
@@ -1823,6 +1836,18 @@
},
"sqlState" : "HY109"
},
+ "INVALID_DATETIME_PATTERN" : {
+ "message" : [
+ "Unrecognized datetime pattern: <pattern>. Please provide correct
datetime pattern."
+ ],
+ "sqlState" : "22007"
+ },
+ "INVALID_DATETIME_PATTERN_LENGTH" : {
Review Comment:
Could you create a sub-class of `INVALID_DATETIME_PATTERN` with the name
`LENGTH`
##########
sql/core/src/test/resources/sql-tests/results/interval.sql.out:
##########
@@ -1266,19 +1266,14 @@ select interval 10 nanoseconds
-- !query schema
struct<>
-- !query output
-org.apache.spark.sql.catalyst.parser.ParseException
+org.apache.spark.SparkIllegalArgumentException
{
- "errorClass" : "_LEGACY_ERROR_TEMP_0062",
+ "errorClass" : "INVALID_INTERVAL_FORMAT",
+ "sqlState" : "22006",
"messageParameters" : {
- "msg" : "Error parsing ' 10 nanoseconds' to interval, invalid unit
'nanoseconds'"
- },
- "queryContext" : [ {
Review Comment:
Why do we loose the context?
##########
sql/core/src/test/resources/sql-tests/results/datetime-formatting-invalid.sql.out:
##########
@@ -40,7 +40,8 @@ struct<>
-- !query output
org.apache.spark.SparkIllegalArgumentException
{
- "errorClass" : "_LEGACY_ERROR_TEMP_3259",
+ "errorClass" : "INVALID_DATETIME_PATTERN_LENGTH",
+ "sqlState" : "22007",
"messageParameters" : {
"style" : "q"
Review Comment:
So, the error message is:
```
Too many pattern letters: q. Please reduce pattern length.
```
It would be better to show invalid pattern:
```
Too many pattern letters: 'qqqqq'. Please reduce pattern length.
```
##########
sql/core/src/test/resources/sql-tests/results/datetime-formatting-invalid.sql.out:
##########
@@ -500,7 +506,8 @@ struct<>
-- !query output
org.apache.spark.SparkIllegalArgumentException
{
- "errorClass" : "_LEGACY_ERROR_TEMP_3257",
+ "errorClass" :
"INCONSISTENT_BEHAVIOR_CROSS_VERSION.DATETIME_WEEK_BASED_PATTERN",
Review Comment:
We recommend to use the `EXTRACT` function in the error message:
```
"Please use the SQL function EXTRACT instead."
```
Could you double check that this recommendation is useful, please.
##########
common/utils/src/main/resources/error/error-classes.json:
##########
@@ -2083,6 +2114,12 @@
},
"sqlState" : "42K0K"
},
+ "INVALID_JSON_DATA_TYPE" : {
+ "message" : [
+ "Failed to convert the JSON string '<other>' to a data type. Please
enter a valid data type."
Review Comment:
```suggestion
"Failed to convert the JSON string '<invalidType>' to a data type.
Please enter a valid data type."
```
##########
common/utils/src/main/resources/error/error-classes.json:
##########
@@ -2807,6 +2844,24 @@
],
"sqlState" : "07501"
},
+ "NONEXISTENT_FIELD_NAME_INDEXING" : {
Review Comment:
Let's create an error class with sub-classes. BTW, cannot you re-use the
existing one `FIELD_NOT_FOUND`?
--
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
