Re: [PR] [SPARK-47254][SQL] Assign names to the error classes _LEGACY_ERROR_TEMP_325[1-9][WIP] [spark]

["MaxGekk (via GitHub)" <gi...@apache.org>]

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-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org