[GitHub] [spark] MaxGekk commented on a diff in pull request #41424: [SPARK-43913][SQL] Assign names to the error class _LEGACY_ERROR_TEMP_[2426-2432]
MaxGekk commented on code in PR #41424:
URL: https://github.com/apache/spark/pull/41424#discussion_r1217821806
##
core/src/main/resources/error/error-classes.json:
##
@@ -1520,6 +1525,25 @@
],
"sqlState" : "42803"
},
+ "MISSING_ATTRIBUTES" : {
+"message" : [
+ "Operator missing attributes."
+],
+"subClass" : {
+ "RESOLVED_ATTRIBUTE_APPEAR_IN_OPERATION" : {
+"message" : [
+ "Resolved attribute(s) missing from in
operator .",
+ "Attribute(s) with the same name appear in the operation:
.",
+ "Please check if the right attribute(s) are used."
+]
+ },
+ "RESOLVED_ATTRIBUTE_MISSING_FROM_INPUT" : {
+"message" : [
+ "Resolved attribute(s) missing from in
operator ."
Review Comment:
The both sub-classes have the same error format. Could you move this to its
parent, and make the parent more specific.
##
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CheckAnalysis.scala:
##
@@ -641,28 +644,34 @@ trait CheckAnalysis extends PredicateHelper with
LookupCatalog with QueryErrorsB
operator match {
case o if o.children.nonEmpty && o.missingInput.nonEmpty =>
-val missingAttributes = o.missingInput.mkString(",")
-val input = o.inputSet.mkString(",")
-val msgForMissingAttributes = s"Resolved attribute(s)
$missingAttributes missing " +
- s"from $input in operator
${operator.simpleString(SQLConf.get.maxToStringFields)}."
+val missingAttributes = o.missingInput.map(attr =>
toSQLExpr(attr)).mkString(",")
+val input = o.inputSet.map(attr => toSQLExpr(attr)).mkString(",")
Review Comment:
Add a gap after `,`
##
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CheckAnalysis.scala:
##
@@ -641,28 +644,34 @@ trait CheckAnalysis extends PredicateHelper with
LookupCatalog with QueryErrorsB
operator match {
case o if o.children.nonEmpty && o.missingInput.nonEmpty =>
-val missingAttributes = o.missingInput.mkString(",")
-val input = o.inputSet.mkString(",")
-val msgForMissingAttributes = s"Resolved attribute(s)
$missingAttributes missing " +
- s"from $input in operator
${operator.simpleString(SQLConf.get.maxToStringFields)}."
+val missingAttributes = o.missingInput.map(attr =>
toSQLExpr(attr)).mkString(",")
+val input = o.inputSet.map(attr => toSQLExpr(attr)).mkString(",")
val resolver = plan.conf.resolver
val attrsWithSameName = o.missingInput.filter { missing =>
o.inputSet.exists(input => resolver(missing.name, input.name))
}
-val msg = if (attrsWithSameName.nonEmpty) {
- val sameNames = attrsWithSameName.map(_.name).mkString(",")
- s"$msgForMissingAttributes Attribute(s) with the same name
appear in the " +
-s"operation: $sameNames. Please check if the right
attribute(s) are used."
+if (attrsWithSameName.nonEmpty) {
+ val sameNames = attrsWithSameName.map(attr =>
toSQLExpr(attr)).mkString(",")
Review Comment:
```suggestion
val sameNames = attrsWithSameName.map(attr =>
toSQLExpr(attr)).mkString(", ")
```
--
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
[GitHub] [spark] MaxGekk commented on a diff in pull request #41424: [SPARK-43913][SQL] Assign names to the error class _LEGACY_ERROR_TEMP_[2426-2432]
MaxGekk commented on code in PR #41424:
URL: https://github.com/apache/spark/pull/41424#discussion_r1215752166
##
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CheckAnalysis.scala:
##
@@ -624,8 +626,8 @@ trait CheckAnalysis extends PredicateHelper with
LookupCatalog with QueryErrorsB
if (badReferences.nonEmpty) {
create.failAnalysis(
-errorClass = "_LEGACY_ERROR_TEMP_2431",
-messageParameters = Map("cols" -> badReferences.mkString(",
")))
+errorClass =
"UNSUPPORTED_FEATURE.PARTITION_WITH_NESTED_COLUMN_IS_UNSUPPORTED",
+messageParameters = Map("cols" -> badReferences.map(r =>
s"`$r`").mkString(", ")))
Review Comment:
Please, use `toSQLId` for quoting ids.
##
core/src/main/resources/error/error-classes.json:
##
@@ -1834,6 +1849,11 @@
],
"sqlState" : "42K05"
},
+ "RESOLVED_ATTRIBUTE_MISSING_FROM_INPUT" : {
+"message" : [
+ ""
Review Comment:
Consider to replace the messages
https://github.com/apache/spark/blob/898ad77900d887ac64800a616bd382def816eea6/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CheckAnalysis.scala#L646-L660
by a sub-class of `UNRESOLVED_COLUMN` or add new ones.
--
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
[GitHub] [spark] MaxGekk commented on a diff in pull request #41424: [SPARK-43913][SQL] Assign names to the error class _LEGACY_ERROR_TEMP_[2426-2432]
MaxGekk commented on code in PR #41424:
URL: https://github.com/apache/spark/pull/41424#discussion_r1214600189
##
core/src/main/resources/error/error-classes.json:
##
@@ -1743,6 +1753,11 @@
],
"sqlState" : "428FT"
},
+ "PARTITION_WITH_NESTED_COLUMN_IS_UNSUPPORTED" : {
Review Comment:
Let's move it to a sub-class of `UNSUPPORTED_FEATURE`
##
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CheckAnalysis.scala:
##
@@ -475,8 +475,8 @@ trait CheckAnalysis extends PredicateHelper with
LookupCatalog with QueryErrorsB
// already pull out those nondeterministic expressions and
evaluate them in
// a Project node.
expr.failAnalysis(
- errorClass = "_LEGACY_ERROR_TEMP_2426",
- messageParameters = Map("sqlExpr" -> expr.sql))
+ errorClass = "GROUP_EXPRESSION_IS_NON_DETERMINISTIC",
Review Comment:
Is it user facing error? The comment above says it is an internal one. If
not, please, add a test.
##
core/src/main/resources/error/error-classes.json:
##
@@ -644,6 +644,11 @@
"The event time has the invalid type , but
expected \"TIMESTAMP\"."
]
},
+ "EXPRESSION_TYPE_IS_NOT_ORDERABLE" : {
+"message" : [
+ "sorting is not supported for columns of type ."
Review Comment:
```suggestion
"Column expression cannot be sorted because its type
is not orderable."
```
##
core/src/main/resources/error/error-classes.json:
##
@@ -1834,6 +1849,11 @@
],
"sqlState" : "42K05"
},
+ "RESOLVED_ATTRIBUTE_MISSING_FROM_INPUT" : {
+"message" : [
+ ""
Review Comment:
This is a red flag. Please, move the message out from source code. Imagine,
we will translate this JSON to different languages.
##
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/analysis/CreateTablePartitioningValidationSuite.scala:
##
@@ -42,7 +42,7 @@ class CreateTablePartitioningValidationSuite extends
AnalysisTest {
assert(!plan.resolved)
assertAnalysisError(plan, Seq(
Review Comment:
Use `assertAnalysisErrorClass` instead of `assertAnalysisError` to don't
depend on error messages in 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-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
