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-5264][SQL] Support drop temporary table [if exists] DDL command #4060

Closed
wants to merge 7 commits into from
Closed

[SPARK-5264][SQL] Support drop temporary table [if exists] DDL command #4060

wants to merge 7 commits into from

Conversation

OopsOutOfMemory
Copy link
Contributor

DROP [TEMPORARY] TABLE [IF EXISTS] tbl_name

Also, refer to http://dev.mysql.com/doc/refman/5.0/en/drop-table.html

@OopsOutOfMemory
Copy link
Contributor Author

@marmbrus @yhuai
Could you review this ?

@liancheng
Copy link
Contributor

ok to test

@SparkQA
Copy link

SparkQA commented Jan 26, 2015

Test build #26108 has finished for PR 4060 at commit df5d929.

  • This patch fails Spark unit tests.
  • This patch does not merge cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Jan 27, 2015

Test build #26143 has finished for PR 4060 at commit 1e47f8a.

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

@SparkQA
Copy link

SparkQA commented Jan 27, 2015

Test build #26144 has finished for PR 4060 at commit dc1c9a0.

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

@OopsOutOfMemory OopsOutOfMemory changed the title [SPARK-5264][SQL] support drop table DDL command [SPARK-5264][SQL] support drop temporary table [if exists] DDL command Jan 29, 2015
@OopsOutOfMemory OopsOutOfMemory changed the title [SPARK-5264][SQL] support drop temporary table [if exists] DDL command [SPARK-5264][SQL] Support drop temporary table [if exists] DDL command Jan 29, 2015
@OopsOutOfMemory OopsOutOfMemory force-pushed the drop_table branch 2 times, most recently from e6f836b to 43ae1da Compare January 29, 2015 09:34
@OopsOutOfMemory
Copy link
Contributor Author

Hi, @liancheng
Would you mind review this ?

@OopsOutOfMemory
Copy link
Contributor Author

/cc @marmbrus

@SparkQA
Copy link

SparkQA commented Jan 29, 2015

Test build #26306 has finished for PR 4060 at commit e6f836b.

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

@SparkQA
Copy link

SparkQA commented Jan 29, 2015

Test build #26309 has finished for PR 4060 at commit 43ae1da.

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

@SparkQA
Copy link

SparkQA commented Jan 29, 2015

Test build #26311 has finished for PR 4060 at commit f047fda.

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

liancheng
liancheng reviewed Jan 29, 2015
View reviewed changes
sql/core/src/main/scala/org/apache/spark/sql/sources/ddl.scala
(DROP ~> TEMPORARY.? <~ TABLE) ~ (IF ~ EXISTS).? ~ ident ^^ {
case temp ~ exists ~ tableName => DropTable(tableName, exists.nonEmpty, temp.nonEmpty)
}
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Please omit the parenthesis. We only need it for something like this:

lazy val someRule = 
  ( branchA
  | branchB
  | branchC
  )

@SparkQA
Copy link

SparkQA commented Feb 2, 2015

Test build #26505 has finished for PR 4060 at commit 998eb3b.

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

@SparkQA
Copy link

SparkQA commented Feb 2, 2015

Test build #26507 has finished for PR 4060 at commit 00ecc25.

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

@OopsOutOfMemory
Copy link
Contributor Author

Updated.
/cc @marmbrus @yhuai @liancheng

@SparkQA
Copy link

SparkQA commented Feb 2, 2015

Test build #26517 has finished for PR 4060 at commit b7146cc.

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

marmbrus
marmbrus reviewed Feb 2, 2015
View reviewed changes
sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveQl.scala
import org.apache.spark.sql.types._
import org.apache.spark.sql.sources.{DropTable => LogicalDropTable}
Copy link
Contributor

Choose a reason for hiding this comment

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

The convention when referencing ambiguous names is to do sources.DropTable.

@SparkQA
Copy link

SparkQA commented Feb 4, 2015

Test build #26764 has finished for PR 4060 at commit e7299aa.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • case class DropHiveTable(

@OopsOutOfMemory
Copy link
Contributor Author

yeah, I use a single logical operator here.
If handle the type of the table and then delegate.
Is that ok to do like below? we need catch exceptions in strategy. If not found I will construct a new MetastoreRelation for placeholder, it would be nice if you can give me some advice : )

try {
val resolvedTable = context.executePlan(UnresolvedRelation(drop.tableIdent)).analyzed
resolvedTable match {
case t: MetastoreRelation =>
ExecutedCommand(DropHiveTable(t, drop.isExists, drop.temporary)) :: Nil
}
}
catch {
case e: InvalidTableException if drop.isExists =>
ExecutedCommand(DropHiveTable(MetastoreRelation("","",None, ), drop.isExists, drop.temporary)) :: Nil
}
...

marmbrus
marmbrus reviewed Mar 12, 2015
View reviewed changes
sql/core/src/main/scala/org/apache/spark/sql/sources/ddl.scala
isExists: Boolean,
temporary: Boolean) extends Command

private[sql] case class DropTempTable(
Copy link
Contributor

Choose a reason for hiding this comment

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

Spell out temporary here.

@marmbrus
Copy link
Contributor

Thanks for working on this! Please merge with master and eliminate conflicts.

@marmbrus
Copy link
Contributor

marmbrus commented Apr 3, 2015

Thanks again for working on this. However, to keep the PR queue small, I propose we close this issue until you have time to update it.

@AmplabJenkins
Copy link

Can one of the admins verify this patch?

@OopsOutOfMemory
Copy link
Contributor Author

OK, Thanks!

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
5 participants
@OopsOutOfMemory @liancheng @SparkQA @marmbrus @AmplabJenkins