[SPARK-16459][SQL] Prevent dropping current database by dongjoon-hyun · Pull Request #14115 · apache/spark

dongjoon-hyun · 2016-07-09T10:48:48Z

What changes were proposed in this pull request?

This PR prevents dropping the current database to avoid errors like the followings.

scala> sql("create database delete_db")
scala> sql("use delete_db")
scala> sql("drop database delete_db")
scala> sql("create table t as select 1")
org.apache.spark.sql.catalyst.analysis.NoSuchDatabaseException: Database `delete_db` not found;

How was this patch tested?

Pass the Jenkins tests including an updated testcase.

SparkQA · 2016-07-09T12:39:14Z

Test build #62022 has finished for PR 14115 at commit 3029821.

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

rxin · 2016-07-09T21:01:28Z

I think it's scary to do this. It'd be better to forbid dropping the current database. This is Postgres' behavior:

testdb1=# drop database testdb1;
ERROR:  cannot drop the currently open database

dongjoon-hyun · 2016-07-09T21:04:34Z

Sure. No problem!

dongjoon-hyun · 2016-07-09T21:04:50Z

I'll fix this one first. This one is easier. :)

…nt database is dropped

rxin · 2016-07-09T21:38:54Z

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

-    if (dbName == "default") {
-      throw new AnalysisException(s"Can not drop default database")
+    if (dbName == DEFAULT_DATABASE || dbName == getCurrentDatabase) {
+      throw new AnalysisException(s"Can not drop `${DEFAULT_DATABASE}` or current database")


hm i think we should break this into two messages rather than a single one.

I see. No problem.

…ent db.

dongjoon-hyun · 2016-07-09T22:00:24Z

Now, it preserves the previous behavior for dropping default db, and throws new exception message for new cases only.

rxin · 2016-07-09T22:22:25Z

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

-    if (dbName == "default") {
+    if (dbName == DEFAULT_DATABASE) {
      throw new AnalysisException(s"Can not drop default database")
+    } else if (dbName == getCurrentDatabase) {


do we need to check case sensitivity?

Oops. formatDatabaseName returns the raw string for case sensitive case.
Hm. That is the same situation with "default" database, isn't?

I'll add two testcases for both and fix this.

Oh, I was confused. For case sensitive case, we don't need to handle that.
Dropping with different names will fails due to lookup failure.

SparkQA · 2016-07-09T22:43:52Z

Test build #62032 has finished for PR 14115 at commit ac5f3ea.

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

SparkQA · 2016-07-09T23:19:16Z

Test build #62034 has finished for PR 14115 at commit 8aa1c2d.

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

SparkQA · 2016-07-10T00:56:42Z

Test build #62037 has finished for PR 14115 at commit 805b2f7.

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

srowen · 2016-07-10T11:20:01Z

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

    hadoopConf: Configuration) extends Logging {
  import CatalogTypes.TablePartitionSpec

+  val DEFAULT_DATABASE = "default"


Nit: this should be in an object SessionCatalog?

Oh, you're right. object is the best to keep this one. Thank you for review, @srowen .

Oh, I remember why I didn't do that. In fact, we don't have object SessionCatalog, yet.
So, I was not sure I could introduce new object for just one constant variable. Is it enough valuable, @srowen and @rxin ?

Anyway, I'll make that first.
It's easy to revert one commit.
And, if possible, I prefer to have object SessionCatalog for the future

dongjoon-hyun · 2016-07-10T18:46:38Z

Thank you, @srowen . It's done!

SparkQA · 2016-07-10T20:32:14Z

Test build #62062 has finished for PR 14115 at commit 1702c7e.

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

dongjoon-hyun · 2016-07-10T20:49:57Z

Hi, @rxin .
Could you review this again?

dongjoon-hyun · 2016-07-10T21:15:29Z

After merging this, I'll add the same prevention for information_schema, too.

rxin · 2016-07-11T00:45:49Z

This looks alright.

cc @hvanhovell can you review and merge?

dongjoon-hyun · 2016-07-11T09:44:47Z

Hi, @hvanhovell .
Could you review this PR, too?

hvanhovell · 2016-07-11T10:03:40Z

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

   */
  private[sql] def reset(): Unit = synchronized {
-    val default = "default"
+    val default = DEFAULT_DATABASE


NIT: Shouldn't we just replace the val default by DEFAULT_DATABASE?

dongjoon-hyun · 2016-07-11T10:16:55Z

Thank you for review, @hvanhovell .
I replaced the local variable.

hvanhovell · 2016-07-11T10:30:35Z

LGTM - pending jenkins.

SparkQA · 2016-07-11T12:33:51Z

Test build #3177 has finished for PR 14115 at commit 19a3160.

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

SparkQA · 2016-07-11T12:50:29Z

Test build #62086 has finished for PR 14115 at commit 19a3160.

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

dongjoon-hyun · 2016-07-11T16:04:44Z

Thank you for review and merging, @rxin and @hvanhovell .

rxin · 2016-07-11T16:29:23Z

@hvanhovell please leave a message on the pr next time when you merge one -- and also indicate the branch this was merged in (master in this case).

rxin · 2016-07-11T16:36:04Z

Should this be in 2.0?

dongjoon-hyun · 2016-07-11T16:40:27Z

If possible, I hope. :)

This PR prevents dropping the current database to avoid errors like the followings. ```scala scala> sql("create database delete_db") scala> sql("use delete_db") scala> sql("drop database delete_db") scala> sql("create table t as select 1") org.apache.spark.sql.catalyst.analysis.NoSuchDatabaseException: Database `delete_db` not found; ``` Pass the Jenkins tests including an updated testcase. Author: Dongjoon Hyun <dongjoon@apache.org> Closes #14115 from dongjoon-hyun/SPARK-16459. (cherry picked from commit 7ac79da) Signed-off-by: Herman van Hovell <hvanhovell@databricks.com>

hvanhovell · 2016-07-11T17:14:20Z

Cherry picked this into 2.0.

dongjoon-hyun added 2 commits July 9, 2016 14:07

[SPARK-16459][SQL] Curren database should become default when curre…

48c19c8

…nt database is dropped

Prevent dropping the current database instead.

ac5f3ea

dongjoon-hyun changed the title ~~[SPARK-16459][SQL] Curren database should become default when current database is dropped~~ [SPARK-16459][SQL] Prevent dropping current database Jul 9, 2016

rxin reviewed Jul 9, 2016
View reviewed changes

Keep the current behavior and add new error message for dropping curr…

8aa1c2d

…ent db.

rxin reviewed Jul 9, 2016
View reviewed changes

Fix HiveContextCompatibilitySuite not to try drop current db.

805b2f7

srowen reviewed Jul 10, 2016
View reviewed changes

Make SessionCatalog object.

1702c7e

dongjoon-hyun mentioned this pull request Jul 10, 2016

[SPARK-16452][SQL] Support basic INFORMATION_SCHEMA #14116

Closed

hvanhovell reviewed Jul 11, 2016
View reviewed changes

Remove useless local variable.

19a3160

asfgit closed this in 7ac79da Jul 11, 2016

dongjoon-hyun mentioned this pull request Jul 14, 2016

[SPARK-16529][SQL][TEST] withTempDatabase should set default database before dropping #14184

Closed

dongjoon-hyun deleted the SPARK-16459 branch July 20, 2016 07:44

roczei mentioned this pull request Sep 9, 2022

[SPARK-35242][SQL] Support changing session catalog's default database #37679

Closed

Conversation

dongjoon-hyun commented Jul 9, 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!

SparkQA commented Jul 9, 2016

Uh oh!

rxin commented Jul 9, 2016

Uh oh!

dongjoon-hyun commented Jul 9, 2016

Uh oh!

dongjoon-hyun commented Jul 9, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun commented Jul 9, 2016

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!

dongjoon-hyun Jul 9, 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!

SparkQA commented Jul 9, 2016

Uh oh!

SparkQA commented Jul 9, 2016

Uh oh!

SparkQA commented Jul 10, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun Jul 10, 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!

dongjoon-hyun commented Jul 10, 2016

Uh oh!

SparkQA commented Jul 10, 2016

Uh oh!

dongjoon-hyun commented Jul 10, 2016

Uh oh!

dongjoon-hyun commented Jul 10, 2016

Uh oh!

rxin commented Jul 11, 2016

Uh oh!

dongjoon-hyun commented Jul 11, 2016

Uh oh!

hvanhovell Jul 11, 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!

dongjoon-hyun commented Jul 11, 2016

Uh oh!

hvanhovell commented Jul 11, 2016

Uh oh!

SparkQA commented Jul 11, 2016

Uh oh!

SparkQA commented Jul 11, 2016

Uh oh!

dongjoon-hyun commented Jul 11, 2016

Uh oh!

rxin commented Jul 11, 2016

Uh oh!

rxin commented Jul 11, 2016

Uh oh!

dongjoon-hyun commented Jul 11, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

dongjoon-hyun commented Jul 9, 2016 •

edited

Loading

dongjoon-hyun Jul 9, 2016 •

edited

Loading

dongjoon-hyun Jul 10, 2016 •

edited

Loading

hvanhovell Jul 11, 2016 •

edited

Loading

dongjoon-hyun commented Jul 11, 2016 •

edited

Loading