You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "NarekDW (via GitHub)" <gi...@apache.org> on 2023/05/02 16:49:38 UTC
[GitHub] [spark] NarekDW opened a new pull request, #41018: [SPARK-37942] Migrate error classes
NarekDW opened a new pull request, #41018:
URL: https://github.com/apache/spark/pull/41018
### What changes were proposed in this pull request?
Rename error classes:
1. QueryCompilationErrors.cannotReadCorruptedTablePropertyError
`_LEGACY_ERROR_TEMP_1091` -> `CORRUPTED_TABLE_PROPERTY`
+ add test case
+ refactor CatalogTable.readLargeTableProp function
2. QueryCompilationErrors.notSupportedInJDBCCatalog
`_LEGACY_ERROR_TEMP_1119` -> `NOT_SUPPORTED_OPERATION_IN_JDBC_CATALOG`
+ add test case
3. QueryCompilationErrors.cannotSetJDBCNamespaceWithPropertyError
+ add test case
4. QueryCompilationErrors.alterTableSerDePropertiesNotSupportedForV2TablesError
`_LEGACY_ERROR_TEMP_1124` -> `NOT_SUPPORTED_OPERATION_FOR_V2_TABLE`
+ add test case
5. QueryCompilationErrors.unsetNonExistentPropertyError rename to QueryCompilationErrors.unsetNonExistentPropertiesError
+ change error message and code to include all the nonexistent keys
+ add test case
6. QueryCompilationErrors.cannotUnsetJDBCNamespaceWithPropertyError
`_LEGACY_ERROR_TEMP_1119` -> `NOT_SUPPORTED_OPERATION_IN_JDBC_CATALOG`
+ **couldn't reproduce this error class with test case.**
### Why are the changes needed?
We should assign proper name to LEGACY_ERROR_TEMP*
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
By existing unit tests and an additional test cases were added.
--
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] NarekDW commented on a diff in pull request #41018: [SPARK-37942][CORE][SQL] Migrate error classes
Posted by "NarekDW (via GitHub)" <gi...@apache.org>.
NarekDW commented on code in PR #41018:
URL: https://github.com/apache/spark/pull/41018#discussion_r1182808209
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryCompilationErrors.scala:
##########
@@ -3232,12 +3236,6 @@ private[sql] object QueryCompilationErrors extends QueryErrorsBase {
messageParameters = Map("columnPath" -> toSQLId(path)))
}
- def notNullConstraintViolationStructFieldError(path: Seq[String]): Throwable = {
Review Comment:
This function is not getting used anywhere.
--
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] NarekDW commented on a diff in pull request #41018: [SPARK-37942][CORE][SQL] Migrate error classes
Posted by "NarekDW (via GitHub)" <gi...@apache.org>.
NarekDW commented on code in PR #41018:
URL: https://github.com/apache/spark/pull/41018#discussion_r1183876429
##########
core/src/main/resources/error/error-classes.json:
##########
@@ -235,6 +235,11 @@
],
"sqlState" : "22018"
},
+ "CORRUPTED_TABLE_PROPERTY" : {
Review Comment:
It is being raised in `CatalogTable.readLargeTableProp` function
```scala
def readLargeTableProp(props: Map[String, String], key: String): Option[String] = {
props.get(key).orElse {
if (props.exists { case (mapKey, _) => mapKey.startsWith(key) }) {
props.get(s"$key.numParts") match {
case None => throw QueryCompilationErrors.cannotReadCorruptedTablePropertyError(key)
case Some(numParts) =>
val parts = (0 until numParts.toInt).map { index =>
props.getOrElse(s"$key.part.$index", {
throw QueryCompilationErrors.cannotReadCorruptedTablePropertyError(
key, Some(s"Missing part $index, $numParts parts are expected."))
})
}
Some(parts.mkString)
}
} else {
None
}
}
}
```
If the key is not found but there is another key with the same prefix - the functions expects that the property has to be partitioned and it will raise exception in case if it didn't find `key.numParts` property or in case if `key.numParts` exists but some exact partitioned property is missed. In fact it means that table properties are corrupted, but it could be named more precisely, like `INSUFFICITENT_TABLE_PROPETIES` or `TABLE_PROPERTY_PARTS_MISSED` or something else...
--
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] NarekDW commented on a diff in pull request #41018: [SPARK-37942][CORE][SQL] Migrate error classes
Posted by "NarekDW (via GitHub)" <gi...@apache.org>.
NarekDW commented on code in PR #41018:
URL: https://github.com/apache/spark/pull/41018#discussion_r1186813749
##########
core/src/main/resources/error/error-classes.json:
##########
@@ -235,6 +235,11 @@
],
"sqlState" : "22018"
},
+ "CORRUPTED_TABLE_PROPERTY" : {
+ "message" : [
+ "Cannot read table property <key> as it's corrupted.<details>"
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
[GitHub] [spark] NarekDW commented on a diff in pull request #41018: [SPARK-37942][CORE][SQL] Migrate error classes
Posted by "NarekDW (via GitHub)" <gi...@apache.org>.
NarekDW commented on code in PR #41018:
URL: https://github.com/apache/spark/pull/41018#discussion_r1182810488
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/command/ddl.scala:
##########
@@ -333,10 +333,11 @@ case class AlterTableUnsetPropertiesCommand(
val catalog = sparkSession.sessionState.catalog
val table = catalog.getTableRawMetadata(tableName)
if (!ifExists) {
- propKeys.foreach { k =>
- if (!table.properties.contains(k) && k != TableCatalog.PROP_COMMENT) {
- throw QueryCompilationErrors.unsetNonExistentPropertyError(k, table.identifier)
- }
+ val nonexistentKeys = propKeys.filter(key => !table.properties.contains(key)
Review Comment:
I think instead of printing the first nonexistent property in the error message, it is better to collect all the properties which are not existent and print them all.
--
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 #41018: [SPARK-37942][CORE][SQL] Migrate error classes
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #41018:
URL: https://github.com/apache/spark/pull/41018#discussion_r1186818170
##########
core/src/main/resources/error/error-classes.json:
##########
@@ -235,6 +235,23 @@
],
"sqlState" : "22018"
},
+ "INSUFFICIENT_TABLE_PROPERTY": {
+ "message" : [
+ "Can't find table property:"
+ ],
+ "subClass" : {
+ "TABLE_PROPERTY_MISSING" : {
Review Comment:
No need to duplicate `TABLE_PROPERTY_` in sub-classes.
--
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] NarekDW commented on a diff in pull request #41018: [SPARK-37942][CORE][SQL] Migrate error classes
Posted by "NarekDW (via GitHub)" <gi...@apache.org>.
NarekDW commented on code in PR #41018:
URL: https://github.com/apache/spark/pull/41018#discussion_r1187099052
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryCompilationErrors.scala:
##########
@@ -1033,10 +1033,19 @@ private[sql] object QueryCompilationErrors extends QueryErrorsBase {
messageParameters = Map("colName" -> colName, "dataType" -> dataType.toString))
}
- def cannotReadCorruptedTablePropertyError(key: String, details: String = ""): Throwable = {
+ def insufficientTablePropertyError(key: String): Throwable = {
new AnalysisException(
- errorClass = "_LEGACY_ERROR_TEMP_1091",
- messageParameters = Map("key" -> key, "details" -> details))
+ errorClass = "INSUFFICIENT_TABLE_PROPERTY.MISSING_KEY",
+ messageParameters = Map("key" -> toSQLId(key)))
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
[GitHub] [spark] NarekDW commented on a diff in pull request #41018: [SPARK-37942][CORE][SQL] Migrate error classes
Posted by "NarekDW (via GitHub)" <gi...@apache.org>.
NarekDW commented on code in PR #41018:
URL: https://github.com/apache/spark/pull/41018#discussion_r1189244834
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryCompilationErrors.scala:
##########
@@ -1269,7 +1278,7 @@ private[sql] object QueryCompilationErrors extends QueryErrorsBase {
private def notSupportedInJDBCCatalog(cmd: String): Throwable = {
new AnalysisException(
- errorClass = "_LEGACY_ERROR_TEMP_1119",
+ errorClass = "NOT_SUPPORTED_OPERATION_IN_JDBC_CATALOG",
messageParameters = Map("cmd" -> cmd))
Review Comment:
Done. Also added `"sqlState" : "46110"` for `NOT_SUPPORTED_COMMAND_FOR_V2_TABLE`
--
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 #41018: [SPARK-37942][CORE][SQL] Migrate error classes
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #41018:
URL: https://github.com/apache/spark/pull/41018#discussion_r1187518057
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryCompilationErrors.scala:
##########
@@ -1269,7 +1278,7 @@ private[sql] object QueryCompilationErrors extends QueryErrorsBase {
private def notSupportedInJDBCCatalog(cmd: String): Throwable = {
new AnalysisException(
- errorClass = "_LEGACY_ERROR_TEMP_1119",
+ errorClass = "NOT_SUPPORTED_OPERATION_IN_JDBC_CATALOG",
messageParameters = Map("cmd" -> cmd))
Review Comment:
because we have some arbitrary text after SQL statements like:
```sql
CREATE NAMESPACE with property $k
```
If so, how about to add sub-classes the commands related to properties?
--
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] NarekDW commented on a diff in pull request #41018: [SPARK-37942][CORE][SQL] Migrate error classes
Posted by "NarekDW (via GitHub)" <gi...@apache.org>.
NarekDW commented on code in PR #41018:
URL: https://github.com/apache/spark/pull/41018#discussion_r1188202039
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryCompilationErrors.scala:
##########
@@ -1269,7 +1278,7 @@ private[sql] object QueryCompilationErrors extends QueryErrorsBase {
private def notSupportedInJDBCCatalog(cmd: String): Throwable = {
new AnalysisException(
- errorClass = "_LEGACY_ERROR_TEMP_1119",
+ errorClass = "NOT_SUPPORTED_OPERATION_IN_JDBC_CATALOG",
messageParameters = Map("cmd" -> cmd))
Review Comment:
How about such an approach?
```json
"NOT_SUPPORTED_IN_JDBC_CATALOG" : {
"message" : [
"Not supported operation in JDBC catalog:"
],
"subClass" : {
"OPERATION" : {
"message" : [
"<cmd>"
]
},
"OPERATION_WITH_PROPERTY" : {
"message" : [
"<cmd> with property <property>."
]
}
},
"sqlState" : "46110"
}
```
(Also I'm not sure if the sql state is the correct one)
And corresponding error messages examples:
- `[NOT_SUPPORTED_IN_JDBC_CATALOG.OPERATION_WITH_PROPERTY] Not supported operation in JDBC catalog: SET NAMESPACE with property "location".`
- `[NOT_SUPPORTED_IN_JDBC_CATALOG.OPERATION] Not supported operation in JDBC catalog: CREATE NAMESPACE ... LOCATION ...`
--
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 #41018: [SPARK-37942][CORE][SQL] Migrate error classes
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #41018:
URL: https://github.com/apache/spark/pull/41018#discussion_r1186902706
##########
core/src/main/resources/error/error-classes.json:
##########
@@ -789,6 +789,23 @@
],
"sqlState" : "21S01"
},
+ "INSUFFICIENT_TABLE_PROPERTY" : {
+ "message" : [
+ "Can't find table property:"
+ ],
+ "subClass" : {
+ "MISSING_KEY" : {
+ "message" : [
+ "<key>."
+ ]
+ },
+ "MISSING_KEY_PART" : {
+ "message" : [
+ "<key>, <totalAmountOfParts> parts are expected..."
Review Comment:
```suggestion
"<key>, <totalAmountOfParts> parts are expected."
```
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryCompilationErrors.scala:
##########
@@ -1033,10 +1033,19 @@ private[sql] object QueryCompilationErrors extends QueryErrorsBase {
messageParameters = Map("colName" -> colName, "dataType" -> dataType.toString))
}
- def cannotReadCorruptedTablePropertyError(key: String, details: String = ""): Throwable = {
+ def insufficientTablePropertyError(key: String): Throwable = {
new AnalysisException(
- errorClass = "_LEGACY_ERROR_TEMP_1091",
- messageParameters = Map("key" -> key, "details" -> details))
+ errorClass = "INSUFFICIENT_TABLE_PROPERTY.MISSING_KEY",
+ messageParameters = Map("key" -> toSQLId(key)))
Review Comment:
Don't think we should consider properties as identifiers. How about to wrap it as a string value by `toSQLValue()`.
--
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] NarekDW commented on a diff in pull request #41018: [SPARK-37942][CORE][SQL] Migrate error classes
Posted by "NarekDW (via GitHub)" <gi...@apache.org>.
NarekDW commented on code in PR #41018:
URL: https://github.com/apache/spark/pull/41018#discussion_r1182805619
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/interface.scala:
##########
@@ -497,21 +497,20 @@ object CatalogTable {
def readLargeTableProp(props: Map[String, String], key: String): Option[String] = {
props.get(key).orElse {
- if (props.filterKeys(_.startsWith(key)).isEmpty) {
- None
- } else {
- val numParts = props.get(s"$key.numParts")
- if (numParts.isEmpty) {
- throw QueryCompilationErrors.cannotReadCorruptedTablePropertyError(key)
- } else {
- val parts = (0 until numParts.get.toInt).map { index =>
- props.getOrElse(s"$key.part.$index", {
- throw QueryCompilationErrors.cannotReadCorruptedTablePropertyError(
- key, s"Missing part $index, $numParts parts are expected.")
Review Comment:
numParts is Option here, instead of printing Some(value) it is better to keep just value in error message.
--
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 #41018: [SPARK-37942][CORE][SQL] Migrate error classes
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #41018:
URL: https://github.com/apache/spark/pull/41018#discussion_r1183436059
##########
core/src/main/resources/error/error-classes.json:
##########
@@ -235,6 +235,11 @@
],
"sqlState" : "22018"
},
+ "CORRUPTED_TABLE_PROPERTY" : {
+ "message" : [
+ "Cannot read table property <key> as it's corrupted.<details>"
Review Comment:
Could you add sub-classes instead of `<details>`, please.
##########
core/src/main/resources/error/error-classes.json:
##########
@@ -235,6 +235,11 @@
],
"sqlState" : "22018"
},
+ "CORRUPTED_TABLE_PROPERTY" : {
Review Comment:
Why do you think the properties are corrupted?
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryCompilationErrors.scala:
##########
@@ -1349,7 +1351,7 @@ private[sql] object QueryCompilationErrors extends QueryErrorsBase {
private def notSupportedForV2TablesError(cmd: String): Throwable = {
new AnalysisException(
- errorClass = "_LEGACY_ERROR_TEMP_1124",
+ errorClass = "NOT_SUPPORTED_OPERATION_FOR_V2_TABLE",
messageParameters = Map("cmd" -> cmd))
Review Comment:
Please, wrap `cmd` by `toSQLStmt()`.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryCompilationErrors.scala:
##########
@@ -2402,12 +2404,14 @@ private[sql] object QueryCompilationErrors extends QueryErrorsBase {
"config" -> SQLConf.ALLOW_NON_EMPTY_LOCATION_IN_CTAS.key))
}
- def unsetNonExistentPropertyError(property: String, table: TableIdentifier): Throwable = {
+ def unsetNonExistentPropertiesError(properties: Seq[String],
+ table: TableIdentifier): Throwable = {
Review Comment:
Please, fix indentation, see https://github.com/databricks/scala-style-guide#indent
--
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 #41018: [SPARK-37942][CORE][SQL] Migrate error classes
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk closed pull request #41018: [SPARK-37942][CORE][SQL] Migrate error classes
URL: https://github.com/apache/spark/pull/41018
--
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] NarekDW commented on a diff in pull request #41018: [SPARK-37942][CORE][SQL] Migrate error classes
Posted by "NarekDW (via GitHub)" <gi...@apache.org>.
NarekDW commented on code in PR #41018:
URL: https://github.com/apache/spark/pull/41018#discussion_r1186642349
##########
core/src/main/resources/error/error-classes.json:
##########
@@ -235,6 +235,11 @@
],
"sqlState" : "22018"
},
+ "CORRUPTED_TABLE_PROPERTY" : {
+ "message" : [
+ "Cannot read table property <key> as it's corrupted.<details>"
Review Comment:
It might be renamed to `INSUFFICIENT_TABLE_PROPERTY` with sub classes like this:
```json
"INSUFFICIENT_TABLE_PROPERTY": {
"message" : [
"Can't find table property:"
],
"subClass" : {
"TABLE_PROPERTY_MISSING" : {
"message" : [
"<key>."
]
},
"TABLE_PROPERTY_PART_MISSING": {
"message" : [
"<key>, <totalAmountOfParts> parts are expected..."
]
}
}
}
```
Error message examples:
- ```[INSUFFICIENT_TABLE_PROPERTY.TABLE_PROPERTY_MISSING] Can't find table property: `spark`.`sql`.`sources`.`schema`.```
- ```[INSUFFICIENT_TABLE_PROPERTY.TABLE_PROPERTY_PART_MISSING] Can't find table property: `spark`.`sql`.`sources`.`schema`.`part`.`1`, 3 parts are expected...```
What do you think? Is it fine?
--
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 #41018: [SPARK-37942][CORE][SQL] Migrate error classes
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #41018:
URL: https://github.com/apache/spark/pull/41018#discussion_r1187087017
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryCompilationErrors.scala:
##########
@@ -1033,10 +1033,19 @@ private[sql] object QueryCompilationErrors extends QueryErrorsBase {
messageParameters = Map("colName" -> colName, "dataType" -> dataType.toString))
}
- def cannotReadCorruptedTablePropertyError(key: String, details: String = ""): Throwable = {
+ def insufficientTablePropertyError(key: String): Throwable = {
new AnalysisException(
- errorClass = "_LEGACY_ERROR_TEMP_1091",
- messageParameters = Map("key" -> key, "details" -> details))
+ errorClass = "INSUFFICIENT_TABLE_PROPERTY.MISSING_KEY",
+ messageParameters = Map("key" -> toSQLId(key)))
Review Comment:
SGTM
--
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] NarekDW commented on a diff in pull request #41018: [SPARK-37942][CORE][SQL] Migrate error classes
Posted by "NarekDW (via GitHub)" <gi...@apache.org>.
NarekDW commented on code in PR #41018:
URL: https://github.com/apache/spark/pull/41018#discussion_r1182805619
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/interface.scala:
##########
@@ -497,21 +497,20 @@ object CatalogTable {
def readLargeTableProp(props: Map[String, String], key: String): Option[String] = {
props.get(key).orElse {
- if (props.filterKeys(_.startsWith(key)).isEmpty) {
- None
- } else {
- val numParts = props.get(s"$key.numParts")
- if (numParts.isEmpty) {
- throw QueryCompilationErrors.cannotReadCorruptedTablePropertyError(key)
- } else {
- val parts = (0 until numParts.get.toInt).map { index =>
- props.getOrElse(s"$key.part.$index", {
- throw QueryCompilationErrors.cannotReadCorruptedTablePropertyError(
- key, s"Missing part $index, $numParts parts are expected.")
Review Comment:
`numParts` is Option here, instead of printing `Some(value)` it is better to keep just `value` in error message.
--
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] NarekDW commented on a diff in pull request #41018: [SPARK-37942][CORE][SQL] Migrate error classes
Posted by "NarekDW (via GitHub)" <gi...@apache.org>.
NarekDW commented on code in PR #41018:
URL: https://github.com/apache/spark/pull/41018#discussion_r1182804192
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/interface.scala:
##########
@@ -497,21 +497,20 @@ object CatalogTable {
def readLargeTableProp(props: Map[String, String], key: String): Option[String] = {
props.get(key).orElse {
- if (props.filterKeys(_.startsWith(key)).isEmpty) {
Review Comment:
No need to iterate through all the props it is enough to find the first match.
--
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 #41018: [SPARK-37942][CORE][SQL] Migrate error classes
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on PR #41018:
URL: https://github.com/apache/spark/pull/41018#issuecomment-1541941861
+1, LGTM. Merging to master.
Thank you, @NarekDW.
--
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 #41018: [SPARK-37942][CORE][SQL] Migrate error classes
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #41018:
URL: https://github.com/apache/spark/pull/41018#discussion_r1186694226
##########
core/src/main/resources/error/error-classes.json:
##########
@@ -235,6 +235,11 @@
],
"sqlState" : "22018"
},
+ "CORRUPTED_TABLE_PROPERTY" : {
+ "message" : [
+ "Cannot read table property <key> as it's corrupted.<details>"
Review Comment:
SGTM
--
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 #41018: [SPARK-37942][CORE][SQL] Migrate error classes
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #41018:
URL: https://github.com/apache/spark/pull/41018#discussion_r1188380385
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryCompilationErrors.scala:
##########
@@ -1269,7 +1278,7 @@ private[sql] object QueryCompilationErrors extends QueryErrorsBase {
private def notSupportedInJDBCCatalog(cmd: String): Throwable = {
new AnalysisException(
- errorClass = "_LEGACY_ERROR_TEMP_1119",
+ errorClass = "NOT_SUPPORTED_OPERATION_IN_JDBC_CATALOG",
messageParameters = Map("cmd" -> cmd))
Review Comment:
Could you replace `OPERATION` by `COMMAND` since we use the last word mostly.
> "sqlState" : "46110"
> (Also I'm not sure if the sql state is the correct one)
SGTM
--
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 #41018: [SPARK-37942][CORE][SQL] Migrate error classes
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #41018:
URL: https://github.com/apache/spark/pull/41018#discussion_r1187512541
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryCompilationErrors.scala:
##########
@@ -1269,7 +1278,7 @@ private[sql] object QueryCompilationErrors extends QueryErrorsBase {
private def notSupportedInJDBCCatalog(cmd: String): Throwable = {
new AnalysisException(
- errorClass = "_LEGACY_ERROR_TEMP_1119",
+ errorClass = "NOT_SUPPORTED_OPERATION_IN_JDBC_CATALOG",
messageParameters = Map("cmd" -> cmd))
Review Comment:
Just wonder why you wrap `cmd` in `NOT_SUPPORTED_OPERATION_FOR_V2_TABLE` but don't wrap it here.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryCompilationErrors.scala:
##########
@@ -1269,7 +1278,7 @@ private[sql] object QueryCompilationErrors extends QueryErrorsBase {
private def notSupportedInJDBCCatalog(cmd: String): Throwable = {
new AnalysisException(
- errorClass = "_LEGACY_ERROR_TEMP_1119",
+ errorClass = "NOT_SUPPORTED_OPERATION_IN_JDBC_CATALOG",
messageParameters = Map("cmd" -> cmd))
Review Comment:
Just wonder why you wrap `cmd` in `NOT_SUPPORTED_OPERATION_FOR_V2_TABLE` but don't wrap it 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
[GitHub] [spark] NarekDW commented on a diff in pull request #41018: [SPARK-37942][CORE][SQL] Migrate error classes
Posted by "NarekDW (via GitHub)" <gi...@apache.org>.
NarekDW commented on code in PR #41018:
URL: https://github.com/apache/spark/pull/41018#discussion_r1186818922
##########
core/src/main/resources/error/error-classes.json:
##########
@@ -235,6 +235,23 @@
],
"sqlState" : "22018"
},
+ "INSUFFICIENT_TABLE_PROPERTY": {
+ "message" : [
+ "Can't find table property:"
+ ],
+ "subClass" : {
+ "TABLE_PROPERTY_MISSING" : {
Review Comment:
Yep, fixed
--
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] NarekDW commented on a diff in pull request #41018: [SPARK-37942][CORE][SQL] Migrate error classes
Posted by "NarekDW (via GitHub)" <gi...@apache.org>.
NarekDW commented on code in PR #41018:
URL: https://github.com/apache/spark/pull/41018#discussion_r1187079983
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryCompilationErrors.scala:
##########
@@ -1033,10 +1033,19 @@ private[sql] object QueryCompilationErrors extends QueryErrorsBase {
messageParameters = Map("colName" -> colName, "dataType" -> dataType.toString))
}
- def cannotReadCorruptedTablePropertyError(key: String, details: String = ""): Throwable = {
+ def insufficientTablePropertyError(key: String): Throwable = {
new AnalysisException(
- errorClass = "_LEGACY_ERROR_TEMP_1091",
- messageParameters = Map("key" -> key, "details" -> details))
+ errorClass = "INSUFFICIENT_TABLE_PROPERTY.MISSING_KEY",
+ messageParameters = Map("key" -> toSQLId(key)))
Review Comment:
Not sure regarding `toSQLValue()` it expects a value of some specific data type...
What do you think about `toSQLConf`? It will wrap property with quotes.
--
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] NarekDW commented on a diff in pull request #41018: [SPARK-37942][CORE][SQL] Migrate error classes
Posted by "NarekDW (via GitHub)" <gi...@apache.org>.
NarekDW commented on code in PR #41018:
URL: https://github.com/apache/spark/pull/41018#discussion_r1186813775
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryCompilationErrors.scala:
##########
@@ -1349,7 +1351,7 @@ private[sql] object QueryCompilationErrors extends QueryErrorsBase {
private def notSupportedForV2TablesError(cmd: String): Throwable = {
new AnalysisException(
- errorClass = "_LEGACY_ERROR_TEMP_1124",
+ errorClass = "NOT_SUPPORTED_OPERATION_FOR_V2_TABLE",
messageParameters = Map("cmd" -> cmd))
Review Comment:
Done
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryCompilationErrors.scala:
##########
@@ -2402,12 +2404,14 @@ private[sql] object QueryCompilationErrors extends QueryErrorsBase {
"config" -> SQLConf.ALLOW_NON_EMPTY_LOCATION_IN_CTAS.key))
}
- def unsetNonExistentPropertyError(property: String, table: TableIdentifier): Throwable = {
+ def unsetNonExistentPropertiesError(properties: Seq[String],
+ table: TableIdentifier): Throwable = {
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
[GitHub] [spark] MaxGekk commented on a diff in pull request #41018: [SPARK-37942][CORE][SQL] Migrate error classes
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #41018:
URL: https://github.com/apache/spark/pull/41018#discussion_r1186818170
##########
core/src/main/resources/error/error-classes.json:
##########
@@ -235,6 +235,23 @@
],
"sqlState" : "22018"
},
+ "INSUFFICIENT_TABLE_PROPERTY": {
+ "message" : [
+ "Can't find table property:"
+ ],
+ "subClass" : {
+ "TABLE_PROPERTY_MISSING" : {
Review Comment:
Not need to duplicate `TABLE_PROPERTY_` in sub-classes.
--
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