[SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command #29146

maropu · 2020-07-17T14:52:05Z

What changes were proposed in this pull request?

This PR modified the parser code to handle invalid usages of a SET/RESET 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 special characters including spaces in the configuration name and reports a user-friendly error instead. In the error message, it tells users a workaround to use quotes or a string literal if they still needs to specify a configuration with them.

Before this PR:

scala> sql("SET spark.sql.ansi.enabled true").show(1, -1)
+---------------------------+-----------+
|key                        |value      |
+---------------------------+-----------+
|spark.sql.ansi.enabled true|<undefined>|
+---------------------------+-----------+

After this PR:

scala> sql("SET spark.sql.ansi.enabled true")
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, please use quotes, e.g., SET `ke y`=value.(line 1, pos 0)

== SQL ==
SET spark.sql.ansi.enabled true
^^^

Why are the changes needed?

For better user-friendly errors.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Added tests in SparkSqlParserSuite.

maropu · 2020-07-17T14:55:00Z

@gatorsmile Do we need to support the case: a configuration with spaces?

SparkQA · 2020-07-17T16:57:48Z

Test build #126055 has finished for PR 29146 at commit 7f8907e.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-07-18T09:33:00Z

Test build #126099 has finished for PR 29146 at commit 7e0d97a.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-07-18T11:51:53Z

Test build #126104 has finished for PR 29146 at commit 0d357ae.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-07-18T16:49:55Z

Test build #126108 has finished for PR 29146 at commit 2e92675.

This patch fails PySpark pip packaging tests.
This patch merges cleanly.
This patch adds no public classes.

maropu · 2020-07-20T01:56:04Z

Looks all the tests are passed in Github Actions.

maropu · 2020-07-20T01:56:59Z

sql/core/src/test/scala/org/apache/spark/sql/internal/SQLConfSuite.scala

      assert(spark.conf.get(SQLConf.SHUFFLE_PARTITIONS) === 10)
    } finally {
-      sql(s"set ${SQLConf.SHUFFLE_PARTITIONS}=$original")
+      sql(s"set ${SQLConf.SHUFFLE_PARTITIONS.key}=$original")


The existing code looks incorrect.

maropu · 2020-07-20T01:57:36Z

cc: @cloud-fan @viirya

HyukjinKwon · 2020-07-24T09:20:49Z

retest this please

sql/core/src/main/scala/org/apache/spark/sql/execution/SparkSqlParser.scala

SparkQA · 2020-07-24T13:57:14Z

Test build #126491 has finished for PR 29146 at commit 2e92675.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

cloud-fan · 2020-07-24T14:34:18Z

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=.*)?

cloud-fan · 2020-07-24T16:30:15Z

BTW after #29202 , let's make sure SET and RESET are consistent after this PR.

viirya · 2020-07-24T20:27:40Z

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?

maropu · 2020-07-25T07:26:06Z

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.

cloud-fan · 2020-07-27T13:44:01Z

sql/core/src/main/scala/org/apache/spark/sql/execution/SparkSqlParser.scala

