You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GitBox <gi...@apache.org> on 2020/12/15 03:53:04 UTC

[GitHub] [spark] imback82 opened a new pull request #30774: [SPARK-33786][SQL] The storage level for a cache should be respected when a table name is altered.

imback82 opened a new pull request #30774:
URL: https://github.com/apache/spark/pull/30774


   <!--
   Thanks for sending a pull request!  Here are some tips for you:
     1. If this is your first time, please read our contributor guidelines: https://spark.apache.org/contributing.html
     2. Ensure you have added or run the appropriate tests for your PR: https://spark.apache.org/developer-tools.html
     3. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP][SPARK-XXXX] Your PR title ...'.
     4. Be sure to keep the PR description updated to reflect all changes.
     5. Please write your PR title to summarize what this PR proposes.
     6. If possible, provide a concise example to reproduce the issue for a faster review.
     7. If you want to add a new configuration, please read the guideline first for naming configurations in
        'core/src/main/scala/org/apache/spark/internal/config/ConfigEntry.scala'.
   -->
   
   ### What changes were proposed in this pull request?
   <!--
   Please clarify what changes you are proposing. The purpose of this section is to outline the changes and how this PR fixes the issue. 
   If possible, please consider writing useful notes for better and faster reviews in your PR. See the examples below.
     1. If you refactor some codes with changing classes, showing the class hierarchy will help reviewers.
     2. If you fix some SQL features, you can provide some references of other DBMSes.
     3. If there is design documentation, please add the link.
     4. If there is a discussion in the mailing list, please add the link.
   -->
   This PR proposes to retain the cache's storage level when a table name is altered by `ALTER TABLE ... RENAME TO ...`.
   
   ### Why are the changes needed?
   <!--
   Please clarify why the changes are needed. For instance,
     1. If you propose a new API, clarify the use case for a new API.
     2. If you fix a bug, you can clarify why it is a bug.
   -->
   Currently, when a table name is altered, the table's cache is refreshed (if exists), but the storage level is not retained. For example:
   ```scala
           def getStorageLevel(tableName: String): StorageLevel = {
             val table = spark.table(tableName)
             val cachedData = spark.sharedState.cacheManager.lookupCachedData(table).get
             cachedData.cachedRepresentation.cacheBuilder.storageLevel
           }
   
           Seq(1 -> "a").toDF("i", "j").write.parquet(path.getCanonicalPath)
           sql(s"CREATE TABLE old USING parquet LOCATION '${path.toURI}'")
           sql("CACHE TABLE old OPTIONS('storageLevel' 'MEMORY_ONLY')")
           val oldStorageLevel = getStorageLevel("old")
   
           sql("ALTER TABLE old RENAME TO new")
           val newStorageLevel = getStorageLevel("new")
   ```
   `oldStorageLevel` will be `StorageLevel(memory, deserialized, 1 replicas)` whereas `newStorageLevel` will be `StorageLevel(disk, memory, deserialized, 1 replicas)`, which is the default storage level.
   
   ### Does this PR introduce _any_ user-facing change?
   <!--
   Note that it means *any* user-facing change including all aspects such as the documentation fix.
   If yes, please clarify the previous behavior and the change this PR proposes - provide the console output, description and/or an example to show the behavior difference if possible.
   If possible, please also clarify if this is a user-facing change compared to the released Spark versions or within the unreleased branches such as master.
   If no, write 'No'.
   -->
   Yes, now the storage level for the cache will be retained.
   
   ### How was this patch tested?
   <!--
   If tests were added, say they were added here. Please make sure to add some test cases that check the changes thoroughly including negative and positive cases if possible.
   If it was tested in a way different from regular unit tests, please clarify how you tested step by step, ideally copy and paste-able, so that other reviewers can test and check, and descendants can verify in the future.
   If tests were not added, please describe why they were not added and/or why it was difficult to add.
   -->
   Added a unit test.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] SparkQA commented on pull request #30774: [SPARK-33786][SQL] The storage level for a cache should be respected when a table name is altered.

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #30774:
URL: https://github.com/apache/spark/pull/30774#issuecomment-745124774


   **[Test build #132814 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/132814/testReport)** for PR 30774 at commit [`11603d5`](https://github.com/apache/spark/commit/11603d54d4a6b222edc2c5dbd4755e3a1e9072a2).


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] imback82 commented on a change in pull request #30774: [SPARK-33786][SQL] The storage level for a cache should be respected when a table name is altered.

Posted by GitBox <gi...@apache.org>.
imback82 commented on a change in pull request #30774:
URL: https://github.com/apache/spark/pull/30774#discussion_r543109924



