[SPARK-17402][SQL] separate the management of temp views and metastore tables/views in SessionCatalog #14962

cloud-fan · 2016-09-05T14:04:07Z

What changes were proposed in this pull request?

In SessionCatalog, we have several operations(getTableMetadata, tableExists, renameTable, dropTable, loopupRelation) that handle both temp views and metastore tables/views. They can save some lines of code for some commands that need to deal with both temp views and metastore tables/views, but also introduce bugs for other commands, because the operation names say nothing about temp views and are very likely to be misused:

DataFrameWriter.saveAsTable/CREATE TABLE USING will fail if a same-name temp view exits
DataFrameWriter.saveAsTable with overwrite mode will mistakenly drop a same-name temp view.
Catalog.dropTempView may drop metastore table mistakenly
ALTER TABLE RECOVER PARTITIONS/LOAD DATA/TRUNCATE TABLE/SHOW CREATE TABLE should report "table not found" instead of "temp view is not supported", if a same-name temp view exists, because these commands don't need to deal with temp views.

In some commands we support temp views mistakenly without mentioning it in document: ShowTablePropertiesCommand, ShowColumnsCommand, Catalog.listColumns.
This PR remove the temp view support in ShowTablePropertiesCommand, as temp view doesn't have properties. And explicitly document that ShowColumnsCommand and Catalog.listColumns support temp view.

Mixing the handling of temp views and metastore tables/views also makes it harder to implement thread-safe operations. e.g. AlterViewAsCommand checks isTemporaryTable first then createTempView, which is not atomic. Most temp view related operations in SessionCatalog hold a lock on the SessionCatalog object, which is unnecessary.

This PR separates the management of temp views and metastore tables/views in SessionCatalog, any commands that need to deal with temp views should explicitly call temp view related operations in SessionCatalog, to fix existing bugs and prevent future bug like this.

How was this patch tested?

existing tests and 3 new tests.

cloud-fan · 2016-09-05T14:04:20Z

cc @yhuai @gatorsmile @liancheng

cloud-fan · 2016-09-05T15:03:06Z

also cc @srinathshankar , it will be very easy to implement global temp view after this

SparkQA · 2016-09-05T15:29:52Z

Test build #64943 has finished for PR 14962 at commit ebed732.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- class TempViewAlreadyExistsException(name: String)
- class TempViewManager

clockfly · 2016-09-06T00:14:45Z

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

Can we avoid adding TempViewManager?

What is the reason? Thanks!

I think it's easier to implement and reason about the thread-safe semantic for temp views if we put temp view management into one place.

Why not just name it tempViewManager?

Since the goal of this PR is to add some view related API. So I think refactoring using TempViewManager is not the major goal?

gatorsmile · 2016-09-06T07:11:09Z

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

If we change it back to HashMap, we need to add synchronized back. Is my understanding right?

Same to all the other related functions.

yea, now we let TempViewManager to implement the thread-safe semantic

SparkQA · 2016-09-06T08:08:12Z

Test build #64973 has finished for PR 14962 at commit 8eb6c42.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- class TempViewAlreadyExistsException(name: String)
- class TempViewManager

SparkQA · 2016-09-06T10:00:28Z

Test build #64984 has finished for PR 14962 at commit 7314a33.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- class TempViewAlreadyExistsException(name: String)
- class TempViewManager

SparkQA · 2016-09-06T11:26:13Z

Test build #64987 has finished for PR 14962 at commit 0075f9d.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- class TempViewAlreadyExistsException(name: String)
- class TempViewManager

gatorsmile · 2016-09-07T02:52:03Z

sql/core/src/main/scala/org/apache/spark/sql/execution/command/ddl.scala

Drop Table is unable to drop a temp view, right?

spark.range(10).createTempView("tempView") sql("DESC tempView").show() sql("DROP TABLE tempView") sql("DESC tempView").show()

Actually I noticed this and fixed it before, but it breaks a lot of tests, because we call "temp view" as "temp table" before. I'd like to keep this behaviour as it was, we can discuss how to fix it in follow-ups.

I see. Thanks!

gatorsmile · 2016-09-07T05:06:28Z

