[SPARK-17502] [SQL] Fix Multiple Bugs in DDL Statements on Temporary Views by gatorsmile · Pull Request #15054 · apache/spark

gatorsmile · 2016-09-12T06:24:28Z

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. For example,

Attempted to unset non-existent property 'p' in table '`testView`';

When ANALYZE TABLE is called on a view or a temporary view, we should issue an error message. However, it reports a strange error:

ANALYZE TABLE is not supported for Project

When inserting into a temporary view that is generated from Range, we will get the following error message:

assertion failed: No plan for 'InsertIntoTable Range (0, 10, step=1, splits=Some(1)), false, false
+- Project [1 AS 1#20]
+- OneRowRelation$

This PR is to fix the above four issues.

How was this patch tested?

Added multiple test cases

SparkQA · 2016-09-12T07:37:34Z

Test build #65240 has finished for PR 15054 at commit cc47c3e.

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

SparkQA · 2016-09-13T06:18:32Z

Test build #65299 has finished for PR 15054 at commit 855df61.

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

gatorsmile · 2016-09-13T06:21:11Z

cc @cloud-fan @yhuai

cloud-fan · 2016-09-14T14:58:10Z

 case logicalRel: LogicalRelation if logicalRel.catalogTable.isDefined =>
 updateTableStats(logicalRel.catalogTable.get, logicalRel.relation.sizeInBytes)

+ case o if !o.isInstanceOf[LeafNode] =>


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

Yeah! I like it!

cloud-fan · 2016-09-14T17:15:07Z

 sql(
 """
- |CREATE TEMPORARY TABLE show1a
+ |CREATE TEMPORARY VIEW show1a


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

Will roll it back

gatorsmile · 2016-09-14T18:15:03Z

Will fix the remaining commands in ddl.scala

SparkQA · 2016-09-14T19:47:34Z

Test build #65389 has finished for PR 15054 at commit 6029a95.

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

SparkQA · 2016-09-14T19:50:56Z

Test build #65381 has finished for PR 15054 at commit 60371d8.

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

gatorsmile · 2016-09-14T23:59:11Z

+ * Retrieve the metadata of 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.
+ */


@cloud-fan In your PR #14962, getTableMetadata will not deal with temporary table. To simplify the potential conflict resolution, I just introduce a new function getNonTempTableMetadata. Thus, you can just basically replace it by the new getTableMetadata. Does it sound OK to you? Sorry for any inconvenience it might cause.

I don't think getTableMetadata should handle temp view, maybe we can fix it in this PR?

Sure, will do it soon.

gatorsmile · 2016-09-15T00:03:58Z

- }.getMessage
- assert(e.contains(s"Operation not allowed: TRUNCATE TABLE on temporary tables: `$viewName`"))
+ private def assertNoSuchTable(query: String): Unit = {
+ intercept[NoSuchTableException] {


Now, the behaviors are consistent. For the DDL commands that are unable to handle temporary views, we issue the same exception NoSuchTableException.

SparkQA · 2016-09-15T02:04:14Z

Test build #65413 has finished for PR 15054 at commit 7c1a8e5.

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

cloud-fan · 2016-09-15T03:54:21Z

@@ -360,6 +360,7 @@ trait CheckAnalysis extends PredicateHelper {

 case InsertIntoTable(t, _, _, _, _)


cc @yhuai do you remember why we have this check? InsertIntoTable can only be used for table right? When will we hit this branch?

spark.sql( s"""CREATE TEMPORARY VIEW normal_orc_source |USING org.apache.spark.sql.hive.orc |OPTIONS ( | PATH '${newFile(orcTableAsDir.getAbsolutePath).getCanonicalPath}' |)""".stripMargin) sql("INSERT INTO TABLE normal_orc_source SELECT * FROM orc_temp_table WHERE intField > 5")
We allow users to insert rows into temporary views that is created using CREATE TEMPORARY VIEW.

If the temporary view is created by createOrReplaceTempView, users are unable to insert rows into the temporary view.

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

I guess this might be designed for JDBC data sources at the beginning. Just a guess.

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?

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

SparkQA · 2016-09-15T09:02:05Z

Test build #65428 has finished for PR 15054 at commit 333497a.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- public static final class UnsignedPrefixComparatorNullsLast extends RadixSortSupport
- public static final class UnsignedPrefixComparatorDescNullsFirst extends RadixSortSupport
- public static final class SignedPrefixComparatorNullsLast extends RadixSortSupport
- public static final class SignedPrefixComparatorDescNullsFirst extends RadixSortSupport
- abstract sealed class NullOrdering
- case class SortOrder(
- trait Command extends LeafNode
- trait RunnableCommand extends logical.Command
- case class CreateTable(
- case class AnalyzeCreateTable(sparkSession: SparkSession) extends Rule[LogicalPlan]
- class SetAccumulator[T] extends AccumulatorV2[T, java.util.Set[T]]

SparkQA · 2016-09-15T09:24:57Z

Test build #65426 has finished for PR 15054 at commit a01f6b3.

This patch passes all tests.
This patch does not merge cleanly.
This patch adds the following public classes (experimental):
- class NoSuchTempViewException(table: String)
- case class ShowColumnsCommand(tableName: TableIdentifier) extends RunnableCommand

gatorsmile · 2016-09-15T15:24:46Z

retest this please

SparkQA · 2016-09-15T17:35:34Z

Test build #65444 has finished for PR 15054 at commit 333497a.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- public static final class UnsignedPrefixComparatorNullsLast extends RadixSortSupport
- public static final class UnsignedPrefixComparatorDescNullsFirst extends RadixSortSupport
- public static final class SignedPrefixComparatorNullsLast extends RadixSortSupport
- public static final class SignedPrefixComparatorDescNullsFirst extends RadixSortSupport
- abstract sealed class NullOrdering
- case class SortOrder(
- trait Command extends LeafNode
- trait RunnableCommand extends logical.Command
- case class CreateTable(
- case class AnalyzeCreateTable(sparkSession: SparkSession) extends Rule[LogicalPlan]
- class SetAccumulator[T] extends AccumulatorV2[T, java.util.Set[T]]

cloud-fan · 2016-09-17T10:29:47Z


- val sourceTableDesc = catalog.getTableMetadata(sourceTable)
+ val sourceTableDesc = if (catalog.isTemporaryTable(sourceTable)) {
+ catalog.getTempViewMetadata(sourceTable.table)


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

Yeah, you are right. Let me fix it.

cloud-fan · 2016-09-18T13:53:30Z

retest this please

SparkQA · 2016-09-18T16:34:32Z

Test build #65574 has finished for PR 15054 at commit 333497a.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- public static final class UnsignedPrefixComparatorNullsLast extends RadixSortSupport
- public static final class UnsignedPrefixComparatorDescNullsFirst extends RadixSortSupport
- public static final class SignedPrefixComparatorNullsLast extends RadixSortSupport
- public static final class SignedPrefixComparatorDescNullsFirst extends RadixSortSupport
- abstract sealed class NullOrdering
- case class SortOrder(
- trait Command extends LeafNode
- trait RunnableCommand extends logical.Command
- case class CreateTable(
- case class AnalyzeCreateTable(sparkSession: SparkSession) extends Rule[LogicalPlan]
- class SetAccumulator[T] extends AccumulatorV2[T, java.util.Set[T]]

SparkQA · 2016-09-19T05:16:27Z

Test build #65587 has finished for PR 15054 at commit f305c4c.

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

cloud-fan · 2016-09-19T07:55:43Z

- val table = formatTableName(name.table)
- val tid = TableIdentifier(table)
- if (isTemporaryTable(name)) {
+ def getTempViewMetadata(name: String): CatalogTable = {


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

where do we need this method?

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

cloud-fan · 2016-09-19T07:57:34Z

 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))


let's follow the existing convention and call it tableIdentWithDB

Done. Clean all the related naming issues.

SparkQA · 2016-09-19T22:37:02Z

Test build #65610 has finished for PR 15054 at commit c662d2c.

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

cloud-fan · 2016-09-20T02:50:59Z

+ * Retrieve the metadata of an existing temporary view.
+ * If the temporary view does not exist, return None.
+ */
+ def getTempViewMetadataOption(name: String): Option[CatalogTable] = synchronized {


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

Yeah, we can combine them. Let me do it. Thanks!

cloud-fan · 2016-09-20T05:57:59Z

+ * 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.
+ */


it should just take a string.

cloud-fan · 2016-09-20T05:59:24Z

 }

- test("getTableMetadata on temporary views") {
+ test("getTableMetadata and getTempViewOrPermanentTableMetadata on temporary views") {


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

SparkQA · 2016-09-20T07:25:01Z

Test build #65637 has finished for PR 15054 at commit 48ce44e.

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

SparkQA · 2016-09-20T07:56:55Z

Test build #65638 has finished for PR 15054 at commit ee3096c.

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

cloud-fan · 2016-09-20T08:16:56Z

retest this please

SparkQA · 2016-09-20T10:26:26Z

Test build #65642 has finished for PR 15054 at commit ee3096c.

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

cloud-fan · 2016-09-20T12:20:38Z

LGTM, merging to master!

cloud-fan · 2016-09-20T12:30:27Z

JIRA is down, I'll resolve the ticket later.

cloud-fan · 2016-09-20T12:43:20Z

@gatorsmile how hard is it to backport this PR to 2.0?

gatorsmile · 2016-09-20T14:16:19Z

It should be not hard. Will make a try today to backport to 2.0. Thanks!

## What changes were proposed in this pull request? After apache#15054 , there is no place in Spark SQL that need `SessionCatalog.tableExists` to check temp views, so this PR makes `SessionCatalog.tableExists` only check permanent table/view and removes some hacks. This PR also improves the `getTempViewOrPermanentTableMetadata` that is introduced in apache#15054 , to make the code simpler. ## How was this patch tested? existing tests Author: Wenchen Fan <wenchen@databricks.com> Closesapache#15160 from cloud-fan/exists.

…tements on Temporary Views ### What changes were proposed in this pull request? This PR is to backport #15054 and #15160 to Spark 2.0. - 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. For example, ``` Attempted to unset non-existent property 'p' in table '`testView`'; ``` - When `ANALYZE TABLE` is called on a view or a temporary view, we should issue an error message. However, it reports a strange error: ``` ANALYZE TABLE is not supported for Project ``` - When inserting into a temporary view that is generated from `Range`, we will get the following error message: ``` assertion failed: No plan for 'InsertIntoTable Range (0, 10, step=1, splits=Some(1)), false, false +- Project [1 AS 1#20] +- OneRowRelation$ ``` This PR is to fix the above four issues. There is no place in Spark SQL that need `SessionCatalog.tableExists` to check temp views, so this PR makes `SessionCatalog.tableExists` only check permanent table/view and removes some hacks. ### How was this patch tested? Added multiple test cases Author: gatorsmile <gatorsmile@gmail.com> Closes#15174 from gatorsmile/PR15054Backport.

fix
cc47c3e

added more test cases
855df61

gatorsmile changed the title ~~[SPARK-17502] [SQL] Fix Multiple Bugs in DDL Statements on Temporary Views [WIP]~~[SPARK-17502] [SQL] Fix Multiple Bugs in DDL Statements on Temporary ViewsSep 13, 2016

cloud-fan reviewed Sep 14, 2016
View reviewed changes

gatorsmile added 2 commits September 14, 2016 09:48

Merge remote-tracking branch 'upstream/master' into tempViewDDL
61749b7

address comments
60371d8

cloud-fan reviewed Sep 14, 2016
View reviewed changes

address comments
6029a95

fix
7c1a8e5

gatorsmile commented Sep 14, 2016
View reviewed changes

gatorsmile commented Sep 15, 2016
View reviewed changes

cloud-fan reviewed Sep 15, 2016
View reviewed changes

gatorsmile added 2 commits September 14, 2016 23:44

address comments
a01f6b3

Merge remote-tracking branch 'upstream/master' into tempViewDDL
333497a

cloud-fan reviewed Sep 17, 2016
View reviewed changes

gatorsmile added 2 commits September 18, 2016 18:05

Merge remote-tracking branch 'upstream/master' into tempViewDDL
5e40880

address comments
f305c4c

cloud-fan reviewed Sep 19, 2016
View reviewed changes

address comments.
c662d2c

cloud-fan reviewed Sep 20, 2016
View reviewed changes

address comments
48ce44e

cloud-fan reviewed Sep 20, 2016
View reviewed changes

address comments
ee3096c

asfgit closed this in d5ec5dbSep 20, 2016

cloud-fan mentioned this pull request Sep 20, 2016
[SPARK-17609][SQL] SessionCatalog.tableExists should not check temp view #15160
Closed

gatorsmile mentioned this pull request Sep 21, 2016
[SPARK-17502] [17609] [SQL] [Backport] [2.0] Fix Multiple Bugs in DDL Statements on Temporary Views #15174
Closed

		@@ -360,6 +360,7 @@ trait CheckAnalysis extends PredicateHelper {

		case InsertIntoTable(t, _, _, _, _)

Uh oh!

Conversation

gatorsmile commented Sep 12, 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 Sep 12, 2016

Uh oh!

SparkQA commented Sep 13, 2016

Uh oh!

gatorsmile commented Sep 13, 2016

Uh oh!

cloud-fanSep 14, 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!

Choose a reason for hiding this comment

Uh oh!

gatorsmile commented Sep 14, 2016

Uh oh!

SparkQA commented Sep 14, 2016

Uh oh!

SparkQA commented Sep 14, 2016

Uh oh!

gatorsmileSep 14, 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!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Sep 15, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gatorsmileSep 15, 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!

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!

SparkQA commented Sep 15, 2016

Uh oh!

SparkQA commented Sep 15, 2016

Uh oh!

gatorsmile commented Sep 15, 2016

Uh oh!

SparkQA commented Sep 15, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cloud-fan commented Sep 18, 2016

Uh oh!

SparkQA commented Sep 18, 2016

Uh oh!

SparkQA commented Sep 19, 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

gatorsmile commented Sep 12, 2016•
edited
Loading

cloud-fanSep 14, 2016•
edited
Loading

gatorsmileSep 14, 2016•
edited
Loading

gatorsmileSep 15, 2016•
edited
Loading