Skip to content

[SPARK-14481] [SQL] Issue Exceptions for All Unsupported Options during Parsing #12255

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 6 commits into from
Closed

[SPARK-14481] [SQL] Issue Exceptions for All Unsupported Options during Parsing #12255

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
8abcb71
warning 2 exceptions
gatorsmile Apr 8, 2016
b1774f0
Explains are exceptions
gatorsmile Apr 8, 2016
12d9167
remove unnecessary options when creating a view.
gatorsmile Apr 8, 2016
bb8d78b
added test cases to cover all the change cases.
gatorsmile Apr 8, 2016
e98a5d3
address comments.
gatorsmile Apr 8, 2016
2440869
address comments.
gatorsmile Apr 9, 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
7 changes: 2 additions & 5 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 @@ -143,10 +143,7 @@ class SparkSqlAstBuilder extends AstBuilder {
override def visitExplain(ctx: ExplainContext): LogicalPlan = withOrigin(ctx) {
val options = ctx.explainOption.asScala
if (options.exists(_.FORMATTED != null)) {
logWarning("EXPLAIN FORMATTED option is ignored.")
}
if (options.exists(_.LOGICAL != null)) {
logWarning("EXPLAIN LOGICAL option is ignored.")
logWarning("Unsupported operation: EXPLAIN FORMATTED option")
}

// Create the explain comment.
Expand Down Expand Up @@ -206,7 +203,7 @@ class SparkSqlAstBuilder extends AstBuilder {
override def visitCreateTableUsing(ctx: CreateTableUsingContext): LogicalPlan = withOrigin(ctx) {
val (table, temp, ifNotExists, external) = visitCreateTableHeader(ctx.createTableHeader)
if (external) {
logWarning("EXTERNAL option is not supported.")
throw new ParseException("Unsupported operation: EXTERNAL option", ctx)
Copy link
Member Author

@gatorsmile gatorsmile Apr 8, 2016

Choose a reason for hiding this comment

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

Should we ignore it? In this case, we are creating data source tables. They should be EXTERNAL? right?

Copy link
Contributor

@andrewor14 andrewor14 Apr 8, 2016

Choose a reason for hiding this comment

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

better to throw exception

Copy link
Contributor

@yhuai yhuai Apr 8, 2016

Choose a reason for hiding this comment

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

Not really. That depends if they provide the location (see https://github.com/apache/spark/blob/master/sql/hive/src/main/scala/org/apache/spark/sql/hive/execution/commands.scala#L141-L149).

Copy link
Member Author

@gatorsmile gatorsmile Apr 8, 2016

Choose a reason for hiding this comment

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

I see. Then, we should check the values in Option to determine if users should specify EXTERNAL? If it does not match, we should issue an exception?

Copy link
Contributor

@yhuai yhuai Apr 8, 2016

Choose a reason for hiding this comment

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

https://github.com/apache/spark/blob/master/sql/hive/src/main/scala/org/apache/spark/sql/hive/execution/commands.scala#L141-L149 already takes care it for data source tables, right? At here, we can still just throw the exception for now because 1.6 does not support CREATE EXTERNAL TABLE ... USING.

Copy link
Member Author

@gatorsmile gatorsmile Apr 8, 2016

Choose a reason for hiding this comment

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

Ok, I see. Thanks!

Copy link
Member Author

@gatorsmile gatorsmile Apr 8, 2016

Choose a reason for hiding this comment

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

Just feel free to let me know if anything is missing in this PR. Thanks!

}
val options = Option(ctx.tablePropertyList).map(visitTablePropertyList).getOrElse(Map.empty)
val provider = ctx.tableProvider.qualifiedName.getText
Expand Down
10 changes: 9 additions & 1 deletion 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 @@ -665,7 +665,7 @@ class DDLCommandSuite extends PlanTest {
comparePlans(parsed2, expected2)
}

test("commands only available in HiveContext") {
test("unsupported operations") {
intercept[ParseException] {
parser.parsePlan("DROP TABLE D1.T1")
}
Expand All @@ -682,6 +682,14 @@ class DDLCommandSuite extends PlanTest {
|TBLPROPERTIES('prop1Key '= "prop1Val", ' `prop2Key` '= "prop2Val")
""".stripMargin)
}
intercept[ParseException] {
parser.parsePlan(
"""
|CREATE EXTERNAL TABLE oneToTenDef
|USING org.apache.spark.sql.sources
|OPTIONS (from '1', to '10')
""".stripMargin)
}
intercept[ParseException] {
parser.parsePlan("SELECT TRANSFORM (key, value) USING 'cat' AS (tKey, tValue) FROM testData")
}
Expand Down
16 changes: 10 additions & 6 deletions sql/hive/src/main/scala/org/apache/spark/sql/hive/execution/HiveSqlParser.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,14 +162,16 @@ class HiveSqlAstBuilder extends SparkSqlAstBuilder {

// Unsupported clauses.
if (temp) {
logWarning("TEMPORARY clause is ignored.")
throw new ParseException(s"Unsupported operation: TEMPORARY clause.", ctx)
}
if (ctx.bucketSpec != null) {
// TODO add this - we need cluster columns in the CatalogTable for this to work.
logWarning("CLUSTERED BY ... [ORDERED BY ...] INTO ... BUCKETS clause is ignored.")
throw new ParseException("Unsupported operation: " +
"CLUSTERED BY ... [ORDERED BY ...] INTO ... BUCKETS clause.", ctx)
Copy link
Contributor

@yhuai yhuai Apr 9, 2016

Choose a reason for hiding this comment

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

Just a note, we will support it and temporary keyword with the implementation of create table command.

Copy link
Member Author

@gatorsmile gatorsmile Apr 9, 2016

Choose a reason for hiding this comment

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

I see. Thank you for letting me know it.

Copy link
Contributor

@yhuai yhuai Apr 9, 2016

Choose a reason for hiding this comment

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

I need to double check if insert into hive command can actually understand these fields, if not, we should not support cluster by and order by for now.

}
if (ctx.skewSpec != null) {
logWarning("SKEWED BY ... ON ... [STORED AS DIRECTORIES] clause is ignored.")
throw new ParseException("Operation not allowed: " +
"SKEWED BY ... ON ... [STORED AS DIRECTORIES] clause.", ctx)
Copy link
Contributor

@yhuai yhuai Apr 9, 2016

Choose a reason for hiding this comment

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

For this one, how about we say it is not allowed.

Copy link
Member Author

@gatorsmile gatorsmile Apr 9, 2016

Choose a reason for hiding this comment

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

Ok, sure, will do

}

// Create the schema.
Expand Down Expand Up @@ -230,7 +232,7 @@ class HiveSqlAstBuilder extends SparkSqlAstBuilder {
throw new ParseException(s"Operation not allowed: partitioned views", ctx)
} else {
if (ctx.STRING != null) {
logWarning("COMMENT clause is ignored.")
throw new ParseException("Unsupported operation: COMMENT clause", ctx)
}
val identifiers = Option(ctx.identifierCommentList).toSeq.flatMap(_.identifierComment.asScala)
val schema = identifiers.map { ic =>
Expand Down Expand Up @@ -296,7 +298,8 @@ class HiveSqlAstBuilder extends SparkSqlAstBuilder {
recordReader: Token,
schemaLess: Boolean): HiveScriptIOSchema = {
if (recordWriter != null || recordReader != null) {
logWarning("Used defined record reader/writer classes are currently ignored.")
throw new ParseException(
"Unsupported operation: Used defined record reader/writer classes.", ctx)
}

// Decode and input/output format.
Expand Down Expand Up @@ -370,7 +373,8 @@ class HiveSqlAstBuilder extends SparkSqlAstBuilder {
ctx: TableFileFormatContext): CatalogStorageFormat = withOrigin(ctx) {
Copy link
Contributor

@yhuai yhuai Apr 8, 2016

Choose a reason for hiding this comment

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

What is InputDriver/OutputDriver?

Copy link
Member Author

@gatorsmile gatorsmile Apr 9, 2016

Choose a reason for hiding this comment

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

https://issues.apache.org/jira/browse/HIVE-1546 I checked the discussion. Based on what Carl Steinbach and Ashutosh Chauhan said,

Hive doesnt care about INPUTDRIVER and OUTPUTDRIVER

I also searched the code base of hive master and confirm they are not used, but the Hive parser accept them. Not sure if we should completely delete them from our Spark Parser? or keep issuing the exceptions. Thanks!

Copy link
Member Author

@gatorsmile gatorsmile Apr 9, 2016

Choose a reason for hiding this comment

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

I will also change it to Operation not allowed:

import ctx._
if (inDriver != null || outDriver != null) {
logWarning("INPUTDRIVER ... OUTPUTDRIVER ... clauses are ignored.")
throw new ParseException(
s"Operation not allowed: INPUTDRIVER ... OUTPUTDRIVER ... clauses", ctx)
}
EmptyStorageFormat.copy(
inputFormat = Option(string(inFmt)),
Expand Down
60 changes: 59 additions & 1 deletion sql/hive/src/test/scala/org/apache/spark/sql/hive/HiveDDLCommandSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,65 @@ class HiveDDLCommandSuite extends PlanTest {
assert(desc.properties == Map(("tbl_p1" -> "p11"), ("tbl_p2" -> "p22")))
}

test("unsupported operations") {
intercept[ParseException] {
parser.parsePlan(
"""
|CREATE TEMPORARY TABLE ctas2
|ROW FORMAT SERDE "org.apache.hadoop.hive.serde2.columnar.ColumnarSerDe"
|WITH SERDEPROPERTIES("serde_p1"="p1","serde_p2"="p2")
|STORED AS RCFile
|TBLPROPERTIES("tbl_p1"="p11", "tbl_p2"="p22")
|AS SELECT key, value FROM src ORDER BY key, value
""".stripMargin)
}
intercept[ParseException] {
parser.parsePlan(
"""CREATE TABLE ctas2
|STORED AS
|INPUTFORMAT "org.apache.hadoop.mapred.TextInputFormat"
|OUTPUTFORMAT "org.apache.hadoop.hive.ql.io.HiveIgnoreKeyTextOutputFormat"
|INPUTDRIVER "org.apache.hadoop.hive.howl.rcfile.RCFileInputDriver"
|OUTPUTDRIVER "org.apache.hadoop.hive.howl.rcfile.RCFileOutputDriver"
|AS SELECT key, value FROM src ORDER BY key, value
""".stripMargin)
}
intercept[ParseException] {
parser.parsePlan(
"""
|CREATE TABLE user_info_bucketed(user_id BIGINT, firstname STRING, lastname STRING)
|CLUSTERED BY(user_id) INTO 256 BUCKETS
|AS SELECT key, value FROM src ORDER BY key, value
""".stripMargin)
}
intercept[ParseException] {
parser.parsePlan(
"""
|CREATE TABLE user_info_bucketed(user_id BIGINT, firstname STRING, lastname STRING)
|SKEWED BY (key) ON (1,5,6)
|AS SELECT key, value FROM src ORDER BY key, value
""".stripMargin)
}
intercept[ParseException] {
parser.parsePlan(
"""
|SELECT TRANSFORM (key, value) USING 'cat' AS (tKey, tValue)
|ROW FORMAT SERDE 'org.apache.hadoop.hive.contrib.serde2.TypedBytesSerDe'
|RECORDREADER 'org.apache.hadoop.hive.contrib.util.typedbytes.TypedBytesRecordReader'
|FROM testData
""".stripMargin)
}
intercept[ParseException] {
parser.parsePlan(
"""
|CREATE OR REPLACE VIEW IF NOT EXISTS view1 (col1, col3)
|COMMENT 'blabla'
|TBLPROPERTIES('prop1Key'="prop1Val")
|AS SELECT * FROM tab1
""".stripMargin)
}
}

test("Invalid interval term should throw AnalysisException") {
def assertError(sql: String, errorMessage: String): Unit = {
val e = intercept[AnalysisException] {
Expand Down Expand Up @@ -277,7 +336,6 @@ class HiveDDLCommandSuite extends PlanTest {
"""
|CREATE OR REPLACE VIEW IF NOT EXISTS view1
|(col1, col3)
|COMMENT 'I cannot spell'
|TBLPROPERTIES('prop1Key'="prop1Val")
|AS SELECT * FROM tab1
""".stripMargin
Expand Down
2 changes: 1 addition & 1 deletion sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveCommandSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ class HiveCommandSuite extends QueryTest with SQLTestUtils with TestHiveSingleto
super.beforeAll()
sql(
"""
|CREATE EXTERNAL TABLE parquet_tab1 (c1 INT, c2 STRING)
|CREATE TABLE parquet_tab1 (c1 INT, c2 STRING)
Copy link
Contributor

@yhuai yhuai Apr 8, 2016

Choose a reason for hiding this comment

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

Do we need to change this?

Copy link
Member Author

@gatorsmile gatorsmile Apr 9, 2016

Choose a reason for hiding this comment

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

https://github.com/apache/spark/pull/12255/files#diff-1bb4f7bd5a2656f48bcd3c857167a11bR206
will issue an exception. Thus, we have to remove EXTERNAL

Copy link
Contributor

@yhuai yhuai Apr 9, 2016

Choose a reason for hiding this comment

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

oh, right. I did not notice the USING part.

|USING org.apache.spark.sql.parquet.DefaultSource
""".stripMargin)

Expand Down
1 change: 0 additions & 1 deletion sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/SQLQuerySuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1475,7 +1475,6 @@ class SQLQuerySuite extends QueryTest with SQLTestUtils with TestHiveSingleton {
sql(
"""CREATE VIEW IF NOT EXISTS
|default.testView (c1 COMMENT 'blabla', c2 COMMENT 'blabla')
|COMMENT 'blabla'
|TBLPROPERTIES ('a' = 'b')
Copy link
Contributor

@yhuai yhuai Apr 9, 2016

Choose a reason for hiding this comment

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

Do we need to change this?

Copy link
Member Author

@gatorsmile gatorsmile Apr 9, 2016

Choose a reason for hiding this comment

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

Yeah, before we just output a comment when users specify comment. Now we issues an exception.
https://github.com/apache/spark/pull/12255/files#diff-1ff3f3d67da039f3cb51cc96665a9216R235

That is why I removed it. Otherwise, we are unable to create this view.

Copy link
Contributor

@yhuai yhuai Apr 9, 2016

Choose a reason for hiding this comment

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

Can you create a jira for supporting that?

Copy link
Member Author

@gatorsmile gatorsmile Apr 9, 2016

Choose a reason for hiding this comment

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

Sure, will do it. Thanks!

Copy link
Member Author

@gatorsmile gatorsmile Apr 10, 2016

Choose a reason for hiding this comment

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

Create a PR: #12288 for this missing support. Thanks!

|AS SELECT * FROM jt""".stripMargin)
checkAnswer(sql("SELECT c1, c2 FROM testView ORDER BY c1"), (1 to 9).map(i => Row(i, i)))
Expand Down