Found a common bug in the following ALTER TABLE commands:

    | ALTER TABLE tableIdentifier (partitionSpec)?
        SET SERDE STRING (WITH SERDEPROPERTIES tablePropertyList)?     #setTableSerDe
    | ALTER TABLE tableIdentifier (partitionSpec)?
        SET SERDEPROPERTIES tablePropertyList                          #setTableSerDe
    | ALTER TABLE tableIdentifier ADD (IF NOT EXISTS)?
        partitionSpecLocation+                                         #addTablePartition
    | ALTER VIEW tableIdentifier ADD (IF NOT EXISTS)?
        partitionSpec+                                                 #addTablePartition
    | ALTER TABLE tableIdentifier
        from=partitionSpec RENAME TO to=partitionSpec                  #renameTablePartition
    | ALTER TABLE tableIdentifier ADD (IF NOT EXISTS)?
        partitionSpecLocation+                                         #addTablePartition
    | ALTER TABLE tableIdentifier partitionSpec? SET locationSpec      #setTableLocation
    | ALTER TABLE tableIdentifier RECOVER PARTITIONS                   #recoverPartitions

We need to issue an exception when the tableType is VIEW. This is not introduced by this PR. Should we fix it here? or create a separate PR?

gatorsmile · 2016-09-07T05:33:18Z

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

In the description of TempViewManager, could we mention the name of temp view is always case sensitive? The caller is responsible for handling case-related issues.

yea good idea

gatorsmile · 2016-09-07T07:15:19Z

The biggest external change is in the table resolution order. Now, three common SessionCatalog functions tableExists, getTableMetadata and lookupRelation are not using the same order. tableExists and getTableMetadata do not check the existence of temp table, but lookupRelation always first checks the temp table. This inconsistency could be risky. That means, we allow users to create a table with the same name like a temp table. When we query the table, we will get the temp view at first.

In addition, we need to carefully review all the places that are using these three functions. For example, the analyzer rule PreWriteCheck is using both lookupRelation and tableExists. We will have a bug after this PR.

cloud-fan · 2016-09-07T12:27:32Z

Found a common bug in the following ALTER TABLE commands:

let's fix them in separated PRs.

In addition, we need to carefully review all the places that are using these three functions. For example, the analyzer rule PreWriteCheck is using both lookupRelation and tableExists. We will have a bug after this PR.

yea, we should review them carefully(I have walked through all of them). PreWriteCheck does have a bug, but not caused by this PR. I'm thinking about if we should also separate lookUpRelation to a metastore version and a temp view version.

gatorsmile · 2016-09-07T16:43:34Z

Sorry, I did not explain it clearly. In the analyzer rule PreWriteCheck, the target metastore table exists and thus tableExists returns true, but the following catalog.lookupRelation actually returns a temporary table. Thus, it could report a wrong error or miss some errors.

…essionCatalog

cloud-fan · 2016-09-08T10:47:19Z

the target metastore table exists and thus tableExists returns true, but the following catalog.lookupRelation actually returns a temporary table.

isn't it true before this PR?

SparkQA · 2016-09-08T12:11:18Z

Test build #65100 has finished for PR 14962 at commit cec6b3e.

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

SparkQA · 2016-09-08T12:25:27Z

Test build #65101 has finished for PR 14962 at commit 01659dc.

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

SparkQA · 2016-09-08T14:59:31Z

Test build #65102 has finished for PR 14962 at commit a8ce27f.

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

gatorsmile · 2016-09-08T17:54:55Z

Found another issue we need to fix.

    val tableName = "tab1"
    withTable(tableName) {
      Seq((1, 2), (3, 4)).toDF("i", "j").createOrReplaceTempView(tableName)
      sql(s"select * from $tableName where i < 2")
        .write.mode(SaveMode.Overwrite).saveAsTable(tableName)
      sql(s"select * from $tableName").show()
    }

Before this PR, the results are correct

+---+---+
|  i|  j|
+---+---+
|  1|  2|
+---+---+

After this PR, the results is incorrect.

+---+---+
|  i|  j|
+---+---+
|  1|  2|
|  3|  4|
+---+---+

gatorsmile · 2016-09-08T18:09:38Z

the target metastore table exists and thus tableExists returns true, but the following catalog.lookupRelation actually returns a temporary table.

