You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "stefanbuk-db (via GitHub)" <gi...@apache.org> on 2024/03/06 16:40:29 UTC
[PR] [SPARK-47254][SQL] Assign names to the error classes _LEGACY_ERROR_TEMP_325[1-9] [spark]
stefanbuk-db opened a new pull request, #45407:
URL: https://github.com/apache/spark/pull/45407
### What changes were proposed in this pull request?
In the PR, I propose to assign the proper names to the legacy error classes _LEGACY_ERROR_TEMP_325[1-9], and modify tests in testing suites to reflect these changes and use checkError() function. Also this PR improves the error messages.
### 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 modified test suits:
`build/sbt "test:testOnly *DateTimeFormatterHelperSuite"`
`build/sbt "test:testOnly *IntervalUtilsSuite"`
`build/sbt "test:testOnly *DataTypeSuite"`
`build/sbt "test:testOnly *StructTypeSuite"`
### Was this patch authored or co-authored using generative AI tooling?
No
--
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
Re: [PR] [WIP][SPARK-47254][SQL] Assign names to the error classes _LEGACY_ERROR_TEMP_325[1-9] [spark]
Posted by "stefanbuk-db (via GitHub)" <gi...@apache.org>.
stefanbuk-db commented on code in PR #45407:
URL: https://github.com/apache/spark/pull/45407#discussion_r1517476444
##########
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:
I think so, this allows user to extract a field from datetime format, even using week characters.
--
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
Re: [PR] [SPARK-47254][SQL] Assign names to the error classes _LEGACY_ERROR_TEMP_325[1-9] [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on PR #45407:
URL: https://github.com/apache/spark/pull/45407#issuecomment-1991209263
+1, LGTM. Merging to master.
Thank you, @stefanbuk-db.
--
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
Re: [PR] [WIP][SPARK-47254][SQL] Assign names to the error classes _LEGACY_ERROR_TEMP_325[1-9] [spark]
Posted by "stefanbuk-db (via GitHub)" <gi...@apache.org>.
stefanbuk-db commented on code in PR #45407:
URL: https://github.com/apache/spark/pull/45407#discussion_r1519208459
##########
sql/api/src/main/scala/org/apache/spark/sql/types/StructType.scala:
##########
@@ -283,10 +283,10 @@ case class StructType(fields: Array[StructField]) extends DataType with Seq[Stru
def apply(name: String): StructField = {
nameToField.getOrElse(name,
throw new SparkIllegalArgumentException(
- errorClass = "_LEGACY_ERROR_TEMP_3254",
+ errorClass = "FIELD_NOT_FOUND",
messageParameters = immutable.Map(
- "name" -> name,
- "fieldNames" -> fieldNames.mkString(", "))))
+ "fieldName" -> name,
Review Comment:
Done in new commits.
##########
sql/api/src/main/scala/org/apache/spark/sql/types/StructType.scala:
##########
@@ -316,10 +316,10 @@ case class StructType(fields: Array[StructField]) extends DataType with Seq[Stru
def fieldIndex(name: String): Int = {
nameToIndex.getOrElse(name,
throw new SparkIllegalArgumentException(
- errorClass = "_LEGACY_ERROR_TEMP_3252",
+ errorClass = "FIELD_NOT_FOUND",
messageParameters = immutable.Map(
- "name" -> name,
- "fieldNames" -> fieldNames.mkString(", "))))
+ "fieldName" -> name,
Review Comment:
Done.
--
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
Re: [PR] [SPARK-47254][SQL] Assign names to the error classes _LEGACY_ERROR_TEMP_325[1-9][WIP] [spark]
Posted by "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
Re: [PR] [WIP][SPARK-47254][SQL] Assign names to the error classes _LEGACY_ERROR_TEMP_325[1-9] [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #45407:
URL: https://github.com/apache/spark/pull/45407#discussion_r1519942137
##########
sql/api/src/main/scala/org/apache/spark/sql/catalyst/util/SparkIntervalUtils.scala:
##########
@@ -131,24 +131,21 @@ trait SparkIntervalUtils {
*/
def stringToInterval(input: UTF8String): CalendarInterval = {
import ParseState._
- def throwIAE(msg: String, e: Exception = null) = {
+ if (input == null) {
throw new SparkIllegalArgumentException(
- errorClass = "_LEGACY_ERROR_TEMP_3255",
+ errorClass = "INVALID_INTERVAL_FORMAT.INPUT_IS_NULL",
messageParameters = Map(
- "input" -> Option(input).map(_.toString).getOrElse("null"),
- "msg" -> msg),
- cause = e)
- }
-
- if (input == null) {
- throwIAE("interval string cannot be null")
+ "input" -> Option(input).map(_.toString).getOrElse("null")))
Review Comment:
`input` is null for sure here, just place `"null"`
##########
sql/api/src/main/scala/org/apache/spark/sql/catalyst/util/SparkIntervalUtils.scala:
##########
@@ -131,24 +131,21 @@ trait SparkIntervalUtils {
*/
def stringToInterval(input: UTF8String): CalendarInterval = {
import ParseState._
- def throwIAE(msg: String, e: Exception = null) = {
+ if (input == null) {
throw new SparkIllegalArgumentException(
- errorClass = "_LEGACY_ERROR_TEMP_3255",
+ errorClass = "INVALID_INTERVAL_FORMAT.INPUT_IS_NULL",
messageParameters = Map(
- "input" -> Option(input).map(_.toString).getOrElse("null"),
- "msg" -> msg),
- cause = e)
- }
-
- if (input == null) {
- throwIAE("interval string cannot be null")
+ "input" -> Option(input).map(_.toString).getOrElse("null")))
}
// scalastyle:off caselocale .toLowerCase
val s = input.trimAll().toLowerCase
// scalastyle:on
val bytes = s.getBytes
if (bytes.isEmpty) {
- throwIAE("interval string cannot be empty")
+ throw new SparkIllegalArgumentException(
+ errorClass = "INVALID_INTERVAL_FORMAT.INPUT_IS_EMPTY",
+ messageParameters = Map(
+ "input" -> Option(input).map(_.toString).getOrElse("null")))
Review Comment:
Can it be `null` here? Seems not.
##########
sql/api/src/main/scala/org/apache/spark/sql/catalyst/util/SparkIntervalUtils.scala:
##########
@@ -183,9 +180,16 @@ trait SparkIntervalUtils {
case PREFIX =>
if (s.startsWith(intervalStr)) {
if (s.numBytes() == intervalStr.numBytes()) {
- throwIAE("interval string cannot be empty")
+ throw new SparkIllegalArgumentException(
+ errorClass = "INVALID_INTERVAL_FORMAT.INPUT_IS_EMPTY",
+ messageParameters = Map(
+ "input" -> Option(input).map(_.toString).getOrElse("null")))
Review Comment:
ditto: can it be `null` here?
--
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
Re: [PR] [WIP][SPARK-47254][SQL] Assign names to the error classes _LEGACY_ERROR_TEMP_325[1-9] [spark]
Posted by "stefanbuk-db (via GitHub)" <gi...@apache.org>.
stefanbuk-db commented on code in PR #45407:
URL: https://github.com/apache/spark/pull/45407#discussion_r1517474681
##########
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:
Done in new commits. Changed relevant tests also.
--
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
Re: [PR] [WIP][SPARK-47254][SQL] Assign names to the error classes _LEGACY_ERROR_TEMP_325[1-9] [spark]
Posted by "stefanbuk-db (via GitHub)" <gi...@apache.org>.
stefanbuk-db commented on code in PR #45407:
URL: https://github.com/apache/spark/pull/45407#discussion_r1519209532
##########
common/utils/src/main/resources/error/error-classes.json:
##########
@@ -2060,6 +2085,74 @@
},
"sqlState" : "42000"
},
+ "INVALID_INTERVAL_FORMAT" : {
+ "message" : [
+ "Error parsing '<input>' to interval. Please ensure that the value provided is in a valid format for defining an interval. You can reference the documentation for the correct format."
+ ],
+ "subClass" : {
+ "ARITHMETIC_EXCEPTION" : {
+ "message" : [
+ "Uncaught arithmetic exception: <e>."
Review Comment:
Since this is an arithmetic error in interval argument, arbitrary `<e>` is removed. Message now provides information that an uncaught arithmetic error happened.
--
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
Re: [PR] [SPARK-47254][SQL] Assign names to the error classes _LEGACY_ERROR_TEMP_325[1-9] [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on PR #45407:
URL: https://github.com/apache/spark/pull/45407#issuecomment-1991214346
@stefanbuk-db Congratulations with your first contribution to Apache Spark!
--
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
Re: [PR] [SPARK-47254][SQL] Assign names to the error classes _LEGACY_ERROR_TEMP_325[1-9] [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk closed pull request #45407: [SPARK-47254][SQL] Assign names to the error classes _LEGACY_ERROR_TEMP_325[1-9]
URL: https://github.com/apache/spark/pull/45407
--
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
Re: [PR] [WIP][SPARK-47254][SQL] Assign names to the error classes _LEGACY_ERROR_TEMP_325[1-9] [spark]
Posted by "stefanbuk-db (via GitHub)" <gi...@apache.org>.
stefanbuk-db commented on code in PR #45407:
URL: https://github.com/apache/spark/pull/45407#discussion_r1517455237
##########
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:
Legacy error thrown here was parsing exception. Now we throw SparkIllegalArgumentException for this query. There are already tests for this error in appropriate suites, so this test is outdated.
--
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
Re: [PR] [WIP][SPARK-47254][SQL] Assign names to the error classes _LEGACY_ERROR_TEMP_325[1-9] [spark]
Posted by "stefanbuk-db (via GitHub)" <gi...@apache.org>.
stefanbuk-db commented on code in PR #45407:
URL: https://github.com/apache/spark/pull/45407#discussion_r1517474227
##########
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:
Done in new commits. New error message is better.
--
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
Re: [PR] [WIP][SPARK-47254][SQL] Assign names to the error classes _LEGACY_ERROR_TEMP_325[1-9] [spark]
Posted by "stefanbuk-db (via GitHub)" <gi...@apache.org>.
stefanbuk-db commented on code in PR #45407:
URL: https://github.com/apache/spark/pull/45407#discussion_r1519208319
##########
common/utils/src/main/resources/error/error-classes.json:
##########
@@ -1823,6 +1830,24 @@
},
"sqlState" : "HY109"
},
+ "INVALID_DATETIME_PATTERN" : {
+ "message" : [
+ "Unrecognized datetime pattern: <pattern>. Please provide correct datetime pattern."
Review Comment:
Done in new commits.
--
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
Re: [PR] [WIP][SPARK-47254][SQL] Assign names to the error classes _LEGACY_ERROR_TEMP_325[1-9] [spark]
Posted by "stefanbuk-db (via GitHub)" <gi...@apache.org>.
stefanbuk-db commented on code in PR #45407:
URL: https://github.com/apache/spark/pull/45407#discussion_r1517477382
##########
common/utils/src/main/resources/error/error-classes.json:
##########
@@ -2807,6 +2844,24 @@
],
"sqlState" : "07501"
},
+ "NONEXISTENT_FIELD_NAME_INDEXING" : {
Review Comment:
`FIELD_NOT_FOUND` is a good existing error to use. Removed duplicates and updated tests.
--
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
Re: [PR] [WIP][SPARK-47254][SQL] Assign names to the error classes _LEGACY_ERROR_TEMP_325[1-9] [spark]
Posted by "stefanbuk-db (via GitHub)" <gi...@apache.org>.
stefanbuk-db commented on code in PR #45407:
URL: https://github.com/apache/spark/pull/45407#discussion_r1517488703
##########
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:
It's an output from a deleted test throwing ParsingException.
--
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
Re: [PR] [WIP][SPARK-47254][SQL] Assign names to the error classes _LEGACY_ERROR_TEMP_325[1-9] [spark]
Posted by "stefanbuk-db (via GitHub)" <gi...@apache.org>.
stefanbuk-db commented on code in PR #45407:
URL: https://github.com/apache/spark/pull/45407#discussion_r1519220632
##########
sql/api/src/main/scala/org/apache/spark/sql/catalyst/util/SparkIntervalUtils.scala:
##########
@@ -229,13 +237,21 @@ trait SparkIntervalUtils {
try {
currentValue = Math.addExact(Math.multiplyExact(10, currentValue), (b - '0'))
} catch {
- case e: ArithmeticException => throwIAE(e.getMessage, e)
+ case e: ArithmeticException => throw new SparkIllegalArgumentException(
+ errorClass = "INVALID_INTERVAL_FORMAT.ARITHMETIC_EXCEPTION",
Review Comment:
It is related to illegal parsed value in interval format argument, thats why we throw this error.
--
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
Re: [PR] [SPARK-47254][SQL] Assign names to the error classes _LEGACY_ERROR_TEMP_325[1-9][WIP] [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on PR #45407:
URL: https://github.com/apache/spark/pull/45407#issuecomment-1983367613
@stefanbuk-db If you are still working on the PR, please, move the tag `[WIP]` at the beginning of PR's title (this is a convention)
--
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
Re: [PR] [WIP][SPARK-47254][SQL] Assign names to the error classes _LEGACY_ERROR_TEMP_325[1-9] [spark]
Posted by "stefanbuk-db (via GitHub)" <gi...@apache.org>.
stefanbuk-db commented on code in PR #45407:
URL: https://github.com/apache/spark/pull/45407#discussion_r1517475297
##########
common/utils/src/main/resources/error/error-classes.json:
##########
@@ -1770,6 +1777,12 @@
],
"sqlState" : "22P03"
},
+ "INVALID_CHARACTER_IN_DATETIME_PATTERN" : {
Review Comment:
Done. Changed relevant tests also.
--
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
Re: [PR] [WIP][SPARK-47254][SQL] Assign names to the error classes _LEGACY_ERROR_TEMP_325[1-9] [spark]
Posted by "stefanbuk-db (via GitHub)" <gi...@apache.org>.
stefanbuk-db commented on code in PR #45407:
URL: https://github.com/apache/spark/pull/45407#discussion_r1517456249
##########
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:
Done in new commits. This class is split in multiple subClasses, without <msg> field.
--
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
Re: [PR] [WIP][SPARK-47254][SQL] Assign names to the error classes _LEGACY_ERROR_TEMP_325[1-9] [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #45407:
URL: https://github.com/apache/spark/pull/45407#discussion_r1518212624
##########
sql/api/src/main/scala/org/apache/spark/sql/types/StructType.scala:
##########
@@ -316,10 +316,10 @@ case class StructType(fields: Array[StructField]) extends DataType with Seq[Stru
def fieldIndex(name: String): Int = {
nameToIndex.getOrElse(name,
throw new SparkIllegalArgumentException(
- errorClass = "_LEGACY_ERROR_TEMP_3252",
+ errorClass = "FIELD_NOT_FOUND",
messageParameters = immutable.Map(
- "name" -> name,
- "fieldNames" -> fieldNames.mkString(", "))))
+ "fieldName" -> name,
Review Comment:
ditto
##########
common/utils/src/main/resources/error/error-classes.json:
##########
@@ -1823,6 +1830,24 @@
},
"sqlState" : "HY109"
},
+ "INVALID_DATETIME_PATTERN" : {
+ "message" : [
+ "Unrecognized datetime pattern: <pattern>. Please provide correct datetime pattern."
Review Comment:
```suggestion
"Unrecognized datetime pattern: <pattern>."
```
The second sentence is useless, I think. Let's leave suggestions to sub-classes.
##########
sql/api/src/main/scala/org/apache/spark/sql/catalyst/util/SparkIntervalUtils.scala:
##########
@@ -229,13 +237,21 @@ trait SparkIntervalUtils {
try {
currentValue = Math.addExact(Math.multiplyExact(10, currentValue), (b - '0'))
} catch {
- case e: ArithmeticException => throwIAE(e.getMessage, e)
+ case e: ArithmeticException => throw new SparkIllegalArgumentException(
+ errorClass = "INVALID_INTERVAL_FORMAT.ARITHMETIC_EXCEPTION",
Review Comment:
Are you sure that the error is related to a format but not to parsed value?
##########
sql/api/src/main/scala/org/apache/spark/sql/types/StructType.scala:
##########
@@ -283,10 +283,10 @@ case class StructType(fields: Array[StructField]) extends DataType with Seq[Stru
def apply(name: String): StructField = {
nameToField.getOrElse(name,
throw new SparkIllegalArgumentException(
- errorClass = "_LEGACY_ERROR_TEMP_3254",
+ errorClass = "FIELD_NOT_FOUND",
messageParameters = immutable.Map(
- "name" -> name,
- "fieldNames" -> fieldNames.mkString(", "))))
+ "fieldName" -> name,
Review Comment:
Field names are identifiers, I guess. If so, please, quote them using `toSQLId`
##########
common/utils/src/main/resources/error/error-classes.json:
##########
@@ -2060,6 +2085,74 @@
},
"sqlState" : "42000"
},
+ "INVALID_INTERVAL_FORMAT" : {
+ "message" : [
+ "Error parsing '<input>' to interval. Please ensure that the value provided is in a valid format for defining an interval. You can reference the documentation for the correct format."
+ ],
+ "subClass" : {
+ "ARITHMETIC_EXCEPTION" : {
+ "message" : [
+ "Uncaught arithmetic exception: <e>."
Review Comment:
Could you avoid the arbitrary text in `<e>`. In the future, we will translate error messages in different languages. We don't need uncontrolled inlining of some English text.
--
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