[SPARK-34197][SQL] `SessionCatalog.refreshTable()` should not invalidate the relation cache for temporary views #31265

MaxGekk · 2021-01-20T21:48:23Z

What changes were proposed in this pull request?

Check the name passed to SessionCatalog.refreshTable, and if it belongs to a temporary view, do not invalidate the relation cache.

Why are the changes needed?

When SessionCatalog.refreshTable refreshes a temporary or global temporary view, it should not invalidate an entry in the relation cache associated to a table with the same name.

Does this PR introduce any user-facing change?

Should not. The change might improve performance slightly.

How was this patch tested?

By running new UT:

$ build/sbt -Phive-2.3 -Phive-thriftserver "test:testOnly *SessionCatalogSuite"

SparkQA · 2021-01-20T22:56:18Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/38878/

SparkQA · 2021-01-20T23:49:50Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/38878/

SparkQA · 2021-01-21T02:14:23Z

Test build #134292 has finished for PR 31265 at commit a73cb59.

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

MaxGekk · 2021-01-21T20:27:13Z

@cloud-fan @dongjoon-hyun @HyukjinKwon @sunchao Could you take a look at the small fix.

sunchao

Thanks @MaxGekk for pinging. Overall looks good to me. BTW do you know whether the refreshTable method is used in places where there could be both temp view and table with the same name and we explicitly do not want to refresh the temp view but just the table relation cache?

sunchao · 2021-01-21T20:55:38Z

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