-    } else if (raw.nonEmpty) {
-      SetCommand(Some(raw.trim -> None))
+    if (ctx.configKey() != null) {
+      val keyStr = normalizeConfigString(ctx.configKey().getText)


do we have existing code to get the text of BACKQUOTED_IDENTIFIER?

No. At least, we don't have such an existing test case.

cloud-fan · 2020-07-27T13:45:00Z

sql/core/src/main/scala/org/apache/spark/sql/execution/SparkSqlParser.scala

+        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, " +


spaces -> special chars

cloud-fan · 2020-07-27T13:45:25Z

sql/core/src/main/scala/org/apache/spark/sql/execution/SparkSqlParser.scala

+        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)


please use quotes or string literal, e.g. ...

cloud-fan · 2020-07-27T13:46:13Z

sql/core/src/main/scala/org/apache/spark/sql/execution/SparkSqlParser.scala

+      remainder(ctx.RESET().getSymbol).trim match {
+        case s if s.isEmpty() => ResetCommand(None)
+        case _ => throw new ParseException("Expected format is 'RESET' or 'RESET key'. " +
+          "If you want to include spaces in key, please use quotes, " +


SparkQA · 2020-07-27T16:12:31Z

Test build #126650 has finished for PR 29146 at commit 62ed9cd.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-07-28T01:54:24Z

Test build #126674 has finished for PR 29146 at commit 31e3fcb.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

cloud-fan · 2020-07-28T05:15:48Z

sql/core/src/main/scala/org/apache/spark/sql/execution/SparkSqlParser.scala

 class SparkSqlAstBuilder(conf: SQLConf) extends AstBuilder(conf) {
  import org.apache.spark.sql.catalyst.parser.ParserUtils._

+  private def normalizeConfigString(s: String) =


I checked tableIdentifier, it includes BACKQUOTED_IDENTIFIER, and in AstBuilder we just use xxx.getText. Why does getText not work here?

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, then it seems back-quotes are removed by the ANTLR parser. Do you know something about that?

I found it. See PostProcessor.exitQuotedIdentifier. quotedIdentifier is a special parser rule which has post porcessor to remove back-quotes.

Oh, nice. I see...

SparkQA · 2020-07-28T05:41:41Z

Test build #126694 has finished for PR 29146 at commit 7a32197.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-07-31T00:42:23Z

Test build #126817 has finished for PR 29146 at commit 6d2221d.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

maropu · 2020-07-31T02:46:02Z

retest this please

SparkQA · 2020-07-31T07:05:02Z

Test build #126851 has finished for PR 29146 at commit 6d2221d.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

maropu · 2020-07-31T08:17:25Z

retest this please

HyukjinKwon · 2020-07-31T08:31:29Z

retest this please

SparkQA · 2020-07-31T12:44:56Z

Test build #126875 has finished for PR 29146 at commit 6d2221d.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-08-01T09:27:01Z

Test build #126913 has finished for PR 29146 at commit 61f5ffc.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-08-01T09:39:36Z

Test build #126915 has finished for PR 29146 at commit 4822210.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-08-02T10:14:44Z

Test build #126934 has finished for PR 29146 at commit 5a80e78.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

maropu · 2020-08-02T10:20:42Z

okay, ready to review. cc: @cloud-fan @viirya @HyukjinKwon

cloud-fan · 2020-08-03T03:05:12Z

sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4

    | unsupportedHiveNativeCommands .*?                                #failNativeCommand
    ;

+quotedConfigKey


hmm, is it necessary to create an alias? How about SET key= quotedIdentifier (EQ value=.*)?

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.

cloud-fan · 2020-08-03T03:06:36Z

sql/core/src/main/scala/org/apache/spark/sql/execution/SparkSqlParser.scala

-      SetCommand(Some(key -> Option(value)))
-    } else if (raw.nonEmpty) {
-      SetCommand(Some(raw.trim -> None))
+    val configKeyValueDef = """([a-zA-Z_\d\\.:]+)\s*=(.*)""".r


Can we put it in the class body so we don't need to compile the regex repeatedly?

sql/core/src/main/scala/org/apache/spark/sql/execution/SparkSqlParser.scala

cloud-fan · 2020-08-03T03:11:48Z

sql/core/src/test/scala/org/apache/spark/sql/execution/SparkSqlParserSuite.scala

+  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)


SQLConf also uses ConfigEntry, I think ConfigEntry.knownConfigs already covers all the registered configs.

cloud-fan · 2020-08-03T07:45:03Z

sql/core/src/main/scala/org/apache/spark/sql/execution/SparkSqlParser.scala

+
+  override def visitSetQuotedConfiguration(ctx: SetQuotedConfigurationContext)
+    : LogicalPlan = withOrigin(ctx) {
+    val keyStr = ctx.key.getText.replaceAll("`", "")


how about visitQuotedIdentifier(ctx.key)

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

SparkQA · 2020-08-03T09:27:05Z

Test build #126963 has finished for PR 29146 at commit b0e9686.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

@cloud-fan

@cloud-fan's brush-up

SparkQA · 2020-08-03T12:46:21Z

Test build #126976 has finished for PR 29146 at commit 3065ed9.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

cloud-fan · 2020-08-03T13:00:02Z

thanks, merging to master!

maropu · 2020-08-03T13:20:43Z

Thanks for the reviews!

viirya

late LGTM

probot-autolabeler bot added the SQL label Jul 17, 2020

maropu changed the title ~~[SPARK-32257][SQL] Reports explicit errors for invalid usage of SET command~~ [WIP][SPARK-32257][SQL] Reports explicit errors for invalid usage of SET command Jul 17, 2020

maropu commented Jul 20, 2020

View reviewed changes

maropu changed the title ~~[WIP][SPARK-32257][SQL] Reports explicit errors for invalid usage of SET command~~ [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET command Jul 20, 2020

HyukjinKwon reviewed Jul 24, 2020

View reviewed changes

sql/core/src/main/scala/org/apache/spark/sql/execution/SparkSqlParser.scala Show resolved Hide resolved

maropu force-pushed the SPARK-32257 branch from 2e92675 to 62ed9cd Compare July 27, 2020 13:38

maropu changed the title ~~[SPARK-32257][SQL] Reports explicit errors for invalid usage of SET command~~ [SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command Jul 27, 2020

cloud-fan reviewed Jul 27, 2020

View reviewed changes

maropu force-pushed the SPARK-32257 branch from c5a10e8 to 31e3fcb Compare July 27, 2020 23:44

maropu changed the title ~~[SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command~~ [WIP][SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command Jul 28, 2020

cloud-fan reviewed Jul 28, 2020

View reviewed changes

maropu added 2 commits July 30, 2020 21:26

bugfix

5a0b008

Fix

6d2221d

Fix

4822210

maropu force-pushed the SPARK-32257 branch from 61f5ffc to 4822210 Compare August 1, 2020 07:11

Fix

5a80e78

cloud-fan reviewed Aug 3, 2020

View reviewed changes

sql/core/src/main/scala/org/apache/spark/sql/execution/SparkSqlParser.scala Show resolved Hide resolved

cloud-fan reviewed Aug 3, 2020

View reviewed changes

sql/core/src/main/scala/org/apache/spark/sql/execution/SparkSqlParser.scala Show resolved Hide resolved

cloud-fan reviewed Aug 3, 2020

View reviewed changes

review

b0e9686

cloud-fan reviewed Aug 3, 2020

View reviewed changes

simplify

473353b

Merge pull request #4 from cloud-fan/help

3065ed9

@cloud-fan's brush-up

cloud-fan closed this in c6109ba Aug 3, 2020

viirya reviewed Aug 3, 2020

View reviewed changes

[SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command #29146

[SPARK-32257][SQL] Reports explicit errors for invalid usage of SET/RESET command #29146

Uh oh!

Conversation

maropu commented Jul 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

maropu commented Jul 17, 2020

Uh oh!

SparkQA commented Jul 17, 2020

Uh oh!

SparkQA commented Jul 18, 2020

Uh oh!

SparkQA commented Jul 18, 2020

Uh oh!

SparkQA commented Jul 18, 2020

Uh oh!

maropu commented Jul 20, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maropu commented Jul 20, 2020

Uh oh!

HyukjinKwon commented Jul 24, 2020

Uh oh!

Uh oh!

SparkQA commented Jul 24, 2020

Uh oh!

cloud-fan commented Jul 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cloud-fan commented Jul 24, 2020

Uh oh!

viirya commented Jul 24, 2020

Uh oh!

maropu commented Jul 25, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jul 27, 2020

Uh oh!

SparkQA commented Jul 28, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jul 28, 2020

Uh oh!

SparkQA commented Jul 31, 2020

Uh oh!

maropu commented Jul 31, 2020

Uh oh!

SparkQA commented Jul 31, 2020

Uh oh!

maropu commented Jul 31, 2020

Uh oh!

HyukjinKwon commented Jul 31, 2020

Uh oh!

SparkQA commented Jul 31, 2020

Uh oh!

SparkQA commented Aug 1, 2020

Uh oh!

SparkQA commented Aug 1, 2020

maropu commented Jul 17, 2020 •

edited

Loading

cloud-fan commented Jul 24, 2020 •

edited

Loading