You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GitBox <gi...@apache.org> on 2022/11/01 07:38:05 UTC
[GitHub] [spark] panbingkun opened a new pull request, #38463: [SPARK-40374][SQL] Migrate type check failures of type creators onto error classes
panbingkun opened a new pull request, #38463:
URL: https://github.com/apache/spark/pull/38463
### What changes were proposed in this pull request?
This pr replaces TypeCheckFailure by DataTypeMismatch in type checks in the complex type creator expressions, includes:
1. CreateMap (3): https://github.com/apache/spark/blob/1431975723d8df30a25b2333eddcfd0bb6c57677/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/complexTypeCreator.scala#L205-L214
2. CreateNamedStruct (3): https://github.com/apache/spark/blob/1431975723d8df30a25b2333eddcfd0bb6c57677/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/complexTypeCreator.scala#L445-L457
3. UpdateFields (2): https://github.com/apache/spark/blob/1431975723d8df30a25b2333eddcfd0bb6c57677/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/complexTypeCreator.scala#L670-L673
### Why are the changes needed?
Migration onto error classes unifies Spark SQL error messages.
### Does this PR introduce _any_ user-facing change?
Yes. The PR changes user-facing error messages.
### How was this patch tested?
1. Add new UT
2. Update existed UT
3. Pass GA
--
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
[GitHub] [spark] panbingkun commented on a diff in pull request #38463: [SPARK-40374][SQL] Migrate type check failures of type creators onto error classes
Posted by GitBox <gi...@apache.org>.
panbingkun commented on code in PR #38463:
URL: https://github.com/apache/spark/pull/38463#discussion_r1010355945
##########
core/src/main/resources/error/error-classes.json:
##########
@@ -155,6 +160,21 @@
"To convert values from <srcType> to <targetType>, you can use the functions <functionNames> instead."
]
},
+ "CREATE_MAP_KEY_DIFF_TYPES" : {
Review Comment:
but
<img width="814" alt="image" src="https://user-images.githubusercontent.com/15246973/199228400-42c1759f-fc16-4205-a052-fc54b1f3d70b.png">
Very hard to reuse it.
Maybe `functionName` pass to: `map's key`, seems unreasonable
--
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
[GitHub] [spark] panbingkun commented on a diff in pull request #38463: [SPARK-40374][SQL] Migrate type check failures of type creators onto error classes
Posted by GitBox <gi...@apache.org>.
panbingkun commented on code in PR #38463:
URL: https://github.com/apache/spark/pull/38463#discussion_r1010317628
##########
core/src/main/resources/error/error-classes.json:
##########
@@ -155,6 +160,21 @@
"To convert values from <srcType> to <targetType>, you can use the functions <functionNames> instead."
]
},
+ "CREATE_MAP_KEY_DIFF_TYPES" : {
Review Comment:
<img width="637" alt="image" src="https://user-images.githubusercontent.com/15246973/199219681-f7089110-7701-42e5-82df-d772f9ed4304.png">
--
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
[GitHub] [spark] MaxGekk closed pull request #38463: [SPARK-40374][SQL] Migrate type check failures of type creators onto error classes
Posted by GitBox <gi...@apache.org>.
MaxGekk closed pull request #38463: [SPARK-40374][SQL] Migrate type check failures of type creators onto error classes
URL: https://github.com/apache/spark/pull/38463
--
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
[GitHub] [spark] MaxGekk commented on a diff in pull request #38463: [SPARK-40374][SQL] Migrate type check failures of type creators onto error classes
Posted by GitBox <gi...@apache.org>.
MaxGekk commented on code in PR #38463:
URL: https://github.com/apache/spark/pull/38463#discussion_r1011505471
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/complexTypeCreator.scala:
##########
@@ -444,17 +460,32 @@ case class CreateNamedStruct(children: Seq[Expression]) extends Expression with
override def checkInputDataTypes(): TypeCheckResult = {
if (children.size % 2 != 0) {
- TypeCheckResult.TypeCheckFailure(s"$prettyName expects an even number of arguments.")
+ DataTypeMismatch(
+ errorSubClass = "WRONG_NUM_ARGS",
+ messageParameters = Map(
+ "functionName" -> toSQLId(prettyName),
+ "expectedNum" -> "2n (n > 0)",
+ "actualNum" -> children.length.toString
+ )
+ )
} else {
val invalidNames = nameExprs.filterNot(e => e.foldable && e.dataType == StringType)
if (invalidNames.nonEmpty) {
- TypeCheckResult.TypeCheckFailure(
- s"Only foldable ${StringType.catalogString} expressions are allowed to appear at odd" +
- s" position, got: ${invalidNames.mkString(",")}")
+ DataTypeMismatch(
+ errorSubClass = "CREATE_NAMED_STRUCT_WITHOUT_FOLDABLE_STRING",
Review Comment:
It seems that would be hard to make NON_FOLDABLE_INPUT more general to cover the case. Let's introduce more specific error class for the case.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/complexTypeCreator.scala:
##########
@@ -668,10 +699,19 @@ case class UpdateFields(structExpr: Expression, fieldOps: Seq[StructFieldsOperat
override def checkInputDataTypes(): TypeCheckResult = {
val dataType = structExpr.dataType
if (!dataType.isInstanceOf[StructType]) {
- TypeCheckResult.TypeCheckFailure("struct argument should be struct type, got: " +
- dataType.catalogString)
+ DataTypeMismatch(
+ errorSubClass = "UNEXPECTED_INPUT_TYPE",
+ messageParameters = Map(
+ "paramIndex" -> "1",
+ "requiredType" -> toSQLType(StructType),
+ "inputSql" -> toSQLExpr(structExpr),
+ "inputType" -> toSQLType(structExpr.dataType))
+ )
} else if (newExprs.isEmpty) {
- TypeCheckResult.TypeCheckFailure("cannot drop all fields in struct")
+ DataTypeMismatch(
Review Comment:
It is some kind of `UNEXPECTED_INPUT_TYPE`: op (drop field) + its args are not supported because they produce empty struct. I am ok w/ this particular error class.
--
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
[GitHub] [spark] AmplabJenkins commented on pull request #38463: [SPARK-40374][SQL] Migrate type check failures of type creators onto error classes
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on PR #38463:
URL: https://github.com/apache/spark/pull/38463#issuecomment-1299294202
Can one of the admins verify this patch?
--
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
[GitHub] [spark] panbingkun commented on a diff in pull request #38463: [SPARK-40374][SQL] Migrate type check failures of type creators onto error classes
Posted by GitBox <gi...@apache.org>.
panbingkun commented on code in PR #38463:
URL: https://github.com/apache/spark/pull/38463#discussion_r1010356813
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/complexTypeCreator.scala:
##########
@@ -444,17 +460,32 @@ case class CreateNamedStruct(children: Seq[Expression]) extends Expression with
override def checkInputDataTypes(): TypeCheckResult = {
if (children.size % 2 != 0) {
- TypeCheckResult.TypeCheckFailure(s"$prettyName expects an even number of arguments.")
+ DataTypeMismatch(
+ errorSubClass = "WRONG_NUM_ARGS",
+ messageParameters = Map(
+ "functionName" -> toSQLId(prettyName),
+ "expectedNum" -> "2n (n > 0)",
+ "actualNum" -> children.length.toString
+ )
+ )
} else {
val invalidNames = nameExprs.filterNot(e => e.foldable && e.dataType == StringType)
if (invalidNames.nonEmpty) {
- TypeCheckResult.TypeCheckFailure(
- s"Only foldable ${StringType.catalogString} expressions are allowed to appear at odd" +
- s" position, got: ${invalidNames.mkString(",")}")
+ DataTypeMismatch(
+ errorSubClass = "CREATE_NAMED_STRUCT_WITHOUT_FOLDABLE_STRING",
Review Comment:
<img width="858" alt="image" src="https://user-images.githubusercontent.com/15246973/199229111-f39526ba-1f5e-46b0-b8b5-6abad300674b.png">
--
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
[GitHub] [spark] MaxGekk commented on pull request #38463: [SPARK-40374][SQL] Migrate type check failures of type creators onto error classes
Posted by GitBox <gi...@apache.org>.
MaxGekk commented on PR #38463:
URL: https://github.com/apache/spark/pull/38463#issuecomment-1300199937
+1, LGTM. Merging to master.
Thank you, @panbingkun and @LuciferYang for review.
--
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
[GitHub] [spark] panbingkun commented on a diff in pull request #38463: [SPARK-40374][SQL] Migrate type check failures of type creators onto error classes
Posted by GitBox <gi...@apache.org>.
panbingkun commented on code in PR #38463:
URL: https://github.com/apache/spark/pull/38463#discussion_r1010355945
##########
core/src/main/resources/error/error-classes.json:
##########
@@ -155,6 +160,21 @@
"To convert values from <srcType> to <targetType>, you can use the functions <functionNames> instead."
]
},
+ "CREATE_MAP_KEY_DIFF_TYPES" : {
Review Comment:
but
<img width="814" alt="image" src="https://user-images.githubusercontent.com/15246973/199228400-42c1759f-fc16-4205-a052-fc54b1f3d70b.png">
Very hard to reuse it.
Maybe `functionName` pass to: `map's keys` or `map's values`, seems unreasonable
--
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
[GitHub] [spark] panbingkun commented on pull request #38463: [SPARK-40374][SQL] Migrate type check failures of type creators onto error classes
Posted by GitBox <gi...@apache.org>.
panbingkun commented on PR #38463:
URL: https://github.com/apache/spark/pull/38463#issuecomment-1299615584
cc @MaxGekk
--
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
[GitHub] [spark] LuciferYang commented on a diff in pull request #38463: [SPARK-40374][SQL] Migrate type check failures of type creators onto error classes
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #38463:
URL: https://github.com/apache/spark/pull/38463#discussion_r1010207329
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/complexTypeCreator.scala:
##########
@@ -444,17 +460,32 @@ case class CreateNamedStruct(children: Seq[Expression]) extends Expression with
override def checkInputDataTypes(): TypeCheckResult = {
if (children.size % 2 != 0) {
- TypeCheckResult.TypeCheckFailure(s"$prettyName expects an even number of arguments.")
+ DataTypeMismatch(
+ errorSubClass = "WRONG_NUM_ARGS",
+ messageParameters = Map(
+ "functionName" -> toSQLId(prettyName),
+ "expectedNum" -> "2n (n > 0)",
+ "actualNum" -> children.length.toString
+ )
+ )
} else {
val invalidNames = nameExprs.filterNot(e => e.foldable && e.dataType == StringType)
if (invalidNames.nonEmpty) {
- TypeCheckResult.TypeCheckFailure(
- s"Only foldable ${StringType.catalogString} expressions are allowed to appear at odd" +
- s" position, got: ${invalidNames.mkString(",")}")
+ DataTypeMismatch(
+ errorSubClass = "CREATE_NAMED_STRUCT_WITHOUT_FOLDABLE_STRING",
Review Comment:
Can `NON_FOLDABLE_INPUT` be reused?
##########
core/src/main/resources/error/error-classes.json:
##########
@@ -155,6 +160,21 @@
"To convert values from <srcType> to <targetType>, you can use the functions <functionNames> instead."
]
},
+ "CREATE_MAP_KEY_DIFF_TYPES" : {
Review Comment:
Cloud we make `DATA_DIFF_TYPES ` more general and reuse it?
##########
core/src/main/resources/error/error-classes.json:
##########
@@ -155,6 +160,21 @@
"To convert values from <srcType> to <targetType>, you can use the functions <functionNames> instead."
]
},
+ "CREATE_MAP_KEY_DIFF_TYPES" : {
+ "message" : [
+ "The given keys of function <functionName> should all be the same type, but they are <dataType>."
+ ]
+ },
+ "CREATE_MAP_VALUE_DIFF_TYPES" : {
Review Comment:
ditto
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/complexTypeCreator.scala:
##########
@@ -668,10 +699,19 @@ case class UpdateFields(structExpr: Expression, fieldOps: Seq[StructFieldsOperat
override def checkInputDataTypes(): TypeCheckResult = {
val dataType = structExpr.dataType
if (!dataType.isInstanceOf[StructType]) {
- TypeCheckResult.TypeCheckFailure("struct argument should be struct type, got: " +
- dataType.catalogString)
+ DataTypeMismatch(
+ errorSubClass = "UNEXPECTED_INPUT_TYPE",
+ messageParameters = Map(
+ "paramIndex" -> "1",
+ "requiredType" -> toSQLType(StructType),
+ "inputSql" -> toSQLExpr(structExpr),
+ "inputType" -> toSQLType(structExpr.dataType))
+ )
} else if (newExprs.isEmpty) {
- TypeCheckResult.TypeCheckFailure("cannot drop all fields in struct")
+ DataTypeMismatch(
Review Comment:
It seems that the original message did not clearly describe this error, the message doesn't look like `DataTypeMismatch`
--
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