You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "srielau (via GitHub)" <gi...@apache.org> on 2026/03/13 01:55:25 UTC
[PR] [SPARK-55964] Cache coherence: clear function registry on DROP DATABASE [spark]
srielau opened a new pull request, #54781:
URL: https://github.com/apache/spark/pull/54781
- Add dropFunctionsInDatabase(db) to FunctionRegistryBase and implementations
- SessionCatalog.dropDatabase clears scalar and table function registry cache for the dropped database so resolution does not see stale entries
- Add SessionCatalogSuite tests for cache coherence
<!--
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'.
8. If you want to add or modify an error type or message, please read the guideline first in
'common/utils/src/main/resources/error/README.md'.
-->
### 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.
-->
We now delete functions from a a schema dropped within the session from the sesssion function registry
### 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.
-->
Without this change the session coudl keep resolving fucntions from a schema it had dropped.
### Does this PR introduce _any_ user-facing change?
<!--
Note that it means *any* user-facing change including all aspects such as new features, bug fixes, or other behavior changes. Documentation-only updates are not considered user-facing changes.
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'.
-->
It fixes a bug that can be observed.
### 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.
If benchmark tests were added, please run the benchmarks in GitHub Actions for the consistent environment, and the instructions could accord to: https://spark.apache.org/developer-tools.html#github-workflow-benchmarks.
-->
Added new testcases
### Was this patch authored or co-authored using generative AI tooling?
<!--
If generative AI tooling has been used in the process of authoring this patch, please include the
phrase: 'Generated-by: ' followed by the name of the tool and its version.
If no, write 'No'.
Please refer to the [ASF Generative Tooling Guidance](https://www.apache.org/legal/generative-tooling.html) for details.
-->
Claude Opus4.6
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
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
Re: [PR] [SPARK-55964] Cache coherence: clear function registry on DROP DATABASE [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #54781:
URL: https://github.com/apache/spark/pull/54781#discussion_r2942460715
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionRegistry.scala:
##########
@@ -263,6 +271,16 @@ trait SimpleFunctionRegistryBase[T] extends FunctionRegistryBase[T] with Logging
functionBuilders.remove(normalizeFuncName(name)).isDefined
}
+ override def dropFunctionsInDatabase(namespace: FunctionIdentifier): Unit = synchronized {
+ val toRemove = listFunction().filter { f =>
+ f.database.exists(d => namespace.database.exists(_.equalsIgnoreCase(d))) &&
+ namespace.catalog.isDefined && (
+ f.catalog == namespace.catalog ||
+ (f.catalog.isEmpty && namespace.catalog.contains(CatalogManager.SESSION_CATALOG_NAME)))
+ }
Review Comment:
```suggestion
val toRemove = listFunction().filter { f =>
f.copy(name = "") == namespace
}
```
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
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
Re: [PR] [SPARK-55964] Cache coherence: clear function registry on DROP DATABASE [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #54781:
URL: https://github.com/apache/spark/pull/54781#discussion_r2928783604
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionRegistry.scala:
##########
@@ -108,6 +108,12 @@ trait FunctionRegistryBase[T] {
/** Drop a function and return whether the function existed. */
def dropFunction(name: FunctionIdentifier): Boolean
+ /**
+ * Remove all cached function entries in the given database.
+ * Keeps the cache coherent when a database is dropped.
+ */
+ def dropFunctionsInDatabase(db: String): Unit
Review Comment:
`dropFunctionsInDatabase(db: String)` filters only by `database`, ignoring the `catalog` field of `FunctionIdentifier`. The registry stores entries with different catalog values (builtins: `None`, session: `Some("system")`, persistent: per-namespace), so matching only on database could clear functions from unrelated catalogs that share the same database name.
Consider accepting a `FunctionIdentifier` with empty `funcName` as a namespace filter instead — this pattern already exists in `SessionCatalog` with `SESSION_NAMESPACE_TEMPLATE` and `BUILTIN_NAMESPACE_TEMPLATE` (SessionCatalog.scala:142-150), which use `FunctionIdentifier(funcName = "", database = ..., catalog = ...)` as namespace templates.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/SessionCatalog.scala:
##########
@@ -491,6 +491,11 @@ class SessionCatalog(
invalidateCachedTable(QualifiedTableName(SESSION_CATALOG_NAME, dbName, t.table))
}
}
+ if (databaseExists(dbName)) {
Review Comment:
The function registry clearing is not guarded by `cascade`, unlike the table cache invalidation at line 489. When `cascade=false`, either the DB is empty (clearing is a no-op) or `externalCatalog.dropDatabase` will throw for a non-empty DB (`InMemoryCatalog`:140-144) — but the registry has already been cleared before that throw. Consider moving the clearing inside the existing `cascade` block, which also consolidates the redundant `databaseExists` call.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/SessionCatalog.scala:
##########
@@ -491,6 +491,11 @@ class SessionCatalog(
invalidateCachedTable(QualifiedTableName(SESSION_CATALOG_NAME, dbName, t.table))
}
}
+ if (databaseExists(dbName)) {
+ // Clear cached functions in this database so cache stays coherent after drop
Review Comment:
Comment says "after drop" but clearing happens before `externalCatalog.dropDatabase` at line 499.
```suggestion
// Clear cached functions in this database so the cache stays coherent on drop
```
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/catalog/SessionCatalogSuite.scala:
##########
@@ -297,6 +297,69 @@ abstract class SessionCatalogSuite extends AnalysisTest with Eventually {
}
}
+ test("drop database clears function registry cache (cache coherence)") {
Review Comment:
Consider adding a negative test that registers a function in a *different* database and verifies it survives the drop. This would catch an overly aggressive implementation that clears too many entries.
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
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
Re: [PR] [SPARK-55964] Cache coherence: clear function registry on DROP DATABASE [spark]
Posted by "srielau (via GitHub)" <gi...@apache.org>.
srielau commented on code in PR #54781:
URL: https://github.com/apache/spark/pull/54781#discussion_r2935432224
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionRegistry.scala:
##########
@@ -108,6 +108,12 @@ trait FunctionRegistryBase[T] {
/** Drop a function and return whether the function existed. */
def dropFunction(name: FunctionIdentifier): Boolean
+ /**
+ * Remove all cached function entries in the given database.
+ * Keeps the cache coherent when a database is dropped.
+ */
+ def dropFunctionsInDatabase(db: String): Unit
Review Comment:
FunctionRegistry.scala — Changed the trait method signature from dropFunctionsInDatabase(db: String) to dropFunctionsInDatabase(namespace: FunctionIdentifier), and updated all three implementations:
FunctionRegistryBase: Updated trait definition and doc comment
SimpleFunctionRegistryBase: Filters by both database (case-insensitive) and catalog (when specified in the namespace), consistent with the SESSION_NAMESPACE_TEMPLATE / BUILTIN_NAMESPACE_TEMPLATE pattern already used in SessionCatalog
EmptyFunctionRegistryBase: Updated signature (still a no-op)
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
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
Re: [PR] [SPARK-55964] Cache coherence: clear function registry on DROP DATABASE [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #54781:
URL: https://github.com/apache/spark/pull/54781#discussion_r2945454554
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/SessionCatalog.scala:
##########
@@ -490,6 +490,11 @@ class SessionCatalog(
listTables(dbName).foreach { t =>
invalidateCachedTable(QualifiedTableName(SESSION_CATALOG_NAME, dbName, t.table))
}
+ // Clear cached functions in this database so the cache stays coherent on drop.
+ // normalizeFuncName stores entries with catalog=None, so the filter must match that.
Review Comment:
This is a bug I've fixed in https://github.com/srielau/spark/pull/3 , we can update the code here later.
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
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
Re: [PR] [SPARK-55964] Cache coherence: clear function registry on DROP DATABASE [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan closed pull request #54781: [SPARK-55964] Cache coherence: clear function registry on DROP DATABASE
URL: https://github.com/apache/spark/pull/54781
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
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
Re: [PR] [SPARK-55964] Cache coherence: clear function registry on DROP DATABASE [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #54781:
URL: https://github.com/apache/spark/pull/54781#issuecomment-4072749384
The test failure seems to be real
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
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
Re: [PR] [SPARK-55964] Cache coherence: clear function registry on DROP DATABASE [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #54781:
URL: https://github.com/apache/spark/pull/54781#discussion_r2939541505
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionRegistry.scala:
##########
@@ -263,6 +270,14 @@ trait SimpleFunctionRegistryBase[T] extends FunctionRegistryBase[T] with Logging
functionBuilders.remove(normalizeFuncName(name)).isDefined
}
+ override def dropFunctionsInDatabase(namespace: FunctionIdentifier): Unit = synchronized {
+ val toRemove = listFunction().filter { f =>
+ f.database.exists(d => namespace.database.exists(_.equalsIgnoreCase(d))) &&
+ (namespace.catalog.isEmpty || f.catalog == namespace.catalog)
Review Comment:
I'm not sure we need to make this API to support empty catalog. This is to drop cached functions when dropping a database, which should always belong to a catalog.
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
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
Re: [PR] [SPARK-55964] Cache coherence: clear function registry on DROP DATABASE [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #54781:
URL: https://github.com/apache/spark/pull/54781#discussion_r2939537349
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/SessionCatalog.scala:
##########
@@ -490,6 +490,10 @@ class SessionCatalog(
listTables(dbName).foreach { t =>
invalidateCachedTable(QualifiedTableName(SESSION_CATALOG_NAME, dbName, t.table))
}
+ // Clear cached functions in this database so the cache stays coherent on drop
+ val namespace = FunctionIdentifier("", Some(dbName), None)
Review Comment:
does this drop builtin/temp functions as well if the database name happen to be `session`/`builtin`? Looking at other methods in this file, we should fill the catalog with `Some(CatalogManager.SESSION_CATALOG_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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
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
Re: [PR] [SPARK-55964] Cache coherence: clear function registry on DROP DATABASE [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #54781:
URL: https://github.com/apache/spark/pull/54781#issuecomment-4074102698
SQL tests all passed, 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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
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