[GitHub] spark pull request #15054: [SPARK-17502] [SQL] Fix Multiple Bugs in DDL Stat...

[asfgit]

Github user asfgit closed the pull request at:

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


---
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 #15054: [SPARK-17502] [SQL] Fix Multiple Bugs in DDL Stat...

[cloud-fan]

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

https://github.com/apache/spark/pull/15054#discussion_r79540609
  
--- Diff: 
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/catalog/SessionCatalogSuite.scala
 ---
@@ -444,7 +444,7 @@ class SessionCatalogSuite extends SparkFunSuite {
 assert(!catalog.tableExists(TableIdentifier("view1", Some("default"
   }
 
-  test("getTableMetadata on temporary views") {
+  test("getTableMetadata and getTempViewOrPermanentTableMetadata on 
temporary views") {
--- End diff --

looks like it's unnecessary to test `getTableMetadata` on temporary views, 
let's just test `getTempViewOrPermanentTableMetadata` here.


---
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 #15054: [SPARK-17502] [SQL] Fix Multiple Bugs in DDL Stat...

[cloud-fan]

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

https://github.com/apache/spark/pull/15054#discussion_r79540494
  
--- Diff: 
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/SessionCatalog.scala
 ---
@@ -282,6 +271,24 @@ class SessionCatalog(
   }
 
   /**
+   * Retrieve the metadata of an existing temporary view or permanent 
table/view.
+   * If the temporary view does not exist, tries to get the metadata an 
existing permanent
+   * table/view. If no database is specified, assume the table/view is in 
the current database.
+   * If the specified table/view is not found in the database then a 
[[NoSuchTableException]] is
+   * thrown.
+   */
+  def getTempViewOrPermanentTableMetadata(name: TableIdentifier): 
CatalogTable = synchronized {
--- End diff --

it should just take a string.


---
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 #15054: [SPARK-17502] [SQL] Fix Multiple Bugs in DDL Stat...

[gatorsmile]

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

https://github.com/apache/spark/pull/15054#discussion_r79539702
  
--- Diff: 
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/SessionCatalog.scala
 ---
@@ -357,6 +346,21 @@ class SessionCatalog(
 tempTables.remove(formatTableName(name))
   }
 
+  /**
+   * Retrieve the metadata of an existing temporary view.
+   * If the temporary view does not exist, return None.
+   */
+  def getTempViewMetadataOption(name: String): Option[CatalogTable] = 
synchronized {
--- End diff --

Yeah, we can combine them. 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 #15054: [SPARK-17502] [SQL] Fix Multiple Bugs in DDL Stat...

[cloud-fan]

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

https://github.com/apache/spark/pull/15054#discussion_r79528505
  
--- Diff: 
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/SessionCatalog.scala
 ---
@@ -357,6 +346,21 @@ class SessionCatalog(
 tempTables.remove(formatTableName(name))
   }
 
+  /**
+   * Retrieve the metadata of an existing temporary view.
+   * If the temporary view does not exist, return None.
+   */
+  def getTempViewMetadataOption(name: String): Option[CatalogTable] = 
synchronized {
--- End diff --

Seems we always use it with the pattern 
`getTempViewMetadataOption.getOrElse(getTableMetadata)`, maybe we should just 
rename it to `getTempViewOrPermanentTableMetadata`?


---
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 #15054: [SPARK-17502] [SQL] Fix Multiple Bugs in DDL Stat...

[gatorsmile]

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

https://github.com/apache/spark/pull/15054#discussion_r79478675
  
--- Diff: 
sql/core/src/main/scala/org/apache/spark/sql/execution/command/AnalyzeTableCommand.scala
 ---
@@ -37,7 +38,9 @@ case class AnalyzeTableCommand(tableName: String, noscan: 
Boolean = true) extend
   override def run(sparkSession: SparkSession): Seq[Row] = {
 val sessionState = sparkSession.sessionState
 val tableIdent = sessionState.sqlParser.parseTableIdentifier(tableName)
-val relation = 
EliminateSubqueryAliases(sessionState.catalog.lookupRelation(tableIdent))
+val db = 
tableIdent.database.getOrElse(sessionState.catalog.getCurrentDatabase)
+val qualifiedName = TableIdentifier(tableIdent.table, Some(db))
--- End diff --

Done. Clean all the related naming issues.


---
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 #15054: [SPARK-17502] [SQL] Fix Multiple Bugs in DDL Stat...

[gatorsmile]

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

https://github.com/apache/spark/pull/15054#discussion_r79478609
  
--- Diff: 
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/SessionCatalog.scala
 ---
@@ -246,30 +246,42 @@ class SessionCatalog(
   }
 
   /**
-   * Retrieve the metadata of an existing metastore table.
-   * If no database is specified, assume the table is in the current 
database.
-   * If the specified table is not found in the database then a 
[[NoSuchTableException]] is thrown.
+   * Retrieve the metadata of an existing temporary view.
+   * If the temporary view does not exist, a [[NoSuchTempViewException]] 
is thrown.
*/
-  def getTableMetadata(name: TableIdentifier): CatalogTable = {
-val db = 
formatDatabaseName(name.database.getOrElse(getCurrentDatabase))
-val table = formatTableName(name.table)
-val tid = TableIdentifier(table)
-if (isTemporaryTable(name)) {
+  def getTempViewMetadata(name: String): CatalogTable = {
--- End diff --

: ) Just removed `getTempViewMetadata` and moved 
`getTempViewMetadataOption` to the position you want


---
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 #15054: [SPARK-17502] [SQL] Fix Multiple Bugs in DDL Stat...

[cloud-fan]

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

https://github.com/apache/spark/pull/15054#discussion_r79344601
  
--- Diff: 
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/SessionCatalog.scala
 ---
@@ -246,30 +246,42 @@ class SessionCatalog(
   }
 
   /**
-   * Retrieve the metadata of an existing metastore table.
-   * If no database is specified, assume the table is in the current 
database.
-   * If the specified table is not found in the database then a 
[[NoSuchTableException]] is thrown.
+   * Retrieve the metadata of an existing temporary view.
+   * If the temporary view does not exist, a [[NoSuchTempViewException]] 
is thrown.
*/
-  def getTableMetadata(name: TableIdentifier): CatalogTable = {
-val db = 
formatDatabaseName(name.database.getOrElse(getCurrentDatabase))
-val table = formatTableName(name.table)
-val tid = TableIdentifier(table)
-if (isTemporaryTable(name)) {
+  def getTempViewMetadata(name: String): CatalogTable = {
--- End diff --

where do we need this method?


---
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 #15054: [SPARK-17502] [SQL] Fix Multiple Bugs in DDL Stat...

[cloud-fan]

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

https://github.com/apache/spark/pull/15054#discussion_r79340704
  
--- Diff: 
sql/core/src/main/scala/org/apache/spark/sql/execution/command/AnalyzeTableCommand.scala
 ---
@@ -37,7 +38,9 @@ case class AnalyzeTableCommand(tableName: String, noscan: 
Boolean = true) extend
   override def run(sparkSession: SparkSession): Seq[Row] = {
 val sessionState = sparkSession.sessionState
 val tableIdent = sessionState.sqlParser.parseTableIdentifier(tableName)
-val relation = 
EliminateSubqueryAliases(sessionState.catalog.lookupRelation(tableIdent))
+val db = 
tableIdent.database.getOrElse(sessionState.catalog.getCurrentDatabase)
+val qualifiedName = TableIdentifier(tableIdent.table, Some(db))
--- End diff --

let's follow the existing convention and call it `tableIdentWithDB`


---
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 #15054: [SPARK-17502] [SQL] Fix Multiple Bugs in DDL Stat...

[cloud-fan]

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

https://github.com/apache/spark/pull/15054#discussion_r79340506
  
--- Diff: 
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/SessionCatalog.scala
 ---
@@ -246,30 +246,42 @@ class SessionCatalog(
   }
 
   /**
-   * Retrieve the metadata of an existing metastore table.
-   * If no database is specified, assume the table is in the current 
database.
-   * If the specified table is not found in the database then a 
[[NoSuchTableException]] is thrown.
+   * Retrieve the metadata of an existing temporary view.
+   * If the temporary view does not exist, a [[NoSuchTempViewException]] 
is thrown.
*/
-  def getTableMetadata(name: TableIdentifier): CatalogTable = {
-val db = 
formatDatabaseName(name.database.getOrElse(getCurrentDatabase))
-val table = formatTableName(name.table)
-val tid = TableIdentifier(table)
-if (isTemporaryTable(name)) {
+  def getTempViewMetadata(name: String): CatalogTable = {
--- End diff --

let's move these 2 methods to the right section: `handles only temp view`


---
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 #15054: [SPARK-17502] [SQL] Fix Multiple Bugs in DDL Stat...

[gatorsmile]

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

https://github.com/apache/spark/pull/15054#discussion_r79320015
  
--- Diff: 
sql/core/src/main/scala/org/apache/spark/sql/execution/command/tables.scala ---
@@ -65,7 +64,11 @@ case class CreateTableLikeCommand(
 s"Source table in CREATE TABLE LIKE does not exist: 
'$sourceTable'")
 }
 
-val sourceTableDesc = catalog.getTableMetadata(sourceTable)
+val sourceTableDesc = if (catalog.isTemporaryTable(sourceTable)) {
+  catalog.getTempViewMetadata(sourceTable.table)
--- End diff --

Yeah, you are right. Let me fix it. 


---
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 #15054: [SPARK-17502] [SQL] Fix Multiple Bugs in DDL Stat...

[cloud-fan]

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

https://github.com/apache/spark/pull/15054#discussion_r79281274
  
--- Diff: 
sql/core/src/main/scala/org/apache/spark/sql/execution/command/tables.scala ---
@@ -65,7 +64,11 @@ case class CreateTableLikeCommand(
 s"Source table in CREATE TABLE LIKE does not exist: 
'$sourceTable'")
 }
 
-val sourceTableDesc = catalog.getTableMetadata(sourceTable)
+val sourceTableDesc = if (catalog.isTemporaryTable(sourceTable)) {
+  catalog.getTempViewMetadata(sourceTable.table)
--- End diff --

this is not atomic, we should have a method that return temp view metadata 
or None.


---
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 #15054: [SPARK-17502] [SQL] Fix Multiple Bugs in DDL Stat...

[gatorsmile]

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

https://github.com/apache/spark/pull/15054#discussion_r79275910
  
--- Diff: 
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CheckAnalysis.scala
 ---
@@ -360,6 +360,7 @@ trait CheckAnalysis extends PredicateHelper {
 
   case InsertIntoTable(t, _, _, _, _)
--- End diff --

I guess the reason is `LogicalRelation` is not in the package of 
`sql\catalyst`. Otherwise, it should be like
```Scala
  case InsertIntoTable(t, _, _, _, _)
if !t.isInstanceOf[SimpleCatalogRelation] && 
!t.isInstanceOf[LogicalRelation]
```


---
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 #15054: [SPARK-17502] [SQL] Fix Multiple Bugs in DDL Stat...

[marmbrus]

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

https://github.com/apache/spark/pull/15054#discussion_r79056087
  
--- Diff: 
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CheckAnalysis.scala
 ---
@@ -360,6 +360,7 @@ trait CheckAnalysis extends PredicateHelper {
 
   case InsertIntoTable(t, _, _, _, _)
--- End diff --

This probably dates back to when we called these temp tables and when we 
didn't have persistent data source tables.  Given that I think we probably have 
to support this for compatibility.  I'm not sure why this isn't a whitelist 
though?


---
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 #15054: [SPARK-17502] [SQL] Fix Multiple Bugs in DDL Stat...

[cloud-fan]

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

https://github.com/apache/spark/pull/15054#discussion_r78900278
  
--- Diff: 
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CheckAnalysis.scala
 ---
@@ -360,6 +360,7 @@ trait CheckAnalysis extends PredicateHelper {
 
   case InsertIntoTable(t, _, _, _, _)
--- End diff --

We can insert data into a temp view? Is it by design? cc @rxin @marmbrus 
@yhuai 


---
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 #15054: [SPARK-17502] [SQL] Fix Multiple Bugs in DDL Stat...

[gatorsmile]

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

https://github.com/apache/spark/pull/15054#discussion_r78803231
  
--- Diff: 
sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/SQLViewSuite.scala 
---
@@ -95,15 +155,38 @@ class SQLViewSuite extends QueryTest with SQLTestUtils 
with TestHiveSingleton {
   e = intercept[AnalysisException] {
 sql(s"""LOAD DATA LOCAL INPATH "$testData" INTO TABLE $viewName""")
   }.getMessage
-  assert(e.contains(s"Target table in LOAD DATA cannot be temporary: 
`$viewName`"))
+  assert(e.contains(s"Operation not allowed: LOAD DATA on temporary 
tables: `$viewName`"))
 
   e = intercept[AnalysisException] {
 sql(s"TRUNCATE TABLE $viewName")
   }.getMessage
   assert(e.contains(s"Operation not allowed: TRUNCATE TABLE on 
temporary tables: `$viewName`"))
+
+  e = intercept[AnalysisException] {
+sql(s"SHOW CREATE TABLE $viewName")
+  }.getMessage
+  assert(e.contains(
+s"Operation not allowed: SHOW CREATE TABLE on temporary tables: 
`$viewName`"))
+
+  e = intercept[AnalysisException] {
+sql(s"SHOW PARTITIONS $viewName")
+  }.getMessage
+  assert(e.contains(s"Operation not allowed: SHOW PARTITIONS on 
temporary tables: `$viewName`"))
+
+  intercept[NoSuchTableException] {
+sql(s"ANALYZE TABLE $viewName COMPUTE STATISTICS")
+  }
 }
   }
 
+  private def createTempView(viewName: String): Unit = {
--- End diff --

Found one more bug after this change. : )


---
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 #15054: [SPARK-17502] [SQL] Fix Multiple Bugs in DDL Stat...

[gatorsmile]

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

https://github.com/apache/spark/pull/15054#discussion_r78802228
  
--- Diff: 
sql/core/src/test/scala/org/apache/spark/sql/execution/command/DDLSuite.scala 
---
@@ -913,7 +913,7 @@ class DDLSuite extends QueryTest with SharedSQLContext 
with BeforeAndAfterEach {
 withTempView("show1a", "show2b") {
   sql(
 """
-  |CREATE TEMPORARY TABLE show1a
+  |CREATE TEMPORARY VIEW show1a
--- End diff --

Will roll it back


---
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 #15054: [SPARK-17502] [SQL] Fix Multiple Bugs in DDL Stat...

[gatorsmile]

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

https://github.com/apache/spark/pull/15054#discussion_r78802202
  
--- Diff: 
sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/SQLViewSuite.scala 
---
@@ -95,15 +155,38 @@ class SQLViewSuite extends QueryTest with SQLTestUtils 
with TestHiveSingleton {
   e = intercept[AnalysisException] {
 sql(s"""LOAD DATA LOCAL INPATH "$testData" INTO TABLE $viewName""")
   }.getMessage
-  assert(e.contains(s"Target table in LOAD DATA cannot be temporary: 
`$viewName`"))
+  assert(e.contains(s"Operation not allowed: LOAD DATA on temporary 
tables: `$viewName`"))
 
   e = intercept[AnalysisException] {
 sql(s"TRUNCATE TABLE $viewName")
   }.getMessage
   assert(e.contains(s"Operation not allowed: TRUNCATE TABLE on 
temporary tables: `$viewName`"))
+
+  e = intercept[AnalysisException] {
+sql(s"SHOW CREATE TABLE $viewName")
+  }.getMessage
+  assert(e.contains(
+s"Operation not allowed: SHOW CREATE TABLE on temporary tables: 
`$viewName`"))
+
+  e = intercept[AnalysisException] {
+sql(s"SHOW PARTITIONS $viewName")
+  }.getMessage
+  assert(e.contains(s"Operation not allowed: SHOW PARTITIONS on 
temporary tables: `$viewName`"))
+
+  intercept[NoSuchTableException] {
+sql(s"ANALYZE TABLE $viewName COMPUTE STATISTICS")
+  }
 }
   }
 
+  private def createTempView(viewName: String): Unit = {
--- End diff --

Sure, will do it.


---
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 #15054: [SPARK-17502] [SQL] Fix Multiple Bugs in DDL Stat...

[cloud-fan]

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

https://github.com/apache/spark/pull/15054#discussion_r78792855
  
--- Diff: 
sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/SQLViewSuite.scala 
---
@@ -95,15 +155,38 @@ class SQLViewSuite extends QueryTest with SQLTestUtils 
with TestHiveSingleton {
   e = intercept[AnalysisException] {
 sql(s"""LOAD DATA LOCAL INPATH "$testData" INTO TABLE $viewName""")
   }.getMessage
-  assert(e.contains(s"Target table in LOAD DATA cannot be temporary: 
`$viewName`"))
+  assert(e.contains(s"Operation not allowed: LOAD DATA on temporary 
tables: `$viewName`"))
 
   e = intercept[AnalysisException] {
 sql(s"TRUNCATE TABLE $viewName")
   }.getMessage
   assert(e.contains(s"Operation not allowed: TRUNCATE TABLE on 
temporary tables: `$viewName`"))
+
+  e = intercept[AnalysisException] {
+sql(s"SHOW CREATE TABLE $viewName")
+  }.getMessage
+  assert(e.contains(
+s"Operation not allowed: SHOW CREATE TABLE on temporary tables: 
`$viewName`"))
+
+  e = intercept[AnalysisException] {
+sql(s"SHOW PARTITIONS $viewName")
+  }.getMessage
+  assert(e.contains(s"Operation not allowed: SHOW PARTITIONS on 
temporary tables: `$viewName`"))
+
+  intercept[NoSuchTableException] {
+sql(s"ANALYZE TABLE $viewName COMPUTE STATISTICS")
+  }
 }
   }
 
+  private def createTempView(viewName: String): Unit = {
--- End diff --

hmm, do we really need this method? I think it's just a one line code, e.g. 
`spark.range(10).createTempView(...)`


---
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 #15054: [SPARK-17502] [SQL] Fix Multiple Bugs in DDL Stat...

[cloud-fan]

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

https://github.com/apache/spark/pull/15054#discussion_r78792645
  
--- Diff: 
sql/core/src/test/scala/org/apache/spark/sql/execution/command/DDLSuite.scala 
---
@@ -913,7 +913,7 @@ class DDLSuite extends QueryTest with SharedSQLContext 
with BeforeAndAfterEach {
 withTempView("show1a", "show2b") {
   sql(
 """
-  |CREATE TEMPORARY TABLE show1a
+  |CREATE TEMPORARY VIEW show1a
--- End diff --

I'd like to keep them unchanged, as `CREATE TEMPORARY TABLE` is still 
supported(but deprecated).


---
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 #15054: [SPARK-17502] [SQL] Fix Multiple Bugs in DDL Stat...

[gatorsmile]

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

https://github.com/apache/spark/pull/15054#discussion_r78784318
  
--- Diff: 
sql/core/src/main/scala/org/apache/spark/sql/execution/command/AnalyzeTableCommand.scala
 ---
@@ -91,6 +91,9 @@ case class AnalyzeTableCommand(tableName: String, noscan: 
Boolean = true) extend
   case logicalRel: LogicalRelation if 
logicalRel.catalogTable.isDefined =>
 updateTableStats(logicalRel.catalogTable.get, 
logicalRel.relation.sizeInBytes)
 
+  case o if !o.isInstanceOf[LeafNode] =>
--- End diff --

Yeah! I like it! 


---
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 #15054: [SPARK-17502] [SQL] Fix Multiple Bugs in DDL Stat...

[cloud-fan]

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

https://github.com/apache/spark/pull/15054#discussion_r78765976
  
--- Diff: 
sql/core/src/main/scala/org/apache/spark/sql/execution/command/AnalyzeTableCommand.scala
 ---
@@ -91,6 +91,9 @@ case class AnalyzeTableCommand(tableName: String, noscan: 
Boolean = true) extend
   case logicalRel: LogicalRelation if 
logicalRel.catalogTable.isDefined =>
 updateTableStats(logicalRel.catalogTable.get, 
logicalRel.relation.sizeInBytes)
 
+  case o if !o.isInstanceOf[LeafNode] =>
--- End diff --

I think a better fix is: when do `loopupRelation`, we always pass in a 
table identifier with database, so that it won't search the temp views.


---
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 #15054: [SPARK-17502] [SQL] Fix Multiple Bugs in DDL Stat...

[cloud-fan]

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

https://github.com/apache/spark/pull/15054#discussion_r78766111
  
--- Diff: 
sql/core/src/main/scala/org/apache/spark/sql/execution/command/ddl.scala ---
@@ -266,6 +267,12 @@ case class AlterTableUnsetPropertiesCommand(
 val catalog = sparkSession.sessionState.catalog
 DDLUtils.verifyAlterTableType(catalog, tableName, isView)
 val table = catalog.getTableMetadata(tableName)
--- End diff --

same here, we can always pass in a table identifier with database


---
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 #15054: [SPARK-17502] [SQL] Fix Multiple Bugs in DDL Stat...

[gatorsmile]

GitHub user gatorsmile opened a pull request:

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

[SPARK-17502] [SQL] Fix Multiple Bugs in DDL Statements on Temporary Views 
[WIP]

### What changes were proposed in this pull request?
- When the permanent tables/views do not exist but the temporary view 
exists, the expected error should be `NoSuchTableException` for 
partition-related ALTER TABLE commands. However, it always reports a confusing 
error message. For example, 
```
Partition spec is invalid. The spec (a, b) must match the partition spec () 
defined in table '`testview`';
```
- When the permanent tables/views do not exist but the temporary view 
exists, the expected error should be `NoSuchTableException` for `ALTER TABLE 
... UNSET TBLPROPERTIES`. However, it reports a missing table property. 
However, the expected error should be `NoSuchTableException`. For example, 
```
Attempted to unset non-existent property 'p' in table '`testView`';
```

TODO: Trying to add more test cases for DDL processing over temporary 
views. 

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

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

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

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

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


commit cc47c3eb07a7628faa4277dd87c2837d01c4f175
Author: gatorsmile 
Date:   2016-09-12T06:08:17Z

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