##########
File path: sql/core/src/main/scala/org/apache/spark/sql/execution/command/tables.scala
##########
@@ -195,16 +195,21 @@ case class AlterTableRenameCommand(
       DDLUtils.verifyAlterTableType(catalog, table, isView)
       // If an exception is thrown here we can just assume the table is uncached;
       // this can happen with Hive tables when the underlying catalog is in-memory.
-      val wasCached = Try(sparkSession.catalog.isCached(oldName.unquotedString)).getOrElse(false)
-      if (wasCached) {
+      // If `optStorageLevel` is defined, the old table was cached.
+      val optStorageLevel = Try {
+        val optCachedData = sparkSession.sharedState.cacheManager.lookupCachedData(
+          sparkSession.table(oldName.unquotedString))
+        optCachedData.map(_.cachedRepresentation.cacheBuilder.storageLevel)
+      }.getOrElse(None)
+      optStorageLevel.foreach { _ =>

Review comment:
       updated.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] AmplabJenkins commented on pull request #30774: [SPARK-33786][SQL] The storage level for a cache should be respected when a table name is altered.

Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #30774:
URL: https://github.com/apache/spark/pull/30774#issuecomment-745073991


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/37400/
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] imback82 commented on pull request #30774: [SPARK-33786][SQL] The storage level for a cache should be respected when a table name is altered.

Posted by GitBox <gi...@apache.org>.
imback82 commented on pull request #30774:
URL: https://github.com/apache/spark/pull/30774#issuecomment-745036108


   I will also update v2 implementation (`RenameTableExec`) once this PR is done.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] AmplabJenkins removed a comment on pull request #30774: [SPARK-33786][SQL] The storage level for a cache should be respected when a table name is altered.

Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #30774:
URL: https://github.com/apache/spark/pull/30774#issuecomment-745152547


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/132814/
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] imback82 commented on a change in pull request #30774: [SPARK-33786][SQL] The storage level for a cache should be respected when a table name is altered.

Posted by GitBox <gi...@apache.org>.
imback82 commented on a change in pull request #30774:
URL: https://github.com/apache/spark/pull/30774#discussion_r543109646



