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/30 07:38:20 UTC
[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
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