[GitHub] spark pull request #15230: [SPARK-17657] [SQL] Disallow Users to Change Tabl...

[asfgit]

Github user asfgit closed the pull request at:

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


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request #15230: [SPARK-17657] [SQL] Disallow Users to Change Tabl...

[gatorsmile]

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

https://github.com/apache/spark/pull/15230#discussion_r83149074
  
--- Diff: 
sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveExternalCatalog.scala ---
@@ -111,6 +111,10 @@ private[spark] class HiveExternalCatalog(conf: 
SparkConf, hadoopConf: Configurat
 s"as table property keys may not start with '$DATASOURCE_PREFIX' 
or '$STATISTICS_PREFIX':" +
 s" ${invalidKeys.mkString("[", ", ", "]")}")
 }
+// External users are not allowed to set/switch the table type.
--- End diff --

Sure, will do it. Thanks!


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request #15230: [SPARK-17657] [SQL] Disallow Users to Change Tabl...

[gatorsmile]

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

https://github.com/apache/spark/pull/15230#discussion_r83138864
  
--- Diff: 
sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveExternalCatalog.scala ---
@@ -111,6 +111,10 @@ private[spark] class HiveExternalCatalog(conf: 
SparkConf, hadoopConf: Configurat
 s"as table property keys may not start with '$DATASOURCE_PREFIX' 
or '$STATISTICS_PREFIX':" +
 s" ${invalidKeys.mkString("[", ", ", "]")}")
 }
+// External users are not allowed to set/switch the table type.
+if (table.properties.contains("EXTERNAL")) {
--- End diff --

I tried Hive. Hive only accepts `EXTERNAL` if users want to change the 
table type. That means, if users do it like `external` or `ExterRnal`, Hive 
just treats it as a regular property key. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request #15230: [SPARK-17657] [SQL] Disallow Users to Change Tabl...

[cloud-fan]

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

https://github.com/apache/spark/pull/15230#discussion_r83138671
  
--- Diff: 
sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveExternalCatalog.scala ---
@@ -111,6 +111,10 @@ private[spark] class HiveExternalCatalog(conf: 
SparkConf, hadoopConf: Configurat
 s"as table property keys may not start with '$DATASOURCE_PREFIX' 
or '$STATISTICS_PREFIX':" +
 s" ${invalidKeys.mkString("[", ", ", "]")}")
 }
+// External users are not allowed to set/switch the table type.
+if (table.properties.contains("EXTERNAL")) {
--- End diff --

should we be case-insensitive here? e.g. `external`, `ExteRNal`, etc. are 
all not allowed


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request #15230: [SPARK-17657] [SQL] Disallow Users to Change Tabl...

[gatorsmile]

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

https://github.com/apache/spark/pull/15230#discussion_r83006784
  
--- Diff: 
sql/core/src/main/scala/org/apache/spark/sql/execution/command/ddl.scala ---
@@ -225,6 +225,11 @@ case class AlterTableSetPropertiesCommand(
 val catalog = sparkSession.sessionState.catalog
 val table = catalog.getTableMetadata(tableName)
 DDLUtils.verifyAlterTableType(catalog, table, isView)
+// Not allowed to switch the table type.
+if (properties.contains("EXTERNAL")) {
--- End diff --

In the initial fix, it is only for Hive. : )

Moving it to `verifyTableProperties` sounds good to me. It also covers 
another case. Users are unable to set `EXTERNAL` in `CREATE TABLE`. Let me do 
it. Thanks!


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request #15230: [SPARK-17657] [SQL] Disallow Users to Change Tabl...

[cloud-fan]

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

https://github.com/apache/spark/pull/15230#discussion_r82990434
  
--- Diff: 
sql/core/src/main/scala/org/apache/spark/sql/execution/command/ddl.scala ---
@@ -225,6 +225,11 @@ case class AlterTableSetPropertiesCommand(
 val catalog = sparkSession.sessionState.catalog
 val table = catalog.getTableMetadata(tableName)
 DDLUtils.verifyAlterTableType(catalog, table, isView)
+// Not allowed to switch the table type.
+if (properties.contains("EXTERNAL")) {
--- End diff --

Then should we put this check in 
`HiveExternalCatalog.verifyTableProperties`? I think it's a hive specific 
limitation.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request #15230: [SPARK-17657] [SQL] Disallow Users to Change Tabl...

[gatorsmile]

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

https://github.com/apache/spark/pull/15230#discussion_r82940270
  
--- Diff: 
sql/core/src/main/scala/org/apache/spark/sql/execution/command/ddl.scala ---
@@ -225,6 +225,11 @@ case class AlterTableSetPropertiesCommand(
 val catalog = sparkSession.sessionState.catalog
 val table = catalog.getTableMetadata(tableName)
 DDLUtils.verifyAlterTableType(catalog, table, isView)
+// Not allowed to switch the table type.
+if (properties.contains("EXTERNAL")) {
--- End diff --

This is officially documented in the Hive document, as shown in the 
[link](https://cwiki.apache.org/confluence/display/Hive/LanguageManual+DDL):
`TBLPROPERTIES ("EXTERNAL"="TRUE") in release 0.6.0+ (HIVE-1329) – Change 
a managed table to an external table and vice versa for "FALSE".`

This is the only property users are not allowed to change. The other 
Hive-specific properties are still allowed to change, because Hive also allows 
it. 

For the our Spark-reserved properties, users are not allowed to change. See 
the function call `verifyTableProperties` in 
`[alterTable](https://github.com/apache/spark/blob/b9a147181d5e38d9abed0c7215f4c5cb695f579c/sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveExternalCatalog.scala#L393)`.
 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request #15230: [SPARK-17657] [SQL] Disallow Users to Change Tabl...

[cloud-fan]

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

https://github.com/apache/spark/pull/15230#discussion_r80376396
  
--- Diff: 
sql/hive/src/main/scala/org/apache/spark/sql/hive/client/HiveClientImpl.scala 
---
@@ -427,6 +427,11 @@ private[hive] class HiveClientImpl(
   }
 
   override def alterTable(tableName: String, table: CatalogTable): Unit = 
withHiveState {
+// Not allowed to switch the table type
+if (table.properties.contains("EXTERNAL")) {
--- End diff --

this check doesn't work for `InMemoryCatalog`, we should think about a 
center place for this kind of checks


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request #15230: [SPARK-17657] [SQL] Disallow Users to Change Tabl...

[gatorsmile]

GitHub user gatorsmile opened a pull request:

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

[SPARK-17657] [SQL] Disallow Users to Change Table Type 

### What changes were proposed in this pull request?
Hive allows users to change the table type from `Managed` to `External` or 
from `External` to `Managed` by altering table's property `EXTERNAL`. See the 
JIRA: https://issues.apache.org/jira/browse/HIVE-1329

So far, Spark SQL does not correctly support it, although users can do it. 
Many assumptions are broken in the implementation. Thus, this PR is to disallow 
users to do it. 

### How was this patch tested?
Added test cases

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

$ git pull https://github.com/gatorsmile/spark alterTableSetExternal

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

https://github.com/apache/spark/pull/15230.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 #15230


commit f028b5ec4730f658c545b6920e4af79ce5acc957
Author: gatorsmile 
Date:   2016-09-24T06:02:46Z

fix.




---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

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