Skip to content

Conversation

@rxin
Copy link
Contributor

@rxin rxin commented Jul 1, 2016

What changes were proposed in this pull request?

This patch fixes the bug that the refresh command does not work on temporary views. This patch is based on #13989, but removes the public Dataset.refresh() API as well as improved test coverage.

Note that I actually think the public refresh() API is very useful. We can in the future implement it by also invalidating the lazy vals in QueryExecution (or alternatively just create a new QueryExecution).

How was this patch tested?

Re-enabled a previously ignored test, and added a new test suite for Hive testing behavior of temporary views against MetastoreRelation.

*/
def invalidateTable(name: TableIdentifier): Unit = { /* no-op */ }
def refreshTable(name: TableIdentifier): Unit = {
// Go through temporary tables and invalidate them.
Copy link
Member

@gatorsmile gatorsmile Jul 1, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the test case of HiveMetadataCacheSuite.scala, users might refresh the base table by using spark.catalog.refreshTable("view_table"). Normally, they do not specify the current database name. Then, its database name is empty. Thus, this table will be treated as a temporary table. This comment might need a correction.

@gatorsmile
Copy link
Member

LGTM except a minor comment.

@SparkQA
Copy link

SparkQA commented Jul 1, 2016

Test build #61599 has finished for PR 14009 at commit 1225cfc.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • class HiveMetadataCacheSuite extends QueryTest with SQLTestUtils with TestHiveSingleton

@SparkQA
Copy link

SparkQA commented Jul 1, 2016

Test build #61600 has finished for PR 14009 at commit 0c47cca.

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

val newCount = sql("select count(*) from view_refresh").first().getLong(0)
assert(newCount > 0 && newCount < 100)
}
}}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This style is pretty weird...

@liancheng
Copy link
Contributor

LGTM except for a minor styling issue.

@rxin
Copy link
Contributor Author

rxin commented Jul 5, 2016

Thanks - I fixed the two comments. Going to merge it in master/2.0.

@asfgit asfgit closed this in 16a2a7d Jul 5, 2016
asfgit pushed a commit that referenced this pull request Jul 5, 2016
## What changes were proposed in this pull request?
This patch fixes the bug that the refresh command does not work on temporary views. This patch is based on #13989, but removes the public Dataset.refresh() API as well as improved test coverage.

Note that I actually think the public refresh() API is very useful. We can in the future implement it by also invalidating the lazy vals in QueryExecution (or alternatively just create a new QueryExecution).

## How was this patch tested?
Re-enabled a previously ignored test, and added a new test suite for Hive testing behavior of temporary views against MetastoreRelation.

Author: Reynold Xin <rxin@databricks.com>
Author: petermaxlee <petermaxlee@gmail.com>

Closes #14009 from rxin/SPARK-16311.

(cherry picked from commit 16a2a7d)
Signed-off-by: Reynold Xin <rxin@databricks.com>
@SparkQA
Copy link

SparkQA commented Jul 5, 2016

Test build #61773 has finished for PR 14009 at commit b131175.

  • This patch fails to build.
  • This patch merges cleanly.
  • This patch adds no public classes.

@dongjoon-hyun
Copy link
Member

@dongjoon-hyun
Copy link
Member

Thank you for fast fix!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants