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 2020/07/17 14:52:23 UTC
[GitHub] [spark] maropu opened a new pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET command
maropu opened a new pull request #29146:
URL: https://github.com/apache/spark/pull/29146
<!--
Thanks for sending a pull request! Here are some tips for you:
1. If this is your first time, please read our contributor guidelines: https://spark.apache.org/contributing.html
2. Ensure you have added or run the appropriate tests for your PR: https://spark.apache.org/developer-tools.html
3. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP][SPARK-XXXX] Your PR title ...'.
4. Be sure to keep the PR description updated to reflect all changes.
5. Please write your PR title to summarize what this PR proposes.
6. If possible, provide a concise example to reproduce the issue for a faster review.
7. If you want to add a new configuration, please read the guideline first for naming configurations in
'core/src/main/scala/org/apache/spark/internal/config/ConfigEntry.scala'.
-->
### What changes were proposed in this pull request?
<!--
Please clarify what changes you are proposing. The purpose of this section is to outline the changes and how this PR fixes the issue.
If possible, please consider writing useful notes for better and faster reviews in your PR. See the examples below.
1. If you refactor some codes with changing classes, showing the class hierarchy will help reviewers.
2. If you fix some SQL features, you can provide some references of other DBMSes.
3. If there is design documentation, please add the link.
4. If there is a discussion in the mailing list, please add the link.
-->
This PR modified the parser code to handle invalid usages of a SET command.
For example;
```
SET spark.sql.ansi.enabled true
```
The above SQL command does not change the configuration value and it just tries to display the value of the configuration
`spark.sql.ansi.enabled true`. This PR disallows using the space in the configuration name and reports a user-friendly error instead. In the error message, it tells users a workaround to use the quote if they still needs to specify a configuration with spaces.
### Why are the changes needed?
<!--
Please clarify why the changes are needed. For instance,
1. If you propose a new API, clarify the use case for a new API.
2. If you fix a bug, you can clarify why it is a bug.
-->
For better user-friendly errors.
### Does this PR introduce _any_ user-facing change?
<!--
Note that it means *any* user-facing change including all aspects such as the documentation fix.
If yes, please clarify the previous behavior and the change this PR proposes - provide the console output, description and/or an example to show the behavior difference if possible.
If possible, please also clarify if this is a user-facing change compared to the released Spark versions or within the unreleased branches such as master.
If no, write 'No'.
-->
No.
### How was this patch tested?
<!--
If tests were added, say they were added here. Please make sure to add some test cases that check the changes thoroughly including negative and positive cases if possible.
If it was tested in a way different from regular unit tests, please clarify how you tested step by step, ideally copy and paste-able, so that other reviewers can test and check, and descendants can verify in the future.
If tests were not added, please describe why they were not added and/or why it was difficult to add.
-->
Added tests in `SparkSqlParserSuite`.
----------------------------------------------------------------
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.
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] AmplabJenkins removed a comment on pull request #29146: [WIP][SPARK-32257][SQL] Reports explicit errors for invalid usage of SET command
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-660456576
Merged build finished. Test FAILed.
----------------------------------------------------------------
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.
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] AmplabJenkins commented on pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-667861732
----------------------------------------------------------------
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.
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] AmplabJenkins commented on pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET command
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-660223741
----------------------------------------------------------------
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.
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] AmplabJenkins removed a comment on pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-667486327
----------------------------------------------------------------
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.
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] maropu commented on pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
maropu commented on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-668018994
Thanks for the reviews!
----------------------------------------------------------------
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.
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] AmplabJenkins commented on pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-664694827
----------------------------------------------------------------
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.
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] SparkQA commented on pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-667101365
**[Test build #126875 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/126875/testReport)** for PR 29146 at commit [`6d2221d`](https://github.com/apache/spark/commit/6d2221daadb7e7934982ed4be8d1700c59c90ad0).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public 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.
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] AmplabJenkins removed a comment on pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-667502817
Merged build finished. Test FAILed.
----------------------------------------------------------------
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.
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] AmplabJenkins removed a comment on pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-666231024
Merged build finished. Test FAILed.
----------------------------------------------------------------
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.
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] AmplabJenkins commented on pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-666967998
----------------------------------------------------------------
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.
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] AmplabJenkins removed a comment on pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-667861732
----------------------------------------------------------------
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.
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] maropu commented on a change in pull request #29146: [WIP][SPARK-32257][SQL] Reports explicit errors for invalid usage of SET command
Posted by GitBox <gi...@apache.org>.
maropu commented on a change in pull request #29146:
URL: https://github.com/apache/spark/pull/29146#discussion_r456989897
##########
File path: sql/core/src/test/scala/org/apache/spark/sql/internal/SQLConfSuite.scala
##########
@@ -112,7 +112,7 @@ class SQLConfSuite extends QueryTest with SharedSparkSession {
sql(s"set ${SQLConf.Deprecated.MAPRED_REDUCE_TASKS}=10")
assert(spark.conf.get(SQLConf.SHUFFLE_PARTITIONS) === 10)
} finally {
- sql(s"set ${SQLConf.SHUFFLE_PARTITIONS}=$original")
+ sql(s"set ${SQLConf.SHUFFLE_PARTITIONS.key}=$original")
Review comment:
The existing code looks incorrect.
----------------------------------------------------------------
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.
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] maropu commented on a change in pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
maropu commented on a change in pull request #29146:
URL: https://github.com/apache/spark/pull/29146#discussion_r462884250
##########
File path: sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4
##########
@@ -244,11 +258,31 @@ statement
| SET TIME ZONE interval #setTimeZone
| SET TIME ZONE timezone=(STRING | LOCAL) #setTimeZone
| SET TIME ZONE .*? #setTimeZone
+ | SET configKey (EQ configValue)? #setConfiguration
| SET .*? #setConfiguration
+ | RESET configKey? #resetConfiguration
| RESET .*? #resetConfiguration
| unsupportedHiveNativeCommands .*? #failNativeCommand
;
+configKey
+ : configIdentifier (('.' | ':') configIdentifier
+ // The two semantic predicates make sure that no token on the hidden channel
+ // (e.g., spaces) exists between the config separator ('.' or ':').
+ {!isHidden(-3)}? {!isHidden(-2)}?)*
+ | quotedIdentifier
+ ;
+
+configValue
+ : vaule=.*?
Review comment:
Removed.
----------------------------------------------------------------
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.
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] SparkQA removed a comment on pull request #29146: [WIP][SPARK-32257][SQL] Reports explicit errors for invalid usage of SET command
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-660461071
**[Test build #126104 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/126104/testReport)** for PR 29146 at commit [`0d357ae`](https://github.com/apache/spark/commit/0d357ae6539822f59aa4ae5ff34f5215f27666e4).
----------------------------------------------------------------
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.
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] SparkQA commented on pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-667487265
**[Test build #126915 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/126915/testReport)** for PR 29146 at commit [`4822210`](https://github.com/apache/spark/commit/48222108242eb5b991705f16b926ea90c1293237).
----------------------------------------------------------------
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.
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] maropu commented on pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
maropu commented on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-666317097
retest this 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.
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] cloud-fan commented on a change in pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #29146:
URL: https://github.com/apache/spark/pull/29146#discussion_r464171523
##########
File path: sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4
##########
@@ -246,11 +246,17 @@ statement
| SET TIME ZONE interval #setTimeZone
| SET TIME ZONE timezone=(STRING | LOCAL) #setTimeZone
| SET TIME ZONE .*? #setTimeZone
+ | SET quotedConfigKey (EQ value=.*)? #setQuotedConfiguration
| SET .*? #setConfiguration
+ | RESET quotedConfigKey #resetQuotedConfiguration
| RESET .*? #resetConfiguration
| unsupportedHiveNativeCommands .*? #failNativeCommand
;
+quotedConfigKey
Review comment:
hmm, is it necessary to create an alias? How about `SET key= quotedIdentifier (EQ value=.*)?`
----------------------------------------------------------------
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.
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] SparkQA commented on pull request #29146: [WIP][SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-665949787
----------------------------------------------------------------
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.
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] AmplabJenkins removed a comment on pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-666845339
Merged build finished. Test FAILed.
----------------------------------------------------------------
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.
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] SparkQA removed a comment on pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-664694491
----------------------------------------------------------------
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.
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] SparkQA commented on pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-667860844
**[Test build #126963 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/126963/testReport)** for PR 29146 at commit [`b0e9686`](https://github.com/apache/spark/commit/b0e9686e9c92045068d253e23137457e47a65f9f).
----------------------------------------------------------------
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.
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] AmplabJenkins removed a comment on pull request #29146: [WIP][SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-665635179
----------------------------------------------------------------
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.
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] SparkQA commented on pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET command
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-660152612
**[Test build #126055 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/126055/testReport)** for PR 29146 at commit [`7f8907e`](https://github.com/apache/spark/commit/7f8907ee3ba2ee9e6c64b1734472ea05da00ddb2).
----------------------------------------------------------------
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.
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] SparkQA commented on pull request #29146: [WIP][SPARK-32257][SQL] Reports explicit errors for invalid usage of SET command
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-660471752
**[Test build #126104 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/126104/testReport)** for PR 29146 at commit [`0d357ae`](https://github.com/apache/spark/commit/0d357ae6539822f59aa4ae5ff34f5215f27666e4).
* This patch **fails Spark unit tests**.
* This patch merges cleanly.
* This patch adds no public 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.
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] maropu commented on a change in pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
maropu commented on a change in pull request #29146:
URL: https://github.com/apache/spark/pull/29146#discussion_r464257964
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/execution/SparkSqlParser.scala
##########
@@ -66,17 +69,28 @@ class SparkSqlAstBuilder(conf: SQLConf) extends AstBuilder(conf) {
* character in the raw string.
*/
override def visitSetConfiguration(ctx: SetConfigurationContext): LogicalPlan = withOrigin(ctx) {
- // Construct the command.
- val raw = remainder(ctx.SET.getSymbol)
- val keyValueSeparatorIndex = raw.indexOf('=')
- if (keyValueSeparatorIndex >= 0) {
- val key = raw.substring(0, keyValueSeparatorIndex).trim
- val value = raw.substring(keyValueSeparatorIndex + 1).trim
- SetCommand(Some(key -> Option(value)))
- } else if (raw.nonEmpty) {
- SetCommand(Some(raw.trim -> None))
+ remainder(ctx.SET.getSymbol).trim match {
+ case configKeyValueDef(key, value) =>
+ SetCommand(Some(key -> Option(value.trim)))
+ case configKeyDef(key) =>
+ SetCommand(Some(key -> None))
+ case s if s == "-v" =>
+ SetCommand(Some("-v" -> None))
+ case s if s.isEmpty =>
+ SetCommand(None)
+ case _ => throw new ParseException("Expected format is 'SET', 'SET key', or " +
+ "'SET key=value'. If you want to include special characters in key, " +
+ "please use quotes, e.g., SET `ke y`=value.", ctx)
+ }
+ }
+
+ override def visitSetQuotedConfiguration(ctx: SetQuotedConfigurationContext)
+ : LogicalPlan = withOrigin(ctx) {
+ val keyStr = ctx.key.getText.replaceAll("`", "")
Review comment:
Yea, I tried (the two cases below) before, but it didn't work, too...
```
- val keyStr = ctx.key.getText.replaceAll("`", "")
+ val keyStr = visitQuotedIdentifier(ctx.key).toString
- val keyStr = ctx.key.getText.replaceAll("`", "")
+ val keyStr = visitQuotedIdentifier(ctx.quotedIdentifier()).toString
```
----------------------------------------------------------------
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.
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] maropu commented on a change in pull request #29146: [WIP][SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
maropu commented on a change in pull request #29146:
URL: https://github.com/apache/spark/pull/29146#discussion_r462610033
##########
File path: sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4
##########
@@ -244,11 +258,31 @@ statement
| SET TIME ZONE interval #setTimeZone
| SET TIME ZONE timezone=(STRING | LOCAL) #setTimeZone
| SET TIME ZONE .*? #setTimeZone
+ | SET configKey (EQ configValue)? #setConfiguration
| SET .*? #setConfiguration
+ | RESET configKey? #resetConfiguration
| RESET .*? #resetConfiguration
| unsupportedHiveNativeCommands .*? #failNativeCommand
;
+configKey
+ : configIdentifier (('.' | ':') configIdentifier
+ // The two semantic predicates make sure that no token on the hidden channel
+ // (e.g., spaces) exists between the config separator ('.' or ':').
+ {!isHidden(-3)}? {!isHidden(-2)}?)*
+ | quotedIdentifier
+ ;
+
+configValue
+ : MINUS? vaule=.*?
Review comment:
Yea, this is one of the last few things I need to check...I'm still looking into why. Please let me know if you know something about this behaivour.
##########
File path: sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4
##########
@@ -244,11 +258,31 @@ statement
| SET TIME ZONE interval #setTimeZone
| SET TIME ZONE timezone=(STRING | LOCAL) #setTimeZone
| SET TIME ZONE .*? #setTimeZone
+ | SET configKey (EQ configValue)? #setConfiguration
| SET .*? #setConfiguration
+ | RESET configKey? #resetConfiguration
| RESET .*? #resetConfiguration
| unsupportedHiveNativeCommands .*? #failNativeCommand
;
+configKey
+ : configIdentifier (('.' | ':') configIdentifier
+ // The two semantic predicates make sure that no token on the hidden channel
+ // (e.g., spaces) exists between the config separator ('.' or ':').
+ {!isHidden(-3)}? {!isHidden(-2)}?)*
+ | quotedIdentifier
+ ;
+
+configValue
+ : MINUS? vaule=.*?
Review comment:
Yea, this is one of the last few things I need to check. I also think we don't need `MINUS` in this definition, but it seems it does not work without it (If we remove `MINUS`, the tests (https://github.com/apache/spark/pull/29146/files#diff-e9f35fff083788e6494cb1220a792b99R82-R83) will fail) I'm still looking into why. Please let me know if you know something about this behaivour.
##########
File path: sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4
##########
@@ -244,11 +258,31 @@ statement
| SET TIME ZONE interval #setTimeZone
| SET TIME ZONE timezone=(STRING | LOCAL) #setTimeZone
| SET TIME ZONE .*? #setTimeZone
+ | SET configKey (EQ configValue)? #setConfiguration
| SET .*? #setConfiguration
+ | RESET configKey? #resetConfiguration
| RESET .*? #resetConfiguration
| unsupportedHiveNativeCommands .*? #failNativeCommand
;
+configKey
+ : configIdentifier (('.' | ':') configIdentifier
+ // The two semantic predicates make sure that no token on the hidden channel
+ // (e.g., spaces) exists between the config separator ('.' or ':').
+ {!isHidden(-3)}? {!isHidden(-2)}?)*
+ | quotedIdentifier
+ ;
+
+configValue
+ : MINUS? vaule=.*?
Review comment:
~Yea, this is one of the last few things I need to check. I also think we don't need `MINUS` in this definition, but it seems it does not work without it (If we remove `MINUS`, the tests (https://github.com/apache/spark/pull/29146/files#diff-e9f35fff083788e6494cb1220a792b99R82-R83) will fail) I'm still looking into why. Please let me know if you know something about this behaivour. ~
Ur, wait. My bad. Seems like we could remove this. I'm checking now.
##########
File path: sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4
##########
@@ -244,11 +258,31 @@ statement
| SET TIME ZONE interval #setTimeZone
| SET TIME ZONE timezone=(STRING | LOCAL) #setTimeZone
| SET TIME ZONE .*? #setTimeZone
+ | SET configKey (EQ configValue)? #setConfiguration
| SET .*? #setConfiguration
+ | RESET configKey? #resetConfiguration
| RESET .*? #resetConfiguration
| unsupportedHiveNativeCommands .*? #failNativeCommand
;
+configKey
+ : configIdentifier (('.' | ':') configIdentifier
+ // The two semantic predicates make sure that no token on the hidden channel
+ // (e.g., spaces) exists between the config separator ('.' or ':').
+ {!isHidden(-3)}? {!isHidden(-2)}?)*
+ | quotedIdentifier
+ ;
+
+configValue
+ : MINUS? vaule=.*?
Review comment:
~Yea, this is one of the last few things I need to check. I also think we don't need `MINUS` in this definition, but it seems it does not work without it (If we remove `MINUS`, the tests (https://github.com/apache/spark/pull/29146/files#diff-e9f35fff083788e6494cb1220a792b99R82-R83) will fail) I'm still looking into why. Please let me know if you know something about this behaivour.~
Ur, wait. My bad. Seems like we could remove this. I'm checking now.
##########
File path: sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4
##########
@@ -244,11 +258,31 @@ statement
| SET TIME ZONE interval #setTimeZone
| SET TIME ZONE timezone=(STRING | LOCAL) #setTimeZone
| SET TIME ZONE .*? #setTimeZone
+ | SET configKey (EQ configValue)? #setConfiguration
| SET .*? #setConfiguration
+ | RESET configKey? #resetConfiguration
| RESET .*? #resetConfiguration
| unsupportedHiveNativeCommands .*? #failNativeCommand
;
+configKey
+ : configIdentifier (('.' | ':') configIdentifier
+ // The two semantic predicates make sure that no token on the hidden channel
+ // (e.g., spaces) exists between the config separator ('.' or ':').
+ {!isHidden(-3)}? {!isHidden(-2)}?)*
+ | quotedIdentifier
+ ;
+
+configValue
+ : MINUS? vaule=.*?
Review comment:
```
| SET configKey (EQ value=.*)? #setConfiguration
```
This will lead to a failure below;
```
[info] - Checks if SET/RESET can accept all the configurations *** FAILED *** (6 seconds, 11 milliseconds)
[info] == FAIL: Plans do not match ===
[info] !SetCommand (spark.sql.avro.deflate.level,Some(1)) SetCommand (spark.sql.avro.deflate.level,Some(-1)) (PlanTest.scala:157)
[info] org.scalatest.exceptions.TestFailedException:
```
But, if we change it like this, it works well...;
```
| RESET configKey? #resetConfiguration
configValue
: vaule=.*?
;
```
##########
File path: sql/core/src/test/scala/org/apache/spark/sql/internal/SQLConfEntrySuite.scala
##########
@@ -107,7 +107,7 @@ class SQLConfEntrySuite extends SparkFunSuite {
test("stringConf") {
val key = "spark.sql.SQLConfEntrySuite.string"
- val confEntry = buildConf(key).stringConf.createWithDefault(null)
+ val confEntry = buildConf(key).stringConf.createWithDefault("")
Review comment:
I update this because;
```
org.apache.spark.sql.execution.SparkSqlParserSuite.Checks if SET/RESET can accept all the configurations
Error Message
org.scalatest.exceptions.TestFailedException: == FAIL: Plans do not match === SetCommand (spark.sql.SQLConfEntrySuite.string,Some(null)) SetCommand (spark.sql.SQLConfEntrySuite.string,Some(null))
```
https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/126793/testReport/org.apache.spark.sql.execution/SparkSqlParserSuite/Checks_if_SET_RESET_can_accept_all_the_configurations/
##########
File path: sql/core/src/test/scala/org/apache/spark/sql/SQLQuerySuite.scala
##########
@@ -962,8 +962,8 @@ class SQLQuerySuite extends QueryTest with SharedSparkSession with AdaptiveSpark
test("SET commands semantics using sql()") {
spark.sessionState.conf.clear()
- val testKey = "test.key.0"
- val testVal = "test.val.0"
+ val testKey = "test.key.k0"
Review comment:
sure.
##########
File path: sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4
##########
@@ -244,11 +258,31 @@ statement
| SET TIME ZONE interval #setTimeZone
| SET TIME ZONE timezone=(STRING | LOCAL) #setTimeZone
| SET TIME ZONE .*? #setTimeZone
+ | SET configKey (EQ configValue)? #setConfiguration
| SET .*? #setConfiguration
+ | RESET configKey? #resetConfiguration
| RESET .*? #resetConfiguration
| unsupportedHiveNativeCommands .*? #failNativeCommand
;
+configKey
+ : configIdentifier (('.' | ':') configIdentifier
+ // The two semantic predicates make sure that no token on the hidden channel
+ // (e.g., spaces) exists between the config separator ('.' or ':').
+ {!isHidden(-3)}? {!isHidden(-2)}?)*
+ | quotedIdentifier
+ ;
+
+configValue
+ : vaule=.*?
Review comment:
okay, I'll check if its 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.
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] SparkQA commented on pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-666841271
**[Test build #126817 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/126817/testReport)** for PR 29146 at commit [`6d2221d`](https://github.com/apache/spark/commit/6d2221daadb7e7934982ed4be8d1700c59c90ad0).
* This patch **fails Spark unit tests**.
* This patch merges cleanly.
* This patch adds no public 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.
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] maropu commented on a change in pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
maropu commented on a change in pull request #29146:
URL: https://github.com/apache/spark/pull/29146#discussion_r462883994
##########
File path: sql/core/src/test/scala/org/apache/spark/sql/execution/SparkSqlParserSuite.scala
##########
@@ -61,6 +63,64 @@ class SparkSqlParserSuite extends AnalysisTest {
private def intercept(sqlCommand: String, messages: String*): Unit =
interceptParseException(parser.parsePlan)(sqlCommand, messages: _*)
+ test("Checks if SET/RESET can accept all the configurations") {
+ // Force to build static SQL configurations
Review comment:
Added tests for the Spark confs, too.
----------------------------------------------------------------
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.
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] SparkQA removed a comment on pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-667941440
**[Test build #126976 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/126976/testReport)** for PR 29146 at commit [`3065ed9`](https://github.com/apache/spark/commit/3065ed9aa5ce232ce4f16335cbf830bd93a73f3f).
----------------------------------------------------------------
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.
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] AmplabJenkins removed a comment on pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-667916079
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/126963/
Test FAILed.
----------------------------------------------------------------
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.
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] maropu commented on pull request #29146: [WIP][SPARK-32257][SQL] Reports explicit errors for invalid usage of SET command
Posted by GitBox <gi...@apache.org>.
maropu commented on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-660756208
Looks all the tests are passed in Github Actions.
----------------------------------------------------------------
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.
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] maropu commented on a change in pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
maropu commented on a change in pull request #29146:
URL: https://github.com/apache/spark/pull/29146#discussion_r462884139
##########
File path: sql/core/src/test/scala/org/apache/spark/sql/execution/SparkSqlParserSuite.scala
##########
@@ -61,6 +63,64 @@ class SparkSqlParserSuite extends AnalysisTest {
private def intercept(sqlCommand: String, messages: String*): Unit =
interceptParseException(parser.parsePlan)(sqlCommand, messages: _*)
+ test("Checks if SET/RESET can accept all the configurations") {
+ // Force to build static SQL configurations
+ StaticSQLConf
+ SQLConf.sqlConfEntries.values.asScala.foreach { e =>
+ assertEqual(s"SET ${e.key}", SetCommand(Some(e.key -> None)))
+ if (e.defaultValue.isDefined) {
+ assertEqual(s"SET ${e.key}=${e.defaultValueString}",
+ SetCommand(Some(e.key -> Some(e.defaultValueString))))
+ }
+ assertEqual(s"RESET ${e.key}", ResetCommand(Some(e.key)))
+ }
+ }
+
+ test("Report Error for invalid usage of SET command") {
+ assertEqual("SET", SetCommand(None))
+ assertEqual("SET -v", SetCommand(Some("-v", None)))
+ assertEqual("SET spark.sql.key", SetCommand(Some("spark.sql.key" -> None)))
+ assertEqual("SET spark:sql:key=false", SetCommand(Some("spark:sql:key" -> Some("false"))))
+ assertEqual("SET spark:sql:key=", SetCommand(Some("spark:sql:key" -> Some(""))))
+ assertEqual("SET spark:sql:key= ", SetCommand(Some("spark:sql:key" -> Some(""))))
+ assertEqual("SET spark:sql:key=-1 ", SetCommand(Some("spark:sql:key" -> Some("-1"))))
+ assertEqual("SET spark:sql:key = -1", SetCommand(Some("spark:sql:key" -> Some("-1"))))
+ assertEqual("SET `spark.sql. key`=value",
+ SetCommand(Some("spark.sql. key" -> Some("value"))))
+
+ val expectedErrMsg = "Expected format is 'SET', 'SET key', or " +
+ "'SET key=value'. If you want to include special characters in key, " +
+ "please use quotes, e.g., SET `ke y`=value."
+ intercept("SET spark.sql.key value", expectedErrMsg)
+ intercept("SET spark.sql.key 'value'", expectedErrMsg)
+ intercept("SET spark.sql.key \"value\" ", expectedErrMsg)
+ intercept("SET spark.sql.key value1 value2", expectedErrMsg)
+
+ // TODO: Needs to make the error message more meaningful for users
+ intercept("SET spark.sql. key=value", "failed predicate")
+ intercept("SET spark.sql :key=value", "failed predicate")
+ intercept("SET spark.sql . key=value", "failed predicate")
+ }
+
+ test("Report Error for invalid usage of RESET command") {
+ assertEqual("RESET", ResetCommand(None))
+ assertEqual("RESET spark.sql.key", ResetCommand(Some("spark.sql.key")))
+ assertEqual("RESET spark.sql.key ", ResetCommand(Some("spark.sql.key")))
+ assertEqual("RESET `spark.sql. key`", ResetCommand(Some("spark.sql. key")))
+
+ val expectedErrMsg = "Expected format is 'RESET' or 'RESET key'. " +
+ "If you want to include special characters in key, " +
+ "please use quotes, e.g., RESET `ke y`."
+ intercept("RESET spark.sql.key1 key2", expectedErrMsg)
+ intercept("RESET spark. sql.key1 key2", expectedErrMsg)
+ intercept("RESET spark.sql.key1 key2 key3", expectedErrMsg)
+
+ // TODO: Needs to make the error message more meaningful for users
+ intercept("RESET spark.sql: key", "failed predicate")
Review comment:
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.
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] AmplabJenkins commented on pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-667642470
----------------------------------------------------------------
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.
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] SparkQA commented on pull request #29146: [WIP][SPARK-32257][SQL] Reports explicit errors for invalid usage of SET command
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-660509730
**[Test build #126108 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/126108/testReport)** for PR 29146 at commit [`2e92675`](https://github.com/apache/spark/commit/2e9267562c37f46e303544ec55f5881e205ed8e8).
* This patch **fails PySpark pip packaging tests**.
* This patch merges cleanly.
* This patch adds no public 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.
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] maropu commented on a change in pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
maropu commented on a change in pull request #29146:
URL: https://github.com/apache/spark/pull/29146#discussion_r464243896
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/execution/SparkSqlParser.scala
##########
@@ -66,17 +68,29 @@ class SparkSqlAstBuilder(conf: SQLConf) extends AstBuilder(conf) {
* character in the raw string.
*/
override def visitSetConfiguration(ctx: SetConfigurationContext): LogicalPlan = withOrigin(ctx) {
- // Construct the command.
- val raw = remainder(ctx.SET.getSymbol)
- val keyValueSeparatorIndex = raw.indexOf('=')
- if (keyValueSeparatorIndex >= 0) {
- val key = raw.substring(0, keyValueSeparatorIndex).trim
- val value = raw.substring(keyValueSeparatorIndex + 1).trim
- SetCommand(Some(key -> Option(value)))
- } else if (raw.nonEmpty) {
- SetCommand(Some(raw.trim -> None))
+ val configKeyValueDef = """([a-zA-Z_\d\\.:]+)\s*=(.*)""".r
+ remainder(ctx.SET.getSymbol).trim match {
+ case configKeyValueDef(key, value) =>
+ SetCommand(Some(key -> Option(value.trim)))
+ case configKeyDef(key) =>
+ SetCommand(Some(key -> None))
+ case s if s == "-v" =>
+ SetCommand(Some("-v" -> None))
+ case s if s.isEmpty =>
+ SetCommand(None)
+ case _ => throw new ParseException("Expected format is 'SET', 'SET key', or " +
+ "'SET key=value'. If you want to include special characters in key, " +
+ "please use quotes, e.g., SET `ke y`=value.", ctx)
+ }
+ }
+
+ override def visitSetQuotedConfiguration(ctx: SetQuotedConfigurationContext)
+ : LogicalPlan = withOrigin(ctx) {
+ val keyStr = ctx.quotedConfigKey().getText
+ if (ctx.value != null) {
+ SetCommand(Some(keyStr -> Option(remainder(ctx.EQ().getSymbol).trim)))
Review comment:
I added `.trim` and checked that the same tests failed. To tell the truth, I'm not sure about why the rule `(EQ value=.*)` parses `v a lu e` into `e` (Probably, this is the effect of the hidden channel?);
```
[info] !SetCommand (spark.sql. key,Some(e)) SetCommand (spark.sql. key,Some(v a lu e)) (PlanTest.scala:157)
```
----------------------------------------------------------------
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.
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] AmplabJenkins removed a comment on pull request #29146: [WIP][SPARK-32257][SQL] Reports explicit errors for invalid usage of SET command
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-660509893
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/126108/
Test FAILed.
----------------------------------------------------------------
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.
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] cloud-fan commented on a change in pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #29146:
URL: https://github.com/apache/spark/pull/29146#discussion_r464172446
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/execution/SparkSqlParser.scala
##########
@@ -66,17 +68,29 @@ class SparkSqlAstBuilder(conf: SQLConf) extends AstBuilder(conf) {
* character in the raw string.
*/
override def visitSetConfiguration(ctx: SetConfigurationContext): LogicalPlan = withOrigin(ctx) {
- // Construct the command.
- val raw = remainder(ctx.SET.getSymbol)
- val keyValueSeparatorIndex = raw.indexOf('=')
- if (keyValueSeparatorIndex >= 0) {
- val key = raw.substring(0, keyValueSeparatorIndex).trim
- val value = raw.substring(keyValueSeparatorIndex + 1).trim
- SetCommand(Some(key -> Option(value)))
- } else if (raw.nonEmpty) {
- SetCommand(Some(raw.trim -> None))
+ val configKeyValueDef = """([a-zA-Z_\d\\.:]+)\s*=(.*)""".r
+ remainder(ctx.SET.getSymbol).trim match {
+ case configKeyValueDef(key, value) =>
+ SetCommand(Some(key -> Option(value.trim)))
+ case configKeyDef(key) =>
+ SetCommand(Some(key -> None))
+ case s if s == "-v" =>
+ SetCommand(Some("-v" -> None))
+ case s if s.isEmpty =>
+ SetCommand(None)
+ case _ => throw new ParseException("Expected format is 'SET', 'SET key', or " +
+ "'SET key=value'. If you want to include special characters in key, " +
+ "please use quotes, e.g., SET `ke y`=value.", ctx)
+ }
+ }
+
+ override def visitSetQuotedConfiguration(ctx: SetQuotedConfigurationContext)
+ : LogicalPlan = withOrigin(ctx) {
+ val keyStr = ctx.quotedConfigKey().getText
+ if (ctx.value != null) {
+ SetCommand(Some(keyStr -> Option(remainder(ctx.EQ().getSymbol).trim)))
Review comment:
`(EQ value=.*)` we have an alias, can we use 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.
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] AmplabJenkins commented on pull request #29146: [WIP][SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-665635179
----------------------------------------------------------------
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.
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] maropu commented on a change in pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
maropu commented on a change in pull request #29146:
URL: https://github.com/apache/spark/pull/29146#discussion_r463377564
##########
File path: sql/core/src/test/scala/org/apache/spark/sql/SQLQuerySuite.scala
##########
@@ -962,8 +962,8 @@ class SQLQuerySuite extends QueryTest with SharedSparkSession with AdaptiveSpark
test("SET commands semantics using sql()") {
spark.sessionState.conf.clear()
- val testKey = "test.key.0"
- val testVal = "test.val.0"
+ val testKey = "test.key.k0"
Review comment:
hm, okay. I'll look into this more.
----------------------------------------------------------------
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.
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] SparkQA removed a comment on pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-667032219
**[Test build #126875 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/126875/testReport)** for PR 29146 at commit [`6d2221d`](https://github.com/apache/spark/commit/6d2221daadb7e7934982ed4be8d1700c59c90ad0).
----------------------------------------------------------------
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.
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] AmplabJenkins commented on pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-666231820
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/126802/
Test FAILed.
----------------------------------------------------------------
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.
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] SparkQA commented on pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-666942100
**[Test build #126851 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/126851/testReport)** for PR 29146 at commit [`6d2221d`](https://github.com/apache/spark/commit/6d2221daadb7e7934982ed4be8d1700c59c90ad0).
----------------------------------------------------------------
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.
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] AmplabJenkins removed a comment on pull request #29146: [WIP][SPARK-32257][SQL] Reports explicit errors for invalid usage of SET command
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-660456580
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/126099/
Test FAILed.
----------------------------------------------------------------
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.
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] cloud-fan commented on pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-668008846
thanks, merging to 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.
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] maropu commented on pull request #29146: [WIP][SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
maropu commented on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-665467020
haha, I'm still checking them... just a sec.
----------------------------------------------------------------
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.
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] AmplabJenkins removed a comment on pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-667504398
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/126915/
Test FAILed.
----------------------------------------------------------------
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.
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] SparkQA commented on pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-667486182
**[Test build #126913 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/126913/testReport)** for PR 29146 at commit [`61f5ffc`](https://github.com/apache/spark/commit/61f5ffcb6a2d20e90ffa652aaa3c9ae14effa589).
----------------------------------------------------------------
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.
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] AmplabJenkins commented on pull request #29146: [WIP][SPARK-32257][SQL] Reports explicit errors for invalid usage of SET command
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-660509892
----------------------------------------------------------------
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.
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] HyukjinKwon commented on a change in pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on a change in pull request #29146:
URL: https://github.com/apache/spark/pull/29146#discussion_r461281062
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/execution/SparkSqlParser.scala
##########
@@ -65,16 +65,19 @@ class SparkSqlAstBuilder(conf: SQLConf) extends AstBuilder(conf) {
*/
override def visitSetConfiguration(ctx: SetConfigurationContext): LogicalPlan = withOrigin(ctx) {
// Construct the command.
- val raw = remainder(ctx.SET.getSymbol)
- val keyValueSeparatorIndex = raw.indexOf('=')
Review comment:
Yup, I am okay with that. I thought it'd be easier to use the same codes so we can just say `SET` expects the same syntax as specified in `spark-default.conf`. But using strings look more natural in SQL context. I am okay either way.
----------------------------------------------------------------
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.
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] AmplabJenkins removed a comment on pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-666231820
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/126802/
Test FAILed.
----------------------------------------------------------------
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.
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] AmplabJenkins removed a comment on pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-666649204
----------------------------------------------------------------
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.
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] maropu commented on a change in pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
maropu commented on a change in pull request #29146:
URL: https://github.com/apache/spark/pull/29146#discussion_r462888722
##########
File path: sql/core/src/test/scala/org/apache/spark/sql/SQLQuerySuite.scala
##########
@@ -962,8 +962,8 @@ class SQLQuerySuite extends QueryTest with SharedSparkSession with AdaptiveSpark
test("SET commands semantics using sql()") {
spark.sessionState.conf.clear()
- val testKey = "test.key.0"
- val testVal = "test.val.0"
+ val testKey = "test.key.k0"
Review comment:
I'm still looking into 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.
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] AmplabJenkins removed a comment on pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-664694827
----------------------------------------------------------------
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.
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] maropu commented on a change in pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
maropu commented on a change in pull request #29146:
URL: https://github.com/apache/spark/pull/29146#discussion_r463019812
##########
File path: sql/core/src/test/scala/org/apache/spark/sql/SQLQuerySuite.scala
##########
@@ -962,8 +962,8 @@ class SQLQuerySuite extends QueryTest with SharedSparkSession with AdaptiveSpark
test("SET commands semantics using sql()") {
spark.sessionState.conf.clear()
- val testKey = "test.key.0"
- val testVal = "test.val.0"
+ val testKey = "test.key.k0"
Review comment:
I tried it though, I found just adding `DIGIT+`([Actually, I added `INTEGER_VALUE` instead](https://github.com/apache/spark/pull/29146/files#diff-8c1cb2af4aa1109e08481dae79052cc3R280)) there did not work well...
For example, a pattern below was okay;
```
SET spark:sql:3=value or SET 1:2:key=value
```
https://github.com/apache/spark/pull/29146/files#diff-e9f35fff083788e6494cb1220a792b99R90-R92
But, a following pattern couldn't be parsed;
```
SET spark.sql.3=value or SET 1.2.key=value
```
https://github.com/apache/spark/pull/29146/files#diff-e9f35fff083788e6494cb1220a792b99R105-R106
That's because `.3` and `1.2` are regarded as the `DECIMAL_VALUE` tokens, so the `configKey` parsing rule cannot match them correctly. I looked for an approach to avoid it, but I haven't found a good idea so far.
Any idea for this? Anyway, I will keep looking for 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.
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] gatorsmile commented on a change in pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
gatorsmile commented on a change in pull request #29146:
URL: https://github.com/apache/spark/pull/29146#discussion_r462777127
##########
File path: sql/core/src/test/scala/org/apache/spark/sql/execution/SparkSqlParserSuite.scala
##########
@@ -61,6 +63,64 @@ class SparkSqlParserSuite extends AnalysisTest {
private def intercept(sqlCommand: String, messages: String*): Unit =
interceptParseException(parser.parsePlan)(sqlCommand, messages: _*)
+ test("Checks if SET/RESET can accept all the configurations") {
+ // Force to build static SQL configurations
Review comment:
How about spark conf ? Check this PR https://github.com/apache/spark/pull/23031/files
----------------------------------------------------------------
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.
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] maropu commented on pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET command
Posted by GitBox <gi...@apache.org>.
maropu commented on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-663822454
> If we don't allow space in the config name by default(requires quoting), I think we can do that for other special chars as well. Then the parser rule can be very simple:
>> SET (multiPartIdent | STRING | BACKQUOTED_IDENTIFIER) (EQ value=.*)?
Oh, it looks pretty smart. I'll update it based on that.
----------------------------------------------------------------
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.
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] AmplabJenkins commented on pull request #29146: [WIP][SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-665946574
----------------------------------------------------------------
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.
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] SparkQA commented on pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-666656415
**[Test build #126817 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/126817/testReport)** for PR 29146 at commit [`6d2221d`](https://github.com/apache/spark/commit/6d2221daadb7e7934982ed4be8d1700c59c90ad0).
----------------------------------------------------------------
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.
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] SparkQA commented on pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET command
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-660223481
**[Test build #126055 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/126055/testReport)** for PR 29146 at commit [`7f8907e`](https://github.com/apache/spark/commit/7f8907ee3ba2ee9e6c64b1734472ea05da00ddb2).
* This patch **fails Spark unit tests**.
* This patch merges cleanly.
* This patch adds no public 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.
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] AmplabJenkins removed a comment on pull request #29146: [WIP][SPARK-32257][SQL] Reports explicit errors for invalid usage of SET command
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-660446524
----------------------------------------------------------------
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.
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] SparkQA removed a comment on pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-667860844
**[Test build #126963 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/126963/testReport)** for PR 29146 at commit [`b0e9686`](https://github.com/apache/spark/commit/b0e9686e9c92045068d253e23137457e47a65f9f).
----------------------------------------------------------------
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.
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] maropu commented on pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
maropu commented on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-667655534
okay, ready to review. cc: @cloud-fan @viirya @HyukjinKwon
----------------------------------------------------------------
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.
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] AmplabJenkins commented on pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-666649204
----------------------------------------------------------------
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.
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] AmplabJenkins removed a comment on pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET command
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-663552241
----------------------------------------------------------------
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.
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] maropu commented on a change in pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
maropu commented on a change in pull request #29146:
URL: https://github.com/apache/spark/pull/29146#discussion_r464231505
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/execution/SparkSqlParser.scala
##########
@@ -66,17 +68,29 @@ class SparkSqlAstBuilder(conf: SQLConf) extends AstBuilder(conf) {
* character in the raw string.
*/
override def visitSetConfiguration(ctx: SetConfigurationContext): LogicalPlan = withOrigin(ctx) {
- // Construct the command.
- val raw = remainder(ctx.SET.getSymbol)
- val keyValueSeparatorIndex = raw.indexOf('=')
- if (keyValueSeparatorIndex >= 0) {
- val key = raw.substring(0, keyValueSeparatorIndex).trim
- val value = raw.substring(keyValueSeparatorIndex + 1).trim
- SetCommand(Some(key -> Option(value)))
- } else if (raw.nonEmpty) {
- SetCommand(Some(raw.trim -> None))
+ val configKeyValueDef = """([a-zA-Z_\d\\.:]+)\s*=(.*)""".r
Review comment:
Ah, right.
----------------------------------------------------------------
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.
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] SparkQA removed a comment on pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET command
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-660152612
**[Test build #126055 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/126055/testReport)** for PR 29146 at commit [`7f8907e`](https://github.com/apache/spark/commit/7f8907ee3ba2ee9e6c64b1734472ea05da00ddb2).
----------------------------------------------------------------
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.
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] SparkQA commented on pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-667914955
**[Test build #126963 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/126963/testReport)** for PR 29146 at commit [`b0e9686`](https://github.com/apache/spark/commit/b0e9686e9c92045068d253e23137457e47a65f9f).
* This patch **fails Spark unit tests**.
* This patch merges cleanly.
* This patch adds no public 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.
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] cloud-fan commented on a change in pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #29146:
URL: https://github.com/apache/spark/pull/29146#discussion_r464172133
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/execution/SparkSqlParser.scala
##########
@@ -66,17 +68,29 @@ class SparkSqlAstBuilder(conf: SQLConf) extends AstBuilder(conf) {
* character in the raw string.
*/
override def visitSetConfiguration(ctx: SetConfigurationContext): LogicalPlan = withOrigin(ctx) {
- // Construct the command.
- val raw = remainder(ctx.SET.getSymbol)
- val keyValueSeparatorIndex = raw.indexOf('=')
- if (keyValueSeparatorIndex >= 0) {
- val key = raw.substring(0, keyValueSeparatorIndex).trim
- val value = raw.substring(keyValueSeparatorIndex + 1).trim
- SetCommand(Some(key -> Option(value)))
- } else if (raw.nonEmpty) {
- SetCommand(Some(raw.trim -> None))
+ val configKeyValueDef = """([a-zA-Z_\d\\.:]+)\s*=(.*)""".r
+ remainder(ctx.SET.getSymbol).trim match {
+ case configKeyValueDef(key, value) =>
+ SetCommand(Some(key -> Option(value.trim)))
+ case configKeyDef(key) =>
Review comment:
ah nvm, we will also match `configKeyValueDef` first.
----------------------------------------------------------------
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.
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] SparkQA commented on pull request #29146: [WIP][SPARK-32257][SQL] Reports explicit errors for invalid usage of SET command
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-660456505
**[Test build #126099 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/126099/testReport)** for PR 29146 at commit [`7e0d97a`](https://github.com/apache/spark/commit/7e0d97ac6612d37ede14e92f14673cf8de5c3c56).
* This patch **fails Spark unit tests**.
* This patch merges cleanly.
* This patch adds no public 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.
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] AmplabJenkins removed a comment on pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-667937591
----------------------------------------------------------------
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.
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] cloud-fan commented on pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET command
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-663623599
BTW after https://github.com/apache/spark/pull/29202 , let's make sure SET and RESET are consistent after 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.
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] AmplabJenkins commented on pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET command
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-663552241
----------------------------------------------------------------
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.
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] AmplabJenkins commented on pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-667504394
----------------------------------------------------------------
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.
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] AmplabJenkins removed a comment on pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-667642470
----------------------------------------------------------------
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.
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] maropu commented on a change in pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
maropu commented on a change in pull request #29146:
URL: https://github.com/apache/spark/pull/29146#discussion_r464224591
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/execution/SparkSqlParser.scala
##########
@@ -66,17 +68,29 @@ class SparkSqlAstBuilder(conf: SQLConf) extends AstBuilder(conf) {
* character in the raw string.
*/
override def visitSetConfiguration(ctx: SetConfigurationContext): LogicalPlan = withOrigin(ctx) {
- // Construct the command.
- val raw = remainder(ctx.SET.getSymbol)
- val keyValueSeparatorIndex = raw.indexOf('=')
- if (keyValueSeparatorIndex >= 0) {
- val key = raw.substring(0, keyValueSeparatorIndex).trim
- val value = raw.substring(keyValueSeparatorIndex + 1).trim
- SetCommand(Some(key -> Option(value)))
- } else if (raw.nonEmpty) {
- SetCommand(Some(raw.trim -> None))
+ val configKeyValueDef = """([a-zA-Z_\d\\.:]+)\s*=(.*)""".r
+ remainder(ctx.SET.getSymbol).trim match {
+ case configKeyValueDef(key, value) =>
+ SetCommand(Some(key -> Option(value.trim)))
+ case configKeyDef(key) =>
+ SetCommand(Some(key -> None))
+ case s if s == "-v" =>
+ SetCommand(Some("-v" -> None))
+ case s if s.isEmpty =>
+ SetCommand(None)
+ case _ => throw new ParseException("Expected format is 'SET', 'SET key', or " +
+ "'SET key=value'. If you want to include special characters in key, " +
+ "please use quotes, e.g., SET `ke y`=value.", ctx)
+ }
+ }
+
+ override def visitSetQuotedConfiguration(ctx: SetQuotedConfigurationContext)
+ : LogicalPlan = withOrigin(ctx) {
+ val keyStr = ctx.quotedConfigKey().getText
+ if (ctx.value != null) {
+ SetCommand(Some(keyStr -> Option(remainder(ctx.EQ().getSymbol).trim)))
Review comment:
I did so first, but the added tests below failed;
```
val keyStr = ctx.quotedConfigKey().getText
if (ctx.value != null) {
- SetCommand(Some(keyStr -> Option(remainder(ctx.EQ().getSymbol).trim)))
+ SetCommand(Some(keyStr -> Option(ctx.value.getText)))
} else {
SetCommand(Some(keyStr -> None))
}
// The failed tests
assertEqual("SET `spark.sql. key`= -1",
SetCommand(Some("spark.sql. key" -> Some("-1"))))
[info] - Report Error for invalid usage of SET command *** FAILED *** (104 milliseconds)
[info] == FAIL: Plans do not match ===
[info] !SetCommand (spark.sql. key,Some(1)) SetCommand (spark.sql. key,Some(-1)) (PlanTest.scala:157)
...
assertEqual("SET `spark.sql. key`= v a lu e ",
SetCommand(Some("spark.sql. key" -> Some("v a lu e"))))
[info] - Report Error for invalid usage of SET command *** FAILED *** (121 milliseconds)
[info] == FAIL: Plans do not match ===
[info] !SetCommand (spark.sql. key,Some(e)) SetCommand (spark.sql. key,Some(v a lu e)) (PlanTest.scala:157)
[info] org.scalatest.exceptions.TestFailedException:
...
```
So, I selected the current one to avoid the test failures. Do you know something about this behaivour?
----------------------------------------------------------------
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.
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] maropu commented on a change in pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
maropu commented on a change in pull request #29146:
URL: https://github.com/apache/spark/pull/29146#discussion_r464224591
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/execution/SparkSqlParser.scala
##########
@@ -66,17 +68,29 @@ class SparkSqlAstBuilder(conf: SQLConf) extends AstBuilder(conf) {
* character in the raw string.
*/
override def visitSetConfiguration(ctx: SetConfigurationContext): LogicalPlan = withOrigin(ctx) {
- // Construct the command.
- val raw = remainder(ctx.SET.getSymbol)
- val keyValueSeparatorIndex = raw.indexOf('=')
- if (keyValueSeparatorIndex >= 0) {
- val key = raw.substring(0, keyValueSeparatorIndex).trim
- val value = raw.substring(keyValueSeparatorIndex + 1).trim
- SetCommand(Some(key -> Option(value)))
- } else if (raw.nonEmpty) {
- SetCommand(Some(raw.trim -> None))
+ val configKeyValueDef = """([a-zA-Z_\d\\.:]+)\s*=(.*)""".r
+ remainder(ctx.SET.getSymbol).trim match {
+ case configKeyValueDef(key, value) =>
+ SetCommand(Some(key -> Option(value.trim)))
+ case configKeyDef(key) =>
+ SetCommand(Some(key -> None))
+ case s if s == "-v" =>
+ SetCommand(Some("-v" -> None))
+ case s if s.isEmpty =>
+ SetCommand(None)
+ case _ => throw new ParseException("Expected format is 'SET', 'SET key', or " +
+ "'SET key=value'. If you want to include special characters in key, " +
+ "please use quotes, e.g., SET `ke y`=value.", ctx)
+ }
+ }
+
+ override def visitSetQuotedConfiguration(ctx: SetQuotedConfigurationContext)
+ : LogicalPlan = withOrigin(ctx) {
+ val keyStr = ctx.quotedConfigKey().getText
+ if (ctx.value != null) {
+ SetCommand(Some(keyStr -> Option(remainder(ctx.EQ().getSymbol).trim)))
Review comment:
I did so first, but the added test below failed;
```
val keyStr = ctx.quotedConfigKey().getText
if (ctx.value != null) {
- SetCommand(Some(keyStr -> Option(remainder(ctx.EQ().getSymbol).trim)))
+ SetCommand(Some(keyStr -> Option(ctx.value.getText)))
} else {
SetCommand(Some(keyStr -> None))
}
// The failed tests
assertEqual("SET `spark.sql. key`= -1",
SetCommand(Some("spark.sql. key" -> Some("-1"))))
[info] - Report Error for invalid usage of SET command *** FAILED *** (104 milliseconds)
[info] == FAIL: Plans do not match ===
[info] !SetCommand (spark.sql. key,Some(1)) SetCommand (spark.sql. key,Some(-1)) (PlanTest.scala:157)
...
```
So, I selected the current one to avoid the test failure. Do you know something about this behaivour?
----------------------------------------------------------------
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.
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] HyukjinKwon commented on pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-667004049
retest this 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.
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] SparkQA removed a comment on pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-666942100
**[Test build #126851 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/126851/testReport)** for PR 29146 at commit [`6d2221d`](https://github.com/apache/spark/commit/6d2221daadb7e7934982ed4be8d1700c59c90ad0).
----------------------------------------------------------------
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.
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] AmplabJenkins removed a comment on pull request #29146: [WIP][SPARK-32257][SQL] Reports explicit errors for invalid usage of SET command
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-660471791
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/126104/
Test FAILed.
----------------------------------------------------------------
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.
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] AmplabJenkins commented on pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-667655237
----------------------------------------------------------------
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.
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] cloud-fan edited a comment on pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET command
Posted by GitBox <gi...@apache.org>.
cloud-fan edited a comment on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-663569282
If we don't allow space in the config name by default(requires quoting), I think we can do that for other special chars as well. Then the parser rule can be very simple:
```
SET (multiPartIdent | STRING | BACKQUOTED_IDENTIFIER) (EQ value=.+)?
```
----------------------------------------------------------------
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.
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] SparkQA commented on pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-666967789
**[Test build #126851 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/126851/testReport)** for PR 29146 at commit [`6d2221d`](https://github.com/apache/spark/commit/6d2221daadb7e7934982ed4be8d1700c59c90ad0).
* This patch **fails due to an unknown error code, -9**.
* This patch merges cleanly.
* This patch adds no public 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.
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] AmplabJenkins commented on pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-667937591
----------------------------------------------------------------
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.
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] AmplabJenkins removed a comment on pull request #29146: [WIP][SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-665946574
----------------------------------------------------------------
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.
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] SparkQA commented on pull request #29146: [WIP][SPARK-32257][SQL] Reports explicit errors for invalid usage of SET command
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-660475380
**[Test build #126108 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/126108/testReport)** for PR 29146 at commit [`2e92675`](https://github.com/apache/spark/commit/2e9267562c37f46e303544ec55f5881e205ed8e8).
----------------------------------------------------------------
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.
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] maropu commented on pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET command
Posted by GitBox <gi...@apache.org>.
maropu commented on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-660756650
cc: @cloud-fan @viirya
----------------------------------------------------------------
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.
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] AmplabJenkins removed a comment on pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-667655237
----------------------------------------------------------------
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.
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] SparkQA removed a comment on pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-665949787
----------------------------------------------------------------
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.
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] AmplabJenkins commented on pull request #29146: [WIP][SPARK-32257][SQL] Reports explicit errors for invalid usage of SET command
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-660461263
----------------------------------------------------------------
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.
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] cloud-fan commented on a change in pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #29146:
URL: https://github.com/apache/spark/pull/29146#discussion_r463106538
##########
File path: sql/core/src/test/scala/org/apache/spark/sql/SQLQuerySuite.scala
##########
@@ -962,8 +962,8 @@ class SQLQuerySuite extends QueryTest with SharedSparkSession with AdaptiveSpark
test("SET commands semantics using sql()") {
spark.sessionState.conf.clear()
- val testKey = "test.key.0"
- val testVal = "test.val.0"
+ val testKey = "test.key.k0"
Review comment:
damn this is really hard. We probably should simplify the parser rule
```
SET key=quotedIdentifier (EQ vaule=.*)? #simpleSetConfig
SET mixed=.* #mixedSetConfig
```
In the mixed case, we split the string by `=`, and make sure the first part(key) has no special chars.
----------------------------------------------------------------
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.
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] AmplabJenkins removed a comment on pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET command
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-660153281
----------------------------------------------------------------
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.
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] cloud-fan commented on a change in pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #29146:
URL: https://github.com/apache/spark/pull/29146#discussion_r464227993
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/execution/SparkSqlParser.scala
##########
@@ -66,17 +68,29 @@ class SparkSqlAstBuilder(conf: SQLConf) extends AstBuilder(conf) {
* character in the raw string.
*/
override def visitSetConfiguration(ctx: SetConfigurationContext): LogicalPlan = withOrigin(ctx) {
- // Construct the command.
- val raw = remainder(ctx.SET.getSymbol)
- val keyValueSeparatorIndex = raw.indexOf('=')
- if (keyValueSeparatorIndex >= 0) {
- val key = raw.substring(0, keyValueSeparatorIndex).trim
- val value = raw.substring(keyValueSeparatorIndex + 1).trim
- SetCommand(Some(key -> Option(value)))
- } else if (raw.nonEmpty) {
- SetCommand(Some(raw.trim -> None))
+ val configKeyValueDef = """([a-zA-Z_\d\\.:]+)\s*=(.*)""".r
+ remainder(ctx.SET.getSymbol).trim match {
+ case configKeyValueDef(key, value) =>
+ SetCommand(Some(key -> Option(value.trim)))
+ case configKeyDef(key) =>
+ SetCommand(Some(key -> None))
+ case s if s == "-v" =>
+ SetCommand(Some("-v" -> None))
+ case s if s.isEmpty =>
+ SetCommand(None)
+ case _ => throw new ParseException("Expected format is 'SET', 'SET key', or " +
+ "'SET key=value'. If you want to include special characters in key, " +
+ "please use quotes, e.g., SET `ke y`=value.", ctx)
+ }
+ }
+
+ override def visitSetQuotedConfiguration(ctx: SetQuotedConfigurationContext)
+ : LogicalPlan = withOrigin(ctx) {
+ val keyStr = ctx.quotedConfigKey().getText
+ if (ctx.value != null) {
+ SetCommand(Some(keyStr -> Option(remainder(ctx.EQ().getSymbol).trim)))
Review comment:
- SetCommand(Some(keyStr -> Option(remainder(ctx.EQ().getSymbol).trim)))
+ SetCommand(Some(keyStr -> Option(ctx.value.getText)))
one has trim but one doesn't. How about `ctx.value.getText.trim`?
----------------------------------------------------------------
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.
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] SparkQA removed a comment on pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-666656415
**[Test build #126817 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/126817/testReport)** for PR 29146 at commit [`6d2221d`](https://github.com/apache/spark/commit/6d2221daadb7e7934982ed4be8d1700c59c90ad0).
----------------------------------------------------------------
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.
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] cloud-fan commented on a change in pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #29146:
URL: https://github.com/apache/spark/pull/29146#discussion_r464172042
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/execution/SparkSqlParser.scala
##########
@@ -66,17 +68,29 @@ class SparkSqlAstBuilder(conf: SQLConf) extends AstBuilder(conf) {
* character in the raw string.
*/
override def visitSetConfiguration(ctx: SetConfigurationContext): LogicalPlan = withOrigin(ctx) {
- // Construct the command.
- val raw = remainder(ctx.SET.getSymbol)
- val keyValueSeparatorIndex = raw.indexOf('=')
- if (keyValueSeparatorIndex >= 0) {
- val key = raw.substring(0, keyValueSeparatorIndex).trim
- val value = raw.substring(keyValueSeparatorIndex + 1).trim
- SetCommand(Some(key -> Option(value)))
- } else if (raw.nonEmpty) {
- SetCommand(Some(raw.trim -> None))
+ val configKeyValueDef = """([a-zA-Z_\d\\.:]+)\s*=(.*)""".r
+ remainder(ctx.SET.getSymbol).trim match {
+ case configKeyValueDef(key, value) =>
+ SetCommand(Some(key -> Option(value.trim)))
+ case configKeyDef(key) =>
Review comment:
Will it match something like `a ###`? Shall we use `([a-zA-Z_\d\\.:]+)$`?
----------------------------------------------------------------
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.
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] SparkQA commented on pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-667032219
**[Test build #126875 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/126875/testReport)** for PR 29146 at commit [`6d2221d`](https://github.com/apache/spark/commit/6d2221daadb7e7934982ed4be8d1700c59c90ad0).
----------------------------------------------------------------
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.
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] SparkQA commented on pull request #29146: [WIP][SPARK-32257][SQL] Reports explicit errors for invalid usage of SET command
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-660461071
**[Test build #126104 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/126104/testReport)** for PR 29146 at commit [`0d357ae`](https://github.com/apache/spark/commit/0d357ae6539822f59aa4ae5ff34f5215f27666e4).
----------------------------------------------------------------
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.
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] AmplabJenkins commented on pull request #29146: [WIP][SPARK-32257][SQL] Reports explicit errors for invalid usage of SET command
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-660456576
----------------------------------------------------------------
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.
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] AmplabJenkins removed a comment on pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-667502824
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/126913/
Test FAILed.
----------------------------------------------------------------
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.
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] HyukjinKwon commented on a change in pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET command
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on a change in pull request #29146:
URL: https://github.com/apache/spark/pull/29146#discussion_r459953432
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/execution/SparkSqlParser.scala
##########
@@ -65,16 +65,19 @@ class SparkSqlAstBuilder(conf: SQLConf) extends AstBuilder(conf) {
*/
override def visitSetConfiguration(ctx: SetConfigurationContext): LogicalPlan = withOrigin(ctx) {
// Construct the command.
- val raw = remainder(ctx.SET.getSymbol)
- val keyValueSeparatorIndex = raw.indexOf('=')
Review comment:
Okay, so the backquote was added just in case users want to use space in the keys. @maropu, maybe it's a good idea to just reuse `Properties` which we use it when we read Spark configurations. For example:
```scala
import scala.collection.JavaConverters._
val p = new java.util.Properties()
p.load(new java.io.StringReader("a=b"))
p.asScala
```
when there's key only, it will be the empty string as its value. To use whitespaces, we could escape by `\` in `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.
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] AmplabJenkins removed a comment on pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-666845348
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/126817/
Test FAILed.
----------------------------------------------------------------
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.
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] AmplabJenkins commented on pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET command
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-663443702
----------------------------------------------------------------
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.
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] SparkQA commented on pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-667502627
**[Test build #126913 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/126913/testReport)** for PR 29146 at commit [`61f5ffc`](https://github.com/apache/spark/commit/61f5ffcb6a2d20e90ffa652aaa3c9ae14effa589).
* This patch **fails Spark unit tests**.
* This patch merges cleanly.
* This patch adds no public 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.
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] cloud-fan closed pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
cloud-fan closed pull request #29146:
URL: https://github.com/apache/spark/pull/29146
----------------------------------------------------------------
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.
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] AmplabJenkins removed a comment on pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-666883260
----------------------------------------------------------------
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.
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] SparkQA removed a comment on pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-667486182
**[Test build #126913 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/126913/testReport)** for PR 29146 at commit [`61f5ffc`](https://github.com/apache/spark/commit/61f5ffcb6a2d20e90ffa652aaa3c9ae14effa589).
----------------------------------------------------------------
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.
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] AmplabJenkins removed a comment on pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-666967998
Merged build finished. Test FAILed.
----------------------------------------------------------------
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.
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] AmplabJenkins commented on pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-666845339
----------------------------------------------------------------
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.
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] cloud-fan edited a comment on pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET command
Posted by GitBox <gi...@apache.org>.
cloud-fan edited a comment on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-663569282
If we don't allow space in the config name by default(requires quoting), I think we can do that for other special chars as well. Then the parser rule can be very simple:
```
SET (multiPartIdent | string) (EQ value=.*)?
```
----------------------------------------------------------------
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.
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] AmplabJenkins commented on pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-667916067
----------------------------------------------------------------
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.
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] AmplabJenkins commented on pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-667486327
----------------------------------------------------------------
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.
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] AmplabJenkins commented on pull request #29146: [WIP][SPARK-32257][SQL] Reports explicit errors for invalid usage of SET command
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-660475527
----------------------------------------------------------------
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.
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] AmplabJenkins commented on pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-668004107
----------------------------------------------------------------
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.
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] maropu commented on a change in pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
maropu commented on a change in pull request #29146:
URL: https://github.com/apache/spark/pull/29146#discussion_r462884875
##########
File path: sql/core/src/test/scala/org/apache/spark/sql/internal/SQLConfEntrySuite.scala
##########
@@ -107,7 +107,7 @@ class SQLConfEntrySuite extends SparkFunSuite {
test("stringConf") {
val key = "spark.sql.SQLConfEntrySuite.string"
- val confEntry = buildConf(key).stringConf.createWithDefault(null)
+ val confEntry = buildConf(key).stringConf.createWithDefault("")
Review comment:
haha, yea, the value in one side is just a string "null" and the value in the other side is actual NULL.
----------------------------------------------------------------
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.
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] SparkQA removed a comment on pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-667487265
**[Test build #126915 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/126915/testReport)** for PR 29146 at commit [`4822210`](https://github.com/apache/spark/commit/48222108242eb5b991705f16b926ea90c1293237).
----------------------------------------------------------------
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.
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] SparkQA commented on pull request #29146: [WIP][SPARK-32257][SQL] Reports explicit errors for invalid usage of SET command
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-660446426
**[Test build #126099 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/126099/testReport)** for PR 29146 at commit [`7e0d97a`](https://github.com/apache/spark/commit/7e0d97ac6612d37ede14e92f14673cf8de5c3c56).
----------------------------------------------------------------
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.
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] cloud-fan edited a comment on pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET command
Posted by GitBox <gi...@apache.org>.
cloud-fan edited a comment on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-663569282
If we don't allow space in the config name by default(requires quoting), I think we can do that for other special chars as well. Then the parser rule can be very simple:
```
SET (multiPartIdent | STRING | BACKQUOTED_IDENTIFIER) (EQ value=.*)?
```
----------------------------------------------------------------
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.
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] AmplabJenkins removed a comment on pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-666968004
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/126851/
Test FAILed.
----------------------------------------------------------------
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.
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] AmplabJenkins removed a comment on pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET command
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-663443702
----------------------------------------------------------------
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.
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] cloud-fan commented on a change in pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #29146:
URL: https://github.com/apache/spark/pull/29146#discussion_r463106538
##########
File path: sql/core/src/test/scala/org/apache/spark/sql/SQLQuerySuite.scala
##########
@@ -962,8 +962,8 @@ class SQLQuerySuite extends QueryTest with SharedSparkSession with AdaptiveSpark
test("SET commands semantics using sql()") {
spark.sessionState.conf.clear()
- val testKey = "test.key.0"
- val testVal = "test.val.0"
+ val testKey = "test.key.k0"
Review comment:
damn this is really hard. We probably should simplify the parser rule
```
SET key=quotedIdentifier (EQ vaule=.*)? #simpleSetConfig
SET mixed=.* #mixedSetConfig
```
In the mixed case, we split the string by `=`, and make sure the first part(key) has no special chars (probably just forbid space and `=`).
----------------------------------------------------------------
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.
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] maropu commented on a change in pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
maropu commented on a change in pull request #29146:
URL: https://github.com/apache/spark/pull/29146#discussion_r461230902
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/execution/SparkSqlParser.scala
##########
@@ -66,17 +69,21 @@ class SparkSqlAstBuilder(conf: SQLConf) extends AstBuilder(conf) {
* character in the raw string.
*/
override def visitSetConfiguration(ctx: SetConfigurationContext): LogicalPlan = withOrigin(ctx) {
- // Construct the command.
- val raw = remainder(ctx.SET.getSymbol)
- val keyValueSeparatorIndex = raw.indexOf('=')
- if (keyValueSeparatorIndex >= 0) {
- val key = raw.substring(0, keyValueSeparatorIndex).trim
- val value = raw.substring(keyValueSeparatorIndex + 1).trim
- SetCommand(Some(key -> Option(value)))
- } else if (raw.nonEmpty) {
- SetCommand(Some(raw.trim -> None))
+ if (ctx.configKey() != null) {
+ val keyStr = normalizeConfigString(ctx.configKey().getText)
+ if (ctx.configValue() != null) {
+ SetCommand(Some(keyStr -> Option(normalizeConfigString(ctx.configValue().getText))))
+ } else {
+ SetCommand(Some(keyStr -> None))
+ }
} else {
- SetCommand(None)
+ remainder(ctx.SET().getSymbol).trim match {
+ case "-v" => SetCommand(Some("-v" -> None))
+ case s if s.isEmpty() => SetCommand(None)
+ case _ => throw new ParseException("Expected format is 'SET', 'SET key', or " +
+ "'SET key=value'. If you want to include spaces in key and value, please use quotes, " +
+ "e.g., SET \"ke y\"=`va lu e`.", ctx)
Review comment:
Updated.
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/execution/SparkSqlParser.scala
##########
@@ -66,17 +69,21 @@ class SparkSqlAstBuilder(conf: SQLConf) extends AstBuilder(conf) {
* character in the raw string.
*/
override def visitSetConfiguration(ctx: SetConfigurationContext): LogicalPlan = withOrigin(ctx) {
- // Construct the command.
- val raw = remainder(ctx.SET.getSymbol)
- val keyValueSeparatorIndex = raw.indexOf('=')
- if (keyValueSeparatorIndex >= 0) {
- val key = raw.substring(0, keyValueSeparatorIndex).trim
- val value = raw.substring(keyValueSeparatorIndex + 1).trim
- SetCommand(Some(key -> Option(value)))
- } else if (raw.nonEmpty) {
- SetCommand(Some(raw.trim -> None))
+ if (ctx.configKey() != null) {
+ val keyStr = normalizeConfigString(ctx.configKey().getText)
Review comment:
No. At least, we don't have such an existing test case.
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/execution/SparkSqlParser.scala
##########
@@ -66,17 +69,21 @@ class SparkSqlAstBuilder(conf: SQLConf) extends AstBuilder(conf) {
* character in the raw string.
*/
override def visitSetConfiguration(ctx: SetConfigurationContext): LogicalPlan = withOrigin(ctx) {
- // Construct the command.
- val raw = remainder(ctx.SET.getSymbol)
- val keyValueSeparatorIndex = raw.indexOf('=')
- if (keyValueSeparatorIndex >= 0) {
- val key = raw.substring(0, keyValueSeparatorIndex).trim
- val value = raw.substring(keyValueSeparatorIndex + 1).trim
- SetCommand(Some(key -> Option(value)))
- } else if (raw.nonEmpty) {
- SetCommand(Some(raw.trim -> None))
+ if (ctx.configKey() != null) {
+ val keyStr = normalizeConfigString(ctx.configKey().getText)
+ if (ctx.configValue() != null) {
+ SetCommand(Some(keyStr -> Option(normalizeConfigString(ctx.configValue().getText))))
+ } else {
+ SetCommand(Some(keyStr -> None))
+ }
} else {
- SetCommand(None)
+ remainder(ctx.SET().getSymbol).trim match {
+ case "-v" => SetCommand(Some("-v" -> None))
+ case s if s.isEmpty() => SetCommand(None)
+ case _ => throw new ParseException("Expected format is 'SET', 'SET key', or " +
+ "'SET key=value'. If you want to include spaces in key and value, please use quotes, " +
+ "e.g., SET \"ke y\"=`va lu e`.", ctx)
Review comment:
Updated.
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/execution/SparkSqlParser.scala
##########
@@ -58,6 +58,9 @@ class SparkSqlParser(conf: SQLConf) extends AbstractSqlParser(conf) {
class SparkSqlAstBuilder(conf: SQLConf) extends AstBuilder(conf) {
import org.apache.spark.sql.catalyst.parser.ParserUtils._
+ private def normalizeConfigString(s: String) =
Review comment:
I looked into `tableIdentifier`, but I couldn't find why back-quotes are removed in the case. I replaced `BACKQUOTED_IDENTIFIER` with `quotedIdentifier` in [this commit](https://github.com/apache/spark/pull/29146/commits/08feee31c5d21aa75f9981efe59f583d46042cb3), then it seems back-quotes are removed by the ANTLR parser. Do you know something about that?
##########
File path: sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4
##########
@@ -244,11 +244,19 @@ statement
| SET TIME ZONE interval #setTimeZone
| SET TIME ZONE timezone=(STRING | LOCAL) #setTimeZone
| SET TIME ZONE .*? #setTimeZone
+ | SET configKey (EQ value=.+)? #setConfiguration
| SET .*? #setConfiguration
+ | RESET configKey? #resetConfiguration
| RESET .*? #resetConfiguration
| unsupportedHiveNativeCommands .*? #failNativeCommand
;
+configKey
+ : IDENTIFIER (('.' | ':') IDENTIFIER)*
Review comment:
Currently, this rule leads to some test failures;
```
sbt.ForkMain$ForkError: org.apache.spark.sql.catalyst.parser.ParseException:
Expected format is 'SET', 'SET key', or 'SET key=value'. If you want to include special characters in key and value, please use quotes or string literal, e.g., SET "ke y"=value.(line 1, pos 0)
== SQL ==
set hive.fetch.task.conversion=more
^^^
```
https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/126674/testReport/org.apache.spark.sql.hive.execution/HiveCompatibilitySuite/binary_constant/
This is because `IDENTIFIER` seems to be conflict with the existing ANTLR token, e.g., `fetch` in the example above. I'm currently looking for another smart rule to avoid it. Please let me know if someone has better idea..
##########
File path: sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4
##########
@@ -244,11 +244,19 @@ statement
| SET TIME ZONE interval #setTimeZone
| SET TIME ZONE timezone=(STRING | LOCAL) #setTimeZone
| SET TIME ZONE .*? #setTimeZone
+ | SET configKey (EQ value=.+)? #setConfiguration
| SET .*? #setConfiguration
+ | RESET configKey? #resetConfiguration
| RESET .*? #resetConfiguration
| unsupportedHiveNativeCommands .*? #failNativeCommand
;
+configKey
+ : IDENTIFIER (('.' | ':') IDENTIFIER)*
+ | quotedIdentifier
+ | STRING
Review comment:
ok, I also think the simpler rule looks better. I'll update it based on your suggestion.
##########
File path: sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4
##########
@@ -244,11 +244,19 @@ statement
| SET TIME ZONE interval #setTimeZone
| SET TIME ZONE timezone=(STRING | LOCAL) #setTimeZone
| SET TIME ZONE .*? #setTimeZone
+ | SET configKey (EQ value=.+)? #setConfiguration
| SET .*? #setConfiguration
+ | RESET configKey? #resetConfiguration
| RESET .*? #resetConfiguration
| unsupportedHiveNativeCommands .*? #failNativeCommand
;
+configKey
+ : IDENTIFIER (('.' | ':') IDENTIFIER)*
+ | quotedIdentifier
+ | STRING
Review comment:
ok, I think the simpler rule looks better. I'll update it based on your suggestion.
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/execution/SparkSqlParser.scala
##########
@@ -58,6 +58,9 @@ class SparkSqlParser(conf: SQLConf) extends AbstractSqlParser(conf) {
class SparkSqlAstBuilder(conf: SQLConf) extends AstBuilder(conf) {
import org.apache.spark.sql.catalyst.parser.ParserUtils._
+ private def normalizeConfigString(s: String) =
Review comment:
Oh, nice. I see...
##########
File path: sql/core/src/test/scala/org/apache/spark/sql/execution/SparkSqlParserSuite.scala
##########
@@ -61,6 +61,39 @@ class SparkSqlParserSuite extends AnalysisTest {
private def intercept(sqlCommand: String, messages: String*): Unit =
interceptParseException(parser.parsePlan)(sqlCommand, messages: _*)
+ test("Report Error for invalid usage of SET command") {
+ assertEqual("SET", SetCommand(None))
+ assertEqual("SET -v", SetCommand(Some("-v", None)))
+ assertEqual("SET spark.sql.key", SetCommand(Some("spark.sql.key" -> None)))
+ assertEqual("SET spark:sql:key=false", SetCommand(Some("spark:sql:key" -> Some("false"))))
+ assertEqual("SET spark.sql. key=value",
Review comment:
hm, I'll try. But, it seems we already accept similar patterns, e.g.,
```
scala> sql("select * from default.t").show()
+---+
| a|
+---+
| 4|
| 5|
| 1|
| 2|
| 3|
+---+
scala> sql("select * from default .t").show()
+---+
| a|
+---+
| 4|
| 5|
| 1|
| 2|
| 3|
+---+
scala> sql("select * from default . t").show()
+---+
| a|
+---+
| 4|
| 5|
| 1|
| 2|
| 3|
+---+
```
Should these also be prohibited? (But, I think it is hard to do so though...)
##########
File path: sql/core/src/test/scala/org/apache/spark/sql/execution/SparkSqlParserSuite.scala
##########
@@ -61,6 +61,39 @@ class SparkSqlParserSuite extends AnalysisTest {
private def intercept(sqlCommand: String, messages: String*): Unit =
interceptParseException(parser.parsePlan)(sqlCommand, messages: _*)
+ test("Report Error for invalid usage of SET command") {
+ assertEqual("SET", SetCommand(None))
+ assertEqual("SET -v", SetCommand(Some("-v", None)))
+ assertEqual("SET spark.sql.key", SetCommand(Some("spark.sql.key" -> None)))
+ assertEqual("SET spark:sql:key=false", SetCommand(Some("spark:sql:key" -> Some("false"))))
+ assertEqual("SET spark.sql. key=value",
Review comment:
hm, I'll check it. But, it seems we already accept similar patterns, e.g.,
```
scala> sql("select * from default.t").show()
+---+
| a|
+---+
| 4|
| 5|
| 1|
| 2|
| 3|
+---+
scala> sql("select * from default .t").show()
+---+
| a|
+---+
| 4|
| 5|
| 1|
| 2|
| 3|
+---+
scala> sql("select * from default . t").show()
+---+
| a|
+---+
| 4|
| 5|
| 1|
| 2|
| 3|
+---+
```
Should these also be prohibited? (But, I think it is hard to do so though...)
----------------------------------------------------------------
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.
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] AmplabJenkins removed a comment on pull request #29146: [WIP][SPARK-32257][SQL] Reports explicit errors for invalid usage of SET command
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-660461263
----------------------------------------------------------------
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.
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] SparkQA removed a comment on pull request #29146: [WIP][SPARK-32257][SQL] Reports explicit errors for invalid usage of SET command
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-660446426
**[Test build #126099 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/126099/testReport)** for PR 29146 at commit [`7e0d97a`](https://github.com/apache/spark/commit/7e0d97ac6612d37ede14e92f14673cf8de5c3c56).
----------------------------------------------------------------
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.
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] cloud-fan commented on a change in pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #29146:
URL: https://github.com/apache/spark/pull/29146#discussion_r464171781
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/execution/SparkSqlParser.scala
##########
@@ -66,17 +68,29 @@ class SparkSqlAstBuilder(conf: SQLConf) extends AstBuilder(conf) {
* character in the raw string.
*/
override def visitSetConfiguration(ctx: SetConfigurationContext): LogicalPlan = withOrigin(ctx) {
- // Construct the command.
- val raw = remainder(ctx.SET.getSymbol)
- val keyValueSeparatorIndex = raw.indexOf('=')
- if (keyValueSeparatorIndex >= 0) {
- val key = raw.substring(0, keyValueSeparatorIndex).trim
- val value = raw.substring(keyValueSeparatorIndex + 1).trim
- SetCommand(Some(key -> Option(value)))
- } else if (raw.nonEmpty) {
- SetCommand(Some(raw.trim -> None))
+ val configKeyValueDef = """([a-zA-Z_\d\\.:]+)\s*=(.*)""".r
Review comment:
Can we put it in the class body so we don't need to compile the regex repeatedly?
----------------------------------------------------------------
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.
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] AmplabJenkins removed a comment on pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-668004107
----------------------------------------------------------------
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.
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] AmplabJenkins removed a comment on pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-666999090
----------------------------------------------------------------
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.
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] AmplabJenkins commented on pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET command
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-660153281
----------------------------------------------------------------
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.
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] viirya commented on pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET command
Posted by GitBox <gi...@apache.org>.
viirya commented on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-663720249
> If we don't allow space in the config name by default(requires quoting), I think we can do that for other special chars as well. Then the parser rule can be very simple:
>
> ```
> SET (multiPartIdent | STRING | BACKQUOTED_IDENTIFIER) (EQ value=.*)?
> ```
+1
Can we try to do that in parser rule?
----------------------------------------------------------------
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.
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] SparkQA removed a comment on pull request #29146: [WIP][SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-665634641
----------------------------------------------------------------
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.
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] cloud-fan commented on a change in pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #29146:
URL: https://github.com/apache/spark/pull/29146#discussion_r463106538
##########
File path: sql/core/src/test/scala/org/apache/spark/sql/SQLQuerySuite.scala
##########
@@ -962,8 +962,8 @@ class SQLQuerySuite extends QueryTest with SharedSparkSession with AdaptiveSpark
test("SET commands semantics using sql()") {
spark.sessionState.conf.clear()
- val testKey = "test.key.0"
- val testVal = "test.val.0"
+ val testKey = "test.key.k0"
Review comment:
damn this is really hard. We probably should simplify the parser rule
```
SET key=quotedIdentifier (EQ vaule=.*)? #simpleSetConfig
SET mixed=.* #mixedSetConfig
```
In the mixed case, we split the string by `=`, and make sure the first part(key) has no special chars (probably just forbid space).
----------------------------------------------------------------
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.
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] SparkQA commented on pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-667941440
**[Test build #126976 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/126976/testReport)** for PR 29146 at commit [`3065ed9`](https://github.com/apache/spark/commit/3065ed9aa5ce232ce4f16335cbf830bd93a73f3f).
----------------------------------------------------------------
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.
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] SparkQA commented on pull request #29146: [WIP][SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-665634641
----------------------------------------------------------------
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.
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] viirya commented on pull request #29146: [WIP][SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
viirya commented on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-665460861
Oh, I thought the test was passed...
----------------------------------------------------------------
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.
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] AmplabJenkins commented on pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-666999090
----------------------------------------------------------------
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.
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] maropu commented on pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET command
Posted by GitBox <gi...@apache.org>.
maropu commented on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-660152286
@gatorsmile Do we need to support the case: a configuration with spaces?
----------------------------------------------------------------
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.
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] maropu commented on pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
maropu commented on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-666998114
retest this 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.
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] maropu commented on a change in pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
maropu commented on a change in pull request #29146:
URL: https://github.com/apache/spark/pull/29146#discussion_r464224591
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/execution/SparkSqlParser.scala
##########
@@ -66,17 +68,29 @@ class SparkSqlAstBuilder(conf: SQLConf) extends AstBuilder(conf) {
* character in the raw string.
*/
override def visitSetConfiguration(ctx: SetConfigurationContext): LogicalPlan = withOrigin(ctx) {
- // Construct the command.
- val raw = remainder(ctx.SET.getSymbol)
- val keyValueSeparatorIndex = raw.indexOf('=')
- if (keyValueSeparatorIndex >= 0) {
- val key = raw.substring(0, keyValueSeparatorIndex).trim
- val value = raw.substring(keyValueSeparatorIndex + 1).trim
- SetCommand(Some(key -> Option(value)))
- } else if (raw.nonEmpty) {
- SetCommand(Some(raw.trim -> None))
+ val configKeyValueDef = """([a-zA-Z_\d\\.:]+)\s*=(.*)""".r
+ remainder(ctx.SET.getSymbol).trim match {
+ case configKeyValueDef(key, value) =>
+ SetCommand(Some(key -> Option(value.trim)))
+ case configKeyDef(key) =>
+ SetCommand(Some(key -> None))
+ case s if s == "-v" =>
+ SetCommand(Some("-v" -> None))
+ case s if s.isEmpty =>
+ SetCommand(None)
+ case _ => throw new ParseException("Expected format is 'SET', 'SET key', or " +
+ "'SET key=value'. If you want to include special characters in key, " +
+ "please use quotes, e.g., SET `ke y`=value.", ctx)
+ }
+ }
+
+ override def visitSetQuotedConfiguration(ctx: SetQuotedConfigurationContext)
+ : LogicalPlan = withOrigin(ctx) {
+ val keyStr = ctx.quotedConfigKey().getText
+ if (ctx.value != null) {
+ SetCommand(Some(keyStr -> Option(remainder(ctx.EQ().getSymbol).trim)))
Review comment:
I did so first, but the added tests below failed;
```
val keyStr = ctx.quotedConfigKey().getText
if (ctx.value != null) {
- SetCommand(Some(keyStr -> Option(remainder(ctx.EQ().getSymbol).trim)))
+ SetCommand(Some(keyStr -> Option(ctx.value.getText)))
} else {
SetCommand(Some(keyStr -> None))
}
// The failed tests
assertEqual("SET `spark.sql. key`= -1",
SetCommand(Some("spark.sql. key" -> Some("-1"))))
[info] - Report Error for invalid usage of SET command *** FAILED *** (104 milliseconds)
[info] == FAIL: Plans do not match ===
[info] !SetCommand (spark.sql. key,Some(1)) SetCommand (spark.sql. key,Some(-1)) (PlanTest.scala:157)
...
```
So, I selected the current one to avoid the test failure. Do you know something about this behaivour?
----------------------------------------------------------------
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.
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] HyukjinKwon commented on pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET command
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-663441298
retest this 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.
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] cloud-fan commented on a change in pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #29146:
URL: https://github.com/apache/spark/pull/29146#discussion_r462761942
##########
File path: sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4
##########
@@ -244,11 +258,31 @@ statement
| SET TIME ZONE interval #setTimeZone
| SET TIME ZONE timezone=(STRING | LOCAL) #setTimeZone
| SET TIME ZONE .*? #setTimeZone
+ | SET configKey (EQ configValue)? #setConfiguration
| SET .*? #setConfiguration
+ | RESET configKey? #resetConfiguration
| RESET .*? #resetConfiguration
| unsupportedHiveNativeCommands .*? #failNativeCommand
;
+configKey
+ : configIdentifier (('.' | ':') configIdentifier
+ // The two semantic predicates make sure that no token on the hidden channel
+ // (e.g., spaces) exists between the config separator ('.' or ':').
+ {!isHidden(-3)}? {!isHidden(-2)}?)*
+ | quotedIdentifier
+ ;
+
+configValue
+ : vaule=.*?
Review comment:
can it be `.*`? * mean 0 or more counts, so it can match empty input.
##########
File path: sql/core/src/test/scala/org/apache/spark/sql/SQLQuerySuite.scala
##########
@@ -962,8 +962,8 @@ class SQLQuerySuite extends QueryTest with SharedSparkSession with AdaptiveSpark
test("SET commands semantics using sql()") {
spark.sessionState.conf.clear()
- val testKey = "test.key.0"
- val testVal = "test.val.0"
+ val testKey = "test.key.k0"
Review comment:
This is a good catch. Shall we support it to maximize the backward compatibility?
```
configIdentifier
: IDENTIFIER
| nonReserved
| strictNonReserved
| DIGIT+
;
```
##########
File path: sql/core/src/test/scala/org/apache/spark/sql/execution/SparkSqlParserSuite.scala
##########
@@ -61,6 +63,64 @@ class SparkSqlParserSuite extends AnalysisTest {
private def intercept(sqlCommand: String, messages: String*): Unit =
interceptParseException(parser.parsePlan)(sqlCommand, messages: _*)
+ test("Checks if SET/RESET can accept all the configurations") {
+ // Force to build static SQL configurations
+ StaticSQLConf
+ SQLConf.sqlConfEntries.values.asScala.foreach { e =>
+ assertEqual(s"SET ${e.key}", SetCommand(Some(e.key -> None)))
+ if (e.defaultValue.isDefined) {
+ assertEqual(s"SET ${e.key}=${e.defaultValueString}",
+ SetCommand(Some(e.key -> Some(e.defaultValueString))))
+ }
+ assertEqual(s"RESET ${e.key}", ResetCommand(Some(e.key)))
+ }
+ }
+
+ test("Report Error for invalid usage of SET command") {
+ assertEqual("SET", SetCommand(None))
+ assertEqual("SET -v", SetCommand(Some("-v", None)))
+ assertEqual("SET spark.sql.key", SetCommand(Some("spark.sql.key" -> None)))
+ assertEqual("SET spark:sql:key=false", SetCommand(Some("spark:sql:key" -> Some("false"))))
+ assertEqual("SET spark:sql:key=", SetCommand(Some("spark:sql:key" -> Some(""))))
+ assertEqual("SET spark:sql:key= ", SetCommand(Some("spark:sql:key" -> Some(""))))
+ assertEqual("SET spark:sql:key=-1 ", SetCommand(Some("spark:sql:key" -> Some("-1"))))
+ assertEqual("SET spark:sql:key = -1", SetCommand(Some("spark:sql:key" -> Some("-1"))))
+ assertEqual("SET `spark.sql. key`=value",
+ SetCommand(Some("spark.sql. key" -> Some("value"))))
+
+ val expectedErrMsg = "Expected format is 'SET', 'SET key', or " +
+ "'SET key=value'. If you want to include special characters in key, " +
+ "please use quotes, e.g., SET `ke y`=value."
+ intercept("SET spark.sql.key value", expectedErrMsg)
+ intercept("SET spark.sql.key 'value'", expectedErrMsg)
+ intercept("SET spark.sql.key \"value\" ", expectedErrMsg)
+ intercept("SET spark.sql.key value1 value2", expectedErrMsg)
+
+ // TODO: Needs to make the error message more meaningful for users
+ intercept("SET spark.sql. key=value", "failed predicate")
+ intercept("SET spark.sql :key=value", "failed predicate")
+ intercept("SET spark.sql . key=value", "failed predicate")
+ }
+
+ test("Report Error for invalid usage of RESET command") {
+ assertEqual("RESET", ResetCommand(None))
+ assertEqual("RESET spark.sql.key", ResetCommand(Some("spark.sql.key")))
+ assertEqual("RESET spark.sql.key ", ResetCommand(Some("spark.sql.key")))
+ assertEqual("RESET `spark.sql. key`", ResetCommand(Some("spark.sql. key")))
+
+ val expectedErrMsg = "Expected format is 'RESET' or 'RESET key'. " +
+ "If you want to include special characters in key, " +
+ "please use quotes, e.g., RESET `ke y`."
+ intercept("RESET spark.sql.key1 key2", expectedErrMsg)
+ intercept("RESET spark. sql.key1 key2", expectedErrMsg)
+ intercept("RESET spark.sql.key1 key2 key3", expectedErrMsg)
+
+ // TODO: Needs to make the error message more meaningful for users
+ intercept("RESET spark.sql: key", "failed predicate")
Review comment:
can we check space in the first dot? `RESET spark . sql.key`
##########
File path: sql/core/src/test/scala/org/apache/spark/sql/internal/SQLConfEntrySuite.scala
##########
@@ -107,7 +107,7 @@ class SQLConfEntrySuite extends SparkFunSuite {
test("stringConf") {
val key = "spark.sql.SQLConfEntrySuite.string"
- val confEntry = buildConf(key).stringConf.createWithDefault(null)
+ val confEntry = buildConf(key).stringConf.createWithDefault("")
Review comment:
```
SetCommand (spark.sql.SQLConfEntrySuite.string,Some(null))
SetCommand (spark.sql.SQLConfEntrySuite.string,Some(null))
```
They are exactly the same, not sure why plan check fails...
----------------------------------------------------------------
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.
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] cloud-fan commented on a change in pull request #29146: [WIP][SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #29146:
URL: https://github.com/apache/spark/pull/29146#discussion_r462359914
##########
File path: sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4
##########
@@ -244,11 +258,31 @@ statement
| SET TIME ZONE interval #setTimeZone
| SET TIME ZONE timezone=(STRING | LOCAL) #setTimeZone
| SET TIME ZONE .*? #setTimeZone
+ | SET configKey (EQ configValue)? #setConfiguration
| SET .*? #setConfiguration
+ | RESET configKey? #resetConfiguration
| RESET .*? #resetConfiguration
| unsupportedHiveNativeCommands .*? #failNativeCommand
;
+configKey
+ : configIdentifier (('.' | ':') configIdentifier
+ // The two semantic predicates make sure that no token on the hidden channel
+ // (e.g., spaces) exists between the config separator ('.' or ':').
+ {!isHidden(-3)}? {!isHidden(-2)}?)*
+ | quotedIdentifier
+ ;
+
+configValue
+ : MINUS? vaule=.*?
Review comment:
hmm, why `.*` can't match MINUS?
----------------------------------------------------------------
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.
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] cloud-fan commented on pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET command
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-663569282
If we don't allow space in the config name by default(requires quoting), I think we can do that for other special chars as well. Then the parser rule can be very simple:
```
SET (multiPartIdent | string) (EQ .*)?
```
----------------------------------------------------------------
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.
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] AmplabJenkins commented on pull request #29146: [WIP][SPARK-32257][SQL] Reports explicit errors for invalid usage of SET command
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-660446524
----------------------------------------------------------------
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.
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] SparkQA commented on pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-664694491
----------------------------------------------------------------
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.
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] AmplabJenkins commented on pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-667487445
----------------------------------------------------------------
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.
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] SparkQA commented on pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-668002897
**[Test build #126976 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/126976/testReport)** for PR 29146 at commit [`3065ed9`](https://github.com/apache/spark/commit/3065ed9aa5ce232ce4f16335cbf830bd93a73f3f).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public 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.
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] maropu commented on a change in pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
maropu commented on a change in pull request #29146:
URL: https://github.com/apache/spark/pull/29146#discussion_r464238349
##########
File path: sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4
##########
@@ -246,11 +246,17 @@ statement
| SET TIME ZONE interval #setTimeZone
| SET TIME ZONE timezone=(STRING | LOCAL) #setTimeZone
| SET TIME ZONE .*? #setTimeZone
+ | SET quotedConfigKey (EQ value=.*)? #setQuotedConfiguration
| SET .*? #setConfiguration
+ | RESET quotedConfigKey #resetQuotedConfiguration
| RESET .*? #resetConfiguration
| unsupportedHiveNativeCommands .*? #failNativeCommand
;
+quotedConfigKey
Review comment:
Yea, I did so first, but that suggested definition did not invoke `PostProcessor.exitQuotedIdentifier` to split backquotes;
```
- | SET quotedConfigKey (EQ value=.*)? #setQuotedConfiguration
+ | SET key=quotedIdentifier (EQ value=.*)? #setQuotedConfiguration
override def visitSetQuotedConfiguration(ctx: SetQuotedConfigurationContext)
: LogicalPlan = withOrigin(ctx) {
- val keyStr = ctx.quotedConfigKey().getText
+ val keyStr = ctx.key.getText
assertEqual("SET `spark.sql. key`=value",
SetCommand(Some("spark.sql. key" -> Some("value"))))
fo] - Report Error for invalid usage of SET command *** FAILED *** (106 milliseconds)
[info] == FAIL: Plans do not match ===
[info] !SetCommand (`spark.sql. key`,Some(value)) SetCommand (spark.sql. key,Some(value)) (PlanTest.scala:157)
[info] org.scalatest.exceptions.TestFailedException:
...
```
So, we need replace("`", "") for that approach. Please check the lates commit.
----------------------------------------------------------------
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.
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] AmplabJenkins removed a comment on pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-667487448
----------------------------------------------------------------
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.
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] AmplabJenkins commented on pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-667502817
----------------------------------------------------------------
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.
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] SparkQA commented on pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-667642264
**[Test build #126934 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/126934/testReport)** for PR 29146 at commit [`5a80e78`](https://github.com/apache/spark/commit/5a80e787a89c26e494dd4527d7e53deb29a87832).
----------------------------------------------------------------
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.
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] SparkQA removed a comment on pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET command
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-663443022
**[Test build #126491 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/126491/testReport)** for PR 29146 at commit [`2e92675`](https://github.com/apache/spark/commit/2e9267562c37f46e303544ec55f5881e205ed8e8).
----------------------------------------------------------------
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.
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] AmplabJenkins removed a comment on pull request #29146: [WIP][SPARK-32257][SQL] Reports explicit errors for invalid usage of SET command
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-660475527
----------------------------------------------------------------
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.
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] AmplabJenkins removed a comment on pull request #29146: [WIP][SPARK-32257][SQL] Reports explicit errors for invalid usage of SET command
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-660509892
Merged build finished. Test FAILed.
----------------------------------------------------------------
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.
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] AmplabJenkins removed a comment on pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-667504394
Merged build finished. Test FAILed.
----------------------------------------------------------------
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.
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] cloud-fan commented on pull request #29146: [WIP][SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-664952148
retest this 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.
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] AmplabJenkins removed a comment on pull request #29146: [WIP][SPARK-32257][SQL] Reports explicit errors for invalid usage of SET command
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-660471789
Merged build finished. Test FAILed.
----------------------------------------------------------------
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.
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] AmplabJenkins removed a comment on pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-667102267
----------------------------------------------------------------
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.
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] SparkQA commented on pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-667655000
**[Test build #126934 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/126934/testReport)** for PR 29146 at commit [`5a80e78`](https://github.com/apache/spark/commit/5a80e787a89c26e494dd4527d7e53deb29a87832).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public 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.
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] SparkQA commented on pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET command
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-663443022
**[Test build #126491 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/126491/testReport)** for PR 29146 at commit [`2e92675`](https://github.com/apache/spark/commit/2e9267562c37f46e303544ec55f5881e205ed8e8).
----------------------------------------------------------------
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.
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] cloud-fan commented on a change in pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #29146:
URL: https://github.com/apache/spark/pull/29146#discussion_r464246525
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/execution/SparkSqlParser.scala
##########
@@ -66,17 +69,28 @@ class SparkSqlAstBuilder(conf: SQLConf) extends AstBuilder(conf) {
* character in the raw string.
*/
override def visitSetConfiguration(ctx: SetConfigurationContext): LogicalPlan = withOrigin(ctx) {
- // Construct the command.
- val raw = remainder(ctx.SET.getSymbol)
- val keyValueSeparatorIndex = raw.indexOf('=')
- if (keyValueSeparatorIndex >= 0) {
- val key = raw.substring(0, keyValueSeparatorIndex).trim
- val value = raw.substring(keyValueSeparatorIndex + 1).trim
- SetCommand(Some(key -> Option(value)))
- } else if (raw.nonEmpty) {
- SetCommand(Some(raw.trim -> None))
+ remainder(ctx.SET.getSymbol).trim match {
+ case configKeyValueDef(key, value) =>
+ SetCommand(Some(key -> Option(value.trim)))
+ case configKeyDef(key) =>
+ SetCommand(Some(key -> None))
+ case s if s == "-v" =>
+ SetCommand(Some("-v" -> None))
+ case s if s.isEmpty =>
+ SetCommand(None)
+ case _ => throw new ParseException("Expected format is 'SET', 'SET key', or " +
+ "'SET key=value'. If you want to include special characters in key, " +
+ "please use quotes, e.g., SET `ke y`=value.", ctx)
+ }
+ }
+
+ override def visitSetQuotedConfiguration(ctx: SetQuotedConfigurationContext)
+ : LogicalPlan = withOrigin(ctx) {
+ val keyStr = ctx.key.getText.replaceAll("`", "")
Review comment:
how about `visitQuotedIdentifier(ctx.key)`
----------------------------------------------------------------
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.
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] AmplabJenkins commented on pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-666231024
Merged build finished. Test FAILed.
----------------------------------------------------------------
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.
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] SparkQA removed a comment on pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-667642264
**[Test build #126934 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/126934/testReport)** for PR 29146 at commit [`5a80e78`](https://github.com/apache/spark/commit/5a80e787a89c26e494dd4527d7e53deb29a87832).
----------------------------------------------------------------
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.
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] AmplabJenkins commented on pull request #29146: [WIP][SPARK-32257][SQL] Reports explicit errors for invalid usage of SET command
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-660471789
----------------------------------------------------------------
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.
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] cloud-fan commented on a change in pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #29146:
URL: https://github.com/apache/spark/pull/29146#discussion_r464247302
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/execution/SparkSqlParser.scala
##########
@@ -66,17 +68,29 @@ class SparkSqlAstBuilder(conf: SQLConf) extends AstBuilder(conf) {
* character in the raw string.
*/
override def visitSetConfiguration(ctx: SetConfigurationContext): LogicalPlan = withOrigin(ctx) {
- // Construct the command.
- val raw = remainder(ctx.SET.getSymbol)
- val keyValueSeparatorIndex = raw.indexOf('=')
- if (keyValueSeparatorIndex >= 0) {
- val key = raw.substring(0, keyValueSeparatorIndex).trim
- val value = raw.substring(keyValueSeparatorIndex + 1).trim
- SetCommand(Some(key -> Option(value)))
- } else if (raw.nonEmpty) {
- SetCommand(Some(raw.trim -> None))
+ val configKeyValueDef = """([a-zA-Z_\d\\.:]+)\s*=(.*)""".r
+ remainder(ctx.SET.getSymbol).trim match {
+ case configKeyValueDef(key, value) =>
+ SetCommand(Some(key -> Option(value.trim)))
+ case configKeyDef(key) =>
+ SetCommand(Some(key -> None))
+ case s if s == "-v" =>
+ SetCommand(Some("-v" -> None))
+ case s if s.isEmpty =>
+ SetCommand(None)
+ case _ => throw new ParseException("Expected format is 'SET', 'SET key', or " +
+ "'SET key=value'. If you want to include special characters in key, " +
+ "please use quotes, e.g., SET `ke y`=value.", ctx)
+ }
+ }
+
+ override def visitSetQuotedConfiguration(ctx: SetQuotedConfigurationContext)
+ : LogicalPlan = withOrigin(ctx) {
+ val keyStr = ctx.quotedConfigKey().getText
+ if (ctx.value != null) {
+ SetCommand(Some(keyStr -> Option(remainder(ctx.EQ().getSymbol).trim)))
Review comment:
ah, seems we should use `(value+=.)*`
----------------------------------------------------------------
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.
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] AmplabJenkins commented on pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-667102267
----------------------------------------------------------------
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.
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] maropu commented on a change in pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
maropu commented on a change in pull request #29146:
URL: https://github.com/apache/spark/pull/29146#discussion_r464224591
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/execution/SparkSqlParser.scala
##########
@@ -66,17 +68,29 @@ class SparkSqlAstBuilder(conf: SQLConf) extends AstBuilder(conf) {
* character in the raw string.
*/
override def visitSetConfiguration(ctx: SetConfigurationContext): LogicalPlan = withOrigin(ctx) {
- // Construct the command.
- val raw = remainder(ctx.SET.getSymbol)
- val keyValueSeparatorIndex = raw.indexOf('=')
- if (keyValueSeparatorIndex >= 0) {
- val key = raw.substring(0, keyValueSeparatorIndex).trim
- val value = raw.substring(keyValueSeparatorIndex + 1).trim
- SetCommand(Some(key -> Option(value)))
- } else if (raw.nonEmpty) {
- SetCommand(Some(raw.trim -> None))
+ val configKeyValueDef = """([a-zA-Z_\d\\.:]+)\s*=(.*)""".r
+ remainder(ctx.SET.getSymbol).trim match {
+ case configKeyValueDef(key, value) =>
+ SetCommand(Some(key -> Option(value.trim)))
+ case configKeyDef(key) =>
+ SetCommand(Some(key -> None))
+ case s if s == "-v" =>
+ SetCommand(Some("-v" -> None))
+ case s if s.isEmpty =>
+ SetCommand(None)
+ case _ => throw new ParseException("Expected format is 'SET', 'SET key', or " +
+ "'SET key=value'. If you want to include special characters in key, " +
+ "please use quotes, e.g., SET `ke y`=value.", ctx)
+ }
+ }
+
+ override def visitSetQuotedConfiguration(ctx: SetQuotedConfigurationContext)
+ : LogicalPlan = withOrigin(ctx) {
+ val keyStr = ctx.quotedConfigKey().getText
+ if (ctx.value != null) {
+ SetCommand(Some(keyStr -> Option(remainder(ctx.EQ().getSymbol).trim)))
Review comment:
I did so first, but the added tests below failed;
```
val keyStr = ctx.quotedConfigKey().getText
if (ctx.value != null) {
- SetCommand(Some(keyStr -> Option(remainder(ctx.EQ().getSymbol).trim)))
+ SetCommand(Some(keyStr -> Option(ctx.value.getText)))
} else {
SetCommand(Some(keyStr -> None))
}
// The failed tests
assertEqual("SET `spark.sql. key`= -1",
SetCommand(Some("spark.sql. key" -> Some("-1"))))
[info] - Report Error for invalid usage of SET command *** FAILED *** (104 milliseconds)
[info] == FAIL: Plans do not match ===
[info] !SetCommand (spark.sql. key,Some(1)) SetCommand (spark.sql. key,Some(-1)) (PlanTest.scala:157)
assertEqual("SET `spark.sql. key`= v a lue ",
SetCommand(Some("spark.sql. key" -> Some("value"))))
[info] - Report Error for invalid usage of SET command *** FAILED *** (146 milliseconds)
[info] == FAIL: Plans do not match ===
[info] !SetCommand (spark.sql. key,Some(lue)) SetCommand (spark.sql. key,Some(value)) (PlanTest.scala:157)
...
```
So, I selected the current one to avoid the test failures. Do you know something about this behaivour?
----------------------------------------------------------------
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.
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] SparkQA commented on pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET command
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-663551579
**[Test build #126491 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/126491/testReport)** for PR 29146 at commit [`2e92675`](https://github.com/apache/spark/commit/2e9267562c37f46e303544ec55f5881e205ed8e8).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public 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.
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] cloud-fan commented on a change in pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #29146:
URL: https://github.com/apache/spark/pull/29146#discussion_r464172835
##########
File path: sql/core/src/test/scala/org/apache/spark/sql/execution/SparkSqlParserSuite.scala
##########
@@ -61,6 +64,80 @@ class SparkSqlParserSuite extends AnalysisTest {
private def intercept(sqlCommand: String, messages: String*): Unit =
interceptParseException(parser.parsePlan)(sqlCommand, messages: _*)
+ test("Checks if SET/RESET can parse all the configurations") {
+ // Force to build static SQL configurations
+ StaticSQLConf
+ (SQLConf.sqlConfEntries.values.asScala ++ ConfigEntry.knownConfigs.values.asScala)
Review comment:
`SQLConf` also uses `ConfigEntry`, I think `ConfigEntry.knownConfigs` already covers all the registered configs.
----------------------------------------------------------------
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.
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] maropu commented on a change in pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
maropu commented on a change in pull request #29146:
URL: https://github.com/apache/spark/pull/29146#discussion_r463019812
##########
File path: sql/core/src/test/scala/org/apache/spark/sql/SQLQuerySuite.scala
##########
@@ -962,8 +962,8 @@ class SQLQuerySuite extends QueryTest with SharedSparkSession with AdaptiveSpark
test("SET commands semantics using sql()") {
spark.sessionState.conf.clear()
- val testKey = "test.key.0"
- val testVal = "test.val.0"
+ val testKey = "test.key.k0"
Review comment:
I tried it though, I found just adding `DIGIT+`([Actually, I added `INTEGER_VALUE` instead](https://github.com/apache/spark/pull/29146/files#diff-8c1cb2af4aa1109e08481dae79052cc3R280)) there did not work well... For example, a pattern below was okay;
```
SET spark:sql:3=value or SET 1:2:key=value
```
https://github.com/apache/spark/pull/29146/files#diff-e9f35fff083788e6494cb1220a792b99R90-R92
But, a following pattern couldn't be parsed;
```
SET spark.sql.3=value or SET 1.2.key=value
```
https://github.com/apache/spark/pull/29146/files#diff-e9f35fff083788e6494cb1220a792b99R105-R106
That's because `.3` and `1.2` are regarded as the `DECIMAL_VALUE` tokens, so the `configKey` parsing rule cannot match them correctly. I looked for an approach to avoid it, but I haven't found a good idea so far.
Any idea for this? Anyway, I will keep looking for 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.
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] AmplabJenkins removed a comment on pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET command
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-660223756
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/126055/
Test FAILed.
----------------------------------------------------------------
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.
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] AmplabJenkins commented on pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-666883260
----------------------------------------------------------------
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.
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] maropu commented on pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
maropu commented on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-666882445
retest this 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.
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] maropu commented on a change in pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET command
Posted by GitBox <gi...@apache.org>.
maropu commented on a change in pull request #29146:
URL: https://github.com/apache/spark/pull/29146#discussion_r460377012
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/execution/SparkSqlParser.scala
##########
@@ -65,16 +65,19 @@ class SparkSqlAstBuilder(conf: SQLConf) extends AstBuilder(conf) {
*/
override def visitSetConfiguration(ctx: SetConfigurationContext): LogicalPlan = withOrigin(ctx) {
// Construct the command.
- val raw = remainder(ctx.SET.getSymbol)
- val keyValueSeparatorIndex = raw.indexOf('=')
Review comment:
Thanks for the comment, @HyukjinKwon. Are you okay with the @cloud-fan idea? https://github.com/apache/spark/pull/29146#issuecomment-663569282
----------------------------------------------------------------
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.
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] cloud-fan commented on a change in pull request #29146: [WIP][SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #29146:
URL: https://github.com/apache/spark/pull/29146#discussion_r461323124
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/execution/SparkSqlParser.scala
##########
@@ -58,6 +58,9 @@ class SparkSqlParser(conf: SQLConf) extends AbstractSqlParser(conf) {
class SparkSqlAstBuilder(conf: SQLConf) extends AstBuilder(conf) {
import org.apache.spark.sql.catalyst.parser.ParserUtils._
+ private def normalizeConfigString(s: String) =
Review comment:
I checked `tableIdentifier`, it includes `BACKQUOTED_IDENTIFIER`, and in `AstBuilder` we just use `xxx.getText`. Why does `getText` not work here?
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/execution/SparkSqlParser.scala
##########
@@ -58,6 +58,9 @@ class SparkSqlParser(conf: SQLConf) extends AbstractSqlParser(conf) {
class SparkSqlAstBuilder(conf: SQLConf) extends AstBuilder(conf) {
import org.apache.spark.sql.catalyst.parser.ParserUtils._
+ private def normalizeConfigString(s: String) =
Review comment:
I found it. See `PostProcessor.exitQuotedIdentifier`. `quotedIdentifier` is a special parser rule which has post porcessor to remove back-quotes.
##########
File path: sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4
##########
@@ -244,11 +244,19 @@ statement
| SET TIME ZONE interval #setTimeZone
| SET TIME ZONE timezone=(STRING | LOCAL) #setTimeZone
| SET TIME ZONE .*? #setTimeZone
+ | SET configKey (EQ value=.+)? #setConfiguration
| SET .*? #setConfiguration
+ | RESET configKey? #resetConfiguration
| RESET .*? #resetConfiguration
| unsupportedHiveNativeCommands .*? #failNativeCommand
;
+configKey
+ : IDENTIFIER (('.' | ':') IDENTIFIER)*
+ | quotedIdentifier
+ | STRING
Review comment:
After a second thought, I'm wondering if we should support STRING. The table identifier can't be string literal, maybe it's better to keep consistent as we also support `quotedIdentifier`.
##########
File path: sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4
##########
@@ -244,11 +244,19 @@ statement
| SET TIME ZONE interval #setTimeZone
| SET TIME ZONE timezone=(STRING | LOCAL) #setTimeZone
| SET TIME ZONE .*? #setTimeZone
+ | SET configKey (EQ value=.+)? #setConfiguration
| SET .*? #setConfiguration
+ | RESET configKey? #resetConfiguration
| RESET .*? #resetConfiguration
| unsupportedHiveNativeCommands .*? #failNativeCommand
;
+configKey
+ : IDENTIFIER (('.' | ':') IDENTIFIER)*
Review comment:
maybe we should replace `IDENTIFIER (('.' | ':') IDENTIFIER)*` with
```
( ~' ' | ~'=' )+
```
##########
File path: sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4
##########
@@ -244,11 +244,19 @@ statement
| SET TIME ZONE interval #setTimeZone
| SET TIME ZONE timezone=(STRING | LOCAL) #setTimeZone
| SET TIME ZONE .*? #setTimeZone
+ | SET configKey (EQ value=.+)? #setConfiguration
| SET .*? #setConfiguration
+ | RESET configKey? #resetConfiguration
| RESET .*? #resetConfiguration
| unsupportedHiveNativeCommands .*? #failNativeCommand
;
+configKey
+ : IDENTIFIER (('.' | ':') IDENTIFIER)*
Review comment:
It's because `IDENTIFIER` doesn't include keywords.
maybe we should replace `IDENTIFIER (('.' | ':') IDENTIFIER)*` with
```
( ~' ' | ~'=' )+
```
##########
File path: sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4
##########
@@ -244,11 +244,19 @@ statement
| SET TIME ZONE interval #setTimeZone
| SET TIME ZONE timezone=(STRING | LOCAL) #setTimeZone
| SET TIME ZONE .*? #setTimeZone
+ | SET configKey (EQ value=.+)? #setConfiguration
| SET .*? #setConfiguration
+ | RESET configKey? #resetConfiguration
| RESET .*? #resetConfiguration
| unsupportedHiveNativeCommands .*? #failNativeCommand
;
+configKey
+ : IDENTIFIER (('.' | ':') IDENTIFIER)*
Review comment:
or replace `IDENTIFIER` with `IDENTIFIER | nonReserved | strictNonReserved`
`nonReserved | strictNonReserved` matches all the keywords
##########
File path: sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4
##########
@@ -244,11 +244,22 @@ statement
| SET TIME ZONE interval #setTimeZone
| SET TIME ZONE timezone=(STRING | LOCAL) #setTimeZone
| SET TIME ZONE .*? #setTimeZone
+ | SET configureKey (EQ value=.+)? #setConfiguration
| SET .*? #setConfiguration
+ | RESET configureKey? #resetConfiguration
| RESET .*? #resetConfiguration
| unsupportedHiveNativeCommands .*? #failNativeCommand
;
+configureKey
Review comment:
configure is a verb. `configKey` or `configurationKey`
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/execution/SparkSqlParser.scala
##########
@@ -66,17 +66,21 @@ class SparkSqlAstBuilder(conf: SQLConf) extends AstBuilder(conf) {
* character in the raw string.
*/
override def visitSetConfiguration(ctx: SetConfigurationContext): LogicalPlan = withOrigin(ctx) {
- // Construct the command.
- val raw = remainder(ctx.SET.getSymbol)
- val keyValueSeparatorIndex = raw.indexOf('=')
- if (keyValueSeparatorIndex >= 0) {
- val key = raw.substring(0, keyValueSeparatorIndex).trim
- val value = raw.substring(keyValueSeparatorIndex + 1).trim
- SetCommand(Some(key -> Option(value)))
- } else if (raw.nonEmpty) {
- SetCommand(Some(raw.trim -> None))
+ if (ctx.configureKey() != null) {
+ val keyStr = ctx.configureKey().getText
+ if (ctx.value != null) {
+ SetCommand(Some(keyStr -> Option(ctx.value.getText)))
+ } else {
+ SetCommand(Some(keyStr -> None))
+ }
} else {
- SetCommand(None)
+ remainder(ctx.SET().getSymbol).trim match {
+ case "-v" => SetCommand(Some("-v" -> None))
+ case s if s.isEmpty => SetCommand(None)
+ case _ => throw new ParseException("Expected format is 'SET', 'SET key', or " +
+ "'SET key=value'. If you want to include special characters in key and value, " +
Review comment:
`in key and value` -> `in key`
##########
File path: sql/core/src/test/scala/org/apache/spark/sql/execution/SparkSqlParserSuite.scala
##########
@@ -61,6 +61,39 @@ class SparkSqlParserSuite extends AnalysisTest {
private def intercept(sqlCommand: String, messages: String*): Unit =
interceptParseException(parser.parsePlan)(sqlCommand, messages: _*)
+ test("Report Error for invalid usage of SET command") {
+ assertEqual("SET", SetCommand(None))
+ assertEqual("SET -v", SetCommand(Some("-v", None)))
+ assertEqual("SET spark.sql.key", SetCommand(Some("spark.sql.key" -> None)))
+ assertEqual("SET spark:sql:key=false", SetCommand(Some("spark:sql:key" -> Some("false"))))
+ assertEqual("SET spark.sql. key=value",
Review comment:
This is a bit weird. Is it possible to forbid it?
##########
File path: sql/core/src/test/scala/org/apache/spark/sql/execution/SparkSqlParserSuite.scala
##########
@@ -61,6 +61,39 @@ class SparkSqlParserSuite extends AnalysisTest {
private def intercept(sqlCommand: String, messages: String*): Unit =
interceptParseException(parser.parsePlan)(sqlCommand, messages: _*)
+ test("Report Error for invalid usage of SET command") {
+ assertEqual("SET", SetCommand(None))
+ assertEqual("SET -v", SetCommand(Some("-v", None)))
+ assertEqual("SET spark.sql.key", SetCommand(Some("spark.sql.key" -> None)))
+ assertEqual("SET spark:sql:key=false", SetCommand(Some("spark:sql:key" -> Some("false"))))
+ assertEqual("SET spark.sql. key=value",
Review comment:
I think table name is fine, but config name is really weird. It's kind of a silent behavior change as `SET spark.sql. key=value` sets config `spark.sql. key` before.
##########
File path: sql/core/src/test/scala/org/apache/spark/sql/execution/SparkSqlParserSuite.scala
##########
@@ -61,6 +61,39 @@ class SparkSqlParserSuite extends AnalysisTest {
private def intercept(sqlCommand: String, messages: String*): Unit =
interceptParseException(parser.parsePlan)(sqlCommand, messages: _*)
+ test("Report Error for invalid usage of SET command") {
+ assertEqual("SET", SetCommand(None))
+ assertEqual("SET -v", SetCommand(Some("-v", None)))
+ assertEqual("SET spark.sql.key", SetCommand(Some("spark.sql.key" -> None)))
+ assertEqual("SET spark:sql:key=false", SetCommand(Some("spark:sql:key" -> Some("false"))))
+ assertEqual("SET spark.sql. key=value",
Review comment:
https://www.antlr3.org/pipermail/antlr-interest/2007-October/024030.html
One way is to look at the hidden channel after parsing `configIdentifier`, and see if the next token is `WS`.
##########
File path: sql/core/src/test/scala/org/apache/spark/sql/execution/SparkSqlParserSuite.scala
##########
@@ -61,6 +63,53 @@ class SparkSqlParserSuite extends AnalysisTest {
private def intercept(sqlCommand: String, messages: String*): Unit =
interceptParseException(parser.parsePlan)(sqlCommand, messages: _*)
+ test("Checks if SET/RESET can accept all the configurations") {
+ // Force to build static SQL configurations
+ StaticSQLConf
+ SQLConf.sqlConfEntries.values.asScala.foreach { e =>
+ assertEqual(s"SET ${e.key}", SetCommand(Some(e.key -> None)))
+ assertEqual(s"RESET ${e.key}", ResetCommand(Some(e.key)))
+ }
+ }
+
+ test("Report Error for invalid usage of SET command") {
+ assertEqual("SET", SetCommand(None))
+ assertEqual("SET -v", SetCommand(Some("-v", None)))
+ assertEqual("SET spark.sql.key", SetCommand(Some("spark.sql.key" -> None)))
+ assertEqual("SET spark:sql:key=false", SetCommand(Some("spark:sql:key" -> Some("false"))))
+ assertEqual("SET `spark.sql. key`=value",
+ SetCommand(Some("spark.sql. key" -> Some("value"))))
+
+ val expectedErrMsg = "Expected format is 'SET', 'SET key', or " +
+ "'SET key=value'. If you want to include special characters in key, " +
+ "please use quotes, e.g., SET `ke y`=value."
+ intercept("SET spark.sql.key value", expectedErrMsg)
+ intercept("SET spark.sql.key 'value'", expectedErrMsg)
+ intercept("SET spark.sql.key \"value\" ", expectedErrMsg)
+ intercept("SET spark.sql.key value1 value2", expectedErrMsg)
+
+ intercept("SET spark.sql. key=value", "no viable alternative at input '.'")
+ intercept("SET spark.sql :key=value", "rule configKey failed predicate")
+ intercept("SET spark.sql . key=value", "rule configKey failed predicate")
Review comment:
I thought it will fallback to `SET .*?` and give nice error message.
We can improve it later.
----------------------------------------------------------------
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.
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] SparkQA removed a comment on pull request #29146: [WIP][SPARK-32257][SQL] Reports explicit errors for invalid usage of SET command
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-660475380
**[Test build #126108 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/126108/testReport)** for PR 29146 at commit [`2e92675`](https://github.com/apache/spark/commit/2e9267562c37f46e303544ec55f5881e205ed8e8).
----------------------------------------------------------------
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.
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] AmplabJenkins removed a comment on pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-667916067
Merged build finished. Test FAILed.
----------------------------------------------------------------
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.
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] AmplabJenkins removed a comment on pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET command
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-660223741
Merged build finished. Test FAILed.
----------------------------------------------------------------
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.
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] SparkQA commented on pull request #29146: [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #29146:
URL: https://github.com/apache/spark/pull/29146#issuecomment-667504249
**[Test build #126915 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/126915/testReport)** for PR 29146 at commit [`4822210`](https://github.com/apache/spark/commit/48222108242eb5b991705f16b926ea90c1293237).
* This patch **fails Spark unit tests**.
* This patch merges cleanly.
* This patch adds no public 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.
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