Before this PR, yeah, it behaves the same. After this PR, we expect catalog.lookupRelation returns a metastore table. However, it returns a temporary table. Thus, we might misse the detection.

cloud-fan · 2016-09-09T14:23:48Z

    val tableName = "tab1"
    withTable(tableName) {
      Seq((1, 2), (3, 4)).toDF("i", "j").createOrReplaceTempView(tableName)
      sql(s"select * from $tableName where i < 2")
        .write.mode(SaveMode.Overwrite).saveAsTable(tableName)
      sql(s"select * from $tableName").show()
    }

@gatorsmile , why it's an issue? In this case, we create a temp view called tab1 with 2 rows first. Then we create a table also called tab1 with 1 row. Finally in select we use the name tab1, which refers to the temp view, it has 2 rows.

cloud-fan · 2016-09-09T14:34:05Z

@gatorsmile It's actually another bug in previous code. When we use DataFrameWriter.saveAsTable with overwrite mode, we may mistakenly drop a same-name temp view. I have updated the PR description to include it.

SparkQA · 2016-09-09T16:14:21Z

Test build #65153 has finished for PR 14962 at commit e0d2427.

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

gatorsmile · 2016-09-09T17:17:39Z

uh... yeah, that is another existing bug. : ) Let me write a test suite for all the DDL statements when the temp view with the same name exists. We might hit more bugs...

gatorsmile · 2016-09-09T18:08:15Z

wow... we are allowed to insert data into temporary views...

Updates: when writing the test cases for insertable temp views, I found a bug in my previous PR. When data sources do not extend SchemaRelationProvider, we are unable to set schema to DataSource. Will try to fix it ASAP. Thanks!

SparkQA · 2016-09-10T03:56:36Z

Test build #65186 has finished for PR 14962 at commit f421355.

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

yhuai · 2016-09-14T04:05:04Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala

    private def lookupTableFromCatalog(u: UnresolvedRelation): LogicalPlan = {
      try {
-        catalog.lookupRelation(u.tableIdentifier, u.alias)
+        catalog.lookupTempViewOrRelation(u.tableIdentifier, u.alias)


This is also for view, right? Should we just keep the old name?

yhuai · 2016-09-14T04:36:00Z

Is it possible to first have a PR to fix the bugs?

yhuai · 2016-09-14T05:08:12Z

sql/core/src/test/scala/org/apache/spark/sql/SQLQuerySuite.scala

+        assert(spark.table("default.same_name").collect().isEmpty)
+      }
+    }
+  }


This is a regression test.

Let's add comments to explain what this test is for in case we accidentally delete it in future.

…-name temp view exists ## What changes were proposed in this pull request? In `SessionCatalog`, we have several operations(`tableExists`, `dropTable`, `loopupRelation`, etc) that handle both temp views and metastore tables/views. This brings some bugs to DDL commands that want to handle temp view only or metastore table/view only. These bugs are: 1. `CREATE TABLE USING` will fail if a same-name temp view exists 2. `Catalog.dropTempView`will un-cache and drop metastore table if a same-name table exists 3. `saveAsTable` will fail or have unexpected behaviour if a same-name temp view exists. These bug fixes are pulled out from #14962 and targets both master and 2.0 branch ## How was this patch tested? new regression tests Author: Wenchen Fan <[email protected]> Closes #15099 from cloud-fan/fix-view.

…-name temp view exists In `SessionCatalog`, we have several operations(`tableExists`, `dropTable`, `loopupRelation`, etc) that handle both temp views and metastore tables/views. This brings some bugs to DDL commands that want to handle temp view only or metastore table/view only. These bugs are: 1. `CREATE TABLE USING` will fail if a same-name temp view exists 2. `Catalog.dropTempView`will un-cache and drop metastore table if a same-name table exists 3. `saveAsTable` will fail or have unexpected behaviour if a same-name temp view exists. These bug fixes are pulled out from #14962 and targets both master and 2.0 branch new regression tests Author: Wenchen Fan <[email protected]> Closes #15099 from cloud-fan/fix-view. (cherry picked from commit 3fe630d) Signed-off-by: Wenchen Fan <[email protected]>

