You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@spark.apache.org by li...@apache.org on 2017/07/03 16:35:54 UTC
spark git commit: [SPARK-21102][SQL] Refresh command is too
aggressive in parsing
Repository: spark
Updated Branches:
refs/heads/master eb7a5a66b -> 17bdc36ef
[SPARK-21102][SQL] Refresh command is too aggressive in parsing
### Idea
This PR adds validation to REFRESH sql statements. Currently, users can specify whatever they want as resource path. For example, spark.sql("REFRESH ! $ !") will be executed without any exceptions.
### Implementation
I am not sure that my current implementation is the most optimal, so any feedback is appreciated. My first idea was to make the grammar as strict as possible. Unfortunately, there were some problems. I tried the approach below:
SqlBase.g4
```
...
| REFRESH TABLE tableIdentifier #refreshTable
| REFRESH resourcePath #refreshResource
...
resourcePath
: STRING
| (IDENTIFIER | number | nonReserved | '/' | '-')+ // other symbols can be added if needed
;
```
It is not flexible enough and requires to explicitly mention all possible symbols. Therefore, I came up with the current approach that is implemented in the code.
Let me know your opinion on which one is better.
Author: aokolnychyi <an...@sap.com>
Closes #18368 from aokolnychyi/spark-21102.
Project: http://git-wip-us.apache.org/repos/asf/spark/repo
Commit: http://git-wip-us.apache.org/repos/asf/spark/commit/17bdc36e
Tree: http://git-wip-us.apache.org/repos/asf/spark/tree/17bdc36e
Diff: http://git-wip-us.apache.org/repos/asf/spark/diff/17bdc36e
Branch: refs/heads/master
Commit: 17bdc36ef16a544b693c628db276fe32db87fe7a
Parents: eb7a5a6
Author: aokolnychyi <an...@sap.com>
Authored: Mon Jul 3 09:35:49 2017 -0700
Committer: gatorsmile <ga...@gmail.com>
Committed: Mon Jul 3 09:35:49 2017 -0700
----------------------------------------------------------------------
.../apache/spark/sql/catalyst/parser/SqlBase.g4 | 2 +-
.../spark/sql/execution/SparkSqlParser.scala | 20 ++++++++++++++++---
.../sql/execution/SparkSqlParserSuite.scala | 21 +++++++++++++++++++-
3 files changed, 38 insertions(+), 5 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/spark/blob/17bdc36e/sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4
----------------------------------------------------------------------
diff --git a/sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4 b/sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4
index 7ffa150..29f5544 100644
--- a/sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4
+++ b/sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4
@@ -149,7 +149,7 @@ statement
| (DESC | DESCRIBE) TABLE? option=(EXTENDED | FORMATTED)?
tableIdentifier partitionSpec? describeColName? #describeTable
| REFRESH TABLE tableIdentifier #refreshTable
- | REFRESH .*? #refreshResource
+ | REFRESH (STRING | .*?) #refreshResource
| CACHE LAZY? TABLE tableIdentifier (AS? query)? #cacheTable
| UNCACHE TABLE (IF EXISTS)? tableIdentifier #uncacheTable
| CLEAR CACHE #clearCache
http://git-wip-us.apache.org/repos/asf/spark/blob/17bdc36e/sql/core/src/main/scala/org/apache/spark/sql/execution/SparkSqlParser.scala
----------------------------------------------------------------------
diff --git a/sql/core/src/main/scala/org/apache/spark/sql/execution/SparkSqlParser.scala b/sql/core/src/main/scala/org/apache/spark/sql/execution/SparkSqlParser.scala
index 3c58c6e..2b79eb5 100644
--- a/sql/core/src/main/scala/org/apache/spark/sql/execution/SparkSqlParser.scala
+++ b/sql/core/src/main/scala/org/apache/spark/sql/execution/SparkSqlParser.scala
@@ -230,11 +230,25 @@ class SparkSqlAstBuilder(conf: SQLConf) extends AstBuilder(conf) {
}
/**
- * Create a [[RefreshTable]] logical plan.
+ * Create a [[RefreshResource]] logical plan.
*/
override def visitRefreshResource(ctx: RefreshResourceContext): LogicalPlan = withOrigin(ctx) {
- val resourcePath = remainder(ctx.REFRESH.getSymbol).trim
- RefreshResource(resourcePath)
+ val path = if (ctx.STRING != null) string(ctx.STRING) else extractUnquotedResourcePath(ctx)
+ RefreshResource(path)
+ }
+
+ private def extractUnquotedResourcePath(ctx: RefreshResourceContext): String = withOrigin(ctx) {
+ val unquotedPath = remainder(ctx.REFRESH.getSymbol).trim
+ validate(
+ unquotedPath != null && !unquotedPath.isEmpty,
+ "Resource paths cannot be empty in REFRESH statements. Use / to match everything",
+ ctx)
+ val forbiddenSymbols = Seq(" ", "\n", "\r", "\t")
+ validate(
+ !forbiddenSymbols.exists(unquotedPath.contains(_)),
+ "REFRESH statements cannot contain ' ', '\\n', '\\r', '\\t' inside unquoted resource paths",
+ ctx)
+ unquotedPath
}
/**
http://git-wip-us.apache.org/repos/asf/spark/blob/17bdc36e/sql/core/src/test/scala/org/apache/spark/sql/execution/SparkSqlParserSuite.scala
----------------------------------------------------------------------
diff --git a/sql/core/src/test/scala/org/apache/spark/sql/execution/SparkSqlParserSuite.scala b/sql/core/src/test/scala/org/apache/spark/sql/execution/SparkSqlParserSuite.scala
index bd9c2eb..d238c76 100644
--- a/sql/core/src/test/scala/org/apache/spark/sql/execution/SparkSqlParserSuite.scala
+++ b/sql/core/src/test/scala/org/apache/spark/sql/execution/SparkSqlParserSuite.scala
@@ -25,7 +25,7 @@ import org.apache.spark.sql.catalyst.expressions.{Ascending, Concat, SortOrder}
import org.apache.spark.sql.catalyst.parser.ParseException
import org.apache.spark.sql.catalyst.plans.logical.{LogicalPlan, Project, RepartitionByExpression, Sort}
import org.apache.spark.sql.execution.command._
-import org.apache.spark.sql.execution.datasources.CreateTable
+import org.apache.spark.sql.execution.datasources.{CreateTable, RefreshResource}
import org.apache.spark.sql.internal.{HiveSerDe, SQLConf}
import org.apache.spark.sql.types.{IntegerType, LongType, StringType, StructType}
@@ -66,6 +66,25 @@ class SparkSqlParserSuite extends AnalysisTest {
}
}
+ test("refresh resource") {
+ assertEqual("REFRESH prefix_path", RefreshResource("prefix_path"))
+ assertEqual("REFRESH /", RefreshResource("/"))
+ assertEqual("REFRESH /path///a", RefreshResource("/path///a"))
+ assertEqual("REFRESH pat1h/112/_1a", RefreshResource("pat1h/112/_1a"))
+ assertEqual("REFRESH pat1h/112/_1a/a-1", RefreshResource("pat1h/112/_1a/a-1"))
+ assertEqual("REFRESH path-with-dash", RefreshResource("path-with-dash"))
+ assertEqual("REFRESH \'path with space\'", RefreshResource("path with space"))
+ assertEqual("REFRESH \"path with space 2\"", RefreshResource("path with space 2"))
+ intercept("REFRESH a b", "REFRESH statements cannot contain")
+ intercept("REFRESH a\tb", "REFRESH statements cannot contain")
+ intercept("REFRESH a\nb", "REFRESH statements cannot contain")
+ intercept("REFRESH a\rb", "REFRESH statements cannot contain")
+ intercept("REFRESH a\r\nb", "REFRESH statements cannot contain")
+ intercept("REFRESH @ $a$", "REFRESH statements cannot contain")
+ intercept("REFRESH ", "Resource paths cannot be empty in REFRESH statements")
+ intercept("REFRESH", "Resource paths cannot be empty in REFRESH statements")
+ }
+
test("show functions") {
assertEqual("show functions", ShowFunctionsCommand(None, None, true, true))
assertEqual("show all functions", ShowFunctionsCommand(None, None, true, true))
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@spark.apache.org
For additional commands, e-mail: commits-help@spark.apache.org