You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "panbingkun (via GitHub)" <gi...@apache.org> on 2023/10/22 13:30:49 UTC
[PR] [SPARK-45626][SQL] Fix variable name of error-class & assign names to the error class _LEGACY_ERROR_TEMP_1055 [spark]
panbingkun opened a new pull request, #43479:
URL: https://github.com/apache/spark/pull/43479
### What changes were proposed in this pull request?
The pr aims to
1.fix variable name of error-class:
2.assign names to the error class _LEGACY_ERROR_TEMP_1055
### Why are the changes needed?
Fix bug.
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
- Pass GA.
- Manually test:
```
```
### Was this patch authored or co-authored using generative AI tooling?
No.
--
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
Re: [PR] [SPARK-45626][SQL] Fix variable name of error-class & convert _LEGACY_ERROR_TEMP_1055 to REQUIRES_SINGLE_PART_NAMESPACE [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #43479:
URL: https://github.com/apache/spark/pull/43479#discussion_r1368181666
##########
common/utils/src/main/resources/error/error-classes.json:
##########
@@ -1553,6 +1553,11 @@
},
"sqlState" : "HY109"
},
+ "INVALID_DATABASE_NAME" : {
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
Re: [PR] [SPARK-45626][SQL] Convert _LEGACY_ERROR_TEMP_1055 to REQUIRES_SINGLE_PART_NAMESPACE [spark]
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on PR #43479:
URL: https://github.com/apache/spark/pull/43479#issuecomment-1776668293
> @panbingkun Could you update PR's description according to the recent changes (screenshot, test example, and so on).
@MaxGekk 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
Re: [PR] [SPARK-45626][SQL] Fix variable name of error-class & assign names to the error class _LEGACY_ERROR_TEMP_1055 [spark]
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on PR #43479:
URL: https://github.com/apache/spark/pull/43479#issuecomment-1774100838
Before:
<img width="1327" alt="image" src="https://github.com/apache/spark/assets/15246973/e7a59837-4f14-4f09-872a-913d78006ede">
After:
<img width="1001" alt="image" src="https://github.com/apache/spark/assets/15246973/fa141a37-53b5-42eb-9ecc-51219222680e">
--
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
Re: [PR] [SPARK-45626][SQL] Fix variable name of error-class & convert _LEGACY_ERROR_TEMP_1055 to REQUIRES_SINGLE_PART_NAMESPACE [spark]
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on code in PR #43479:
URL: https://github.com/apache/spark/pull/43479#discussion_r1368087736
##########
common/utils/src/main/resources/error/error-classes.json:
##########
@@ -1553,6 +1553,11 @@
},
"sqlState" : "HY109"
},
+ "INVALID_DATABASE_NAME" : {
Review Comment:
Maybe we can re-use error class: `REQUIRES_SINGLE_PART_NAMESPACE`?
But the prompt message is called `namespace` instead of `database`, seems to be okay? I'm too sure
https://github.com/apache/spark/blob/376de8a502fca6b46d7f21560a60024d643144ea/common/utils/src/main/resources/error/error-classes.json#L2784-L2789
--
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
Re: [PR] [SPARK-45626][SQL] Fix variable name of error-class & convert _LEGACY_ERROR_TEMP_1055 to REQUIRES_SINGLE_PART_NAMESPACE [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on PR #43479:
URL: https://github.com/apache/spark/pull/43479#issuecomment-1776612443
@panbingkun Could you update PR's description according to the recent changes (screenshot, test example, and so on).
--
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
Re: [PR] [SPARK-45626][SQL] Convert _LEGACY_ERROR_TEMP_1055 to REQUIRES_SINGLE_PART_NAMESPACE [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on PR #43479:
URL: https://github.com/apache/spark/pull/43479#issuecomment-1776690920
+1, LGTM. Merging to master.
Thank you, @panbingkun.
--
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
Re: [PR] [SPARK-45626][SQL] Convert _LEGACY_ERROR_TEMP_1055 to REQUIRES_SINGLE_PART_NAMESPACE [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk closed pull request #43479: [SPARK-45626][SQL] Convert _LEGACY_ERROR_TEMP_1055 to REQUIRES_SINGLE_PART_NAMESPACE
URL: https://github.com/apache/spark/pull/43479
--
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
Re: [PR] [SPARK-45626][SQL] Fix variable name of error-class & assign names to the error class _LEGACY_ERROR_TEMP_1055 [spark]
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on PR #43479:
URL: https://github.com/apache/spark/pull/43479#issuecomment-1774101171
cc @MaxGekk
--
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
Re: [PR] [SPARK-45626][SQL] Fix variable name of error-class & assign names to the error class _LEGACY_ERROR_TEMP_1055 [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #43479:
URL: https://github.com/apache/spark/pull/43479#discussion_r1367943424
##########
common/utils/src/main/resources/error/error-classes.json:
##########
@@ -1553,6 +1553,11 @@
},
"sqlState" : "HY109"
},
+ "INVALID_DATABASE_NAME" : {
Review Comment:
Can't you re-use the existing error class: `INVALID_SCHEMA_OR_RELATION_NAME`? If not, could you make it more specific.
##########
sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/command/CreateNamespaceSuite.scala:
##########
@@ -25,4 +26,15 @@ import org.apache.spark.sql.execution.command.v1
*/
class CreateNamespaceSuite extends v1.CreateNamespaceSuiteBase with CommandSuiteBase {
override def commandVersion: String = super[CreateNamespaceSuiteBase].commandVersion
+
+ test("INVALID_DATABASE_NAME") {
+ val namespace = "ns1.ns2"
+ checkError(
+ exception = intercept[AnalysisException] {
+ sql(s"CREATE NAMESPACE $catalog.$namespace")
Review Comment:
When an user observes the error `The database name is not valid:`, not clear why it is invalid. We should provide some clues.
--
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
Re: [PR] [SPARK-45626][SQL] Fix variable name of error-class & convert _LEGACY_ERROR_TEMP_1055 to REQUIRES_SINGLE_PART_NAMESPACE [spark]
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on code in PR #43479:
URL: https://github.com/apache/spark/pull/43479#discussion_r1368087736
##########
common/utils/src/main/resources/error/error-classes.json:
##########
@@ -1553,6 +1553,11 @@
},
"sqlState" : "HY109"
},
+ "INVALID_DATABASE_NAME" : {
Review Comment:
Maybe we can re-use error class: `REQUIRES_SINGLE_PART_NAMESPACE`?
But the prompt message is called `namespace` instead of `database`, seems to be okay?
I'm not certain , would like to hear your suggestions.
https://github.com/apache/spark/blob/376de8a502fca6b46d7f21560a60024d643144ea/common/utils/src/main/resources/error/error-classes.json#L2784-L2789
--
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