[SPARK-14481] [SQL] Issue Exceptions for All Unsupported Options during Parsing #12255
Conversation
|
Let us see how many test cases will fail after the changes. |
|
Test build #55329 has finished for PR 12255 at commit
|
|
Test build #55344 has finished for PR 12255 at commit
|
gatorsmile
changed the title
| logWarning("EXPLAIN FORMATTED option is ignored.") | ||
| logWarning("Unsupported operation: EXPLAIN FORMATTED option") | ||
| } | ||
| if (options.exists(_.LOGICAL != null)) { |
There was a problem hiding this comment.
I think we don't need to throw an exception here. LOGICAL is more or less the opposite of EXTENDED. So I think we can safely ignore it instead of logging a warning or throwing an exception.
There was a problem hiding this comment.
sure, will remove it. Thanks!
|
Test build #55361 has finished for PR 12255 at commit
|
|
retest this please |
|
Test build #55357 has finished for PR 12255 at commit
|
|
Test build #55368 has finished for PR 12255 at commit
|
|
The code is ready for review. Thanks! @yhuai @andrewor14 @hvanhovell |
| val (table, temp, ifNotExists, external) = visitCreateTableHeader(ctx.createTableHeader) | ||
| if (external) { | ||
| logWarning("EXTERNAL option is not supported.") | ||
| throw new ParseException("Unsupported operation: EXTERNAL option", ctx) |
There was a problem hiding this comment.
Should we ignore it? In this case, we are creating data source tables. They should be EXTERNAL? right?
There was a problem hiding this comment.
better to throw exception
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Just feel free to let me know if anything is missing in this PR. Thanks!
|
Test build #55417 has finished for PR 12255 at commit
|
| sql( | ||
| """CREATE VIEW IF NOT EXISTS | ||
| |default.testView (c1 COMMENT 'blabla', c2 COMMENT 'blabla') | ||
| |COMMENT 'blabla' |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Can you create a jira for supporting that?
There was a problem hiding this comment.
Sure, will do it. Thanks!
There was a problem hiding this comment.
Create a PR: #12288 for this missing support. Thanks!
|
Thanks. Merging to master. |
5 participants
What changes were proposed in this pull request?
"Not good to slightly ignore all the un-supported options/clauses. We should either support it or throw an exception." A comment from @yhuai in another PR #12146
Explainbe an exception? TheFormattedclause is used inHiveCompatibilitySuite.Drop Tableare handled in a separate PR: [SPARK-14362] [SPARK-14406] [SQL] DDL Native Support: Drop View and Drop Table #12146How was this patch tested?
Test cases are added to verify all the cases.