Skip to content
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

Jump to bottom

[SPARK-15711][SQL] Ban CREATE TEMPORARY TABLE USING AS SELECT #13451

Closed
wants to merge 3 commits into from
Closed

[SPARK-15711][SQL] Ban CREATE TEMPORARY TABLE USING AS SELECT #13451

wants to merge 3 commits into from

Conversation

clockfly
Copy link
Contributor

@clockfly clockfly commented Jun 1, 2016 •
edited
Loading

What changes were proposed in this pull request?

This PR bans syntax like CREATE TEMPORARY TABLE USING AS SELECT

CREATE TEMPORARY TABLE ... USING ... AS ... is not properly implemented, the temporary data is not cleaned up when the session exits. Before a full fix, we probably should ban this syntax.

This PR only impact syntax like CREATE TEMPORARY TABLE ... USING ... AS ....
Other syntax like CREATE TEMPORARY TABLE .. USING ... and CREATE TABLE ... USING ... are not impacted.

How was this patch tested?

Unit test.

@clockfly clockfly force-pushed the ban_create_temp_table_using_as branch from e48caf0 to fa1db6a Compare June 1, 2016 20:35
@andrewor14
Copy link
Contributor

Looks good. Can you add a test for the new exception that you throw?

@SparkQA
Copy link

SparkQA commented Jun 1, 2016

Test build #59770 has finished for PR 13451 at commit fa1db6a.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Jun 1, 2016

Test build #59776 has finished for PR 13451 at commit 9bedd70.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

gatorsmile
gatorsmile reviewed Jun 2, 2016
View reviewed changes
sql/hive/src/test/scala/org/apache/spark/sql/hive/MetastoreDataSourcesSuite.scala
@@ -1104,4 +1104,22 @@ class MetastoreDataSourcesSuite extends QueryTest with SQLTestUtils with TestHiv
}
}
}

test("CTAS: disallow create temporary table ... using ... as ...") {
Copy link
Member

Choose a reason for hiding this comment

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

Can you move this test case to HiveDDLCommandSuite, which contains the Hive-specific parser test cases? You also can see the other CTAS test cases there. Thanks!

Copy link
Contributor Author

@clockfly clockfly Jun 2, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

@gatorsmile Thanks. The test should be included in CreateTableAsSelectSuite

HiveDDLCommandSuite contains "create table as select", CreateTableAsSelectSuite contains "create table ... using ... as select ..."

Copy link
Member

Choose a reason for hiding this comment

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

Normally, parser-specific test cases are put in the same suite. I am fine if you put it to the CTAS-specific test suite, especially when we will support it in the near future.

@clockfly clockfly force-pushed the ban_create_temp_table_using_as branch 5 times, most recently from b7bb43d to 90fb80a Compare June 2, 2016 05:54
@clockfly clockfly changed the title [SPARK-15711][SQL][WIP]Ban CREATE TEMPORARY TABLE USING AS SELECT [SPARK-15711][SQL]Ban CREATE TEMPORARY TABLE USING AS SELECT Jun 2, 2016
@clockfly clockfly changed the title [SPARK-15711][SQL]Ban CREATE TEMPORARY TABLE USING AS SELECT [SPARK-15711][SQL] Ban CREATE TEMPORARY TABLE USING AS SELECT Jun 2, 2016
@clockfly clockfly force-pushed the ban_create_temp_table_using_as branch from 90fb80a to 2f0355f Compare June 2, 2016 06:07
@clockfly
Copy link
Contributor Author

clockfly commented Jun 2, 2016

cc @cloud-fan @liancheng @andrewor14

gatorsmile
gatorsmile reviewed Jun 2, 2016
View reviewed changes
sql/core/src/test/scala/org/apache/spark/sql/sources/CreateTableAsSelectSuite.scala

checkAnswer(
sql("SELECT a, b FROM jsonTable"),
sql("SELECT a, b FROM jt").collect())
Copy link
Member

Choose a reason for hiding this comment

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

Nit: you do not need to call collect

@cloud-fan
Copy link
Contributor

LGTM except some minor issues which are already pointed out by others.

@clockfly
Copy link
Contributor Author

clockfly commented Jun 2, 2016

Thanks @gatorsmile

@gatorsmile
Copy link
Member

LGTM. Thanks!

@SparkQA
Copy link

SparkQA commented Jun 2, 2016

Test build #59818 has finished for PR 13451 at commit d4f5ab6.

  • This patch passes all tests.
  • This patch does not merge cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Jun 2, 2016

Test build #59822 has finished for PR 13451 at commit b7bb43d.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Jun 2, 2016

Test build #59828 has finished for PR 13451 at commit 90fb80a.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Jun 2, 2016

Test build #59830 has finished for PR 13451 at commit 2f0355f.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Jun 2, 2016

Test build #59834 has finished for PR 13451 at commit aaca7f0.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

tejasapatil
tejasapatil reviewed Jun 2, 2016
View reviewed changes
sql/core/src/main/scala/org/apache/spark/sql/execution/SparkSqlParser.scala
@@ -317,17 +317,19 @@ class SparkSqlAstBuilder(conf: SQLConf) extends AstBuilder {
// Get the backing query.
val query = plan(ctx.query)

if (temp) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: can you please rename this to temporary

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably not in the scope of this PR, temp is a existing variable defined at https://github.com/clockfly/spark/blob/aaca7f01882de19c9dda47500f7d4f03712d3383/sql/core/src/main/scala/org/apache/spark/sql/execution/SparkSqlParser.scala#L304.

@liancheng
Copy link
Contributor

@andrewor14 Could you please help sign off this one? Thanks!

@andrewor14
Copy link
Contributor

LGTM!

@andrewor14
Copy link
Contributor

Merging into amster 2.0

@asfgit asfgit closed this in d109a1b Jun 2, 2016
asfgit pushed a commit that referenced this pull request Jun 2, 2016
@clockfly
[SPARK-15711][SQL] Ban CREATE TEMPORARY TABLE USING AS SELECT
a55454e 
## What changes were proposed in this pull request?

This PR bans syntax like `CREATE TEMPORARY TABLE USING AS SELECT`

`CREATE TEMPORARY TABLE ... USING ... AS ...` is not properly implemented, the temporary data is not cleaned up when the session exits. Before a full fix, we probably should ban this syntax.

This PR only impact syntax like `CREATE TEMPORARY TABLE ... USING ... AS ...`.
Other syntax like `CREATE TEMPORARY TABLE .. USING ...` and `CREATE TABLE ... USING ...` are not impacted.

## How was this patch tested?

Unit test.

Author: Sean Zhong <seanzhong@databricks.com>

Closes #13451 from clockfly/ban_create_temp_table_using_as.

(cherry picked from commit d109a1b)
Signed-off-by: Andrew Or <andrew@databricks.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
8 participants
@clockfly @andrewor14 @SparkQA @cloud-fan @gatorsmile @liancheng @viirya @tejasapatil