You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "beliefer (via GitHub)" <gi...@apache.org> on 2023/06/12 13:12:35 UTC
[GitHub] [spark] beliefer opened a new pull request, #41553: [SPARK-43915][SQL] Assign names to the error class _LEGACY_ERROR_TEMP_[2438-2445]
beliefer opened a new pull request, #41553:
URL: https://github.com/apache/spark/pull/41553
### What changes were proposed in this pull request?
The pr aims to assign names to the error class _LEGACY_ERROR_TEMP_[2438-2445].
### Why are the changes needed?
Improve the error framework.
### Does this PR introduce _any_ user-facing change?
'No'.
### How was this patch tested?
Exists test cases updated.
--
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] beliefer commented on a diff in pull request #41553: [SPARK-43915][SQL] Assign names to the error class _LEGACY_ERROR_TEMP_[2438-2445]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on code in PR #41553:
URL: https://github.com/apache/spark/pull/41553#discussion_r1235124801
##########
sql/core/src/test/scala/org/apache/spark/sql/connector/UpdateTableSuiteBase.scala:
##########
@@ -538,7 +538,7 @@ abstract class UpdateTableSuiteBase extends RowLevelOperationSuiteBase {
val e = intercept[AnalysisException] {
sql(s"UPDATE $tableNameAsString SET dep = 'invalid' WHERE id <= 1 AND rand() > 0.5")
}
- assert(e.message.contains("nondeterministic expressions are only allowed"))
+ assert(e.message.contains("The operator expects a deterministic expression"))
Review Comment:
Sorry. I forgot it.
--
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 #41553: [SPARK-43915][SQL] Assign names to the error class _LEGACY_ERROR_TEMP_[2438-2445]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #41553:
URL: https://github.com/apache/spark/pull/41553#discussion_r1233252365
##########
core/src/main/resources/error/error-classes.json:
##########
@@ -633,6 +633,11 @@
],
"sqlState" : "23505"
},
+ "DUPLICATED_OBSERVED_METRICS_NAME" : {
+ "message" : [
+ "Multiple definitions of observed metrics named '<name>': <plan>."
Review Comment:
Could you borrow the message from the comment of `checkCollectedMetrics()`:
```suggestion
"The metric name is not unique: <metricName>. The same name cannot be used for metrics with different results. However multiple instances of metrics with with same result and name are allowed (e.g. self-joins)."
```
I think the metric name is enough, and the plan can be omitted.
##########
core/src/main/resources/error/error-classes.json:
##########
@@ -633,6 +633,11 @@
],
"sqlState" : "23505"
},
+ "DUPLICATED_OBSERVED_METRICS_NAME" : {
Review Comment:
```suggestion
"DUPLICATED_METRICS_NAME" : {
```
##########
core/src/main/resources/error/error-classes.json:
##########
@@ -1967,6 +1984,11 @@
"The seed expression <seedExpr> of the expression <exprWithSeed> must be foldable."
]
},
+ "SET_OPERATION_ON_MAP_TYPE_UNSUPPORTED" : {
Review Comment:
Technically, it can be supported, right? Could you add a sub-class to `UNSUPPORTED_FEATURE`.
##########
core/src/main/resources/error/error-classes.json:
##########
@@ -1203,6 +1208,13 @@
}
}
},
+ "INVALID_NON_DETERMINISTIC_EXPRESSIONS" : {
+ "message" : [
+ "nondeterministic expressions are only allowed in Project, Filter, Aggregate or Window, found:",
Review Comment:
You missed `Expand` and `LateralJoin`. BTW, could you make the message more generic:
```suggestion
"The operator expects a deterministic expression, but the actual expression is <expr>.",
```
--
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] beliefer commented on pull request #41553: [SPARK-43915][SQL] Assign names to the error class _LEGACY_ERROR_TEMP_[2438-2445]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on PR #41553:
URL: https://github.com/apache/spark/pull/41553#issuecomment-1603647615
@MaxGekk Thank you a lot!
--
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] beliefer commented on a diff in pull request #41553: [SPARK-43915][SQL] Assign names to the error class _LEGACY_ERROR_TEMP_[2438-2445]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on code in PR #41553:
URL: https://github.com/apache/spark/pull/41553#discussion_r1226650973
##########
sql/core/src/test/scala/org/apache/spark/sql/connector/DeltaBasedDeleteFromTableSuite.scala:
##########
@@ -27,6 +27,30 @@ class DeltaBasedDeleteFromTableSuite extends DeleteFromTableSuiteBase {
props
}
+ test("delete with nondeterministic conditions") {
+ createAndInitTable("pk INT NOT NULL, id INT, dep STRING",
+ """{ "pk": 1, "id": 1, "dep": "hr" }
+ |{ "pk": 2, "id": 2, "dep": "software" }
+ |{ "pk": 3, "id": 3, "dep": "hr" }
+ |""".stripMargin)
+
+ checkError(
+ exception = intercept[AnalysisException](
+ sql(s"DELETE FROM $tableNameAsString WHERE id <= 1 AND rand() > 0.5")),
+ errorClass = "INVALID_NON_DETERMINISTIC_EXPRESSIONS",
+ parameters = Map(
+ "sqlExprs" -> "\"((id <= 1) AND (rand() > 0.5))\"",
+ "operator" ->
+ """
+ |WriteDelta RelationV2[pk#2810, id#2811, dep#2812] cat.ns1.test_table cat.ns1.test_table
Review Comment:
@MaxGekk The `exprId` for expression is not stable. Do you have any suggestions?
--
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 #41553: [SPARK-43915][SQL] Assign names to the error class _LEGACY_ERROR_TEMP_[2438-2445]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on PR #41553:
URL: https://github.com/apache/spark/pull/41553#issuecomment-1601359027
+1, LGTM. Merging to master.
Thank you, @beliefer.
--
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 #41553: [SPARK-43915][SQL] Assign names to the error class _LEGACY_ERROR_TEMP_[2438-2445]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #41553:
URL: https://github.com/apache/spark/pull/41553#discussion_r1231392906
##########
core/src/main/resources/error/error-classes.json:
##########
@@ -1473,6 +1485,11 @@
],
"sqlState" : "42604"
},
+ "INVALID_UDF_IMPLEMENTATION" : {
+ "message" : [
+ "Function '<funcName>' does not implement ScalarFunction or AggregateFunction."
Review Comment:
As I can see you quote `funcName` by `toSQLId`, please, remove ''
--
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 #41553: [SPARK-43915][SQL] Assign names to the error class _LEGACY_ERROR_TEMP_[2438-2445]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk closed pull request #41553: [SPARK-43915][SQL] Assign names to the error class _LEGACY_ERROR_TEMP_[2438-2445]
URL: https://github.com/apache/spark/pull/41553
--
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] beliefer commented on pull request #41553: [SPARK-43915][SQL] Assign names to the error class _LEGACY_ERROR_TEMP_[2438-2445]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on PR #41553:
URL: https://github.com/apache/spark/pull/41553#issuecomment-1595971742
ping @MaxGekk The GA failure is unrelated to this PR.
--
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 #41553: [SPARK-43915][SQL] Assign names to the error class _LEGACY_ERROR_TEMP_[2438-2445]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #41553:
URL: https://github.com/apache/spark/pull/41553#discussion_r1234980739
##########
core/src/main/resources/error/error-classes.json:
##########
@@ -2388,6 +2403,11 @@
"<property> is a reserved namespace property, <msg>."
]
},
+ "SET_OPERATION_ON_MAP_TYPE_UNSUPPORTED" : {
+ "message" : [
+ "Cannot have map type columns in DataFrame which calls set operations(intersect, except, etc.), but the type of column <colName> is <dataType>."
Review Comment:
Could you follow the style of `toSQLType` and `toSQLStmt`
```suggestion
"Cannot have MAP type columns in DataFrame which calls set operations (INTERSECT, EXCEPT, etc.), but the type of column <colName> is <dataType>."
```
##########
sql/core/src/test/scala/org/apache/spark/sql/connector/UpdateTableSuiteBase.scala:
##########
@@ -538,7 +538,7 @@ abstract class UpdateTableSuiteBase extends RowLevelOperationSuiteBase {
val e = intercept[AnalysisException] {
sql(s"UPDATE $tableNameAsString SET dep = 'invalid' WHERE id <= 1 AND rand() > 0.5")
}
- assert(e.message.contains("nondeterministic expressions are only allowed"))
+ assert(e.message.contains("The operator expects a deterministic expression"))
Review Comment:
Could you use `checkError`
##########
core/src/main/resources/error/error-classes.json:
##########
@@ -2388,6 +2403,11 @@
"<property> is a reserved namespace property, <msg>."
]
},
+ "SET_OPERATION_ON_MAP_TYPE_UNSUPPORTED" : {
Review Comment:
You can omit `_UNSUPPORTED` in the sub-class of `UNSUPPORTED_FEATURE`.
##########
sql/core/src/test/scala/org/apache/spark/sql/DataFrameSetOperationsSuite.scala:
##########
@@ -352,20 +352,40 @@ class DataFrameSetOperationsSuite extends QueryTest with SharedSparkSession {
test("SPARK-19893: cannot run set operations with map type") {
val df = spark.range(1).select(map(lit("key"), $"id").as("m"))
- val e = intercept[AnalysisException](df.intersect(df))
- assert(e.message.contains(
- "Cannot have map type columns in DataFrame which calls set operations"))
- val e2 = intercept[AnalysisException](df.except(df))
- assert(e2.message.contains(
- "Cannot have map type columns in DataFrame which calls set operations"))
- val e3 = intercept[AnalysisException](df.distinct())
- assert(e3.message.contains(
- "Cannot have map type columns in DataFrame which calls set operations"))
+ checkError(
+ exception = intercept[AnalysisException](df.intersect(df)),
+ errorClass = "UNSUPPORTED_FEATURE.SET_OPERATION_ON_MAP_TYPE_UNSUPPORTED",
+ parameters = Map(
+ "colName" -> "\"m\"",
Review Comment:
Shouldn't `m` be considered as an id and quoted by backticks instead of ""?
--
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] beliefer commented on a diff in pull request #41553: [SPARK-43915][SQL] Assign names to the error class _LEGACY_ERROR_TEMP_[2438-2445]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on code in PR #41553:
URL: https://github.com/apache/spark/pull/41553#discussion_r1226650973
##########
sql/core/src/test/scala/org/apache/spark/sql/connector/DeltaBasedDeleteFromTableSuite.scala:
##########
@@ -27,6 +27,30 @@ class DeltaBasedDeleteFromTableSuite extends DeleteFromTableSuiteBase {
props
}
+ test("delete with nondeterministic conditions") {
+ createAndInitTable("pk INT NOT NULL, id INT, dep STRING",
+ """{ "pk": 1, "id": 1, "dep": "hr" }
+ |{ "pk": 2, "id": 2, "dep": "software" }
+ |{ "pk": 3, "id": 3, "dep": "hr" }
+ |""".stripMargin)
+
+ checkError(
+ exception = intercept[AnalysisException](
+ sql(s"DELETE FROM $tableNameAsString WHERE id <= 1 AND rand() > 0.5")),
+ errorClass = "INVALID_NON_DETERMINISTIC_EXPRESSIONS",
+ parameters = Map(
+ "sqlExprs" -> "\"((id <= 1) AND (rand() > 0.5))\"",
+ "operator" ->
+ """
+ |WriteDelta RelationV2[pk#2810, id#2811, dep#2812] cat.ns1.test_table cat.ns1.test_table
Review Comment:
@MaxGekk The `exprId` for expression is not stable. Do you have any suggestions?
--
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