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 2021/01/24 19:04:02 UTC
[GitHub] [spark] MaxGekk opened a new pull request #31308: [SPARK-34215][SQL] Keep tables cached after truncation
MaxGekk opened a new pull request #31308:
URL: https://github.com/apache/spark/pull/31308
### What changes were proposed in this pull request?
Invoke `CatalogImpl.refreshTable()` instead of combination of `SessionCatalog.refreshTable()` + `uncacheQuery()`. This allows to clear cached table data while keeping the table cached.
### Why are the changes needed?
1. To improve user experience with Spark SQL
2. To be consistent to other commands, see https://github.com/apache/spark/pull/31206
### Does this PR introduce _any_ user-facing change?
Yes.
Before:
```scala
scala> sql("CREATE TABLE tbl (c0 int)")
res1: org.apache.spark.sql.DataFrame = []
scala> sql("INSERT INTO tbl SELECT 0")
res2: org.apache.spark.sql.DataFrame = []
scala> sql("CACHE TABLE tbl")
res3: org.apache.spark.sql.DataFrame = []
scala> sql("SELECT * FROM tbl").show(false)
+---+
|c0 |
+---+
|0 |
+---+
scala> spark.catalog.isCached("tbl")
res5: Boolean = true
scala> sql("TRUNCATE TABLE tbl")
res6: org.apache.spark.sql.DataFrame = []
scala> spark.catalog.isCached("tbl")
res7: Boolean = false
```
After:
```scala
scala> sql("TRUNCATE TABLE tbl")
res6: org.apache.spark.sql.DataFrame = []
scala> spark.catalog.isCached("tbl")
res7: Boolean = true
```
### How was this patch tested?
Added new test to `CachedTableSuite`:
```
$ build/sbt -Phive -Phive-thriftserver "test:testOnly *CachedTableSuite"
$ build/sbt -Phive -Phive-thriftserver "test:testOnly *CatalogedDDLSuite"
```
----------------------------------------------------------------
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 #31308: [SPARK-34215][SQL] Keep tables cached after truncation
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31308:
URL: https://github.com/apache/spark/pull/31308#issuecomment-766416753
**[Test build #134413 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/134413/testReport)** for PR 31308 at commit [`25b8583`](https://github.com/apache/spark/commit/25b8583d65246372f8b6ac35071ead118b1c97f2).
----------------------------------------------------------------
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] sunchao commented on a change in pull request #31308: [SPARK-34215][SQL] Keep tables cached after truncation
Posted by GitBox <gi...@apache.org>.
sunchao commented on a change in pull request #31308:
URL: https://github.com/apache/spark/pull/31308#discussion_r563534499
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/execution/command/tables.scala
##########
@@ -561,16 +561,9 @@ case class TruncateTableCommand(
}
}
}
- // After deleting the data, invalidate the table to make sure we don't keep around a stale
- // file relation in the metastore cache.
- spark.sessionState.refreshTable(tableName.unquotedString)
- // Also try to drop the contents of the table from the columnar cache
- try {
- spark.sharedState.cacheManager.uncacheQuery(spark.table(table.identifier), cascade = true)
- } catch {
- case NonFatal(e) =>
- log.warn(s"Exception when attempting to uncache table $tableIdentWithDB", e)
- }
+ // After deleting the data, refresh the table to make sure we don't keep around a stale
+ // file relation in the metastore cache and cached table data in the cache manager.
+ spark.catalog.refreshTable(tableIdentWithDB)
Review comment:
Personally I'd keep just the try-catch logic here because I think the above do happens and we shouldn't skip updating stats in the case. But I don't really have strong opinion on this.
----------------------------------------------------------------
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 #31308: [SPARK-34215][SQL] Keep tables cached after truncation
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #31308:
URL: https://github.com/apache/spark/pull/31308#discussion_r563551173
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/execution/command/tables.scala
##########
@@ -561,16 +561,9 @@ case class TruncateTableCommand(
}
}
}
- // After deleting the data, invalidate the table to make sure we don't keep around a stale
- // file relation in the metastore cache.
- spark.sessionState.refreshTable(tableName.unquotedString)
- // Also try to drop the contents of the table from the columnar cache
- try {
- spark.sharedState.cacheManager.uncacheQuery(spark.table(table.identifier), cascade = true)
- } catch {
- case NonFatal(e) =>
- log.warn(s"Exception when attempting to uncache table $tableIdentWithDB", e)
- }
+ // After deleting the data, refresh the table to make sure we don't keep around a stale
+ // file relation in the metastore cache and cached table data in the cache manager.
+ spark.catalog.refreshTable(tableIdentWithDB)
Review comment:
can we fix the inconsistency first? i.e. reach an agreement about whether we should add `try-cache` or not for all other commands.
----------------------------------------------------------------
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] MaxGekk commented on a change in pull request #31308: [SPARK-34215][SQL] Keep tables cached after truncation
Posted by GitBox <gi...@apache.org>.
MaxGekk commented on a change in pull request #31308:
URL: https://github.com/apache/spark/pull/31308#discussion_r564409484
##########
File path: sql/hive/src/test/scala/org/apache/spark/sql/hive/CachedTableSuite.scala
##########
@@ -501,4 +501,17 @@ class CachedTableSuite extends QueryTest with SQLTestUtils with TestHiveSingleto
}
}
}
+
+ test("SPARK-34215: keep table cached after truncation") {
Review comment:
Do you mean copy-paste the test there?
----------------------------------------------------------------
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] MaxGekk commented on a change in pull request #31308: [SPARK-34215][SQL] Keep tables cached after truncation
Posted by GitBox <gi...@apache.org>.
MaxGekk commented on a change in pull request #31308:
URL: https://github.com/apache/spark/pull/31308#discussion_r563347923
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/execution/command/tables.scala
##########
@@ -561,16 +561,9 @@ case class TruncateTableCommand(
}
}
}
- // After deleting the data, invalidate the table to make sure we don't keep around a stale
- // file relation in the metastore cache.
- spark.sessionState.refreshTable(tableName.unquotedString)
- // Also try to drop the contents of the table from the columnar cache
- try {
- spark.sharedState.cacheManager.uncacheQuery(spark.table(table.identifier), cascade = true)
- } catch {
- case NonFatal(e) =>
- log.warn(s"Exception when attempting to uncache table $tableIdentWithDB", e)
- }
+ // After deleting the data, refresh the table to make sure we don't keep around a stale
+ // file relation in the metastore cache and cached table data in the cache manager.
+ spark.catalog.refreshTable(tableIdentWithDB)
Review comment:
1. We don't catch any exceptions in other commands
2. What kind of exceptions should we hide from users 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] SparkQA removed a comment on pull request #31308: [SPARK-34215][SQL] Keep tables cached after truncation
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #31308:
URL: https://github.com/apache/spark/pull/31308#issuecomment-766416753
**[Test build #134413 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/134413/testReport)** for PR 31308 at commit [`25b8583`](https://github.com/apache/spark/commit/25b8583d65246372f8b6ac35071ead118b1c97f2).
----------------------------------------------------------------
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] MaxGekk commented on a change in pull request #31308: [SPARK-34215][SQL] Keep tables cached after truncation
Posted by GitBox <gi...@apache.org>.
MaxGekk commented on a change in pull request #31308:
URL: https://github.com/apache/spark/pull/31308#discussion_r563347923
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/execution/command/tables.scala
##########
@@ -561,16 +561,9 @@ case class TruncateTableCommand(
}
}
}
- // After deleting the data, invalidate the table to make sure we don't keep around a stale
- // file relation in the metastore cache.
- spark.sessionState.refreshTable(tableName.unquotedString)
- // Also try to drop the contents of the table from the columnar cache
- try {
- spark.sharedState.cacheManager.uncacheQuery(spark.table(table.identifier), cascade = true)
- } catch {
- case NonFatal(e) =>
- log.warn(s"Exception when attempting to uncache table $tableIdentWithDB", e)
- }
+ // After deleting the data, refresh the table to make sure we don't keep around a stale
+ // file relation in the metastore cache and cached table data in the cache manager.
+ spark.catalog.refreshTable(tableIdentWithDB)
Review comment:
1. We don't catch any exceptions in other commands
2. What kind of exceptions should we hide (catch) from users 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] SparkQA commented on pull request #31308: [SPARK-34215][SQL] Keep tables cached after truncation
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31308:
URL: https://github.com/apache/spark/pull/31308#issuecomment-766416753
----------------------------------------------------------------
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 #31308: [SPARK-34215][SQL] Keep tables cached after truncation
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #31308:
URL: https://github.com/apache/spark/pull/31308#issuecomment-766433956
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/38999/
----------------------------------------------------------------
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] sunchao commented on a change in pull request #31308: [SPARK-34215][SQL] Keep tables cached after truncation
Posted by GitBox <gi...@apache.org>.
sunchao commented on a change in pull request #31308:
URL: https://github.com/apache/spark/pull/31308#discussion_r564400567
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/execution/command/tables.scala
##########
@@ -561,16 +561,9 @@ case class TruncateTableCommand(
}
}
}
- // After deleting the data, invalidate the table to make sure we don't keep around a stale
- // file relation in the metastore cache.
- spark.sessionState.refreshTable(tableName.unquotedString)
- // Also try to drop the contents of the table from the columnar cache
- try {
- spark.sharedState.cacheManager.uncacheQuery(spark.table(table.identifier), cascade = true)
- } catch {
- case NonFatal(e) =>
- log.warn(s"Exception when attempting to uncache table $tableIdentWithDB", e)
- }
+ // After deleting the data, refresh the table to make sure we don't keep around a stale
+ // file relation in the metastore cache and cached table data in the cache manager.
+ spark.catalog.refreshTable(tableIdentWithDB)
Review comment:
So I wasn't able to reproduce it with the above example, sorry for the false alarm. Turned out the analysis exception will be thrown later when the cache is actually queried (rather than in `recacheByPlan` itself). Therefore, I think it should be fine in this case.
I do agree we should keep it consistent (whether try-catch or not). IMO it can be done separately tho.
----------------------------------------------------------------
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] MaxGekk commented on a change in pull request #31308: [SPARK-34215][SQL] Keep tables cached after truncation
Posted by GitBox <gi...@apache.org>.
MaxGekk commented on a change in pull request #31308:
URL: https://github.com/apache/spark/pull/31308#discussion_r564410192
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/execution/command/tables.scala
##########
@@ -561,16 +561,9 @@ case class TruncateTableCommand(
}
}
}
- // After deleting the data, invalidate the table to make sure we don't keep around a stale
- // file relation in the metastore cache.
- spark.sessionState.refreshTable(tableName.unquotedString)
Review comment:
Let me document them separately from this 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] SparkQA commented on pull request #31308: [SPARK-34215][SQL] Keep tables cached after truncation
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31308:
URL: https://github.com/apache/spark/pull/31308#issuecomment-766422567
Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/38999/
----------------------------------------------------------------
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] MaxGekk commented on a change in pull request #31308: [SPARK-34215][SQL] Keep tables cached after truncation
Posted by GitBox <gi...@apache.org>.
MaxGekk commented on a change in pull request #31308:
URL: https://github.com/apache/spark/pull/31308#discussion_r563565508
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/execution/command/tables.scala
##########
@@ -561,16 +561,9 @@ case class TruncateTableCommand(
}
}
}
- // After deleting the data, invalidate the table to make sure we don't keep around a stale
- // file relation in the metastore cache.
- spark.sessionState.refreshTable(tableName.unquotedString)
- // Also try to drop the contents of the table from the columnar cache
- try {
- spark.sharedState.cacheManager.uncacheQuery(spark.table(table.identifier), cascade = true)
- } catch {
- case NonFatal(e) =>
- log.warn(s"Exception when attempting to uncache table $tableIdentWithDB", e)
- }
+ // After deleting the data, refresh the table to make sure we don't keep around a stale
+ // file relation in the metastore cache and cached table data in the cache manager.
+ spark.catalog.refreshTable(tableIdentWithDB)
Review comment:
> ... someone could drop a table or permanent view in a different session, or drop a Hive table through beeline or HMS API.
If somebody dropped the table in parallel, updating statistics wouldn't matter any more. We should show the error to user as soon as possible.
> can we fix the inconsistency first?
@sunchao Can you write a test which reproduces the issue?
----------------------------------------------------------------
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 #31308: [SPARK-34215][SQL] Keep tables cached after truncation
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #31308:
URL: https://github.com/apache/spark/pull/31308#issuecomment-766433956
----------------------------------------------------------------
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] MaxGekk commented on pull request #31308: [SPARK-34215][SQL] Keep tables cached after truncation
Posted by GitBox <gi...@apache.org>.
MaxGekk commented on pull request #31308:
URL: https://github.com/apache/spark/pull/31308#issuecomment-766420525
----------------------------------------------------------------
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 #31308: [SPARK-34215][SQL] Keep tables cached after truncation
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #31308:
URL: https://github.com/apache/spark/pull/31308#issuecomment-766458130
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/134413/
----------------------------------------------------------------
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 #31308: [SPARK-34215][SQL] Keep tables cached after truncation
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #31308:
URL: https://github.com/apache/spark/pull/31308#discussion_r563497900
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/execution/command/tables.scala
##########
@@ -561,16 +561,9 @@ case class TruncateTableCommand(
}
}
}
- // After deleting the data, invalidate the table to make sure we don't keep around a stale
- // file relation in the metastore cache.
- spark.sessionState.refreshTable(tableName.unquotedString)
Review comment:
can we update the doc of these 2 `refreshTable` and make it clear when we should use which?
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/execution/command/tables.scala
##########
@@ -561,16 +561,9 @@ case class TruncateTableCommand(
}
}
}
- // After deleting the data, invalidate the table to make sure we don't keep around a stale
- // file relation in the metastore cache.
- spark.sessionState.refreshTable(tableName.unquotedString)
- // Also try to drop the contents of the table from the columnar cache
- try {
- spark.sharedState.cacheManager.uncacheQuery(spark.table(table.identifier), cascade = true)
- } catch {
- case NonFatal(e) =>
- log.warn(s"Exception when attempting to uncache table $tableIdentWithDB", e)
- }
+ // After deleting the data, refresh the table to make sure we don't keep around a stale
+ // file relation in the metastore cache and cached table data in the cache manager.
+ spark.catalog.refreshTable(tableIdentWithDB)
Review comment:
can we fix the inconsistency first? i.e. reach an agreement about whether we should add `try-cache` or not for all other commands.
----------------------------------------------------------------
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] MaxGekk commented on a change in pull request #31308: [SPARK-34215][SQL] Keep tables cached after truncation
Posted by GitBox <gi...@apache.org>.
MaxGekk commented on a change in pull request #31308:
URL: https://github.com/apache/spark/pull/31308#discussion_r564411390
##########
File path: sql/hive/src/test/scala/org/apache/spark/sql/hive/CachedTableSuite.scala
##########
@@ -501,4 +501,17 @@ class CachedTableSuite extends QueryTest with SQLTestUtils with TestHiveSingleto
}
}
}
+
+ test("SPARK-34215: keep table cached after truncation") {
Review comment:
BTW, I am going to write unified tests for `TRUNCATE TABLE` command, and move the test there to run it for both v1 In-Memory and Hive external catalogs.
----------------------------------------------------------------
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 #31308: [SPARK-34215][SQL] Keep tables cached after truncation
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on pull request #31308:
URL: https://github.com/apache/spark/pull/31308#issuecomment-767626399
thanks, merging to master!
----------------------------------------------------------------
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] sunchao commented on a change in pull request #31308: [SPARK-34215][SQL] Keep tables cached after truncation
Posted by GitBox <gi...@apache.org>.
sunchao commented on a change in pull request #31308:
URL: https://github.com/apache/spark/pull/31308#discussion_r564121866
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/execution/command/tables.scala
##########
@@ -561,16 +561,9 @@ case class TruncateTableCommand(
}
}
}
- // After deleting the data, invalidate the table to make sure we don't keep around a stale
- // file relation in the metastore cache.
- spark.sessionState.refreshTable(tableName.unquotedString)
- // Also try to drop the contents of the table from the columnar cache
- try {
- spark.sharedState.cacheManager.uncacheQuery(spark.table(table.identifier), cascade = true)
- } catch {
- case NonFatal(e) =>
- log.warn(s"Exception when attempting to uncache table $tableIdentWithDB", e)
- }
+ // After deleting the data, refresh the table to make sure we don't keep around a stale
+ // file relation in the metastore cache and cached table data in the cache manager.
+ spark.catalog.refreshTable(tableIdentWithDB)
Review comment:
We might utilize `HiveThriftServer2Suites` for this - I can check when I got time.
----------------------------------------------------------------
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] MaxGekk commented on pull request #31308: [SPARK-34215][SQL] Keep tables cached after truncation
Posted by GitBox <gi...@apache.org>.
MaxGekk commented on pull request #31308:
URL: https://github.com/apache/spark/pull/31308#issuecomment-766575636
> BTW, do we have a documentation for caching behaviour?
@HyukjinKwon Probably, not. At least I don't know where there are such docs.
> it would be great to have one.
Yes, it would be great.
----------------------------------------------------------------
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] MaxGekk commented on a change in pull request #31308: [SPARK-34215][SQL] Keep tables cached after truncation
Posted by GitBox <gi...@apache.org>.
MaxGekk commented on a change in pull request #31308:
URL: https://github.com/apache/spark/pull/31308#discussion_r563350111
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/execution/command/tables.scala
##########
@@ -561,16 +561,9 @@ case class TruncateTableCommand(
}
}
}
- // After deleting the data, invalidate the table to make sure we don't keep around a stale
- // file relation in the metastore cache.
- spark.sessionState.refreshTable(tableName.unquotedString)
- // Also try to drop the contents of the table from the columnar cache
- try {
- spark.sharedState.cacheManager.uncacheQuery(spark.table(table.identifier), cascade = true)
- } catch {
- case NonFatal(e) =>
- log.warn(s"Exception when attempting to uncache table $tableIdentWithDB", e)
- }
+ // After deleting the data, refresh the table to make sure we don't keep around a stale
+ // file relation in the metastore cache and cached table data in the cache manager.
+ spark.catalog.refreshTable(tableIdentWithDB)
Review comment:
Not clear which exceptions are caught here. `uncacheQuery()` doesn't throw anything, `spark.table(table.identifier).logicalPlan` could fail but in that case it is not clear how we reached this point.
----------------------------------------------------------------
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] sunchao commented on a change in pull request #31308: [SPARK-34215][SQL] Keep tables cached after truncation
Posted by GitBox <gi...@apache.org>.
sunchao commented on a change in pull request #31308:
URL: https://github.com/apache/spark/pull/31308#discussion_r564416206
##########
File path: sql/hive/src/test/scala/org/apache/spark/sql/hive/CachedTableSuite.scala
##########
@@ -501,4 +501,17 @@ class CachedTableSuite extends QueryTest with SQLTestUtils with TestHiveSingleto
}
}
}
+
+ test("SPARK-34215: keep table cached after truncation") {
Review comment:
OK sounds good. We can keep it for now then.
----------------------------------------------------------------
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 #31308: [SPARK-34215][SQL] Keep tables cached after truncation
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #31308:
URL: https://github.com/apache/spark/pull/31308#issuecomment-766433956
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/38999/
----------------------------------------------------------------
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] MaxGekk commented on a change in pull request #31308: [SPARK-34215][SQL] Keep tables cached after truncation
Posted by GitBox <gi...@apache.org>.
MaxGekk commented on a change in pull request #31308:
URL: https://github.com/apache/spark/pull/31308#discussion_r563347923
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/execution/command/tables.scala
##########
@@ -561,16 +561,9 @@ case class TruncateTableCommand(
}
}
}
- // After deleting the data, invalidate the table to make sure we don't keep around a stale
- // file relation in the metastore cache.
- spark.sessionState.refreshTable(tableName.unquotedString)
- // Also try to drop the contents of the table from the columnar cache
- try {
- spark.sharedState.cacheManager.uncacheQuery(spark.table(table.identifier), cascade = true)
- } catch {
- case NonFatal(e) =>
- log.warn(s"Exception when attempting to uncache table $tableIdentWithDB", e)
- }
+ // After deleting the data, refresh the table to make sure we don't keep around a stale
+ // file relation in the metastore cache and cached table data in the cache manager.
+ spark.catalog.refreshTable(tableIdentWithDB)
Review comment:
1. We don't catch any exception in other commands
2. What kind of exceptions should we hide from users here?
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/execution/command/tables.scala
##########
@@ -561,16 +561,9 @@ case class TruncateTableCommand(
}
}
}
- // After deleting the data, invalidate the table to make sure we don't keep around a stale
- // file relation in the metastore cache.
- spark.sessionState.refreshTable(tableName.unquotedString)
- // Also try to drop the contents of the table from the columnar cache
- try {
- spark.sharedState.cacheManager.uncacheQuery(spark.table(table.identifier), cascade = true)
- } catch {
- case NonFatal(e) =>
- log.warn(s"Exception when attempting to uncache table $tableIdentWithDB", e)
- }
+ // After deleting the data, refresh the table to make sure we don't keep around a stale
+ // file relation in the metastore cache and cached table data in the cache manager.
+ spark.catalog.refreshTable(tableIdentWithDB)
Review comment:
1. We don't catch any exceptions in other commands
2. What kind of exceptions should we hide from users here?
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/execution/command/tables.scala
##########
@@ -561,16 +561,9 @@ case class TruncateTableCommand(
}
}
}
- // After deleting the data, invalidate the table to make sure we don't keep around a stale
- // file relation in the metastore cache.
- spark.sessionState.refreshTable(tableName.unquotedString)
- // Also try to drop the contents of the table from the columnar cache
- try {
- spark.sharedState.cacheManager.uncacheQuery(spark.table(table.identifier), cascade = true)
- } catch {
- case NonFatal(e) =>
- log.warn(s"Exception when attempting to uncache table $tableIdentWithDB", e)
- }
+ // After deleting the data, refresh the table to make sure we don't keep around a stale
+ // file relation in the metastore cache and cached table data in the cache manager.
+ spark.catalog.refreshTable(tableIdentWithDB)
Review comment:
1. We don't catch any exceptions in other commands
2. What kind of exceptions should we hide (catch) from users here?
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/execution/command/tables.scala
##########
@@ -561,16 +561,9 @@ case class TruncateTableCommand(
}
}
}
- // After deleting the data, invalidate the table to make sure we don't keep around a stale
- // file relation in the metastore cache.
- spark.sessionState.refreshTable(tableName.unquotedString)
- // Also try to drop the contents of the table from the columnar cache
- try {
- spark.sharedState.cacheManager.uncacheQuery(spark.table(table.identifier), cascade = true)
- } catch {
- case NonFatal(e) =>
- log.warn(s"Exception when attempting to uncache table $tableIdentWithDB", e)
- }
+ // After deleting the data, refresh the table to make sure we don't keep around a stale
+ // file relation in the metastore cache and cached table data in the cache manager.
+ spark.catalog.refreshTable(tableIdentWithDB)
Review comment:
Not clear which exceptions are caught here. `uncacheQuery()` doesn't throw anything, `spark.table(table.identifier).logicalPlan` could fail but in that case it is not clear how we reached this point.
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/execution/command/tables.scala
##########
@@ -561,16 +561,9 @@ case class TruncateTableCommand(
}
}
}
- // After deleting the data, invalidate the table to make sure we don't keep around a stale
- // file relation in the metastore cache.
- spark.sessionState.refreshTable(tableName.unquotedString)
- // Also try to drop the contents of the table from the columnar cache
- try {
- spark.sharedState.cacheManager.uncacheQuery(spark.table(table.identifier), cascade = true)
- } catch {
- case NonFatal(e) =>
- log.warn(s"Exception when attempting to uncache table $tableIdentWithDB", e)
- }
+ // After deleting the data, refresh the table to make sure we don't keep around a stale
+ // file relation in the metastore cache and cached table data in the cache manager.
+ spark.catalog.refreshTable(tableIdentWithDB)
Review comment:
The case which you described here is applicable to any other commands like add/drop/rename/recover partitions. I do believe we either should "fix" all commands with tests for the case, or apply the approach w/o catching exceptions here as we do in other commands so far (otherwise the implementation looks inconsistent).
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/execution/command/tables.scala
##########
@@ -561,16 +561,9 @@ case class TruncateTableCommand(
}
}
}
- // After deleting the data, invalidate the table to make sure we don't keep around a stale
- // file relation in the metastore cache.
- spark.sessionState.refreshTable(tableName.unquotedString)
- // Also try to drop the contents of the table from the columnar cache
- try {
- spark.sharedState.cacheManager.uncacheQuery(spark.table(table.identifier), cascade = true)
- } catch {
- case NonFatal(e) =>
- log.warn(s"Exception when attempting to uncache table $tableIdentWithDB", e)
- }
+ // After deleting the data, refresh the table to make sure we don't keep around a stale
+ // file relation in the metastore cache and cached table data in the cache manager.
+ spark.catalog.refreshTable(tableIdentWithDB)
Review comment:
> ... someone could drop a table or permanent view in a different session, or drop a Hive table through beeline or HMS API.
If somebody dropped the table in parallel, updating statistics wouldn't matter any more. We should show the error to user as soon as possible.
> can we fix the inconsistency first?
@sunchao Can you write a test which reproduces the issue?
----------------------------------------------------------------
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 #31308: [SPARK-34215][SQL] Keep tables cached after truncation
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #31308:
URL: https://github.com/apache/spark/pull/31308#issuecomment-766458130
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/134413/
----------------------------------------------------------------
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] MaxGekk commented on a change in pull request #31308: [SPARK-34215][SQL] Keep tables cached after truncation
Posted by GitBox <gi...@apache.org>.
MaxGekk commented on a change in pull request #31308:
URL: https://github.com/apache/spark/pull/31308#discussion_r567250736
##########
File path: sql/hive/src/test/scala/org/apache/spark/sql/hive/CachedTableSuite.scala
##########
@@ -501,4 +501,17 @@ class CachedTableSuite extends QueryTest with SQLTestUtils with TestHiveSingleto
}
}
}
+
+ test("SPARK-34215: keep table cached after truncation") {
Review comment:
@sunchao Here is the PR https://github.com/apache/spark/pull/31387 which unifies the `TRUNCATE TABLE` tests.
----------------------------------------------------------------
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] sunchao commented on a change in pull request #31308: [SPARK-34215][SQL] Keep tables cached after truncation
Posted by GitBox <gi...@apache.org>.
sunchao commented on a change in pull request #31308:
URL: https://github.com/apache/spark/pull/31308#discussion_r563346341
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/execution/command/tables.scala
##########
@@ -561,16 +561,9 @@ case class TruncateTableCommand(
}
}
}
- // After deleting the data, invalidate the table to make sure we don't keep around a stale
- // file relation in the metastore cache.
- spark.sessionState.refreshTable(tableName.unquotedString)
- // Also try to drop the contents of the table from the columnar cache
- try {
- spark.sharedState.cacheManager.uncacheQuery(spark.table(table.identifier), cascade = true)
- } catch {
- case NonFatal(e) =>
- log.warn(s"Exception when attempting to uncache table $tableIdentWithDB", e)
- }
+ // After deleting the data, refresh the table to make sure we don't keep around a stale
+ // file relation in the metastore cache and cached table data in the cache manager.
+ spark.catalog.refreshTable(tableIdentWithDB)
Review comment:
Hmm... Not sure if we should catch any non-fatal exception like the previous one, since otherwise we'd skip updating stats?
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/execution/command/tables.scala
##########
@@ -561,16 +561,9 @@ case class TruncateTableCommand(
}
}
}
- // After deleting the data, invalidate the table to make sure we don't keep around a stale
- // file relation in the metastore cache.
- spark.sessionState.refreshTable(tableName.unquotedString)
- // Also try to drop the contents of the table from the columnar cache
- try {
- spark.sharedState.cacheManager.uncacheQuery(spark.table(table.identifier), cascade = true)
- } catch {
- case NonFatal(e) =>
- log.warn(s"Exception when attempting to uncache table $tableIdentWithDB", e)
- }
+ // After deleting the data, refresh the table to make sure we don't keep around a stale
+ // file relation in the metastore cache and cached table data in the cache manager.
+ spark.catalog.refreshTable(tableIdentWithDB)
Review comment:
I think someone could drop a table or permanent view in a different session, or drop a Hive table through beeline or HMS API. This may cause some cache which depend on them AND this truncated table to become invalid, and potentially analysis exception when recaching them. I haven't got a chance to verify this though.
I feel overall it will be a good practice to recover from unknown errors here and continue. Also `DropTableCommand` does this as well.
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/execution/command/tables.scala
##########
@@ -561,16 +561,9 @@ case class TruncateTableCommand(
}
}
}
- // After deleting the data, invalidate the table to make sure we don't keep around a stale
- // file relation in the metastore cache.
- spark.sessionState.refreshTable(tableName.unquotedString)
- // Also try to drop the contents of the table from the columnar cache
- try {
- spark.sharedState.cacheManager.uncacheQuery(spark.table(table.identifier), cascade = true)
- } catch {
- case NonFatal(e) =>
- log.warn(s"Exception when attempting to uncache table $tableIdentWithDB", e)
- }
+ // After deleting the data, refresh the table to make sure we don't keep around a stale
+ // file relation in the metastore cache and cached table data in the cache manager.
+ spark.catalog.refreshTable(tableIdentWithDB)
Review comment:
I think someone could drop a table or permanent view in a different session, or drop a Hive table through beeline or HMS API. This may cause some cache which depend on them AND this truncated table to become invalid, and potentially analysis exception when recaching them. I haven't got a chance to verify this though.
I feel overall it will be a good practice to recover from unknown errors here and continue. `DropTableCommand` does this as well.
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/execution/command/tables.scala
##########
@@ -561,16 +561,9 @@ case class TruncateTableCommand(
}
}
}
- // After deleting the data, invalidate the table to make sure we don't keep around a stale
- // file relation in the metastore cache.
- spark.sessionState.refreshTable(tableName.unquotedString)
- // Also try to drop the contents of the table from the columnar cache
- try {
- spark.sharedState.cacheManager.uncacheQuery(spark.table(table.identifier), cascade = true)
- } catch {
- case NonFatal(e) =>
- log.warn(s"Exception when attempting to uncache table $tableIdentWithDB", e)
- }
+ // After deleting the data, refresh the table to make sure we don't keep around a stale
+ // file relation in the metastore cache and cached table data in the cache manager.
+ spark.catalog.refreshTable(tableIdentWithDB)
Review comment:
Personally I'd keep just the try-catch logic here because I think the above do happens and we shouldn't skip updating stats in the case. But I don't really have strong opinion on this.
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/execution/command/tables.scala
##########
@@ -561,16 +561,9 @@ case class TruncateTableCommand(
}
}
}
- // After deleting the data, invalidate the table to make sure we don't keep around a stale
- // file relation in the metastore cache.
- spark.sessionState.refreshTable(tableName.unquotedString)
- // Also try to drop the contents of the table from the columnar cache
- try {
- spark.sharedState.cacheManager.uncacheQuery(spark.table(table.identifier), cascade = true)
- } catch {
- case NonFatal(e) =>
- log.warn(s"Exception when attempting to uncache table $tableIdentWithDB", e)
- }
+ // After deleting the data, refresh the table to make sure we don't keep around a stale
+ // file relation in the metastore cache and cached table data in the cache manager.
+ spark.catalog.refreshTable(tableIdentWithDB)
Review comment:
We might utilize `HiveThriftServer2Suites` for this - I can check when I got time.
----------------------------------------------------------------
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 #31308: [SPARK-34215][SQL] Keep tables cached after truncation
Posted by GitBox <gi...@apache.org>.
cloud-fan closed pull request #31308:
URL: https://github.com/apache/spark/pull/31308
----------------------------------------------------------------
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 #31308: [SPARK-34215][SQL] Keep tables cached after truncation
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31308:
URL: https://github.com/apache/spark/pull/31308#issuecomment-766426885
Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/38999/
----------------------------------------------------------------
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] sunchao commented on a change in pull request #31308: [SPARK-34215][SQL] Keep tables cached after truncation
Posted by GitBox <gi...@apache.org>.
sunchao commented on a change in pull request #31308:
URL: https://github.com/apache/spark/pull/31308#discussion_r563448718
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/execution/command/tables.scala
##########
@@ -561,16 +561,9 @@ case class TruncateTableCommand(
}
}
}
- // After deleting the data, invalidate the table to make sure we don't keep around a stale
- // file relation in the metastore cache.
- spark.sessionState.refreshTable(tableName.unquotedString)
- // Also try to drop the contents of the table from the columnar cache
- try {
- spark.sharedState.cacheManager.uncacheQuery(spark.table(table.identifier), cascade = true)
- } catch {
- case NonFatal(e) =>
- log.warn(s"Exception when attempting to uncache table $tableIdentWithDB", e)
- }
+ // After deleting the data, refresh the table to make sure we don't keep around a stale
+ // file relation in the metastore cache and cached table data in the cache manager.
+ spark.catalog.refreshTable(tableIdentWithDB)
Review comment:
I think someone could drop a table or permanent view in a different session, or drop a Hive table through beeline or HMS API. This may cause some cache which depend on them AND this truncated table to become invalid, and potentially analysis exception when recaching them. I haven't got a chance to verify this though.
I feel overall it will be a good practice to recover from unknown errors here and continue. Also `DropTableCommand` does this as well.
----------------------------------------------------------------
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] sunchao commented on a change in pull request #31308: [SPARK-34215][SQL] Keep tables cached after truncation
Posted by GitBox <gi...@apache.org>.
sunchao commented on a change in pull request #31308:
URL: https://github.com/apache/spark/pull/31308#discussion_r563346341
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/execution/command/tables.scala
##########
@@ -561,16 +561,9 @@ case class TruncateTableCommand(
}
}
}
- // After deleting the data, invalidate the table to make sure we don't keep around a stale
- // file relation in the metastore cache.
- spark.sessionState.refreshTable(tableName.unquotedString)
- // Also try to drop the contents of the table from the columnar cache
- try {
- spark.sharedState.cacheManager.uncacheQuery(spark.table(table.identifier), cascade = true)
- } catch {
- case NonFatal(e) =>
- log.warn(s"Exception when attempting to uncache table $tableIdentWithDB", e)
- }
+ // After deleting the data, refresh the table to make sure we don't keep around a stale
+ // file relation in the metastore cache and cached table data in the cache manager.
+ spark.catalog.refreshTable(tableIdentWithDB)
Review comment:
Hmm... Not sure if we should catch any non-fatal exception like the previous one, since otherwise we'd skip updating stats?
----------------------------------------------------------------
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] sunchao commented on a change in pull request #31308: [SPARK-34215][SQL] Keep tables cached after truncation
Posted by GitBox <gi...@apache.org>.
sunchao commented on a change in pull request #31308:
URL: https://github.com/apache/spark/pull/31308#discussion_r563448718
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/execution/command/tables.scala
##########
@@ -561,16 +561,9 @@ case class TruncateTableCommand(
}
}
}
- // After deleting the data, invalidate the table to make sure we don't keep around a stale
- // file relation in the metastore cache.
- spark.sessionState.refreshTable(tableName.unquotedString)
- // Also try to drop the contents of the table from the columnar cache
- try {
- spark.sharedState.cacheManager.uncacheQuery(spark.table(table.identifier), cascade = true)
- } catch {
- case NonFatal(e) =>
- log.warn(s"Exception when attempting to uncache table $tableIdentWithDB", e)
- }
+ // After deleting the data, refresh the table to make sure we don't keep around a stale
+ // file relation in the metastore cache and cached table data in the cache manager.
+ spark.catalog.refreshTable(tableIdentWithDB)
Review comment:
I think someone could drop a table or permanent view in a different session, or drop a Hive table through beeline or HMS API. This may cause some cache which depend on them AND this truncated table to become invalid, and potentially analysis exception when recaching them. I haven't got a chance to verify this though.
I feel overall it will be a good practice to recover from unknown errors here and continue. `DropTableCommand` does this as well.
----------------------------------------------------------------
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] MaxGekk commented on a change in pull request #31308: [SPARK-34215][SQL] Keep tables cached after truncation
Posted by GitBox <gi...@apache.org>.
MaxGekk commented on a change in pull request #31308:
URL: https://github.com/apache/spark/pull/31308#discussion_r563347923
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/execution/command/tables.scala
##########
@@ -561,16 +561,9 @@ case class TruncateTableCommand(
}
}
}
- // After deleting the data, invalidate the table to make sure we don't keep around a stale
- // file relation in the metastore cache.
- spark.sessionState.refreshTable(tableName.unquotedString)
- // Also try to drop the contents of the table from the columnar cache
- try {
- spark.sharedState.cacheManager.uncacheQuery(spark.table(table.identifier), cascade = true)
- } catch {
- case NonFatal(e) =>
- log.warn(s"Exception when attempting to uncache table $tableIdentWithDB", e)
- }
+ // After deleting the data, refresh the table to make sure we don't keep around a stale
+ // file relation in the metastore cache and cached table data in the cache manager.
+ spark.catalog.refreshTable(tableIdentWithDB)
Review comment:
1. We don't catch any exception in other commands
2. What kind of exceptions should we hide from users 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] cloud-fan commented on a change in pull request #31308: [SPARK-34215][SQL] Keep tables cached after truncation
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #31308:
URL: https://github.com/apache/spark/pull/31308#discussion_r563497900
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/execution/command/tables.scala
##########
@@ -561,16 +561,9 @@ case class TruncateTableCommand(
}
}
}
- // After deleting the data, invalidate the table to make sure we don't keep around a stale
- // file relation in the metastore cache.
- spark.sessionState.refreshTable(tableName.unquotedString)
Review comment:
can we update the doc of these 2 `refreshTable` and make it clear when we should use which?
----------------------------------------------------------------
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] MaxGekk commented on pull request #31308: [SPARK-34215][SQL] Keep tables cached after truncation
Posted by GitBox <gi...@apache.org>.
MaxGekk commented on pull request #31308:
URL: https://github.com/apache/spark/pull/31308#issuecomment-766420525
@HyukjinKwon @dongjoon-hyun @sunchao @cloud-fan Could you review this PR, please.
----------------------------------------------------------------
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] sunchao commented on a change in pull request #31308: [SPARK-34215][SQL] Keep tables cached after truncation
Posted by GitBox <gi...@apache.org>.
sunchao commented on a change in pull request #31308:
URL: https://github.com/apache/spark/pull/31308#discussion_r564402221
##########
File path: sql/hive/src/test/scala/org/apache/spark/sql/hive/CachedTableSuite.scala
##########
@@ -501,4 +501,17 @@ class CachedTableSuite extends QueryTest with SQLTestUtils with TestHiveSingleto
}
}
}
+
+ test("SPARK-34215: keep table cached after truncation") {
Review comment:
can we move this to `org.apache.spark.sql.CachedTableSuite`? the truncate table command is not limited to Hive.
----------------------------------------------------------------
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 #31308: [SPARK-34215][SQL] Keep tables cached after truncation
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31308:
URL: https://github.com/apache/spark/pull/31308#issuecomment-766457780
**[Test build #134413 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/134413/testReport)** for PR 31308 at commit [`25b8583`](https://github.com/apache/spark/commit/25b8583d65246372f8b6ac35071ead118b1c97f2).
* This patch passes all 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] HyukjinKwon commented on pull request #31308: [SPARK-34215][SQL] Keep tables cached after truncation
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on pull request #31308:
URL: https://github.com/apache/spark/pull/31308#issuecomment-766485778
BTW, do we have a documentation for caching behaviour? it would be great to have one.
----------------------------------------------------------------
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 #31308: [SPARK-34215][SQL] Keep tables cached after truncation
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #31308:
URL: https://github.com/apache/spark/pull/31308#issuecomment-766416753
**[Test build #134413 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/134413/testReport)** for PR 31308 at commit [`25b8583`](https://github.com/apache/spark/commit/25b8583d65246372f8b6ac35071ead118b1c97f2).
----------------------------------------------------------------
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] HyukjinKwon commented on pull request #31308: [SPARK-34215][SQL] Keep tables cached after truncation
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on pull request #31308:
URL: https://github.com/apache/spark/pull/31308#issuecomment-766485778
BTW, do we have a documentation for caching behaviour? it would be great to have one.
----------------------------------------------------------------
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 #31308: [SPARK-34215][SQL] Keep tables cached after truncation
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #31308:
URL: https://github.com/apache/spark/pull/31308#issuecomment-766433956
----------------------------------------------------------------
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] MaxGekk commented on a change in pull request #31308: [SPARK-34215][SQL] Keep tables cached after truncation
Posted by GitBox <gi...@apache.org>.
MaxGekk commented on a change in pull request #31308:
URL: https://github.com/apache/spark/pull/31308#discussion_r563484494
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/execution/command/tables.scala
##########
@@ -561,16 +561,9 @@ case class TruncateTableCommand(
}
}
}
- // After deleting the data, invalidate the table to make sure we don't keep around a stale
- // file relation in the metastore cache.
- spark.sessionState.refreshTable(tableName.unquotedString)
- // Also try to drop the contents of the table from the columnar cache
- try {
- spark.sharedState.cacheManager.uncacheQuery(spark.table(table.identifier), cascade = true)
- } catch {
- case NonFatal(e) =>
- log.warn(s"Exception when attempting to uncache table $tableIdentWithDB", e)
- }
+ // After deleting the data, refresh the table to make sure we don't keep around a stale
+ // file relation in the metastore cache and cached table data in the cache manager.
+ spark.catalog.refreshTable(tableIdentWithDB)
Review comment:
The case which you described here is applicable to any other commands like add/drop/rename/recover partitions. I do believe we either should "fix" all commands with tests for the case, or apply the approach w/o catching exceptions here as we do in other commands so far (otherwise the implementation looks inconsistent).
----------------------------------------------------------------
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