##########
File path: sql/core/src/main/scala/org/apache/spark/sql/execution/command/tables.scala
##########
@@ -195,16 +195,21 @@ case class AlterTableRenameCommand(
       DDLUtils.verifyAlterTableType(catalog, table, isView)
       // If an exception is thrown here we can just assume the table is uncached;
       // this can happen with Hive tables when the underlying catalog is in-memory.
-      val wasCached = Try(sparkSession.catalog.isCached(oldName.unquotedString)).getOrElse(false)
-      if (wasCached) {
+      // If `optStorageLevel` is defined, the old table was cached.
+      val optStorageLevel = Try {
+        val optCachedData = sparkSession.sharedState.cacheManager.lookupCachedData(
+          sparkSession.table(oldName.unquotedString))
+        optCachedData.map(_.cachedRepresentation.cacheBuilder.storageLevel)
+      }.getOrElse(None)
+      optStorageLevel.foreach { _ =>
         CommandUtils.uncacheTableOrView(sparkSession, oldName.unquotedString)
       }
       // Invalidate the table last, otherwise uncaching the table would load the logical plan
       // back into the hive metastore cache
       catalog.refreshTable(oldName)
       catalog.renameTable(oldName, newName)
-      if (wasCached) {
-        sparkSession.catalog.cacheTable(newName.unquotedString)
+      optStorageLevel.foreach { storageLevel =>
+        sparkSession.catalog.cacheTable(newName.unquotedString, storageLevel)

Review comment:
       Sorry, I didn't get this question. This is creating a new cache with a new table name.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] viirya commented on a change in pull request #30774: [SPARK-33786][SQL] The storage level for a cache should be respected when a table name is altered.

Posted by GitBox <gi...@apache.org>.
viirya commented on a change in pull request #30774:
URL: https://github.com/apache/spark/pull/30774#discussion_r543088536



##########
File path: sql/core/src/main/scala/org/apache/spark/sql/execution/command/tables.scala
##########
@@ -195,16 +195,21 @@ case class AlterTableRenameCommand(
       DDLUtils.verifyAlterTableType(catalog, table, isView)
       // If an exception is thrown here we can just assume the table is uncached;
       // this can happen with Hive tables when the underlying catalog is in-memory.
-      val wasCached = Try(sparkSession.catalog.isCached(oldName.unquotedString)).getOrElse(false)
-      if (wasCached) {
+      // If `optStorageLevel` is defined, the old table was cached.
+      val optStorageLevel = Try {
+        val optCachedData = sparkSession.sharedState.cacheManager.lookupCachedData(
+          sparkSession.table(oldName.unquotedString))
+        optCachedData.map(_.cachedRepresentation.cacheBuilder.storageLevel)
+      }.getOrElse(None)
+      optStorageLevel.foreach { _ =>

Review comment:
       +1




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] cloud-fan commented on pull request #30774: [SPARK-33786][SQL] The storage level for a cache should be respected when a table name is altered.

Posted by GitBox <gi...@apache.org>.
cloud-fan commented on pull request #30774:
URL: https://github.com/apache/spark/pull/30774#issuecomment-745779804


   @imback82 can you open a backport PR for 3.0? thanks!


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] SparkQA commented on pull request #30774: [SPARK-33786][SQL] The storage level for a cache should be respected when a table name is altered.

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #30774:
URL: https://github.com/apache/spark/pull/30774#issuecomment-745063659


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


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] viirya commented on a change in pull request #30774: [SPARK-33786][SQL] The storage level for a cache should be respected when a table name is altered.

Posted by GitBox <gi...@apache.org>.
viirya commented on a change in pull request #30774:
URL: https://github.com/apache/spark/pull/30774#discussion_r543157855



##########
File path: sql/core/src/main/scala/org/apache/spark/sql/execution/command/tables.scala
##########
@@ -195,16 +195,21 @@ case class AlterTableRenameCommand(
       DDLUtils.verifyAlterTableType(catalog, table, isView)
       // If an exception is thrown here we can just assume the table is uncached;
       // this can happen with Hive tables when the underlying catalog is in-memory.
-      val wasCached = Try(sparkSession.catalog.isCached(oldName.unquotedString)).getOrElse(false)
-      if (wasCached) {
+      // If `optStorageLevel` is defined, the old table was cached.
+      val optStorageLevel = Try {
+        val optCachedData = sparkSession.sharedState.cacheManager.lookupCachedData(
+          sparkSession.table(oldName.unquotedString))
+        optCachedData.map(_.cachedRepresentation.cacheBuilder.storageLevel)
+      }.getOrElse(None)
+      optStorageLevel.foreach { _ =>
         CommandUtils.uncacheTableOrView(sparkSession, oldName.unquotedString)
       }
       // Invalidate the table last, otherwise uncaching the table would load the logical plan
       // back into the hive metastore cache
       catalog.refreshTable(oldName)
       catalog.renameTable(oldName, newName)
-      if (wasCached) {
-        sparkSession.catalog.cacheTable(newName.unquotedString)
+      optStorageLevel.foreach { storageLevel =>
+        sparkSession.catalog.cacheTable(newName.unquotedString, storageLevel)

Review comment:
       Hm, you can check the change like #30769. Especially how it recaches the table. There is a `cacheName` parameter. If the table was cached with a cache name, when recaching it, I think we should keep it.
   
   ```scala
       val cache = session.sharedState.cacheManager.lookupCachedData(v2Relation)
       session.sharedState.cacheManager.uncacheQuery(session, v2Relation, cascade = true)
       session.sharedState.cacheManager.uncacheQuery(session, v2Relation, cascade = true)
       if (recacheTable && cache.isDefined) {
         // save the cache name and cache level for recreation
         val cacheName = cache.get.cachedRepresentation.cacheBuilder.tableName
         val cacheLevel = cache.get.cachedRepresentation.cacheBuilder.storageLevel
   
         // recache with the same name and cache level.
         val ds = Dataset.ofRows(session, v2Relation)
         session.sharedState.cacheManager.cacheQuery(ds, cacheName, cacheLevel)
       }
   ```




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] viirya commented on a change in pull request #30774: [SPARK-33786][SQL] The storage level for a cache should be respected when a table name is altered.

Posted by GitBox <gi...@apache.org>.
viirya commented on a change in pull request #30774:
URL: https://github.com/apache/spark/pull/30774#discussion_r543168002



##########
File path: sql/core/src/main/scala/org/apache/spark/sql/execution/command/tables.scala
##########
@@ -195,16 +195,21 @@ case class AlterTableRenameCommand(
       DDLUtils.verifyAlterTableType(catalog, table, isView)
       // If an exception is thrown here we can just assume the table is uncached;
       // this can happen with Hive tables when the underlying catalog is in-memory.
-      val wasCached = Try(sparkSession.catalog.isCached(oldName.unquotedString)).getOrElse(false)
-      if (wasCached) {
+      // If `optStorageLevel` is defined, the old table was cached.
+      val optStorageLevel = Try {
+        val optCachedData = sparkSession.sharedState.cacheManager.lookupCachedData(
+          sparkSession.table(oldName.unquotedString))
+        optCachedData.map(_.cachedRepresentation.cacheBuilder.storageLevel)
+      }.getOrElse(None)
+      optStorageLevel.foreach { _ =>
         CommandUtils.uncacheTableOrView(sparkSession, oldName.unquotedString)
       }
       // Invalidate the table last, otherwise uncaching the table would load the logical plan
       // back into the hive metastore cache
       catalog.refreshTable(oldName)
       catalog.renameTable(oldName, newName)
-      if (wasCached) {
-        sparkSession.catalog.cacheTable(newName.unquotedString)
+      optStorageLevel.foreach { storageLevel =>
+        sparkSession.catalog.cacheTable(newName.unquotedString, storageLevel)

Review comment:
       No, the refresh table command for v2 doesn't recache the table before #30769.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] AmplabJenkins removed a comment on pull request #30774: [SPARK-33786][SQL] The storage level for a cache should be respected when a table name is altered.

Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #30774:
URL: https://github.com/apache/spark/pull/30774#issuecomment-745073991


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/37400/
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] cloud-fan commented on a change in pull request #30774: [SPARK-33786][SQL] The storage level for a cache should be respected when a table name is altered.

Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #30774:
URL: https://github.com/apache/spark/pull/30774#discussion_r543069288



##########
File path: sql/core/src/main/scala/org/apache/spark/sql/execution/command/tables.scala
##########
@@ -195,16 +195,21 @@ case class AlterTableRenameCommand(
       DDLUtils.verifyAlterTableType(catalog, table, isView)
       // If an exception is thrown here we can just assume the table is uncached;
       // this can happen with Hive tables when the underlying catalog is in-memory.
-      val wasCached = Try(sparkSession.catalog.isCached(oldName.unquotedString)).getOrElse(false)
-      if (wasCached) {
+      // If `optStorageLevel` is defined, the old table was cached.
+      val optStorageLevel = Try {
+        val optCachedData = sparkSession.sharedState.cacheManager.lookupCachedData(
+          sparkSession.table(oldName.unquotedString))
+        optCachedData.map(_.cachedRepresentation.cacheBuilder.storageLevel)
+      }.getOrElse(None)
+      optStorageLevel.foreach { _ =>

Review comment:
       nit: `if (optStorageLevel.isDefine)`. It's clearer as a check to see if the table is cached or not.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] viirya commented on a change in pull request #30774: [SPARK-33786][SQL] The storage level for a cache should be respected when a table name is altered.

Posted by GitBox <gi...@apache.org>.
viirya commented on a change in pull request #30774:
URL: https://github.com/apache/spark/pull/30774#discussion_r543155487



##########
File path: sql/core/src/main/scala/org/apache/spark/sql/execution/command/tables.scala
##########
@@ -195,16 +195,21 @@ case class AlterTableRenameCommand(
       DDLUtils.verifyAlterTableType(catalog, table, isView)
       // If an exception is thrown here we can just assume the table is uncached;
       // this can happen with Hive tables when the underlying catalog is in-memory.
-      val wasCached = Try(sparkSession.catalog.isCached(oldName.unquotedString)).getOrElse(false)
-      if (wasCached) {
+      // If `optStorageLevel` is defined, the old table was cached.
+      val optStorageLevel = Try {

Review comment:
       +1 to remove it. We use `lookupCachedData` in many places and I think we don't need to add `Try` here.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] dongjoon-hyun commented on pull request #30774: [SPARK-33786][SQL] The storage level for a cache should be respected when a table name is altered.

Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on pull request #30774:
URL: https://github.com/apache/spark/pull/30774#issuecomment-745901323


   Thank you for the fix, @imback82 and @cloud-fan .
   
   cc @sunchao 


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] SparkQA removed a comment on pull request #30774: [SPARK-33786][SQL] The storage level for a cache should be respected when a table name is altered.

Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #30774:
URL: https://github.com/apache/spark/pull/30774#issuecomment-745038276


   **[Test build #132798 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/132798/testReport)** for PR 30774 at commit [`eb60d1e`](https://github.com/apache/spark/commit/eb60d1efd056cdf1ad837dcce1da2dc4643f6cb8).


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] AmplabJenkins commented on pull request #30774: [SPARK-33786][SQL] The storage level for a cache should be respected when a table name is altered.

Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #30774:
URL: https://github.com/apache/spark/pull/30774#issuecomment-745163413


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/37415/
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] imback82 commented on a change in pull request #30774: [SPARK-33786][SQL] The storage level for a cache should be respected when a table name is altered.

Posted by GitBox <gi...@apache.org>.
imback82 commented on a change in pull request #30774:
URL: https://github.com/apache/spark/pull/30774#discussion_r543109318



##########
File path: sql/core/src/main/scala/org/apache/spark/sql/execution/command/tables.scala
##########
@@ -195,16 +195,21 @@ case class AlterTableRenameCommand(
       DDLUtils.verifyAlterTableType(catalog, table, isView)
       // If an exception is thrown here we can just assume the table is uncached;
       // this can happen with Hive tables when the underlying catalog is in-memory.
-      val wasCached = Try(sparkSession.catalog.isCached(oldName.unquotedString)).getOrElse(false)
-      if (wasCached) {
+      // If `optStorageLevel` is defined, the old table was cached.
+      val optStorageLevel = Try {

Review comment:
       `lookupCachedData` doesn't throw an exception. I believe the existing code is wrapped in `Try` because `isCached` is calling `spark.table` (could be related to "this can happen with Hive tables when the underlying catalog is in-memory"). So, I was keeping the same behavior.
   
   Do you think `Try` can be removed?
   




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] SparkQA commented on pull request #30774: [SPARK-33786][SQL] The storage level for a cache should be respected when a table name is altered.

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #30774:
URL: https://github.com/apache/spark/pull/30774#issuecomment-745038276


   **[Test build #132798 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/132798/testReport)** for PR 30774 at commit [`eb60d1e`](https://github.com/apache/spark/commit/eb60d1efd056cdf1ad837dcce1da2dc4643f6cb8).


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] SparkQA removed a comment on pull request #30774: [SPARK-33786][SQL] The storage level for a cache should be respected when a table name is altered.

Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #30774:
URL: https://github.com/apache/spark/pull/30774#issuecomment-745124774


   **[Test build #132814 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/132814/testReport)** for PR 30774 at commit [`11603d5`](https://github.com/apache/spark/commit/11603d54d4a6b222edc2c5dbd4755e3a1e9072a2).


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] AmplabJenkins commented on pull request #30774: [SPARK-33786][SQL] The storage level for a cache should be respected when a table name is altered.

Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #30774:
URL: https://github.com/apache/spark/pull/30774#issuecomment-745098158


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/132798/
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] SparkQA commented on pull request #30774: [SPARK-33786][SQL] The storage level for a cache should be respected when a table name is altered.

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #30774:
URL: https://github.com/apache/spark/pull/30774#issuecomment-745144681


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


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] cloud-fan commented on a change in pull request #30774: [SPARK-33786][SQL] The storage level for a cache should be respected when a table name is altered.

Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #30774:
URL: https://github.com/apache/spark/pull/30774#discussion_r543069288



##########
File path: sql/core/src/main/scala/org/apache/spark/sql/execution/command/tables.scala
##########
@@ -195,16 +195,21 @@ case class AlterTableRenameCommand(
       DDLUtils.verifyAlterTableType(catalog, table, isView)
       // If an exception is thrown here we can just assume the table is uncached;
       // this can happen with Hive tables when the underlying catalog is in-memory.
-      val wasCached = Try(sparkSession.catalog.isCached(oldName.unquotedString)).getOrElse(false)
-      if (wasCached) {
+      // If `optStorageLevel` is defined, the old table was cached.
+      val optStorageLevel = Try {
+        val optCachedData = sparkSession.sharedState.cacheManager.lookupCachedData(
+          sparkSession.table(oldName.unquotedString))
+        optCachedData.map(_.cachedRepresentation.cacheBuilder.storageLevel)
+      }.getOrElse(None)
+      optStorageLevel.foreach { _ =>

Review comment:
       nit: `if (optStorageLevel.isDefine)`. It's clearer to check if the table is cached or not.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] AmplabJenkins commented on pull request #30774: [SPARK-33786][SQL] The storage level for a cache should be respected when a table name is altered.

Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #30774:
URL: https://github.com/apache/spark/pull/30774#issuecomment-745152547


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/132814/
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] cloud-fan commented on a change in pull request #30774: [SPARK-33786][SQL] The storage level for a cache should be respected when a table name is altered.

Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #30774:
URL: https://github.com/apache/spark/pull/30774#discussion_r543164927



##########
File path: sql/core/src/main/scala/org/apache/spark/sql/execution/command/tables.scala
##########
@@ -195,16 +195,21 @@ case class AlterTableRenameCommand(
       DDLUtils.verifyAlterTableType(catalog, table, isView)
       // If an exception is thrown here we can just assume the table is uncached;
       // this can happen with Hive tables when the underlying catalog is in-memory.
-      val wasCached = Try(sparkSession.catalog.isCached(oldName.unquotedString)).getOrElse(false)
-      if (wasCached) {
+      // If `optStorageLevel` is defined, the old table was cached.
+      val optStorageLevel = Try {
+        val optCachedData = sparkSession.sharedState.cacheManager.lookupCachedData(
+          sparkSession.table(oldName.unquotedString))
+        optCachedData.map(_.cachedRepresentation.cacheBuilder.storageLevel)
+      }.getOrElse(None)
+      optStorageLevel.foreach { _ =>
         CommandUtils.uncacheTableOrView(sparkSession, oldName.unquotedString)
       }
       // Invalidate the table last, otherwise uncaching the table would load the logical plan
       // back into the hive metastore cache
       catalog.refreshTable(oldName)
       catalog.renameTable(oldName, newName)
-      if (wasCached) {
-        sparkSession.catalog.cacheTable(newName.unquotedString)
+      optStorageLevel.foreach { storageLevel =>
+        sparkSession.catalog.cacheTable(newName.unquotedString, storageLevel)

Review comment:
       The previous code seems also recache with the new name?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] imback82 commented on a change in pull request #30774: [SPARK-33786][SQL] The storage level for a cache should be respected when a table name is altered.

Posted by GitBox <gi...@apache.org>.
imback82 commented on a change in pull request #30774:
URL: https://github.com/apache/spark/pull/30774#discussion_r543780949



##########
File path: sql/core/src/main/scala/org/apache/spark/sql/execution/command/tables.scala
##########
@@ -195,16 +195,21 @@ case class AlterTableRenameCommand(
       DDLUtils.verifyAlterTableType(catalog, table, isView)
       // If an exception is thrown here we can just assume the table is uncached;
       // this can happen with Hive tables when the underlying catalog is in-memory.
-      val wasCached = Try(sparkSession.catalog.isCached(oldName.unquotedString)).getOrElse(false)
-      if (wasCached) {
+      // If `optStorageLevel` is defined, the old table was cached.
+      val optStorageLevel = Try {
+        val optCachedData = sparkSession.sharedState.cacheManager.lookupCachedData(
+          sparkSession.table(oldName.unquotedString))
+        optCachedData.map(_.cachedRepresentation.cacheBuilder.storageLevel)
+      }.getOrElse(None)
+      optStorageLevel.foreach { _ =>
         CommandUtils.uncacheTableOrView(sparkSession, oldName.unquotedString)
       }
       // Invalidate the table last, otherwise uncaching the table would load the logical plan
       // back into the hive metastore cache
       catalog.refreshTable(oldName)
       catalog.renameTable(oldName, newName)
-      if (wasCached) {
-        sparkSession.catalog.cacheTable(newName.unquotedString)
+      optStorageLevel.foreach { storageLevel =>
+        sparkSession.catalog.cacheTable(newName.unquotedString, storageLevel)

Review comment:
       I believe cache name is used for debug purpose only (for `RDD` name and `InMemoryTableScanExec`). So if the cache name - which is tied to the table name - doesn't change when the table is changed, wouldn't it cause a confusion since it will still refer to the old table name? I can do a follow up PR if this seems like a bug.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] SparkQA commented on pull request #30774: [SPARK-33786][SQL] The storage level for a cache should be respected when a table name is altered.

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #30774:
URL: https://github.com/apache/spark/pull/30774#issuecomment-745053690


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


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] imback82 commented on a change in pull request #30774: [SPARK-33786][SQL] The storage level for a cache should be respected when a table name is altered.

Posted by GitBox <gi...@apache.org>.
imback82 commented on a change in pull request #30774:
URL: https://github.com/apache/spark/pull/30774#discussion_r543026892



##########
File path: sql/core/src/main/scala/org/apache/spark/sql/execution/command/tables.scala
##########
@@ -195,16 +195,21 @@ case class AlterTableRenameCommand(
       DDLUtils.verifyAlterTableType(catalog, table, isView)
       // If an exception is thrown here we can just assume the table is uncached;
       // this can happen with Hive tables when the underlying catalog is in-memory.
-      val wasCached = Try(sparkSession.catalog.isCached(oldName.unquotedString)).getOrElse(false)

Review comment:
       The existing implementation uses `Catalog` APIs (`isCached`), whereas this PR uses `CacheManager` directly. If this approach is not desired, we can update `Catalog` API to expose `StorageLevel`.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] cloud-fan closed pull request #30774: [SPARK-33786][SQL] The storage level for a cache should be respected when a table name is altered.

Posted by GitBox <gi...@apache.org>.
cloud-fan closed pull request #30774:
URL: https://github.com/apache/spark/pull/30774


   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] AmplabJenkins removed a comment on pull request #30774: [SPARK-33786][SQL] The storage level for a cache should be respected when a table name is altered.

Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #30774:
URL: https://github.com/apache/spark/pull/30774#issuecomment-745163413


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/37415/
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] cloud-fan commented on a change in pull request #30774: [SPARK-33786][SQL] The storage level for a cache should be respected when a table name is altered.

Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #30774:
URL: https://github.com/apache/spark/pull/30774#discussion_r543110908



##########
File path: sql/core/src/main/scala/org/apache/spark/sql/execution/command/tables.scala
##########
@@ -195,16 +195,21 @@ case class AlterTableRenameCommand(
       DDLUtils.verifyAlterTableType(catalog, table, isView)
       // If an exception is thrown here we can just assume the table is uncached;
       // this can happen with Hive tables when the underlying catalog is in-memory.
-      val wasCached = Try(sparkSession.catalog.isCached(oldName.unquotedString)).getOrElse(false)
-      if (wasCached) {
+      // If `optStorageLevel` is defined, the old table was cached.
+      val optStorageLevel = Try {

Review comment:
       Yea I think we can remove it




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] SparkQA commented on pull request #30774: [SPARK-33786][SQL] The storage level for a cache should be respected when a table name is altered.

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #30774:
URL: https://github.com/apache/spark/pull/30774#issuecomment-745141445


   **[Test build #132814 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/132814/testReport)** for PR 30774 at commit [`11603d5`](https://github.com/apache/spark/commit/11603d54d4a6b222edc2c5dbd4755e3a1e9072a2).
    * This patch **fails Spark unit tests**.
    * This patch merges cleanly.
    * This patch adds no public classes.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] AmplabJenkins removed a comment on pull request #30774: [SPARK-33786][SQL] The storage level for a cache should be respected when a table name is altered.

Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #30774:
URL: https://github.com/apache/spark/pull/30774#issuecomment-745098158


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/132798/
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] cloud-fan commented on a change in pull request #30774: [SPARK-33786][SQL] The storage level for a cache should be respected when a table name is altered.

Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #30774:
URL: https://github.com/apache/spark/pull/30774#discussion_r543368461



##########
File path: sql/core/src/main/scala/org/apache/spark/sql/execution/command/tables.scala
##########
@@ -195,16 +195,21 @@ case class AlterTableRenameCommand(
       DDLUtils.verifyAlterTableType(catalog, table, isView)
       // If an exception is thrown here we can just assume the table is uncached;
       // this can happen with Hive tables when the underlying catalog is in-memory.
-      val wasCached = Try(sparkSession.catalog.isCached(oldName.unquotedString)).getOrElse(false)
-      if (wasCached) {
+      // If `optStorageLevel` is defined, the old table was cached.
+      val optStorageLevel = Try {
+        val optCachedData = sparkSession.sharedState.cacheManager.lookupCachedData(
+          sparkSession.table(oldName.unquotedString))
+        optCachedData.map(_.cachedRepresentation.cacheBuilder.storageLevel)
+      }.getOrElse(None)
+      optStorageLevel.foreach { _ =>
         CommandUtils.uncacheTableOrView(sparkSession, oldName.unquotedString)
       }
       // Invalidate the table last, otherwise uncaching the table would load the logical plan
       // back into the hive metastore cache
       catalog.refreshTable(oldName)
       catalog.renameTable(oldName, newName)
-      if (wasCached) {
-        sparkSession.catalog.cacheTable(newName.unquotedString)
+      optStorageLevel.foreach { storageLevel =>
+        sparkSession.catalog.cacheTable(newName.unquotedString, storageLevel)

Review comment:
       I mean the previous code in `AlterTableRenameCommand`. We shouldn't change its behavior regarding cache name in this bug fix PR.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] viirya commented on a change in pull request #30774: [SPARK-33786][SQL] The storage level for a cache should be respected when a table name is altered.

Posted by GitBox <gi...@apache.org>.
viirya commented on a change in pull request #30774:
URL: https://github.com/apache/spark/pull/30774#discussion_r543092152



##########
File path: sql/core/src/main/scala/org/apache/spark/sql/execution/command/tables.scala
##########
@@ -195,16 +195,21 @@ case class AlterTableRenameCommand(
       DDLUtils.verifyAlterTableType(catalog, table, isView)
       // If an exception is thrown here we can just assume the table is uncached;
       // this can happen with Hive tables when the underlying catalog is in-memory.
-      val wasCached = Try(sparkSession.catalog.isCached(oldName.unquotedString)).getOrElse(false)
-      if (wasCached) {
+      // If `optStorageLevel` is defined, the old table was cached.
+      val optStorageLevel = Try {
+        val optCachedData = sparkSession.sharedState.cacheManager.lookupCachedData(
+          sparkSession.table(oldName.unquotedString))
+        optCachedData.map(_.cachedRepresentation.cacheBuilder.storageLevel)
+      }.getOrElse(None)
+      optStorageLevel.foreach { _ =>
         CommandUtils.uncacheTableOrView(sparkSession, oldName.unquotedString)
       }
       // Invalidate the table last, otherwise uncaching the table would load the logical plan
       // back into the hive metastore cache
       catalog.refreshTable(oldName)
       catalog.renameTable(oldName, newName)
-      if (wasCached) {
-        sparkSession.catalog.cacheTable(newName.unquotedString)
+      optStorageLevel.foreach { storageLevel =>
+        sparkSession.catalog.cacheTable(newName.unquotedString, storageLevel)

Review comment:
       Does this miss the `tableName` if there is in the original cache?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] viirya commented on a change in pull request #30774: [SPARK-33786][SQL] The storage level for a cache should be respected when a table name is altered.

Posted by GitBox <gi...@apache.org>.
viirya commented on a change in pull request #30774:
URL: https://github.com/apache/spark/pull/30774#discussion_r543090285



##########
File path: sql/core/src/main/scala/org/apache/spark/sql/execution/command/tables.scala
##########
@@ -195,16 +195,21 @@ case class AlterTableRenameCommand(
       DDLUtils.verifyAlterTableType(catalog, table, isView)
       // If an exception is thrown here we can just assume the table is uncached;
       // this can happen with Hive tables when the underlying catalog is in-memory.
-      val wasCached = Try(sparkSession.catalog.isCached(oldName.unquotedString)).getOrElse(false)
-      if (wasCached) {
+      // If `optStorageLevel` is defined, the old table was cached.
+      val optStorageLevel = Try {

Review comment:
       Will `lookupCachedData` throw exception actually? If it's not cached, `None` should be returned. Isn't?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] SparkQA commented on pull request #30774: [SPARK-33786][SQL] The storage level for a cache should be respected when a table name is altered.

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #30774:
URL: https://github.com/apache/spark/pull/30774#issuecomment-745163386


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


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] SparkQA commented on pull request #30774: [SPARK-33786][SQL] The storage level for a cache should be respected when a table name is altered.

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #30774:
URL: https://github.com/apache/spark/pull/30774#issuecomment-745086998


   **[Test build #132798 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/132798/testReport)** for PR 30774 at commit [`eb60d1e`](https://github.com/apache/spark/commit/eb60d1efd056cdf1ad837dcce1da2dc4643f6cb8).
    * This patch **fails Spark unit tests**.
    * This patch merges cleanly.
    * This patch adds no public classes.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] viirya commented on a change in pull request #30774: [SPARK-33786][SQL] The storage level for a cache should be respected when a table name is altered.

Posted by GitBox <gi...@apache.org>.
viirya commented on a change in pull request #30774:
URL: https://github.com/apache/spark/pull/30774#discussion_r543550354



##########
File path: sql/core/src/main/scala/org/apache/spark/sql/execution/command/tables.scala
##########
@@ -195,16 +195,21 @@ case class AlterTableRenameCommand(
       DDLUtils.verifyAlterTableType(catalog, table, isView)
       // If an exception is thrown here we can just assume the table is uncached;
       // this can happen with Hive tables when the underlying catalog is in-memory.
-      val wasCached = Try(sparkSession.catalog.isCached(oldName.unquotedString)).getOrElse(false)
-      if (wasCached) {
+      // If `optStorageLevel` is defined, the old table was cached.
+      val optStorageLevel = Try {
+        val optCachedData = sparkSession.sharedState.cacheManager.lookupCachedData(
+          sparkSession.table(oldName.unquotedString))
+        optCachedData.map(_.cachedRepresentation.cacheBuilder.storageLevel)
+      }.getOrElse(None)
+      optStorageLevel.foreach { _ =>
         CommandUtils.uncacheTableOrView(sparkSession, oldName.unquotedString)
       }
       // Invalidate the table last, otherwise uncaching the table would load the logical plan
       // back into the hive metastore cache
       catalog.refreshTable(oldName)
       catalog.renameTable(oldName, newName)
-      if (wasCached) {
-        sparkSession.catalog.cacheTable(newName.unquotedString)
+      optStorageLevel.foreach { storageLevel =>
+        sparkSession.catalog.cacheTable(newName.unquotedString, storageLevel)

Review comment:
       Hmm okay, actually it also sounds like a bug if alter table command changes the cache name. I'm fine to leave it unchanged here.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] imback82 commented on pull request #30774: [SPARK-33786][SQL] The storage level for a cache should be respected when a table name is altered.

Posted by GitBox <gi...@apache.org>.
imback82 commented on pull request #30774:
URL: https://github.com/apache/spark/pull/30774#issuecomment-745065924


   cc @cloud-fan @viirya 


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] cloud-fan commented on pull request #30774: [SPARK-33786][SQL] The storage level for a cache should be respected when a table name is altered.

Posted by GitBox <gi...@apache.org>.
cloud-fan commented on pull request #30774:
URL: https://github.com/apache/spark/pull/30774#issuecomment-745779528


   GA passed, merging to master/3.1, thanks!


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org