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/12 05:46:49 UTC
[PR] [SPARK-55964] system catalog wins over user catalog for BUILTIN, SESSION, AI schemas. [spark]
srielau opened a new pull request, #54765:
URL: https://github.com/apache/spark/pull/54765
<!--
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.
-->
### 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.
-->
### 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'.
-->
### 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.
-->
### 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.
-->
--
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] system catalog wins over user catalog for BUILTIN, and SESSION schemas. [spark]
Posted by "srielau (via GitHub)" <gi...@apache.org>.
srielau commented on code in PR #54765:
URL: https://github.com/apache/spark/pull/54765#discussion_r2943182165
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionResolution.scala:
##########
@@ -75,21 +75,52 @@ class FunctionResolution(
/**
* Produces the ordered list of fully qualified candidate names for resolution.
+ * Every candidate is 3-part (catalog.database.function), assuming search path
+ * entries are catalog.database (2-part).
*
* @param nameParts The function name parts.
- * @return A sequence of fully qualified function names to attempt resolution with.
+ * @return A sequence of fully qualified 3-part names to attempt resolution with.
Review Comment:
- Doc no longer says “every candidate is 3-part”.
- It now states that candidates are usually 3-part, and that a 2-part candidate can be returned when a V2 catalog has no default namespace (and is handled by expandIdentifier).
- @return updated to: “A sequence of candidate names (usually 3-part) to attempt resolution with.”
--
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] system catalog wins over user catalog for BUILTIN, and SESSION schemas. [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #54765:
URL: https://github.com/apache/spark/pull/54765#discussion_r2942477961
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionResolution.scala:
##########
@@ -75,21 +75,52 @@ class FunctionResolution(
/**
* Produces the ordered list of fully qualified candidate names for resolution.
+ * Every candidate is 3-part (catalog.database.function), assuming search path
+ * entries are catalog.database (2-part).
*
* @param nameParts The function name parts.
- * @return A sequence of fully qualified function names to attempt resolution with.
+ * @return A sequence of fully qualified 3-part names to attempt resolution with.
*/
private def resolutionCandidates(nameParts: Seq[String]): Seq[Seq[String]] = {
+ def ensureThreePart(parts: Seq[String]): Seq[String] =
Review Comment:
The only qualification work needs to be done within `resolutionCandidates` is to expand `session/builtin.name` to `system.session/builtin.name`, to make the intention clear. Otherwise, it should just focus on generating the candidates. They should either be qualified already (search path + single part name), or can safely qualified by the main framework (e.g. expand session.name with the current 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] system catalog wins over user catalog for BUILTIN, and SESSION schemas. [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #54765:
URL: https://github.com/apache/spark/pull/54765#discussion_r2939633627
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionResolution.scala:
##########
@@ -75,21 +75,49 @@ class FunctionResolution(
/**
* Produces the ordered list of fully qualified candidate names for resolution.
+ * Every candidate is 3-part (catalog.database.function), assuming search path
+ * entries are catalog.database (2-part).
*
* @param nameParts The function name parts.
- * @return A sequence of fully qualified function names to attempt resolution with.
+ * @return A sequence of fully qualified 3-part names to attempt resolution with.
*/
private def resolutionCandidates(nameParts: Seq[String]): Seq[Seq[String]] = {
+ def ensureThreePart(parts: Seq[String]): Seq[String] =
+ if (parts.length == 3) parts
+ else if (parts.length == 2) currentCatalogPath.head +: parts
+ else parts // Safety fallback; valid paths yield 2 or 3 parts before this point.
+
if (nameParts.size == 1) {
val searchPath = SQLConf.get.resolutionSearchPath(currentCatalogPath)
- searchPath.map(_ ++ nameParts)
+ searchPath.map(_ ++ nameParts).map(ensureThreePart)
Review Comment:
do we need it? the configured search path should always be able to fully qualify a single-part 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] system catalog wins over user catalog for BUILTIN, and SESSION schemas. [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #54765:
URL: https://github.com/apache/spark/pull/54765#discussion_r2942458744
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionResolution.scala:
##########
@@ -75,21 +75,52 @@ class FunctionResolution(
/**
* Produces the ordered list of fully qualified candidate names for resolution.
+ * Every candidate is 3-part (catalog.database.function), assuming search path
+ * entries are catalog.database (2-part).
*
* @param nameParts The function name parts.
- * @return A sequence of fully qualified function names to attempt resolution with.
+ * @return A sequence of fully qualified 3-part names to attempt resolution with.
Review Comment:
The Scaladoc says "Every candidate is 3-part" but line 117 returns 2-part names for catalogs without a default namespace (the comment on line 116 even says "use 2-part").
```suggestion
* Candidates are normally 3-part (catalog.database.function), assuming search path
* entries are catalog.database (2-part). A 2-part candidate is returned when a
* V2 catalog has no default namespace.
*
* @param nameParts The function name parts.
* @return A sequence of candidate names (usually 3-part) to attempt resolution with.
```
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionResolution.scala:
##########
@@ -75,21 +75,52 @@ class FunctionResolution(
/**
* Produces the ordered list of fully qualified candidate names for resolution.
+ * Every candidate is 3-part (catalog.database.function), assuming search path
+ * entries are catalog.database (2-part).
*
* @param nameParts The function name parts.
- * @return A sequence of fully qualified function names to attempt resolution with.
+ * @return A sequence of fully qualified 3-part names to attempt resolution with.
*/
private def resolutionCandidates(nameParts: Seq[String]): Seq[Seq[String]] = {
+ def ensureThreePart(parts: Seq[String]): Seq[String] =
+ if (parts.length == 3) parts
+ else if (parts.length == 2) currentCatalogPath.head +: parts
+ else parts // Safety fallback; valid paths yield 2 or 3 parts before this point.
+
if (nameParts.size == 1) {
+ // Search path entries are 2-part (catalog.database), so path ++ nameParts is always 3-part.
val searchPath = SQLConf.get.resolutionSearchPath(currentCatalogPath)
searchPath.map(_ ++ nameParts)
} else {
nameParts.size match {
- case 2 if FunctionResolution.sessionNamespaceKind(nameParts).isDefined =>
- // Partially qualified builtin/session: try persistent first so user schema wins
- Seq(nameParts, Seq(CatalogManager.SYSTEM_CATALOG_NAME) ++ nameParts)
+ case 2
+ if FunctionResolution.sessionNamespaceKind(nameParts).isDefined =>
+ // Partially qualified builtin/session: produce both candidates as 3-part
+ // (catalog.database.func).
+ val systemCandidate = CatalogManager.SYSTEM_CATALOG_NAME +: nameParts
+ val persistentCandidate = currentCatalogPath.head +: nameParts
+ if (SQLConf.get.prioritizeSystemCatalog) {
+ Seq(systemCandidate, persistentCandidate)
+ } else {
+ Seq(persistentCandidate, systemCandidate)
+ }
+ case 2 if catalogManager.isCatalogRegistered(nameParts(0)) =>
Review Comment:
This `case 2 if catalogManager.isCatalogRegistered` branch is new behavior not present in the old code and not mentioned in the PR description. Previously, 2-part names like `mycatalog.func` passed through unchanged to `resolveQualifiedFunction`, where `CatalogAndIdentifier.unapply` would create an empty-namespace identifier — the same convention used for tables (`mycatalog.mytable` also gets an empty namespace, and the catalog fills in its default internally).
The new code explicitly fetches `cat.defaultNamespace()` and pre-fills it, which:
- Is **inconsistent with table resolution**, where V2 catalogs handle empty namespaces themselves
- **Drops multi-level default namespaces** — `defaultNs(0)` only takes the first element, so `Array("ns1", "ns2")` loses `"ns2"`
- Has no test coverage
This should either be removed (let `CatalogAndIdentifier` handle it, as before) or justified and tested separately.
##########
sql/core/src/main/scala/org/apache/spark/sql/SparkSessionExtensions.scala:
##########
@@ -363,24 +364,55 @@ class SparkSessionExtensions {
private[this] val injectedTableFunctions = mutable.Buffer.empty[TableFunctionDescription]
+ /**
+ * Normalizes an extension function identifier to a fully qualified one for registration.
+ * Accepts unqualified (1-part), fully qualified (3-part), or 2-part system names
+ * (builtin.func, session.func). It rejects other partially qualified names.
+ */
+ private def fullyQualifiedFunctionIdentifier(name: FunctionIdentifier): FunctionIdentifier = {
+ (name.catalog.isEmpty, name.database.isEmpty) match {
+ case (true, true) =>
+ // Unqualified names are registered in system.builtin.
+ FunctionRegistry.builtinFunctionIdentifier(name.funcName)
+ case (false, false) =>
+ // Fully qualified names are used as-is.
+ name
+ case (true, false) =>
Review Comment:
`fullyQualifiedFunctionIdentifier` maps ALL 2-part names to the system catalog, not just `builtin` and `session`. If a user calls `injectFunction` with `FunctionIdentifier("func", Some("custom_db"))`, it would be stored as `system.custom_db.func` — unreachable by any standard resolution path. No error, the function just silently never resolves.
Consider validating that the database is `builtin` or `session`, or falling through to the invalid case for other databases.
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/command/functions.scala:
##########
@@ -197,8 +197,8 @@ case class ShowFunctionsCommand(
sparkSession.sessionState.catalog
.listFunctions(db, pattern.getOrElse("*"))
.collect {
- case (f, "USER") if showUserFunctions => f.unquotedString
- case (f, "SYSTEM") if showSystemFunctions => f.unquotedString
+ case (f, "USER") if showUserFunctions => f.displayNameForShowFunctions
Review Comment:
With the 3-part key change, `listFunctions` now produces different tuple values for a builtin and a same-named temp function. Both display as "abs" but have different `FunctionIdentifier` shapes and different categories ("SYSTEM" vs "USER"), so the upstream `.distinct` on tuples doesn't collapse them. `ShowFunctionsCommand` would show "abs" twice when a temp function shadows a builtin.
The v2 path (`ShowFunctionsExec`) avoids this with `.distinct` on display-name strings. Consider adding `.distinct` to `functionNames` here as well — e.g., after the `.collect { ... }` block.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionRegistry.scala:
##########
@@ -210,10 +210,26 @@ trait SimpleFunctionRegistryBase[T] extends FunctionRegistryBase[T] with Logging
protected val functionBuilders =
new mutable.HashMap[FunctionIdentifier, (ExpressionInfo, FunctionBuilder)]
- // Resolution of the function name is always case insensitive, but the database name
- // depends on the caller
+ // Function name resolution is case-insensitive; database and catalog are preserved so
+ // system.session.foo and spark_catalog.session.foo do not collide. Unqualified (1-part)
+ // lookups resolve to the builtin 3-part key so that callers using FunctionIdentifier("time")
+ // find functions registered as system.builtin.time. A 2-part name with database=session
+ // and no catalog resolves to the temp 3-part key.
private def normalizeFuncName(name: FunctionIdentifier): FunctionIdentifier = {
- FunctionIdentifier(name.funcName.toLowerCase(Locale.ROOT), name.database)
+ if (name.database.isEmpty && name.catalog.isEmpty) {
Review Comment:
`normalizeFuncName` now has branches beyond lowercasing, encoding the assumption "unqualified = builtin" and "database=session without catalog = system.session" in the storage layer. The registry is a storage layer — ideally it should only normalize case. The caller (SessionCatalog / FunctionResolution) already knows what kind of function it's registering or looking up and should provide the exact storage key.
--
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] system catalog wins over user catalog for BUILTIN, and SESSION schemas. [spark]
Posted by "srielau (via GitHub)" <gi...@apache.org>.
srielau commented on code in PR #54765:
URL: https://github.com/apache/spark/pull/54765#discussion_r2943183896
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionRegistry.scala:
##########
@@ -210,10 +210,26 @@ trait SimpleFunctionRegistryBase[T] extends FunctionRegistryBase[T] with Logging
protected val functionBuilders =
new mutable.HashMap[FunctionIdentifier, (ExpressionInfo, FunctionBuilder)]
- // Resolution of the function name is always case insensitive, but the database name
- // depends on the caller
+ // Function name resolution is case-insensitive; database and catalog are preserved so
+ // system.session.foo and spark_catalog.session.foo do not collide. Unqualified (1-part)
+ // lookups resolve to the builtin 3-part key so that callers using FunctionIdentifier("time")
+ // find functions registered as system.builtin.time. A 2-part name with database=session
+ // and no catalog resolves to the temp 3-part key.
private def normalizeFuncName(name: FunctionIdentifier): FunctionIdentifier = {
- FunctionIdentifier(name.funcName.toLowerCase(Locale.ROOT), name.database)
+ if (name.database.isEmpty && name.catalog.isEmpty) {
Review Comment:
- Removed the branch that was prefilling the catalog’s default namespace and inlining it into the candidate (inconsistent with table resolution, only used defaultNs(0), not in PR description, no tests).
- Other 2-part names (e.g. mycatalog.func) now fall through to the default case _ => Seq(ensureThreePart(nameParts)), and resolution is left to resolveQualifiedFunction → expandIdentifier, as before.
--
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] system catalog wins over user catalog for BUILTIN, and SESSION schemas. [spark]
Posted by "srielau (via GitHub)" <gi...@apache.org>.
srielau commented on code in PR #54765:
URL: https://github.com/apache/spark/pull/54765#discussion_r2943323493
##########
sql/core/src/main/scala/org/apache/spark/sql/SparkSessionExtensions.scala:
##########
@@ -363,24 +364,55 @@ class SparkSessionExtensions {
private[this] val injectedTableFunctions = mutable.Buffer.empty[TableFunctionDescription]
+ /**
+ * Normalizes an extension function identifier to a fully qualified one for registration.
+ * Accepts unqualified (1-part), fully qualified (3-part), or 2-part system names
+ * (builtin.func, session.func). It rejects other partially qualified names.
+ */
+ private def fullyQualifiedFunctionIdentifier(name: FunctionIdentifier): FunctionIdentifier = {
+ (name.catalog.isEmpty, name.database.isEmpty) match {
+ case (true, true) =>
+ // Unqualified names are registered in system.builtin.
+ FunctionRegistry.builtinFunctionIdentifier(name.funcName)
+ case (false, false) =>
+ // Fully qualified names are used as-is.
+ name
+ case (true, false) =>
Review Comment:
- For 2-part identifiers (catalog empty, database set), we now allow only builtin or session (case-insensitive).
- Any other database (e.g. FunctionIdentifier("func", Some("custom_db"))) throws IllegalArgumentException with a message that 2-part must be builtin or session, so we no longer silently store e.g. system.custom_db.func.
--
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] system catalog wins over user catalog for BUILTIN, SESSION, AI schemas. [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #54765:
URL: https://github.com/apache/spark/pull/54765#discussion_r2929039738
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/identifiers.scala:
##########
@@ -148,6 +148,11 @@ case class FunctionIdentifier(funcName: String, database: Option[String], catalo
def this(funcName: String) = this(funcName, None, None)
def this(funcName: String, database: Option[String]) = this(funcName, database, None)
+ /** Display name for SHOW FUNCTIONS: simple name for system catalog, full qualified otherwise. */
Review Comment:
Typo:
```suggestion
/** Display name for SHOW FUNCTIONS: simple name for system catalog, fully qualified otherwise. */
```
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala:
##########
@@ -2361,6 +2361,16 @@ object SQLConf {
.checkValues(Set("first", "second", "last"))
.createWithDefault("second")
+ val PRIORITIZE_SYSTEM_CATALOG =
+ buildConf("spark.sql.legacy.prioritizeSystemCatalog")
+ .internal()
Review Comment:
The `spark.sql.legacy.prioritizeSystemCatalog` config uses the `legacy` prefix, but in Spark convention, `spark.sql.legacy.*` configs exist to restore **old** behavior and typically default to `false`. Here, `true` is the new, safer default. This reverses the convention — users who want the old behavior must set a "legacy" config to `false`, which is counterintuitive.
Consider either:
- Dropping the `legacy` prefix: `spark.sql.prioritizeSystemCatalog`
- Or inverting the semantics: `spark.sql.legacy.persistentCatalogFirst` (default `false`)
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/SessionCatalog.scala:
##########
@@ -2331,7 +2330,7 @@ class SessionCatalog(
// Check if it exists as temp (with TEMP_FUNCTION_DB db) or builtin (without db) or persistent
Review Comment:
This comment says "builtin (without db)" but builtins now have `database=Some("builtin")`.
```suggestion
// Check if it exists as temp (with TEMP_FUNCTION_DB db) or builtin (3-part key) or persistent
```
--
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] system catalog wins over user catalog for BUILTIN, and SESSION schemas. [spark]
Posted by "srielau (via GitHub)" <gi...@apache.org>.
srielau commented on code in PR #54765:
URL: https://github.com/apache/spark/pull/54765#discussion_r2943188234
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionResolution.scala:
##########
@@ -75,21 +75,52 @@ class FunctionResolution(
/**
* Produces the ordered list of fully qualified candidate names for resolution.
+ * Every candidate is 3-part (catalog.database.function), assuming search path
+ * entries are catalog.database (2-part).
*
* @param nameParts The function name parts.
- * @return A sequence of fully qualified function names to attempt resolution with.
+ * @return A sequence of fully qualified 3-part names to attempt resolution with.
*/
private def resolutionCandidates(nameParts: Seq[String]): Seq[Seq[String]] = {
+ def ensureThreePart(parts: Seq[String]): Seq[String] =
Review Comment:
After the .collect { ... } that builds the list of display names, added .distinct so that when a temp function shadows a builtin (same display name, different FunctionIdentifier and SYSTEM vs USER), the name appears only once, matching the v2 path (ShowFunctionsExec).
--
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] system catalog wins over user catalog for BUILTIN, and SESSION schemas. [spark]
Posted by "srielau (via GitHub)" <gi...@apache.org>.
srielau commented on code in PR #54765:
URL: https://github.com/apache/spark/pull/54765#discussion_r2943185632
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/command/functions.scala:
##########
@@ -197,8 +197,8 @@ case class ShowFunctionsCommand(
sparkSession.sessionState.catalog
.listFunctions(db, pattern.getOrElse("*"))
.collect {
- case (f, "USER") if showUserFunctions => f.unquotedString
- case (f, "SYSTEM") if showSystemFunctions => f.unquotedString
+ case (f, "USER") if showUserFunctions => f.displayNameForShowFunctions
Review Comment:
- For 2-part identifiers (catalog empty, database set), we now allow only builtin or session (case-insensitive).
- Any other database (e.g. FunctionIdentifier("func", Some("custom_db"))) throws IllegalArgumentException with a message that 2-part must be builtin or session, so we no longer silently store e.g. system.custom_db.func.
--
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] system catalog wins over user catalog for BUILTIN, and SESSION schemas. [spark]
Posted by "srielau (via GitHub)" <gi...@apache.org>.
srielau commented on code in PR #54765:
URL: https://github.com/apache/spark/pull/54765#discussion_r2943327075
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionResolution.scala:
##########
@@ -75,21 +75,52 @@ class FunctionResolution(
/**
* Produces the ordered list of fully qualified candidate names for resolution.
+ * Every candidate is 3-part (catalog.database.function), assuming search path
+ * entries are catalog.database (2-part).
*
* @param nameParts The function name parts.
- * @return A sequence of fully qualified function names to attempt resolution with.
+ * @return A sequence of fully qualified 3-part names to attempt resolution with.
*/
private def resolutionCandidates(nameParts: Seq[String]): Seq[Seq[String]] = {
+ def ensureThreePart(parts: Seq[String]): Seq[String] =
+ if (parts.length == 3) parts
+ else if (parts.length == 2) currentCatalogPath.head +: parts
+ else parts // Safety fallback; valid paths yield 2 or 3 parts before this point.
+
if (nameParts.size == 1) {
+ // Search path entries are 2-part (catalog.database), so path ++ nameParts is always 3-part.
val searchPath = SQLConf.get.resolutionSearchPath(currentCatalogPath)
searchPath.map(_ ++ nameParts)
} else {
nameParts.size match {
- case 2 if FunctionResolution.sessionNamespaceKind(nameParts).isDefined =>
- // Partially qualified builtin/session: try persistent first so user schema wins
- Seq(nameParts, Seq(CatalogManager.SYSTEM_CATALOG_NAME) ++ nameParts)
+ case 2
+ if FunctionResolution.sessionNamespaceKind(nameParts).isDefined =>
+ // Partially qualified builtin/session: produce both candidates as 3-part
+ // (catalog.database.func).
+ val systemCandidate = CatalogManager.SYSTEM_CATALOG_NAME +: nameParts
+ val persistentCandidate = currentCatalogPath.head +: nameParts
+ if (SQLConf.get.prioritizeSystemCatalog) {
+ Seq(systemCandidate, persistentCandidate)
+ } else {
+ Seq(persistentCandidate, systemCandidate)
+ }
+ case 2 if catalogManager.isCatalogRegistered(nameParts(0)) =>
Review Comment:
- Removed the branch that was prefilling the catalog’s default namespace and inlining it into the candidate (inconsistent with table resolution, only used defaultNs(0), not in PR description, no tests).
- Other 2-part names (e.g. mycatalog.func) now fall through to the default case _ => Seq(ensureThreePart(nameParts)), and resolution is left to resolveQualifiedFunction → expandIdentifier, as before.
--
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