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:36:31 UTC
[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
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