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:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