…-name temp view exists ## What changes were proposed in this pull request? In `SessionCatalog`, we have several operations(`tableExists`, `dropTable`, `loopupRelation`, etc) that handle both temp views and metastore tables/views. This brings some bugs to DDL commands that want to handle temp view only or metastore table/view only. These bugs are: 1. `CREATE TABLE USING` will fail if a same-name temp view exists 2. `Catalog.dropTempView`will un-cache and drop metastore table if a same-name table exists 3. `saveAsTable` will fail or have unexpected behaviour if a same-name temp view exists. These bug fixes are pulled out from apache#14962 and targets both master and 2.0 branch ## How was this patch tested? new regression tests Author: Wenchen Fan <[email protected]> Closes apache#15099 from cloud-fan/fix-view.

gatorsmile · 2016-09-21T06:12:52Z

Maybe update this PR now? The only issue left is the atomicity issues. It sounds like we do not need to backport the fix, since the temporary view is session-specific. Right? We have to be very careful when implementing the global temporary views.

cloud-fan · 2016-09-22T15:12:15Z

I'll update this PR after the global temp view

cloud-fan · 2016-10-18T13:12:07Z

closing it as most of them were fixed in other PRs

clockfly reviewed Sep 6, 2016
View reviewed changes

cloud-fan force-pushed the temp-view branch from ebed732 to 8eb6c42 Compare September 6, 2016 06:15

gatorsmile reviewed Sep 6, 2016
View reviewed changes

cloud-fan force-pushed the temp-view branch 2 times, most recently from 7314a33 to 0075f9d Compare September 6, 2016 09:04

gatorsmile reviewed Sep 7, 2016
View reviewed changes

separate the management of temp views and metastore tables/views in S…

03ded04

…essionCatalog

cloud-fan force-pushed the temp-view branch 2 times, most recently from cec6b3e to 01659dc Compare September 8, 2016 11:04

address comments

a8ce27f

cloud-fan force-pushed the temp-view branch from 01659dc to a8ce27f Compare September 8, 2016 13:08

improve test

e0d2427

fix test

f421355

yhuai reviewed Sep 14, 2016
View reviewed changes

cloud-fan mentioned this pull request Sep 14, 2016

[SPARK-17541][SQL] fix some DDL bugs about table management when same-name temp view exists #15099

Closed

gatorsmile mentioned this pull request Sep 14, 2016

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

Closed

cloud-fan closed this Oct 18, 2016

cloud-fan deleted the temp-view branch December 14, 2016 12:33

[SPARK-17402][SQL] separate the management of temp views and metastore tables/views in SessionCatalog #14962

[SPARK-17402][SQL] separate the management of temp views and metastore tables/views in SessionCatalog #14962

Uh oh!

Conversation

cloud-fan commented Sep 5, 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!

cloud-fan commented Sep 5, 2016

Uh oh!

cloud-fan commented Sep 5, 2016

Uh oh!

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

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gatorsmile Sep 6, 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!

SparkQA commented Sep 6, 2016

Uh oh!

SparkQA commented Sep 6, 2016

Uh oh!

SparkQA commented Sep 6, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cloud-fan Sep 7, 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!

gatorsmile commented Sep 7, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gatorsmile commented Sep 7, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cloud-fan commented Sep 7, 2016

Uh oh!

gatorsmile commented Sep 7, 2016

Uh oh!

cloud-fan commented Sep 8, 2016

Uh oh!

SparkQA commented Sep 8, 2016

Uh oh!

SparkQA commented Sep 8, 2016

Uh oh!

SparkQA commented Sep 8, 2016

Uh oh!

gatorsmile commented Sep 8, 2016

Uh oh!

gatorsmile commented Sep 8, 2016

Uh oh!

cloud-fan commented Sep 9, 2016

Uh oh!

cloud-fan commented Sep 9, 2016

Uh oh!

SparkQA commented Sep 9, 2016

Uh oh!

gatorsmile commented Sep 9, 2016

Uh oh!

gatorsmile commented Sep 9, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

cloud-fan commented Sep 5, 2016 •

edited

Loading

gatorsmile Sep 6, 2016 •

edited

Loading

cloud-fan Sep 7, 2016 •

edited

Loading

gatorsmile commented Sep 7, 2016 •

edited

Loading

gatorsmile commented Sep 7, 2016 •

edited

Loading

gatorsmile commented Sep 9, 2016 •

edited

Loading