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/05/23 11:26:40 UTC
[GitHub] [spark] beliefer opened a new pull request, #41279: [SPARK-43749][SPARK-43750][SQL] Assign a name to the error class _LEGACY_ERROR_TEMP_240[4-5]
beliefer opened a new pull request, #41279:
URL: https://github.com/apache/spark/pull/41279
### What changes were proposed in this pull request?
The pr aims to assign a name to the error class _LEGACY_ERROR_TEMP_240[4-5].
### Why are the changes needed?
Improve the error framework.
### Does this PR introduce _any_ user-facing change?
'No'.
### How was this patch tested?
N/A
--
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 #41279: [SPARK-43749][SPARK-43750][SQL] Assign names to the error class _LEGACY_ERROR_TEMP_240[4-5]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on code in PR #41279:
URL: https://github.com/apache/spark/pull/41279#discussion_r1205344905
##########
core/src/main/resources/error/error-classes.json:
##########
@@ -5680,4 +5687,4 @@
"Failed to get block <blockId>, which is not a shuffle block"
]
}
-}
\ No newline at end of file
+}
Review Comment:
Yes. In fact, the empty line still here. It seems the GitHub hide it. I can see the empty line locally.
--
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 #41279: [SPARK-43749][SPARK-43750][SQL] Assign names to the error class _LEGACY_ERROR_TEMP_240[4-5]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on code in PR #41279:
URL: https://github.com/apache/spark/pull/41279#discussion_r1205344905
##########
core/src/main/resources/error/error-classes.json:
##########
@@ -5680,4 +5687,4 @@
"Failed to get block <blockId>, which is not a shuffle block"
]
}
-}
\ No newline at end of file
+}
Review Comment:
Yes. In fact, the empty line still here. It seems the GitHub hide it. I can see the empty line on local environment.
--
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 #41279: [SPARK-43749][SPARK-43750][SQL] Assign names to the error class _LEGACY_ERROR_TEMP_240[4-5]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on code in PR #41279:
URL: https://github.com/apache/spark/pull/41279#discussion_r1205260168
##########
core/src/main/resources/error/error-classes.json:
##########
@@ -5680,4 +5687,4 @@
"Failed to get block <blockId>, which is not a shuffle block"
]
}
-}
\ No newline at end of file
+}
Review Comment:
This line generated by `SparkThrowableSuite`.
--
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 #41279: [SPARK-43749][SPARK-43750][SQL] Assign names to the error class _LEGACY_ERROR_TEMP_240[4-5]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #41279:
URL: https://github.com/apache/spark/pull/41279#discussion_r1203854477
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/command/v2/ShowPartitionsSuite.scala:
##########
@@ -33,7 +35,9 @@ class ShowPartitionsSuite extends command.ShowPartitionsSuiteBase with CommandSu
val errMsg = intercept[AnalysisException] {
sql(s"SHOW PARTITIONS $table")
}.getMessage
- assert(errMsg.contains(s"Table $table does not support partition management"))
+ val tableName =
+ UnresolvedAttribute.parseAttributeName(table).map(quoteIdentifier).mkString(".")
+ assert(errMsg.contains(s"Table $tableName does not support partition management"))
Review Comment:
Use `checkError`
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/command/v2/AlterTableAddPartitionSuite.scala:
##########
@@ -38,7 +39,8 @@ class AlterTableAddPartitionSuite
val errMsg = intercept[AnalysisException] {
sql(s"ALTER TABLE $t ADD PARTITION (id=1)")
}.getMessage
- assert(errMsg.contains(s"Table $t does not support partition management"))
+ val tableName = UnresolvedAttribute.parseAttributeName(t).map(quoteIdentifier).mkString(".")
+ assert(errMsg.contains(s"Table $tableName does not support partition management"))
Review Comment:
Use `checkError`. This will allow to modify the error messages in the JSON files by tech editors w/o modifying the internal Spark tests.
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/command/ShowPartitionsSuiteBase.scala:
##########
@@ -69,9 +71,11 @@ trait ShowPartitionsSuiteBase extends QueryTest with DDLCommandTestUtils {
val errMsg = intercept[AnalysisException] {
sql(s"SHOW PARTITIONS $t")
}.getMessage
+ lazy val tableName =
+ UnresolvedAttribute.parseAttributeName(t).map(quoteIdentifier).mkString(".")
assert(errMsg.contains("not allowed on a table that is not partitioned") ||
// V2 error message.
- errMsg.contains(s"Table $t is not partitioned"))
+ errMsg.contains(s"Table $tableName is not partitioned"))
Review Comment:
Please, use `checkError()`
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/command/v2/TruncateTableSuite.scala:
##########
@@ -36,7 +36,7 @@ class TruncateTableSuite extends command.TruncateTableSuiteBase with CommandSuit
sql(s"TRUNCATE TABLE $t PARTITION (c0=1)")
}.getMessage
assert(errMsg.contains(
- "Table non_part_test_catalog.ns.tbl does not support partition management"))
+ "Table `non_part_test_catalog`.`ns`.`tbl` does not support partition management"))
Review Comment:
Use `checkError`
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/command/v2/AlterTableDropPartitionSuite.scala:
##########
@@ -36,7 +38,8 @@ class AlterTableDropPartitionSuite
val errMsg = intercept[AnalysisException] {
sql(s"ALTER TABLE $t DROP PARTITION (id=1)")
}.getMessage
- assert(errMsg.contains(s"Table $t does not support partition management"))
+ val tableName = UnresolvedAttribute.parseAttributeName(t).map(quoteIdentifier).mkString(".")
+ assert(errMsg.contains(s"Table $tableName does not support partition management"))
Review Comment:
ditto: Use `checkError`
--
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 #41279: [SPARK-43749][SPARK-43750][SQL] Assign names to the error class _LEGACY_ERROR_TEMP_240[4-5]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on PR #41279:
URL: https://github.com/apache/spark/pull/41279#issuecomment-1562182627
@MaxGekk The failure tests seems 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] beliefer commented on a diff in pull request #41279: [SPARK-43749][SPARK-43750][SQL] Assign names to the error class _LEGACY_ERROR_TEMP_240[4-5]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on code in PR #41279:
URL: https://github.com/apache/spark/pull/41279#discussion_r1206157992
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/command/v1/ShowPartitionsSuite.scala:
##########
@@ -117,6 +119,22 @@ class ShowPartitionsSuite extends ShowPartitionsSuiteBase with CommandSuiteBase
}
}
+ test("show partitions of non-partitioned table") {
+ withNamespaceAndTable("ns", "not_partitioned_table") { t =>
+ sql(s"CREATE TABLE $t (col1 int) $defaultUsing")
+ val sqlText = s"SHOW PARTITIONS $t"
+ val tableName =
+ UnresolvedAttribute.parseAttributeName(t).map(quoteIdentifier).mkString(".")
+
+ checkError(
+ exception = intercept[AnalysisException] {
+ sql(sqlText)
+ },
+ errorClass = "_LEGACY_ERROR_TEMP_1269",
Review Comment:
OK
--
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 #41279: [SPARK-43749][SPARK-43750][SQL] Assign names to the error class _LEGACY_ERROR_TEMP_240[4-5]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on code in PR #41279:
URL: https://github.com/apache/spark/pull/41279#discussion_r1205344905
##########
core/src/main/resources/error/error-classes.json:
##########
@@ -5680,4 +5687,4 @@
"Failed to get block <blockId>, which is not a shuffle block"
]
}
-}
\ No newline at end of file
+}
Review Comment:
Yes. In fact, the empty line still here. It seems the GitHub hide 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] beliefer commented on a diff in pull request #41279: [SPARK-43749][SPARK-43750][SQL] Assign names to the error class _LEGACY_ERROR_TEMP_240[4-5]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on code in PR #41279:
URL: https://github.com/apache/spark/pull/41279#discussion_r1205344905
##########
core/src/main/resources/error/error-classes.json:
##########
@@ -5680,4 +5687,4 @@
"Failed to get block <blockId>, which is not a shuffle block"
]
}
-}
\ No newline at end of file
+}
Review Comment:
Yes. In fact the empty line still here. It seems the GitHub hide 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] beliefer commented on pull request #41279: [SPARK-43749][SPARK-43750][SQL] Assign names to the error class _LEGACY_ERROR_TEMP_240[4-5]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on PR #41279:
URL: https://github.com/apache/spark/pull/41279#issuecomment-1563719304
@MaxGekk Thank you!
--
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 #41279: [SPARK-43749][SPARK-43750][SQL] Assign names to the error class _LEGACY_ERROR_TEMP_240[4-5]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #41279:
URL: https://github.com/apache/spark/pull/41279#discussion_r1205301953
##########
core/src/main/resources/error/error-classes.json:
##########
@@ -5680,4 +5687,4 @@
"Failed to get block <blockId>, which is not a shuffle block"
]
}
-}
\ No newline at end of file
+}
Review Comment:
After https://github.com/apache/spark/pull/41256, the empty line should be generated at the end. cc @panbingkun . Just in case, @beliefer have you rebased on the recent master?
--
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 #41279: [SPARK-43749][SPARK-43750][SQL] Assign names to the error class _LEGACY_ERROR_TEMP_240[4-5]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #41279:
URL: https://github.com/apache/spark/pull/41279#discussion_r1205137320
##########
core/src/main/resources/error/error-classes.json:
##########
@@ -5680,4 +5687,4 @@
"Failed to get block <blockId>, which is not a shuffle block"
]
}
-}
\ No newline at end of file
+}
Review Comment:
Please, revert the changes.
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/command/v1/ShowPartitionsSuite.scala:
##########
@@ -117,6 +117,17 @@ class ShowPartitionsSuite extends ShowPartitionsSuiteBase with CommandSuiteBase
}
}
+ test("show partitions of non-partitioned table") {
+ withNamespaceAndTable("ns", "not_partitioned_table") { t =>
+ sql(s"CREATE TABLE $t (col1 int) $defaultUsing")
+ val errMsg = intercept[AnalysisException] {
+ sql(s"SHOW PARTITIONS $t")
+ }.getMessage
+
+ assert(errMsg.contains("not allowed on a table that is not partitioned"))
Review Comment:
Could you use `checkError`, please.
--
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 #41279: [SPARK-43749][SPARK-43750][SQL] Assign names to the error class _LEGACY_ERROR_TEMP_240[4-5]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #41279:
URL: https://github.com/apache/spark/pull/41279#discussion_r1205805664
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/command/v1/ShowPartitionsSuite.scala:
##########
@@ -117,6 +119,22 @@ class ShowPartitionsSuite extends ShowPartitionsSuiteBase with CommandSuiteBase
}
}
+ test("show partitions of non-partitioned table") {
+ withNamespaceAndTable("ns", "not_partitioned_table") { t =>
+ sql(s"CREATE TABLE $t (col1 int) $defaultUsing")
+ val sqlText = s"SHOW PARTITIONS $t"
+ val tableName =
+ UnresolvedAttribute.parseAttributeName(t).map(quoteIdentifier).mkString(".")
+
+ checkError(
+ exception = intercept[AnalysisException] {
+ sql(sqlText)
+ },
+ errorClass = "_LEGACY_ERROR_TEMP_1269",
Review Comment:
Should we migrate the error class `_LEGACY_ERROR_TEMP_1269` on to `PARTITION_SCHEMA_IS_EMPTY`?
--
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 #41279: [SPARK-43749][SPARK-43750][SQL] Assign names to the error class _LEGACY_ERROR_TEMP_240[4-5]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk closed pull request #41279: [SPARK-43749][SPARK-43750][SQL] Assign names to the error class _LEGACY_ERROR_TEMP_240[4-5]
URL: https://github.com/apache/spark/pull/41279
--
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 #41279: [SPARK-43749][SPARK-43750][SQL] Assign names to the error class _LEGACY_ERROR_TEMP_240[4-5]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #41279:
URL: https://github.com/apache/spark/pull/41279#discussion_r1202501821
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CheckAnalysis.scala:
##########
@@ -211,12 +211,12 @@ trait CheckAnalysis extends PredicateHelper with LookupCatalog with QueryErrorsB
case t: SupportsPartitionManagement =>
if (t.partitionSchema.isEmpty) {
r.failAnalysis(
- errorClass = "_LEGACY_ERROR_TEMP_2404",
+ errorClass = "INVALID_PARTITION_OPERATION.PARTITION_SCHEMA_IS_EMPTY",
messageParameters = Map("name" -> r.name))
Review Comment:
Could you quote the table name by `toSQLId()`, please.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CheckAnalysis.scala:
##########
@@ -211,12 +211,12 @@ trait CheckAnalysis extends PredicateHelper with LookupCatalog with QueryErrorsB
case t: SupportsPartitionManagement =>
if (t.partitionSchema.isEmpty) {
r.failAnalysis(
- errorClass = "_LEGACY_ERROR_TEMP_2404",
+ errorClass = "INVALID_PARTITION_OPERATION.PARTITION_SCHEMA_IS_EMPTY",
messageParameters = Map("name" -> r.name))
}
case _ =>
r.failAnalysis(
- errorClass = "_LEGACY_ERROR_TEMP_2405",
+ errorClass = "INVALID_PARTITION_OPERATION.PARTITION_MANAGEMENT_IS_UNSUPPORTED",
messageParameters = Map("name" -> r.name))
Review Comment:
Please, wrap the table name.
--
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 #41279: [SPARK-43749][SPARK-43750][SQL] Assign names to the error class _LEGACY_ERROR_TEMP_240[4-5]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on PR #41279:
URL: https://github.com/apache/spark/pull/41279#issuecomment-1563244806
+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] beliefer commented on a diff in pull request #41279: [SPARK-43749][SPARK-43750][SQL] Assign names to the error class _LEGACY_ERROR_TEMP_240[4-5]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on code in PR #41279:
URL: https://github.com/apache/spark/pull/41279#discussion_r1205260168
##########
core/src/main/resources/error/error-classes.json:
##########
@@ -5680,4 +5687,4 @@
"Failed to get block <blockId>, which is not a shuffle block"
]
}
-}
\ No newline at end of file
+}
Review Comment:
This line generated by `SparkThrowableSuite`.
So, do we need the empty line at end of file or not ?
--
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