Skip to content

[SPARK-22934] [SQL] Make optional clauses order insensitive for CREATE TABLE SQL statement #20133

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
gatorsmile wants to merge 4 commits into from
Closed

[SPARK-22934] [SQL] Make optional clauses order insensitive for CREATE TABLE SQL statement #20133

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8ae8f18
fix
gatorsmile Jan 1, 2018
0894f5e
fix.
gatorsmile Jan 1, 2018
68170bb
fix.
gatorsmile Jan 2, 2018
9818ab5
nit
gatorsmile Jan 2, 2018
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
Prev Previous commit
Next Next commit
fix.
  • Loading branch information
@gatorsmile
gatorsmile committed Jan 2, 2018
commit 68170bb45c64bb5b694bfdffd7c7f02801f9b82e
8 changes: 2 additions & 6 deletions sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/ParserUtils.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,12 +41,8 @@ object ParserUtils {
throw new ParseException(s"Operation not allowed: $message", ctx)
}

def duplicateClausesNotAllowed(message: String, ctx: ParserRuleContext): Nothing = {
throw new ParseException(s"Found duplicate clauses: $message", ctx)
}

def checkDuplicateClauses(
nodes: util.List[TerminalNode], clauseName: String, ctx: ParserRuleContext): Unit = {
def checkDuplicateClauses[T](
nodes: util.List[T], clauseName: String, ctx: ParserRuleContext): Unit = {
if (nodes.size() > 1) {
throw new ParseException(s"Found duplicate clauses: $clauseName", ctx)
}
Expand Down
48 changes: 15 additions & 33 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 @@ -383,11 +383,11 @@ class SparkSqlAstBuilder(conf: SQLConf) extends AstBuilder(conf) {
* {{{
* CREATE [TEMPORARY] TABLE [IF NOT EXISTS] [db_name.]table_name
* USING table_provider
* [OPTIONS table_property_list]
* create_table_clauses
* [[AS] select_statement];
*
* create_table_clauses (order insensitive):
* [OPTIONS table_property_list]
* [PARTITIONED BY (col_name, col_name, ...)]
Copy link
Member

@viirya viirya Jan 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't [OPTIONS table_property_list] one of create_table_clauses?

Copy link
Member Author

@gatorsmile gatorsmile Jan 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

forgot it.

* [CLUSTERED BY (col_name, col_name, ...)
* [SORTED BY (col_name [ASC|DESC], ...)]
Expand All @@ -408,6 +408,8 @@ class SparkSqlAstBuilder(conf: SQLConf) extends AstBuilder(conf) {
checkDuplicateClauses(ctx.OPTIONS, "OPTIONS", ctx)
checkDuplicateClauses(ctx.PARTITIONED, "PARTITIONED BY", ctx)
checkDuplicateClauses(ctx.COMMENT, "COMMENT", ctx)
checkDuplicateClauses(ctx.bucketSpec(), "CLUSTERED BY", ctx)
checkDuplicateClauses(ctx.locationSpec, "LOCATION", ctx)

val options = Option(ctx.options).map(visitPropertyKeyValues).getOrElse(Map.empty)
val provider = ctx.tableProvider.qualifiedName.getText
Expand All @@ -417,17 +419,9 @@ class SparkSqlAstBuilder(conf: SQLConf) extends AstBuilder(conf) {
.map(visitIdentifierList(_).toArray)
.getOrElse(Array.empty[String])
val properties = Option(ctx.tableProps).map(visitPropertyKeyValues).getOrElse(Map.empty)
val bucketSpec = if (ctx.bucketSpec().size > 1) {
duplicateClausesNotAllowed("CLUSTERED BY", ctx)
} else {
ctx.bucketSpec().asScala.headOption.map(visitBucketSpec)
}
val bucketSpec = ctx.bucketSpec().asScala.headOption.map(visitBucketSpec)

val location = if (ctx.locationSpec.size > 1) {
duplicateClausesNotAllowed("LOCATION", ctx)
} else {
ctx.locationSpec.asScala.headOption.map(visitLocationSpec)
}
val location = ctx.locationSpec.asScala.headOption.map(visitLocationSpec)
val storage = DataSource.buildStorageFormatFromOptions(options)

if (location.isDefined && storage.locationUri.isDefined) {
Expand Down Expand Up @@ -1131,16 +1125,16 @@ class SparkSqlAstBuilder(conf: SQLConf) extends AstBuilder(conf) {
checkDuplicateClauses(ctx.TBLPROPERTIES, "TBLPROPERTIES", ctx)
checkDuplicateClauses(ctx.PARTITIONED, "PARTITIONED BY", ctx)
checkDuplicateClauses(ctx.COMMENT, "COMMENT", ctx)
checkDuplicateClauses(ctx.bucketSpec(), "CLUSTERED BY", ctx)
checkDuplicateClauses(ctx.createFileFormat, "STORED AS/BY", ctx)
checkDuplicateClauses(ctx.rowFormat, "ROW FORMAT", ctx)
checkDuplicateClauses(ctx.locationSpec, "LOCATION", ctx)

val dataCols = Option(ctx.columns).map(visitColTypeList).getOrElse(Nil)
val partitionCols = Option(ctx.partitionColumns).map(visitColTypeList).getOrElse(Nil)
val properties = Option(ctx.tableProps).map(visitPropertyKeyValues).getOrElse(Map.empty)
Copy link
Contributor

@cloud-fan cloud-fan Jan 3, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's the meaning of ctx.tableProps now? the union of all TABLE PROPERTY list?

Copy link
Member Author

@gatorsmile gatorsmile Jan 3, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The last one, if we have multiple clauses. However, we blocks this in the above checks.

val selectQuery = Option(ctx.query).map(plan)
val bucketSpec = if (ctx.bucketSpec().size > 1) {
duplicateClausesNotAllowed("CLUSTERED BY", ctx)
} else {
ctx.bucketSpec().asScala.headOption.map(visitBucketSpec)
}
val bucketSpec = ctx.bucketSpec().asScala.headOption.map(visitBucketSpec)

// Note: Hive requires partition columns to be distinct from the schema, so we need
// to include the partition columns here explicitly
Expand All @@ -1149,23 +1143,11 @@ class SparkSqlAstBuilder(conf: SQLConf) extends AstBuilder(conf) {
// Storage format
val defaultStorage = HiveSerDe.getDefaultStorage(conf)
validateRowFormatFileFormat(ctx.rowFormat.asScala, ctx.createFileFormat.asScala, ctx)
val fileStorage = if (ctx.createFileFormat.size > 1) {
duplicateClausesNotAllowed("STORED AS/BY", ctx)
} else {
ctx.createFileFormat.asScala.headOption.map(visitCreateFileFormat)
.getOrElse(CatalogStorageFormat.empty)
}
val rowStorage = if (ctx.rowFormat.size > 1) {
duplicateClausesNotAllowed("ROW FORMAT", ctx)
} else {
ctx.rowFormat.asScala.headOption.map(visitRowFormat)
.getOrElse(CatalogStorageFormat.empty)
}
val location = if (ctx.locationSpec.size > 1) {
duplicateClausesNotAllowed("LOCATION", ctx)
} else {
ctx.locationSpec.asScala.headOption.map(visitLocationSpec)
}
val fileStorage = ctx.createFileFormat.asScala.headOption.map(visitCreateFileFormat)
.getOrElse(CatalogStorageFormat.empty)
val rowStorage = ctx.rowFormat.asScala.headOption.map(visitRowFormat)
.getOrElse(CatalogStorageFormat.empty)
val location = ctx.locationSpec.asScala.headOption.map(visitLocationSpec)
// If we are creating an EXTERNAL table, then the LOCATION field is required
if (external && location.isEmpty) {
operationNotAllowed("CREATE EXTERNAL TABLE must be accompanied by LOCATION", ctx)
Expand Down