Skip to content

[SPARK-15421][SQL] Validate DDL property values #13205

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
andrewor14 wants to merge 2 commits into from
Closed

[SPARK-15421][SQL] Validate DDL property values #13205

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a61b260
Validate table and database properties
May 19, 2016
3920d2a
Add tests
May 19, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 36 additions & 9 deletions sql/core/src/main/scala/org/apache/spark/sql/execution/SparkSqlParser.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -293,7 +293,7 @@ class SparkSqlAstBuilder(conf: SQLConf) extends AstBuilder {
if (external) {
throw operationNotAllowed("CREATE EXTERNAL TABLE ... USING", ctx)
}
val options = Option(ctx.tablePropertyList).map(visitTablePropertyList).getOrElse(Map.empty)
val options = Option(ctx.tablePropertyList).map(visitPropertyKeyValues).getOrElse(Map.empty)
val provider = ctx.tableProvider.qualifiedName.getText
val partitionColumnNames =
Option(ctx.partitionColumnNames)
Expand Down Expand Up @@ -352,6 +352,7 @@ class SparkSqlAstBuilder(conf: SQLConf) extends AstBuilder {

/**
* Convert a table property list into a key-value map.
* This should be called through [[visitPropertyKeyValues]] or [[visitPropertyKeys]].
*/
override def visitTablePropertyList(
ctx: TablePropertyListContext): Map[String, String] = withOrigin(ctx) {
Expand All @@ -362,6 +363,32 @@ class SparkSqlAstBuilder(conf: SQLConf) extends AstBuilder {
}.toMap
}

/**
* Parse a key-value map from a [[TablePropertyListContext]], assuming all values are specified.
*/
private def visitPropertyKeyValues(ctx: TablePropertyListContext): Map[String, String] = {
val props = visitTablePropertyList(ctx)
val badKeys = props.filter { case (_, v) => v == null }.keys
if (badKeys.nonEmpty) {
throw operationNotAllowed(
s"Values must be specified for key(s): ${badKeys.mkString("[", ",", "]")}", ctx)
}
props
}

/**
* Parse a list of keys from a [[TablePropertyListContext]], assuming no values are specified.
*/
private def visitPropertyKeys(ctx: TablePropertyListContext): Seq[String] = {
val props = visitTablePropertyList(ctx)
val badKeys = props.filter { case (_, v) => v != null }.keys
if (badKeys.nonEmpty) {
throw operationNotAllowed(
s"Values should not be specified for key(s): ${badKeys.mkString("[", ",", "]")}", ctx)
}
props.keys.toSeq
}

/**
* A table property key can either be String or a collection of dot separated elements. This
* function extracts the property key based on whether its a string literal or a table property
Expand Down Expand Up @@ -390,7 +417,7 @@ class SparkSqlAstBuilder(conf: SQLConf) extends AstBuilder {
ctx.EXISTS != null,
Option(ctx.locationSpec).map(visitLocationSpec),
Option(ctx.comment).map(string),
Option(ctx.tablePropertyList).map(visitTablePropertyList).getOrElse(Map.empty))
Option(ctx.tablePropertyList).map(visitPropertyKeyValues).getOrElse(Map.empty))
}

/**
Expand All @@ -405,7 +432,7 @@ class SparkSqlAstBuilder(conf: SQLConf) extends AstBuilder {
ctx: SetDatabasePropertiesContext): LogicalPlan = withOrigin(ctx) {
AlterDatabaseProperties(
ctx.identifier.getText,
visitTablePropertyList(ctx.tablePropertyList))
visitPropertyKeyValues(ctx.tablePropertyList))
}

/**
Expand Down Expand Up @@ -521,7 +548,7 @@ class SparkSqlAstBuilder(conf: SQLConf) extends AstBuilder {
ctx: SetTablePropertiesContext): LogicalPlan = withOrigin(ctx) {
AlterTableSetProperties(
visitTableIdentifier(ctx.tableIdentifier),
visitTablePropertyList(ctx.tablePropertyList),
visitPropertyKeyValues(ctx.tablePropertyList),
ctx.VIEW != null)
}

Expand All @@ -538,7 +565,7 @@ class SparkSqlAstBuilder(conf: SQLConf) extends AstBuilder {
ctx: UnsetTablePropertiesContext): LogicalPlan = withOrigin(ctx) {
AlterTableUnsetProperties(
visitTableIdentifier(ctx.tableIdentifier),
visitTablePropertyList(ctx.tablePropertyList).keys.toSeq,
visitPropertyKeys(ctx.tablePropertyList),
ctx.EXISTS != null,
ctx.VIEW != null)
}
Expand All @@ -556,7 +583,7 @@ class SparkSqlAstBuilder(conf: SQLConf) extends AstBuilder {
AlterTableSerDeProperties(
visitTableIdentifier(ctx.tableIdentifier),
Option(ctx.STRING).map(string),
Option(ctx.tablePropertyList).map(visitTablePropertyList),
Option(ctx.tablePropertyList).map(visitPropertyKeyValues),
// TODO a partition spec is allowed to have optional values. This is currently violated.
Option(ctx.partitionSpec).map(visitNonOptionalPartitionSpec))
}
Expand Down Expand Up @@ -764,7 +791,7 @@ class SparkSqlAstBuilder(conf: SQLConf) extends AstBuilder {
val comment = Option(ctx.STRING).map(string)
val partitionCols = Option(ctx.partitionColumns).toSeq.flatMap(visitCatalogColumns)
val cols = Option(ctx.columns).toSeq.flatMap(visitCatalogColumns)
val properties = Option(ctx.tablePropertyList).map(visitTablePropertyList).getOrElse(Map.empty)
val properties = Option(ctx.tablePropertyList).map(visitPropertyKeyValues).getOrElse(Map.empty)
val selectQuery = Option(ctx.query).map(plan)

// Note: Hive requires partition columns to be distinct from the schema, so we need
Expand Down Expand Up @@ -925,7 +952,7 @@ class SparkSqlAstBuilder(conf: SQLConf) extends AstBuilder {
import ctx._
EmptyStorageFormat.copy(
serde = Option(string(name)),
serdeProperties = Option(tablePropertyList).map(visitTablePropertyList).getOrElse(Map.empty))
serdeProperties = Option(tablePropertyList).map(visitPropertyKeyValues).getOrElse(Map.empty))
}

/**
Expand Down Expand Up @@ -982,7 +1009,7 @@ class SparkSqlAstBuilder(conf: SQLConf) extends AstBuilder {
comment = Option(ctx.STRING).map(string),
schema,
ctx.query,
Option(ctx.tablePropertyList).map(visitTablePropertyList).getOrElse(Map.empty),
Option(ctx.tablePropertyList).map(visitPropertyKeyValues).getOrElse(Map.empty),
ctx.EXISTS != null,
ctx.REPLACE != null,
ctx.TEMPORARY != null
Expand Down
41 changes: 41 additions & 0 deletions sql/core/src/test/scala/org/apache/spark/sql/execution/command/DDLCommandSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,12 @@ class DDLCommandSuite extends PlanTest {
comparePlans(parsed, expected)
}

test("create database - property values must be set") {
assertUnsupported(
sql = "CREATE DATABASE my_db WITH DBPROPERTIES('key_without_value', 'key_with_value'='x')",
containsThesePhrases = Seq("key_without_value"))
}

test("drop database") {
val sql1 = "DROP DATABASE IF EXISTS database_name RESTRICT"
val sql2 = "DROP DATABASE IF EXISTS database_name CASCADE"
Expand Down Expand Up @@ -121,6 +127,12 @@ class DDLCommandSuite extends PlanTest {
comparePlans(parsed2, expected2)
}

test("alter database - property values must be set") {
assertUnsupported(
sql = "ALTER DATABASE my_db SET DBPROPERTIES('key_without_value', 'key_with_value'='x')",
containsThesePhrases = Seq("key_without_value"))
}

test("describe database") {
// DESCRIBE DATABASE [EXTENDED] db_name;
val sql1 = "DESCRIBE DATABASE EXTENDED db_name"
Expand Down Expand Up @@ -228,6 +240,16 @@ class DDLCommandSuite extends PlanTest {
}
}

test("create table - property values must be set") {
assertUnsupported(
sql = "CREATE TABLE my_tab TBLPROPERTIES('key_without_value', 'key_with_value'='x')",
containsThesePhrases = Seq("key_without_value"))
assertUnsupported(
sql = "CREATE TABLE my_tab ROW FORMAT SERDE 'serde' " +
"WITH SERDEPROPERTIES('key_without_value', 'key_with_value'='x')",
containsThesePhrases = Seq("key_without_value"))
}

test("create table - location implies external") {
val query = "CREATE TABLE my_tab LOCATION '/something/anything'"
parser.parsePlan(query) match {
Expand Down Expand Up @@ -349,6 +371,18 @@ class DDLCommandSuite extends PlanTest {
comparePlans(parsed3_view, expected3_view)
}

test("alter table - property values must be set") {
assertUnsupported(
sql = "ALTER TABLE my_tab SET TBLPROPERTIES('key_without_value', 'key_with_value'='x')",
containsThesePhrases = Seq("key_without_value"))
}

test("alter table unset properties - property values must NOT be set") {
assertUnsupported(
sql = "ALTER TABLE my_tab UNSET TBLPROPERTIES('key_without_value', 'key_with_value'='x')",
containsThesePhrases = Seq("key_with_value"))
}

test("alter table: SerDe properties") {
val sql1 = "ALTER TABLE table_name SET SERDE 'org.apache.class'"
val sql2 =
Expand Down Expand Up @@ -404,6 +438,13 @@ class DDLCommandSuite extends PlanTest {
comparePlans(parsed5, expected5)
}

test("alter table - SerDe property values must be set") {
assertUnsupported(
sql = "ALTER TABLE my_tab SET SERDE 'serde' " +
"WITH SERDEPROPERTIES('key_without_value', 'key_with_value'='x')",
containsThesePhrases = Seq("key_without_value"))
}

// ALTER TABLE table_name ADD [IF NOT EXISTS] PARTITION partition_spec
// [LOCATION 'location1'] partition_spec [LOCATION 'location2'] ...;
test("alter table: add partition") {
Expand Down