[SPARK-29987][SQL] Add CatalogTable cache in SessionCatalog to improve performance by ulysses-you · Pull Request #26627 · apache/spark

ulysses-you · 2019-11-21T11:07:49Z

What changes were proposed in this pull request?

This pr is an another idea to improve sql performance. More details see 26603

Here is the idea:
Execute command plan always simply and quickly, but session catalog maybe called multi times. We can cache CatalogTable and keep a short time to improve this scene. This pr use a time-based
policy to cache CatalogTable.

Why are the changes needed?

Improve sql performance.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Add UT.

ulysses-you · 2019-11-21T11:08:45Z

@rdblue @dongjoon-hyun Do you have time to take a review ?

rdblue · 2019-11-21T17:04:10Z

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


  private val validNameFormat = "([\\w_]+)".r

+  private val cachedCatalogTable = {


This is a cache, not a table. So the variable name would make more sense if it were catalogTableCache.

rdblue · 2019-11-21T17:06:19Z

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

+    cachedCatalogTable.put(qtn, catalogTable)
+  }
+
+  private[sql] def invalidateAllCachedCatalogTable(): Unit = {


This should be plural: invalidateAllCachedCatalogTables

rdblue · 2019-11-21T17:12:06Z

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

+    }
+  }
+
+  private[sql] def cacheCatalogTable(qtn: QualifiedTableName, catalogTable: CatalogTable): Unit = {


Caches should pass a Callable so that populating the cache can be combined with a get operation (get or initialize).

Instead of cacheCatalogTable, this should be getOrCacheCatalogTable(qtn: QualifiedTableName, init: Callable[CatalogTable]) that calls catalogTableCache.get(qtn, init).

OK, fix these.

Here getOrCacheCatalogTable is just a simple wrapper doing nothing.
I suggest to move the lambda parameter in the caller into this method. It is in fact a part of "Get or Cache".

dongjoon-hyun · 2019-11-21T21:56:21Z

ok to test

SparkQA · 2019-11-21T23:25:38Z

Test build #114256 has finished for PR 26627 at commit 9d88db8.

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

SparkQA · 2019-11-22T00:33:28Z

Test build #114259 has finished for PR 26627 at commit 83edf2a.

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

SparkQA · 2019-11-22T02:05:28Z

Test build #114260 has finished for PR 26627 at commit ba900c6.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2019-11-22T03:21:34Z

Test build #114266 has finished for PR 26627 at commit e710324.

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

SparkQA · 2019-11-22T05:14:34Z

Test build #114271 has finished for PR 26627 at commit 9d0ed7b.

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

konjac · 2019-11-22T10:47:37Z

sql/catalyst/src/main/scala/org/apache/spark/sql/internal/StaticSQLConf.scala

+      .internal()
+      .doc("The maximum expire seconds time of cache table catalog.")
+      .intConf
+      .checkValue(cacheSize => cacheSize >= 0 && cacheSize < 8,


cacheSize seems being copied from existing config items. Need rewording.

konjac · 2019-11-22T10:52:59Z

sql/catalyst/src/main/scala/org/apache/spark/sql/internal/StaticSQLConf.scala

+    buildStaticConf("spark.sql.filesourceTableCatalogCacheExpireSeconds")
+      .internal()
+      .doc("The maximum expire seconds time of cache table catalog.")
+      .intConf


What about timeConf ?

SparkQA · 2019-11-22T11:41:51Z

Test build #114291 has finished for PR 26627 at commit 8dc7f91.

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

konjac · 2019-11-22T11:04:55Z

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

@@ -222,6 +228,7 @@ class SessionCatalog(
    if (cascade && databaseExists(dbName)) {
      listTables(dbName).foreach { t =>
        invalidateCachedTable(QualifiedTableName(dbName, t.table))


These two methods invalidateCachedTable and invalidateCachedCatalogTable are really confusing in their names. I suggest to introduce some rewording to let the names more intuitive.

konjac · 2019-11-22T11:42:42Z

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

+    invalidateAllCachedCatalogTables()
+  }
+
+  private[sql] def getCachedCatalogTable(qtn: QualifiedTableName): Option[CatalogTable] = {


Add comments. Other new methods as well.

konjac · 2019-11-22T12:12:52Z

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

+    }
+  }
+
+  private[sql] def cacheCatalogTable(qtn: QualifiedTableName, catalogTable: CatalogTable): Unit = {


Here getOrCacheCatalogTable is just a simple wrapper doing nothing.
I suggest to move the lambda parameter in the caller into this method. It is in fact a part of "Get or Cache".

ulysses-you · 2019-11-22T13:38:03Z

It seems that StatisticsSuite use the underlying method hiveClient.alterTable that makes test failed.

SparkQA · 2019-11-22T13:42:27Z

Test build #114298 has finished for PR 26627 at commit 9bc6abd.

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

SparkQA · 2019-11-22T16:50:27Z

Test build #114301 has finished for PR 26627 at commit be169d8.

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

init

83b3c64

Fix

9d88db8

rdblue reviewed Nov 21, 2019

View reviewed changes

dongjoon-hyun added the SQL label Nov 21, 2019

Fix

83edf2a

unpack Cache ExecutionException

ba900c6

Fix style

e710324

Fix warn

9d0ed7b

Fix

8dc7f91

konjac reviewed Nov 22, 2019

View reviewed changes

Fix

9bc6abd

Fix

be169d8

ulysses-you closed this Dec 10, 2019


		private val validNameFormat = "([\\w_]+)".r

		private val cachedCatalogTable = {

Conversation

ulysses-you commented Nov 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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?

Uh oh!

ulysses-you commented Nov 21, 2019

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!

dongjoon-hyun commented Nov 21, 2019

Uh oh!

SparkQA commented Nov 21, 2019

Uh oh!

SparkQA commented Nov 22, 2019

Uh oh!

SparkQA commented Nov 22, 2019

Uh oh!

SparkQA commented Nov 22, 2019

Uh oh!

SparkQA commented Nov 22, 2019

Uh oh!

konjac Nov 22, 2019 • 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 Nov 22, 2019

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!

ulysses-you commented Nov 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SparkQA commented Nov 22, 2019

Uh oh!

SparkQA commented Nov 22, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ulysses-you commented Nov 21, 2019 •

edited

Loading

konjac Nov 22, 2019 •

edited

Loading

ulysses-you commented Nov 22, 2019 •

edited

Loading