[SPARK-14124] [SQL] [FOLLOWUP] Implement Database-related DDL Commands #12081

gatorsmile · 2016-03-31T06:58:53Z

What changes were proposed in this pull request?

First, a few test cases failed in mac OS X because the property value of java.io.tmpdir does not include a trailing slash on some platform. Hive always removes the last trailing slash. For example, what I got in the web:

Win NT  --> C:\TEMP\
Win XP  --> C:\TEMP
Solaris --> /var/tmp/
Linux   --> /var/tmp

Second, a couple of test cases are added to verify if the commands work properly.

How was this patch tested?

Added a test case for it and correct the previous test cases.

andrewor14 · 2016-05-03T23:01:15Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/SessionCatalog.scala


-  def getDefaultDBPath(db: String): String = {
+  /** Get the path for creating a non-default database. */
+  def createDatabasePath(db: String, path: Option[String]): String = {


actually this doesn't seem to belong to the session catalog. It's only used in 1 place so I would just move it to CreateDatabase itself.

Sure, will do

I did try it, but it involves a lot of code changes.

The major issue is the type mismatch between the path in case class CreateDatabase and the locationUri in CatalogDatabase. The first one has to be Option[String] since users might not provide location in the create table command. The second one has to be String since we always have the location.

https://github.com/gatorsmile/spark/blob/mkdir/sql/core/src/main/scala/org/apache/spark/sql/execution/command/ddl.scala#L63

If we change the type of locationUri in CatalogDatabase to Option[String], the related code changes look not clean.

let's just call this defaultDatabasePath

Sure, will do the change.

andrewor14 · 2016-05-03T23:10:26Z

@gatorsmile I took a quick look and I think the logic can be simpler. There are a lot of random checks that don't seem necessary to me. Actually I thought the original goal was to create the dir, but this patch doesn't seem to do that. I'm not sure what the non-test changes here achieve.

gatorsmile · 2016-05-04T01:54:12Z

@andrewor14 You know, this PR has many changes since its initial version. Originally, the focus of this PR is to create a correct warehouse directory. Later, a few related PRs were merged. Thus, I removed the contents. : )

Yeah, sure. I can clean it and try to create the dir.

cloud-fan · 2016-05-04T03:53:29Z

sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveDDLSuite.scala

+      val fs = new Path(path).getFileSystem(hiveContext.sessionState.newHadoopConf())
+      withTable(tabName) {
+        assert(tmpDir.listFiles.isEmpty)
+        sql(s"CREATE DATABASE $dbName Location '$tmpDir'")


If we create a database with a given location, then we drop this database, will we still keep the dir like external table?

The directory will be dropped by the Hive metastore API. The test case in this PR verified this behavior.
https://github.com/gatorsmile/spark/blob/mkdir/sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveDDLSuite.scala#L394

…NewNewNewNew

gatorsmile · 2016-05-04T06:00:24Z

@andrewor14 I also think about creating directory in implementation of create database.

Metastore APIs created the directory for us no matter whether the provided location/directory exists or not. The following test cases verified it.
https://github.com/gatorsmile/spark/blob/mkdir/sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveDDLSuite.scala#L401-L411

Should we still create the database directory?

If yes, could we do it in a separate PR? This PR has a long change history. It has been rebased 8 times.
It is hard for the future readers to follow what we did in this PR.

SparkQA · 2016-05-04T07:01:33Z

Test build #57717 has finished for PR 12081 at commit 59a9805.

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

SparkQA · 2016-05-04T07:20:47Z

Test build #57722 has finished for PR 12081 at commit b885f7b.

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

gatorsmile · 2016-05-04T15:09:21Z

: ) This one needs to be rebased again. 9 times

…wNewNewNewNew # Conflicts: # sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveDDLSuite.scala

SparkQA · 2016-05-04T18:59:14Z

Test build #57772 has finished for PR 12081 at commit 838eaeb.

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

yhuai · 2016-05-04T19:32:35Z

sql/core/src/main/scala/org/apache/spark/sql/execution/command/ddl.scala

        databaseName,
        comment.getOrElse(""),
-        path.getOrElse(catalog.getDefaultDBPath(databaseName)),
+        catalog.createDatabasePath(databaseName, path),


looking at it again. Seems we do not really need to pass in path, right? Also, how about we keep the name getDefulatDBPath because createDatabasePath implied file system operations (sorry....). But, it will be good to add comment to this method to explain its purpose.

oh, we have defaultTablePath in SessionCatalog. Let's use defaultDatabasePath as the better. What do you think?

yeah, actually how is new Path(p).toString different from p? Looks like that is the only functional difference here? If there's no particular reason why we need to do that then let's just revert all these changes here and only commit the test changes.

Yeah, it sounds like this issue has been fixed by the other PRs. Will revert it back.

Just FYI, getDefaultDBPath is just used for the following use case:

Get the path for creating a non-default database when database location is not provided by users.

yhuai · 2016-05-04T19:43:36Z

@gatorsmile Thank you for updating this. Can you address my comments? Then, let's get it in!

SparkQA · 2016-05-05T04:23:49Z

Test build #57830 has finished for PR 12081 at commit 55cf518.

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

SparkQA · 2016-05-05T04:27:03Z

Test build #57831 has finished for PR 12081 at commit b190b86.

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

andrewor14 · 2016-05-05T21:33:55Z

Alright, this is mainly just test changes now. LGTM merging into master 2.0.

#### What changes were proposed in this pull request? First, a few test cases failed in mac OS X because the property value of `java.io.tmpdir` does not include a trailing slash on some platform. Hive always removes the last trailing slash. For example, what I got in the web: ``` Win NT --> C:\TEMP\ Win XP --> C:\TEMP Solaris --> /var/tmp/ Linux --> /var/tmp ``` Second, a couple of test cases are added to verify if the commands work properly. #### How was this patch tested? Added a test case for it and correct the previous test cases. Author: gatorsmile <gatorsmile@gmail.com> Author: xiaoli <lixiao1983@gmail.com> Author: Xiao Li <xiaoli@Xiaos-MacBook-Pro.local> Closes #12081 from gatorsmile/mkdir. (cherry picked from commit 8cba57a) Signed-off-by: Andrew Or <andrew@databricks.com>

gatorsmile and others added 30 commits November 13, 2015 14:50

Merge remote-tracking branch 'upstream/master'

01e4cdf

Merge remote-tracking branch 'upstream/master'

6835704

Merge remote-tracking branch 'upstream/master'

9180687

SPARK-11633

b38a21e

Merge remote-tracking branch 'upstream/master' into joinMakeCopy

d2b84af

Merge remote-tracking branch 'upstream/master'

fda8025

Merge branch 'master' of https://github.com/gatorsmile/spark

ac0dccd

Merge remote-tracking branch 'upstream/master'

6e0018b

converge

0546772

converge

b37a64f

Merge remote-tracking branch 'upstream/master'

c2a872c

Merge remote-tracking branch 'upstream/master'

ab6dbd7

Merge remote-tracking branch 'upstream/master'

4276356

Merge remote-tracking branch 'upstream/master'

2dab708

Merge remote-tracking branch 'upstream/master'

0458770

Merge remote-tracking branch 'upstream/master'

1debdfa

Merge remote-tracking branch 'upstream/master'

763706d

Merge remote-tracking branch 'upstream/master'

4de6ec1

Merge remote-tracking branch 'upstream/master'

9422a4f

Merge remote-tracking branch 'upstream/master'

52bdf48

Merge remote-tracking branch 'upstream/master'

1e95df3

Merge remote-tracking branch 'upstream/master'

fab24cf

Merge remote-tracking branch 'upstream/master'

8b2e33b

Merge remote-tracking branch 'upstream/master'

2ee1876

Merge remote-tracking branch 'upstream/master'

b9f0090

Merge remote-tracking branch 'upstream/master'

ade6f7e

Merge remote-tracking branch 'upstream/master'

9fd63d2

Merge remote-tracking branch 'upstream/master'

5199d49

Merge remote-tracking branch 'upstream/master'

404214c

Merge remote-tracking branch 'upstream/master'

c001dd9

andrewor14 reviewed May 3, 2016
View reviewed changes

cloud-fan reviewed May 4, 2016
View reviewed changes

gatorsmile added 3 commits May 3, 2016 21:16

Merge remote-tracking branch 'upstream/master' into mkdirNewNewNewNew…

2ec4d59

…NewNewNewNew

address comments.

59a9805

added a new test case

b885f7b

Merge remote-tracking branch 'upstream/master'

8089c6f

gatorsmile added 2 commits May 4, 2016 10:20

Merge remote-tracking branch 'upstream/master'

a6c7518

Merge branch 'mkdirNewNewNewNewNewNewNewNew' into mkdirNewNewNewNewNe…

838eaeb

…wNewNewNewNew # Conflicts: # sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveDDLSuite.scala

yhuai reviewed May 4, 2016
View reviewed changes

gatorsmile added 2 commits May 4, 2016 19:50

address comments

55cf518

address comments

b190b86

asfgit closed this in 8cba57a May 5, 2016

[SPARK-14124] [SQL] [FOLLOWUP] Implement Database-related DDL Commands #12081

[SPARK-14124] [SQL] [FOLLOWUP] Implement Database-related DDL Commands #12081

Uh oh!

Conversation

gatorsmile commented Mar 31, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrewor14 commented May 3, 2016

Uh oh!

gatorsmile commented May 4, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gatorsmile May 4, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gatorsmile commented May 4, 2016

Uh oh!

SparkQA commented May 4, 2016

Uh oh!

SparkQA commented May 4, 2016

Uh oh!

gatorsmile commented May 4, 2016

Uh oh!

SparkQA commented May 4, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrewor14 May 4, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yhuai commented May 4, 2016

Uh oh!

SparkQA commented May 5, 2016

Uh oh!

SparkQA commented May 5, 2016

Uh oh!

andrewor14 commented May 5, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

gatorsmile commented Mar 31, 2016 •

edited

Loading

gatorsmile May 4, 2016 •

edited

Loading

andrewor14 May 4, 2016 •

edited

Loading