@@ -994,15 +994,17 @@ class SessionCatalog(
    // Go through temporary views and invalidate them.
    // If the database is defined, this may be a global temporary view.
    // If the database is not defined, there is a good chance this is a temp view.
-    if (name.database.isEmpty) {
-      tempViews.get(tableName).foreach(_.refresh())
+    val isTempView = if (name.database.isEmpty) {


nit: the code structure here looks pretty similar to isTemporaryTable, and it would be nice if we can do:

lookupTemporaryTable(name).map(_.refresh()).getOrElse { // Also invalidate the table relation cache. val qualifiedTableName = QualifiedTableName(dbName, tableName) tableRelationCache.invalidate(qualifiedTableName) }

I did that.

SparkQA · 2021-01-21T21:49:11Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/38929/

SparkQA · 2021-01-21T21:53:25Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/38929/

SparkQA · 2021-01-22T01:13:34Z

Test build #134342 has finished for PR 31265 at commit 3b49a35.

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

MaxGekk · 2021-01-22T09:26:31Z

BTW do you know whether the refreshTable method is used in places where there could be both temp view and table with the same name and we explicitly do not want to refresh the temp view but just the table relation cache?

Theoretically, I could imagine the situation when an user run any v1 command for a table, for instance ALTER TABLE tbl DROP PARTITION, and tbl is resolved to a v1 table:

case class AlterTableDropPartitionCommand(
    tableName: TableIdentifier,
...

tableName refers to tbl. While running the command, the user creates a temp view tbl in a parallel thread somewhere in the middle of run():

override def run(sparkSession: SparkSession): Seq[Row] = {
    val table = catalog.getTableMetadata(tableName)
    ...
    // USER CREATES A TEMP VIEW WITH THE SAME NAME
    ...
    sparkSession.catalog.refreshTable(table.identifier.quotedString)

And at the end of run(), we refresh the view instead of the table. So we wanted to refresh the table but refreshed the view.

SparkQA · 2021-01-22T09:57:06Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/38946/

SparkQA · 2021-01-22T10:30:37Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/38946/

SparkQA · 2021-01-22T13:23:37Z

Test build #134360 has finished for PR 31265 at commit dcae0ef.

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

cloud-fan · 2021-01-22T13:33:15Z

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

@@ -877,21 +877,27 @@ class SessionCatalog(
    isTemporaryTable(nameParts.asTableIdentifier)
  }

+  private def lookupTempView(name: TableIdentifier): Option[LogicalPlan] = {
+    val table = formatTableName(name.table)
+    val dbName = formatDatabaseName(name.database.getOrElse(currentDb))


we don't need to check the current database, as we can't set global_tmp as the current database.

The previous code didn't check the current database as well.

@cloud-fan This is "previous" code:

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

Line 998 in e79c1cd

val dbName = formatDatabaseName(name.database.getOrElse(currentDb))

or you are speaking about another previous code?

I mean https://github.com/apache/spark/pull/31265/files#diff-9dd0899e5406230aeff96654432da54f35255f6dc60eecb87264a5c508a8c826L890

Sorry, do you propose to call database.get?

yes, let's follow that "previous" code.

I did that. ... The function name isTemporaryTable looks weird, so, I would replace it by:

def isTempView(name: TableIdentifier): Boolean

Here is the PR #31295 which renames isTemporaryTable().

SparkQA · 2021-01-22T15:55:09Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/38961/

SparkQA · 2021-01-22T16:23:35Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/38961/

SparkQA · 2021-01-22T19:30:03Z

Test build #134374 has finished for PR 31265 at commit 5c15355.

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

…-refresh-table # Conflicts: # sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/SessionCatalog.scala

SparkQA · 2021-01-23T21:05:08Z

Test build #134402 has finished for PR 31265 at commit 4c3350e.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class BitwiseGet(left: Expression, right: Expression)
new RuntimeException(s\"class$`

MaxGekk · 2021-01-24T21:23:22Z

Any objections for the changes?

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/catalog/SessionCatalogSuite.scala

cloud-fan · 2021-01-25T07:37:20Z

thanks, merging to master!

…ate the relation cache for temporary views ### What changes were proposed in this pull request? Check the name passed to `SessionCatalog.refreshTable`, and if it belongs to a temporary view, do not invalidate the relation cache. ### Why are the changes needed? When `SessionCatalog.refreshTable` refreshes a temporary or global temporary view, it should not invalidate an entry in the relation cache associated to a table with the same name. ### Does this PR introduce _any_ user-facing change? Should not. The change might improve performance slightly. ### How was this patch tested? By running new UT: ``` $ build/sbt -Phive-2.3 -Phive-thriftserver "test:testOnly *SessionCatalogSuite" ``` Closes apache#31265 from MaxGekk/fix-session-catalog-refresh-table. Authored-by: Max Gekk <max.gekk@gmail.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>

Fix

9946fe6

github-actions bot added the SQL label Jan 20, 2021

MaxGekk added 2 commits January 21, 2021 00:57

Fix 2

69fbb67

Refactor

a73cb59

Add test

3b49a35

MaxGekk changed the title ~~[WIP][SQL] Don't invalidate the relation cache for temp and global views~~ [SPARK-34197][SQL] SessionCatalog.refreshTable() should not invalidate the relation cache for temporary views Jan 21, 2021

MaxGekk mentioned this pull request Jan 21, 2021

[SPARK-34138][SQL] Keep dependants cached while refreshing v1 tables #31206

Closed

sunchao reviewed Jan 21, 2021

View reviewed changes

Address a review comment

dcae0ef

cloud-fan reviewed Jan 22, 2021

View reviewed changes

Address Wenchen review comment

5c15355

Merge remote-tracking branch 'origin/master' into fix-session-catalog…

4c3350e

…-refresh-table # Conflicts: # sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/SessionCatalog.scala

HyukjinKwon approved these changes Jan 25, 2021

View reviewed changes

cloud-fan reviewed Jan 25, 2021

View reviewed changes

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/catalog/SessionCatalogSuite.scala Show resolved Hide resolved

cloud-fan closed this in 6fe5a8a Jan 25, 2021

YannByron mentioned this pull request Mar 23, 2023

[HUDI-5697] Revisiting refreshing of Hudi relations after write operations on the tables apache/hudi#7847

Open

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-34197][SQL] `SessionCatalog.refreshTable()` should not invalidate the relation cache for temporary views #31265

[SPARK-34197][SQL] `SessionCatalog.refreshTable()` should not invalidate the relation cache for temporary views #31265

MaxGekk commented Jan 20, 2021 •

edited

SparkQA commented Jan 20, 2021

SparkQA commented Jan 20, 2021

SparkQA commented Jan 21, 2021

MaxGekk commented Jan 21, 2021

sunchao left a comment

sunchao Jan 21, 2021

MaxGekk Jan 22, 2021

SparkQA commented Jan 21, 2021

SparkQA commented Jan 21, 2021

SparkQA commented Jan 22, 2021

MaxGekk commented Jan 22, 2021 •

edited

SparkQA commented Jan 22, 2021

SparkQA commented Jan 22, 2021

SparkQA commented Jan 22, 2021

cloud-fan Jan 22, 2021

cloud-fan Jan 22, 2021

MaxGekk Jan 22, 2021

cloud-fan Jan 22, 2021

MaxGekk Jan 22, 2021

cloud-fan Jan 22, 2021

MaxGekk Jan 22, 2021

MaxGekk Jan 22, 2021

SparkQA commented Jan 22, 2021

SparkQA commented Jan 22, 2021

SparkQA commented Jan 22, 2021

SparkQA commented Jan 23, 2021

MaxGekk commented Jan 24, 2021

cloud-fan commented Jan 25, 2021

[SPARK-34197][SQL] SessionCatalog.refreshTable() should not invalidate the relation cache for temporary views #31265

[SPARK-34197][SQL] SessionCatalog.refreshTable() should not invalidate the relation cache for temporary views #31265

Conversation

MaxGekk commented Jan 20, 2021 • edited

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

SparkQA commented Jan 20, 2021

SparkQA commented Jan 20, 2021

SparkQA commented Jan 21, 2021

MaxGekk commented Jan 21, 2021

sunchao left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Jan 21, 2021

SparkQA commented Jan 21, 2021

SparkQA commented Jan 22, 2021

MaxGekk commented Jan 22, 2021 • edited

SparkQA commented Jan 22, 2021

SparkQA commented Jan 22, 2021

SparkQA commented Jan 22, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Jan 22, 2021

SparkQA commented Jan 22, 2021

SparkQA commented Jan 22, 2021

SparkQA commented Jan 23, 2021

MaxGekk commented Jan 24, 2021

cloud-fan commented Jan 25, 2021

[SPARK-34197][SQL] `SessionCatalog.refreshTable()` should not invalidate the relation cache for temporary views #31265

[SPARK-34197][SQL] `SessionCatalog.refreshTable()` should not invalidate the relation cache for temporary views #31265

MaxGekk commented Jan 20, 2021 •

edited

MaxGekk commented Jan 22, 2021 •

edited