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/14 07:58:36 UTC
[GitHub] [spark] itholic opened a new pull request, #38650: [SPARK-41135][SQL] Rename `UNSUPPORTED_EMPTY_LOCATION` to `INVALID_EMPTY_LOCATION`
itholic opened a new pull request, #38650:
URL: https://github.com/apache/spark/pull/38650
### What changes were proposed in this pull request?
This PR proposes to rename `UNSUPPORTED_EMPTY_LOCATION` to `INVALID_EMPTY_LOCATION`.
### Why are the changes needed?
Error class and its message should be clear/brief, and should not ambiguously specific when it illustrates things that may be supported in the future.
### Does this PR introduce _any_ user-facing change?
Error message changes
From
```
"Unsupported empty location."
```
To
```
"A specified location cannot be empty."
```
### How was this patch tested?
```
./build/sbt “sql/testOnly org.apache.spark.sql.SQLQueryTestSuite*”
build/sbt "core/testOnly *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] itholic commented on a diff in pull request #38650: [SPARK-41135][SQL] Rename `UNSUPPORTED_EMPTY_LOCATION` to `INVALID_EMPTY_LOCATION`
Posted by GitBox <gi...@apache.org>.
itholic commented on code in PR #38650:
URL: https://github.com/apache/spark/pull/38650#discussion_r1028818844
##########
core/src/main/resources/error/error-classes.json:
##########
@@ -656,6 +656,11 @@
],
"sqlState" : "42000"
},
+ "INVALID_EMPTY_LOCATION" : {
+ "message" : [
+ "The location name cannot be empty string or null, but `<location>` was given."
Review Comment:
Good catch.
I tried to add a test with NULL, but it complains [PARSE_SYNTAX_ERROR] rather than [INVALID_EMPTY_LOCATION].
Let me exclude the "null" from the 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] itholic commented on a diff in pull request #38650: [SPARK-41135][SQL] Rename `UNSUPPORTED_EMPTY_LOCATION` to `INVALID_EMPTY_LOCATION`
Posted by GitBox <gi...@apache.org>.
itholic commented on code in PR #38650:
URL: https://github.com/apache/spark/pull/38650#discussion_r1028818844
##########
core/src/main/resources/error/error-classes.json:
##########
@@ -656,6 +656,11 @@
],
"sqlState" : "42000"
},
+ "INVALID_EMPTY_LOCATION" : {
+ "message" : [
+ "The location name cannot be empty string or null, but `<location>` was given."
Review Comment:
Good catch.
I tried to add a test with NULL, but it complains [PARSE_SYNTAX_ERROR] rather than [INVALID_EMPTY_LOCATION].
Let me exclude the null from the 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 pull request #38650: [SPARK-41135][SQL] Rename `UNSUPPORTED_EMPTY_LOCATION` to `INVALID_EMPTY_LOCATION`
Posted by GitBox <gi...@apache.org>.
MaxGekk commented on PR #38650:
URL: https://github.com/apache/spark/pull/38650#issuecomment-1323419671
+1, LGTM. Merging to master.
Thank you, @itholic.
--
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 #38650: [SPARK-41135][SQL] Rename `UNSUPPORTED_EMPTY_LOCATION` to `INVALID_EMPTY_LOCATION`
Posted by GitBox <gi...@apache.org>.
MaxGekk commented on code in PR #38650:
URL: https://github.com/apache/spark/pull/38650#discussion_r1024814862
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/DataSourceV2Strategy.scala:
##########
@@ -369,7 +369,7 @@ class DataSourceV2Strategy(session: SparkSession) extends Strategy with Predicat
case CreateNamespace(ResolvedNamespace(catalog, ns), ifNotExists, properties) =>
val location = properties.get(SupportsNamespaces.PROP_LOCATION)
if (location.isDefined && location.get.isEmpty) {
- throw QueryExecutionErrors.unsupportedEmptyLocationError()
+ throw QueryExecutionErrors.invalidEmptyLocationError(location.toString)
Review Comment:
Please, get the value from the `location` Option.
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/command/CreateNamespaceSuiteBase.scala:
##########
@@ -74,8 +74,8 @@ trait CreateNamespaceSuiteBase extends QueryTest with DDLCommandTestUtils {
exception = intercept[SparkIllegalArgumentException] {
sql(sqlText)
},
- errorClass = "UNSUPPORTED_EMPTY_LOCATION",
- parameters = Map.empty)
+ errorClass = "INVALID_EMPTY_LOCATION",
+ parameters = Map("location" -> "Some()"))
Review Comment:
`Some()`? Could you get the value, 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 closed pull request #38650: [SPARK-41135][SQL] Rename `UNSUPPORTED_EMPTY_LOCATION` to `INVALID_EMPTY_LOCATION`
Posted by GitBox <gi...@apache.org>.
MaxGekk closed pull request #38650: [SPARK-41135][SQL] Rename `UNSUPPORTED_EMPTY_LOCATION` to `INVALID_EMPTY_LOCATION`
URL: https://github.com/apache/spark/pull/38650
--
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 #38650: [SPARK-41135][SQL] Rename `UNSUPPORTED_EMPTY_LOCATION` to `INVALID_EMPTY_LOCATION`
Posted by GitBox <gi...@apache.org>.
MaxGekk commented on code in PR #38650:
URL: https://github.com/apache/spark/pull/38650#discussion_r1021209665
##########
core/src/main/resources/error/error-classes.json:
##########
@@ -616,6 +616,11 @@
],
"sqlState" : "42000"
},
+ "INVALID_EMPTY_LOCATION" : {
+ "message" : [
+ "A specified location cannot be empty."
Review Comment:
This still confuses me. The location path doesn't contain any data/files, or it is an empty string, or it is null. Looking at:
```scala
if (StringUtils.isEmpty(location)) {
throw QueryExecutionErrors.unsupportedEmptyLocationError()
}
```
and at `StringUtils.isEmpty`:
```scala
public static boolean isEmpty(final CharSequence cs) {
return cs == null || cs.length() == 0;
}
```
the error is about the path string, not data.
@itholic Could you make the error more precise, and rename `QueryExecutionErrors.unsupportedEmptyLocationError`, 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] itholic commented on pull request #38650: [SPARK-41135][SQL] Rename `UNSUPPORTED_EMPTY_LOCATION` to `INVALID_EMPTY_LOCATION`
Posted by GitBox <gi...@apache.org>.
itholic commented on PR #38650:
URL: https://github.com/apache/spark/pull/38650#issuecomment-1313235817
cc @MaxGekk @srielau
--
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 #38650: [SPARK-41135][SQL] Rename `UNSUPPORTED_EMPTY_LOCATION` to `INVALID_EMPTY_LOCATION`
Posted by GitBox <gi...@apache.org>.
MaxGekk commented on code in PR #38650:
URL: https://github.com/apache/spark/pull/38650#discussion_r1026258331
##########
core/src/main/resources/error/error-classes.json:
##########
@@ -656,6 +656,11 @@
],
"sqlState" : "42000"
},
+ "INVALID_EMPTY_LOCATION" : {
+ "message" : [
+ "The location name cannot be empty string or null, but `<location>` was given."
Review Comment:
Could you add a test when the location is NULL. I just worry that parameters substitution might not handle this. Please, check this.
--
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