[GitHub] spark pull request #21001: [SPARK-19724][SQL][FOLLOW-UP]Check location of ma...

[asfgit]

Github user asfgit closed the pull request at:

https://github.com/apache/spark/pull/21001


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #21001: [SPARK-19724][SQL][FOLLOW-UP]Check location of ma...

[cloud-fan]

Github user cloud-fan commented on a diff in the pull request:

https://github.com/apache/spark/pull/21001#discussion_r180397137
  
--- Diff: 
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/InMemoryCatalog.scala
 ---
@@ -187,38 +187,32 @@ class InMemoryCatalog(
 val db = tableDefinition.identifier.database.get
 requireDbExists(db)
 val table = tableDefinition.identifier.table
-if (tableExists(db, table)) {
-  if (!ignoreIfExists) {
-throw new TableAlreadyExistsException(db = db, table = table)
--- End diff --

We should not remove this logic here, even it's also done in 
`SessionCatalog`


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #21001: [SPARK-19724][SQL][FOLLOW-UP]Check location of ma...

[gatorsmile]

Github user gatorsmile commented on a diff in the pull request:

https://github.com/apache/spark/pull/21001#discussion_r180297255
  
--- Diff: 
sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveDDLSuite.scala 
---
@@ -1459,11 +1459,6 @@ class HiveDDLSuite
   sql(s"ALTER TABLE tbl UNSET TBLPROPERTIES 
('${forbiddenPrefix}foo')")
 }
 assert(e2.getMessage.contains(forbiddenPrefix + "foo"))
-
-val e3 = intercept[AnalysisException] {
-  sql(s"CREATE TABLE tbl (a INT) TBLPROPERTIES 
('${forbiddenPrefix}foo'='anything')")
-}
-assert(e3.getMessage.contains(forbiddenPrefix + "foo"))
--- End diff --

Instead of deleting the test, we should change the name of the table


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #21001: [SPARK-19724][SQL][FOLLOW-UP]Check location of ma...

[gengliangwang]

Github user gengliangwang commented on a diff in the pull request:

https://github.com/apache/spark/pull/21001#discussion_r180190850
  
--- Diff: 
sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveDDLSuite.scala 
---
@@ -1459,11 +1459,6 @@ class HiveDDLSuite
   sql(s"ALTER TABLE tbl UNSET TBLPROPERTIES 
('${forbiddenPrefix}foo')")
 }
 assert(e2.getMessage.contains(forbiddenPrefix + "foo"))
-
-val e3 = intercept[AnalysisException] {
-  sql(s"CREATE TABLE tbl (a INT) TBLPROPERTIES 
('${forbiddenPrefix}foo'='anything')")
-}
-assert(e3.getMessage.contains(forbiddenPrefix + "foo"))
--- End diff --

The error message is 
```
"Table or view 'tbl' already exists in database 'default';"
```
This is soft of behavior change, as we check table existence first.


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #21001: [SPARK-19724][SQL][FOLLOW-UP]Check location of ma...

[gengliangwang]

Github user gengliangwang commented on a diff in the pull request:

https://github.com/apache/spark/pull/21001#discussion_r180191279
  
--- Diff: 
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/catalog/ExternalCatalogSuite.scala
 ---
@@ -167,15 +167,6 @@ abstract class ExternalCatalogSuite extends 
SparkFunSuite with BeforeAndAfterEac
 assert(actual.tableType === CatalogTableType.EXTERNAL)
   }
 
-  test("create table when the table already exists") {
--- End diff --

We check table existence in `SessionCatalog` instead of `ExternalCatalog` 
now. 
`SessionCatalogSuite` has similar test case, so no need to create new one.


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #21001: [SPARK-19724][SQL][FOLLOW-UP]Check location of ma...

[gengliangwang]

GitHub user gengliangwang opened a pull request:

https://github.com/apache/spark/pull/21001

[SPARK-19724][SQL][FOLLOW-UP]Check location of managed table when 
ignoreIfExists is true

## What changes were proposed in this pull request?

In the PR #20886, I mistakenly check the table location only when 
`ignoreIfExists` is false, which was following the original deprecated PR.
That was wrong. When `ignoreIfExists` is true and the target table doesn't 
exist, we should also check the table location. In other word, 
**`ignoreIfExists` has nothing to do with table location validation**.
This is a follow-up PR to fix the mistake. 

## How was this patch tested?

Add one unit test.


You can merge this pull request into a Git repository by running:

$ git pull https://github.com/gengliangwang/spark SPARK-19724-followup

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/spark/pull/21001.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #21001


commit 3d1c90960f88042c51012f1b4df8eaffb73994c8
Author: Gengliang Wang 
Date:   2018-04-06T17:37:55Z

SPARK-19724 follow up to fix ignoreIfExists branch




---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org