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 2025/12/22 19:56:12 UTC
[PR] [SPARK-54807] Allow qualified names for built-in and session functions [spark]
srielau opened a new pull request, #53570:
URL: https://github.com/apache/spark/pull/53570
<!--
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-54807][SQL] Allow qualified names for built-in and session functions [spark]
Posted by "srielau (via GitHub)" <gi...@apache.org>.
srielau commented on code in PR #53570:
URL: https://github.com/apache/spark/pull/53570#discussion_r2870574866
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/SessionCatalog.scala:
##########
@@ -1935,20 +2163,50 @@ class SessionCatalog(
overrideIfExists: Boolean,
functionBuilder: Option[FunctionBuilder] = None): Unit = {
val builder = functionBuilder.getOrElse(makeFunctionBuilder(funcDefinition))
- registerFunction(funcDefinition, overrideIfExists, functionRegistry, builder)
+ // Use composite keys for temporary functions, but not for persistent functions
+ val useComposite = funcDefinition.identifier.database.isEmpty
+ registerFunction(funcDefinition, overrideIfExists, functionRegistry, builder, useComposite)
}
private def registerFunction[T](
funcDefinition: CatalogFunction,
overrideIfExists: Boolean,
registry: FunctionRegistryBase[T],
- functionBuilder: FunctionRegistryBase[T]#FunctionBuilder): Unit = {
+ functionBuilder: FunctionRegistryBase[T]#FunctionBuilder,
+ useCompositeKey: Boolean): Unit = {
Review Comment:
removed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
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-54807][SQL] Allow qualified names for built-in and session functions [spark]
Posted by "srielau (via GitHub)" <gi...@apache.org>.
srielau commented on code in PR #53570:
URL: https://github.com/apache/spark/pull/53570#discussion_r2867579445
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala:
##########
@@ -1823,23 +1823,34 @@ class Analyzer(
case _ => false
}
+ /**
+ * Checks if the given function name parts match the expected builtin function name.
+ * This is used for special syntax transformations (e.g., COUNT(*) -> COUNT(1)) that
+ * should only apply to builtin functions, not to user-defined functions.
+ */
+ private def matchesFunctionName(nameParts: Seq[String], expectedName: String): Boolean = {
+ FunctionResolution.isUnqualifiedOrBuiltinFunctionName(nameParts, expectedName)
+ }
+
/**
* Expands the matching attribute.*'s in `child`'s output.
*/
def expandStarExpression(expr: Expression, child: LogicalPlan): Expression = {
expr.transformUp {
case f0: UnresolvedFunction if !f0.isDistinct &&
- f0.nameParts.map(_.toLowerCase(Locale.ROOT)) == Seq("count") &&
+ matchesFunctionName(f0.nameParts, "count") &&
isCountStarExpansionAllowed(f0.arguments) =>
// Transform COUNT(*) into COUNT(1).
- f0.copy(nameParts = Seq("count"), arguments = Seq(Literal(1)))
Review Comment:
Unclear. I do not know
--
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-54807][SQL] Allow qualified names for built-in and session functions [spark]
Posted by "vladimirg-db (via GitHub)" <gi...@apache.org>.
vladimirg-db commented on code in PR #53570:
URL: https://github.com/apache/spark/pull/53570#discussion_r2873212123
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/resolver/ResolverGuard.scala:
##########
@@ -345,14 +347,46 @@ class ResolverGuard(catalogManager: CatalogManager) extends SQLConfHelper {
createNamedStruct.children.forall(checkExpression)
}
- private def checkUnresolvedFunction(unresolvedFunction: UnresolvedFunction) =
- unresolvedFunction.nameParts.size == 1 &&
- !ResolverGuard.UNSUPPORTED_FUNCTION_NAMES.contains(unresolvedFunction.nameParts.head) &&
- // UDFs are not supported
- FunctionRegistry.functionSet.contains(
- FunctionIdentifier(unresolvedFunction.nameParts.head.toLowerCase(Locale.ROOT))
- ) &&
- unresolvedFunction.children.forall(checkExpression)
+ /**
+ * Checks if an unresolved function is supported by the single-pass analyzer.
+ *
+ * The single-pass analyzer supports built-in functions. For qualified function names,
+ * we allow them to pass through to the resolver, which will handle them appropriately.
+ * This includes:
+ * - Built-in functions: `builtin.count`, `system.builtin.count`, unqualified `count`
+ * - Session/temp functions: `session.my_func`, `system.session.my_func` (may or may not work)
+ * - Persistent functions: `catalog.db.func` (not currently supported, but not actively blocked)
+ *
+ * We only explicitly reject certain built-in functions that require special handling
+ * (lambdas, generators, etc.) via the UNSUPPORTED_FUNCTION_NAMES list.
+ *
+ * @param unresolvedFunction the unresolved function to check
+ * @return true if the function should be allowed to attempt resolution, false otherwise
+ */
+ private def checkUnresolvedFunction(unresolvedFunction: UnresolvedFunction) = {
+ val nameParts = unresolvedFunction.nameParts
+ val functionName = nameParts.last
+
+ // Reject only when we know it's the builtin: unqualified or explicitly builtin-qualified
+ // (system.builtin.func or builtin.func). Any other qualification (session, catalog.db) we let
+ // through since it's not a forbidden builtin.
+ val shouldReject = if (nameParts.length == 1) {
+ // Unqualified: only allow if it's a known builtin (excluding unsupported ones)
+ ResolverGuard.UNSUPPORTED_FUNCTION_NAMES.contains(functionName) ||
+ !FunctionRegistry.functionSet.contains(
+ FunctionIdentifier(functionName.toLowerCase(Locale.ROOT))
+ )
+ } else if (FunctionResolution.sessionNamespaceKind(nameParts)
+ .contains(SessionCatalog.Builtin)) {
+ // Explicitly builtin-qualified (builtin.func, system.builtin.func): reject if unsupported
+ ResolverGuard.UNSUPPORTED_FUNCTION_NAMES.contains(functionName)
+ } else {
+ // Other qualification (session, catalog.db, etc.): allow through
+ false
Review Comment:
Let's make sure we have a test for this in ResolverGuardSuite
--
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-54807][SQL] Allow qualified names for built-in and session functions [spark]
Posted by "gengliangwang (via GitHub)" <gi...@apache.org>.
gengliangwang closed pull request #53570: [SPARK-54807][SQL] Allow qualified names for built-in and session functions
URL: https://github.com/apache/spark/pull/53570
--
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-54807][SQL] Allow qualified names for built-in and session functions [spark]
Posted by "vladimirg-db (via GitHub)" <gi...@apache.org>.
vladimirg-db commented on code in PR #53570:
URL: https://github.com/apache/spark/pull/53570#discussion_r2685556320
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionRegistry.scala:
##########
@@ -78,10 +79,17 @@ trait FunctionRegistryBase[T] {
/* Create or replace a temporary function. */
final def createOrReplaceTempFunction(
name: String, builder: FunctionBuilder, source: String): Unit = {
- registerFunction(
- FunctionIdentifier(name),
- builder,
- source)
+ // Internal functions (source="internal") are NOT qualified with
+ // CatalogManager.SESSION_NAMESPACE database because they use a separate
+ // internal registry and are resolved differently
+ val identifier = if (source == "internal") {
+ FunctionIdentifier(name)
+ } else {
+ // Regular temporary functions are qualified with CatalogManager.SESSION_NAMESPACE
+ // to enable coexistence with builtin functions of the same name
+ FunctionIdentifier(name, Some(CatalogManager.SESSION_NAMESPACE))
Review Comment:
`ResolverGuard`'s `checkUnresolvedFunction` also depends on the fact that built-in functions have one-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-54807][SQL] Allow qualified names for built-in and session functions [spark]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #53570:
URL: https://github.com/apache/spark/pull/53570#issuecomment-3790049885
## JIRA Issue Information
=== Sub-task SPARK-54807 ===
Summary: Support SYSTEM.BUILTIN and SYSTEM.SESSION for function resolution
Assignee: None
Status: Open
Affected: ["4.2.0"]
---
*This comment was automatically generated by GitHub Actions*
--
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-54807][SQL] Allow qualified names for built-in and session functions [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #53570:
URL: https://github.com/apache/spark/pull/53570#discussion_r2730657677
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionRegistry.scala:
##########
@@ -78,10 +79,17 @@ trait FunctionRegistryBase[T] {
/* Create or replace a temporary function. */
final def createOrReplaceTempFunction(
name: String, builder: FunctionBuilder, source: String): Unit = {
- registerFunction(
- FunctionIdentifier(name),
- builder,
- source)
+ // Internal functions (source="internal") are NOT qualified with
+ // CatalogManager.SESSION_NAMESPACE database because they use a separate
+ // internal registry and are resolved differently
+ val identifier = if (source == "internal") {
+ FunctionIdentifier(name)
+ } else {
+ // Regular temporary functions are qualified with CatalogManager.SESSION_NAMESPACE
+ // to enable coexistence with builtin functions of the same name
+ FunctionIdentifier(name, Some(CatalogManager.SESSION_NAMESPACE))
Review Comment:
FunctionIdentifier has both database and catalog fields, shall we set both?
--
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-54807][SQL] Allow qualified names for built-in and session functions [spark]
Posted by "gengliangwang (via GitHub)" <gi...@apache.org>.
gengliangwang commented on code in PR #53570:
URL: https://github.com/apache/spark/pull/53570#discussion_r2850390605
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/resolver/ResolverGuard.scala:
##########
@@ -345,14 +346,54 @@ class ResolverGuard(catalogManager: CatalogManager) extends SQLConfHelper {
createNamedStruct.children.forall(checkExpression)
}
- private def checkUnresolvedFunction(unresolvedFunction: UnresolvedFunction) =
- unresolvedFunction.nameParts.size == 1 &&
- !ResolverGuard.UNSUPPORTED_FUNCTION_NAMES.contains(unresolvedFunction.nameParts.head) &&
- // UDFs are not supported
- FunctionRegistry.functionSet.contains(
- FunctionIdentifier(unresolvedFunction.nameParts.head.toLowerCase(Locale.ROOT))
- ) &&
- unresolvedFunction.children.forall(checkExpression)
+ /**
+ * Checks if an unresolved function is supported by the single-pass analyzer.
+ *
+ * The single-pass analyzer supports built-in functions. For qualified function names,
+ * we allow them to pass through to the resolver, which will handle them appropriately.
+ * This includes:
+ * - Built-in functions: `builtin.count`, `system.builtin.count`, unqualified `count`
+ * - Session/temp functions: `session.my_func`, `system.session.my_func` (may or may not work)
+ * - Persistent functions: `catalog.db.func` (not currently supported, but not actively blocked)
+ *
+ * We only explicitly reject certain built-in functions that require special handling
+ * (lambdas, generators, etc.) via the UNSUPPORTED_FUNCTION_NAMES list.
+ *
+ * @param unresolvedFunction the unresolved function to check
+ * @return true if the function should be allowed to attempt resolution, false otherwise
+ */
+ private def checkUnresolvedFunction(unresolvedFunction: UnresolvedFunction) = {
+ val nameParts = unresolvedFunction.nameParts
+ val functionName = nameParts.last
+
+ // Only reject functions that are explicitly unsupported or not in the builtin registry
+ // for unqualified names. For qualified names, let the resolver attempt resolution.
+ val shouldReject = if (nameParts.length == 1) {
+ // Unqualified: only allow if it's a known builtin (excluding unsupported ones)
+ ResolverGuard.UNSUPPORTED_FUNCTION_NAMES.contains(functionName) ||
+ !FunctionRegistry.functionSet.contains(
+ FunctionIdentifier(functionName.toLowerCase(Locale.ROOT))
+ )
+ } else {
+ // Qualified: allow to pass through, let downstream resolver handle it
Review Comment:
shall we add more rejection check 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.
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-54807][SQL] Allow qualified names for built-in and session functions [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #53570:
URL: https://github.com/apache/spark/pull/53570#issuecomment-3982113106
*This review was generated by Cursor with Claude Opus 4.6.*
Additional implementation review comments beyond what's already been raised:
### 1. `lookupFunctionInfo` — table functions with `kind == Temp` skip view context filtering (bug)
For `kind == Temp`, the scalar branch applies view context filtering via `lookupTempFuncWithViewContext`, but the table function branch does a plain `registry.functionExists / registry.lookupFunction` with no view filtering:
```scala
case SessionCatalog.Temp =>
synchronized {
if (tableFunction) {
// No view context filtering!
if (registry.functionExists(identifier)) registry.lookupFunction(identifier) else None
} else {
lookupTempFuncWithViewContext(
name, _ => false, _ => registry.lookupFunction(identifier))
}
}
```
This means a temp table function could be visible inside a view even when it shouldn't be. Both branches should apply `handleViewContext`.
### 2. `registerUserDefinedFunction` — `isTableFunction` flag is redundant
The method already receives `registry: FunctionRegistryBase[T]` which is either `functionRegistry` or `tableFunctionRegistry`. The "other" registry can be derived:
```scala
val otherRegistry = if (registry eq functionRegistry) tableFunctionRegistry else functionRegistry
```
No need for the extra `isTableFunction` parameter.
### 3. `resolveQualifiedTableFunction` — `case _: Exception => // ignore` is dangerous
In the inner try-catch that checks whether a scalar function exists (to throw `NOT_A_TABLE_FUNCTION`), all exceptions are silently swallowed:
```scala
} catch {
case _: Exception => // ignore
}
```
This could mask real errors (permission failures, catalog connectivity, etc.). It should catch only the specific exceptions that indicate "function not found" — `NoSuchFunctionException`, `NoSuchNamespaceException`, `CatalogNotFoundException`, and the `FORBIDDEN_OPERATION` `AnalysisException` (matching the outer catch).
### 4. `FunctionType` sealed trait is over-engineered for its single use
`FunctionType` (with 5 case objects) is only used by `lookupFunctionType`, which is only called from `LookupFunctions` in `Analyzer.scala`. The `Builtin`/`Temporary` distinction is never actually used — both hit the `throw SparkException.internalError(...)` branch. Consider simplifying or inlining the logic.
### 5. `resolveBuiltinOrTempTableFunctionRespectingPathOrder` — appears to be dead code
This public method returns `Option[Either[LogicalPlan, Unit]]` but doesn't appear to be called anywhere in the diff. If it is needed, `Right(())` to signal "scalar found but no table function" is not self-documenting and should use a clearer type. If it's not called, it should be removed.
### 6. `createOrReplaceTempFunction` — the `source == "internal"` check is dead code
The PR adds a `source == "internal"` branch to avoid qualifying internal functions with `SESSION_NAMESPACE`. But `FunctionRegistry.internal` is a completely separate `SimpleFunctionRegistry` instance — internal functions are registered via `registerInternalExpression` directly into that registry, never through `createOrReplaceTempFunction`. No caller ever passes `"internal"` as the source. The conditional should be removed and the method should always qualify with `SESSION_NAMESPACE`.
### 7. Duplicated scaladoc on `lookupFunctionWithShadowing`
There are two consecutive scaladoc blocks before `lookupFunctionWithShadowing` — the first is an orphaned leftover that should be removed.
### 8. Indentation-only changes pollute the diff
Several places in `SessionCatalog.scala` have whitespace-only changes that shift indentation without any logical change (e.g. `fetchCatalogFunction`, the persistent branch of `registerUserDefinedFunction`, `registerFunction` call sites). These should be reverted to keep `git blame` clean.
---
_This comment was generated with [GitHub MCP](http://go/mcps)._
--
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-54807][SQL] Allow qualified names for built-in and session functions [spark]
Posted by "gengliangwang (via GitHub)" <gi...@apache.org>.
gengliangwang commented on code in PR #53570:
URL: https://github.com/apache/spark/pull/53570#discussion_r2850348516
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/SessionCatalog.scala:
##########
@@ -2101,85 +2437,335 @@ class SessionCatalog(
}
/**
- * Look up the `ExpressionInfo` of the given function by name if it's a built-in or temp function.
+ * Handles view resolution context for temporary functions (both scalar and table-valued).
+ * When resolving a view, only returns the result if the function is explicitly referred
+ * by that view. Otherwise, tracks the function reference for future view creation.
+ *
+ * This generic helper works for both scalar functions (FunctionRegistry) and table-valued
+ * functions (TableFunctionRegistry) due to the type parameter T.
+ *
+ * @param name The function name (unqualified)
+ * @param result The result to wrap with view context handling
+ * @tparam T The result type (ExpressionInfo, Expression, or LogicalPlan)
+ * @return The result if visible in current context, None otherwise
+ */
+ private def handleViewContext[T](name: String, result: Option[T]): Option[T] =
+ result.filter { _ =>
+ val isResolvingView = AnalysisContext.get.catalogAndNamespace.nonEmpty
+ val referredTempFunctionNames = AnalysisContext.get.referredTempFunctionNames
+
+ if (isResolvingView) {
+ // When resolving a view, only return a temp function if it's referred by this view.
+ referredTempFunctionNames.contains(name)
+ } else {
+ // We are not resolving a view and the function is a temp one, add it to
+ // AnalysisContext so if a view is being created, it can be checked.
+ AnalysisContext.get.referredTempFunctionNames.add(name)
+ true
+ }
+ }
+
+ /**
+ * Generic helper for looking up functions with temp/builtin shadowing and view context.
+ * Checks temp function first (with TEMP_FUNCTION_DB database qualifier), then built-in
+ * (without database qualifier). For temp functions, applies view resolution context.
+ *
+ * @param name The function name (unqualified)
+ * @param registry The registry to search (FunctionRegistry or TableFunctionRegistry)
+ * @param checkBuiltinOperators Whether to check built-in operators first (scalar functions only)
+ * @tparam T The registry's type parameter (Expression for FunctionRegistry,
+ * LogicalPlan for TableFunctionRegistry)
+ * @return ExpressionInfo if function found, None otherwise
+ */
+ /**
+ * Looks up functions using PATH-based resolution.
+ * Searches through the resolution path (session then builtin) with view context handling.
+ *
+ * @param name The function name (unqualified).
+ * @param registry The registry to search (FunctionRegistry or TableFunctionRegistry).
+ * @param checkBuiltinOperators Whether to check built-in operators first (scalar functions only).
+ * @tparam T The registry's type parameter (Expression for FunctionRegistry,
+ * LogicalPlan for TableFunctionRegistry).
+ * @return ExpressionInfo if function found, None otherwise.
+ */
+ private def lookupFunctionWithShadowing[T](
+ name: String,
+ registry: FunctionRegistryBase[T],
+ checkBuiltinOperators: Boolean): Option[ExpressionInfo] = {
+
+ // Check built-in operators first (only for scalar functions).
+ val operatorResult = if (checkBuiltinOperators) {
+ FunctionRegistry.builtinOperators.get(name.toLowerCase(Locale.ROOT))
+ } else {
+ None
+ }
+
+ operatorResult.orElse {
+ // Use PATH-based resolution: iterate through namespaces until a match is found.
+ val path = resolutionPath()
+
+ // Use iterator for short-circuit evaluation (stops at first match).
+ path.iterator.flatMap { namespace =>
+ lookupInNamespace(namespace, name, registry)
+ }.nextOption()
+ }
+ }
+
+ /**
+ * Look up the `ExpressionInfo` of the given function by name.
+ * Resolution order follows the configured path (e.g. builtin then session);
+ * extension functions are stored in the builtin namespace.
* This only supports scalar functions.
*/
def lookupBuiltinOrTempFunction(name: String): Option[ExpressionInfo] = {
- FunctionRegistry.builtinOperators.get(name.toLowerCase(Locale.ROOT)).orElse {
- synchronized(lookupTempFuncWithViewContext(
- name, FunctionRegistry.builtin.functionExists, functionRegistry.lookupFunction))
+ lookupFunctionWithShadowing(name, functionRegistry, checkBuiltinOperators = true)
+ }
+
+ /**
+ * Look up the `ExpressionInfo` of the given function by name.
+ * Resolution order follows the configured path (e.g. builtin then session);
+ * extension functions are stored in the builtin namespace.
+ */
+ def lookupBuiltinOrTempTableFunction(name: String): Option[ExpressionInfo] = {
+ lookupFunctionWithShadowing(name, tableFunctionRegistry, checkBuiltinOperators = false)
+ }
+
+ /**
+ * Look up only builtin function (no temp).
+ */
+ def lookupBuiltinFunction(name: String): Option[ExpressionInfo] = {
+ val builtinIdentifier = FunctionIdentifier(format(name))
+ functionRegistry.lookupFunction(builtinIdentifier)
+ }
+
+ /**
+ * Look up only builtin table function (no temp).
+ */
+ def lookupBuiltinTableFunction(name: String): Option[ExpressionInfo] = {
+ val builtinIdentifier = FunctionIdentifier(format(name))
+ tableFunctionRegistry.lookupFunction(builtinIdentifier)
+ }
+
+ /**
+ * Look up only temp function (no builtin).
+ */
+ def lookupTempFunction(name: String): Option[ExpressionInfo] = {
+ val tempIdentifier = tempFunctionIdentifier(name)
+ synchronized(lookupTempFuncWithViewContext(
+ name,
+ // Return false if temp exists (not builtin)
+ ident => !functionRegistry.functionExists(tempIdentifier),
+ _ => functionRegistry.lookupFunction(tempIdentifier)))
+ }
+
+ /**
+ * Look up only temp table function (no builtin).
+ */
+ def lookupTempTableFunction(name: String): Option[ExpressionInfo] = {
+ val tempIdentifier = tempFunctionIdentifier(name)
+ if (tableFunctionRegistry.functionExists(tempIdentifier)) {
+ tableFunctionRegistry.lookupFunction(tempIdentifier)
+ } else {
+ None
}
}
/**
- * Look up the `ExpressionInfo` of the given function by name if it's a built-in or
- * temp table function.
+ * Look up only extension function (no builtin or temp).
*/
- def lookupBuiltinOrTempTableFunction(name: String): Option[ExpressionInfo] = synchronized {
Review Comment:
shall we keep the `synchronized` keyword for thread safety?
--
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-54807][SQL] Allow qualified names for built-in and session functions [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #53570:
URL: https://github.com/apache/spark/pull/53570#discussion_r2867244759
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/resolver/ResolverGuard.scala:
##########
@@ -345,14 +346,56 @@ class ResolverGuard(catalogManager: CatalogManager) extends SQLConfHelper {
createNamedStruct.children.forall(checkExpression)
}
- private def checkUnresolvedFunction(unresolvedFunction: UnresolvedFunction) =
- unresolvedFunction.nameParts.size == 1 &&
- !ResolverGuard.UNSUPPORTED_FUNCTION_NAMES.contains(unresolvedFunction.nameParts.head) &&
- // UDFs are not supported
- FunctionRegistry.functionSet.contains(
- FunctionIdentifier(unresolvedFunction.nameParts.head.toLowerCase(Locale.ROOT))
- ) &&
- unresolvedFunction.children.forall(checkExpression)
+ /**
+ * Checks if an unresolved function is supported by the single-pass analyzer.
+ *
+ * The single-pass analyzer supports built-in functions. For qualified function names,
+ * we allow them to pass through to the resolver, which will handle them appropriately.
+ * This includes:
+ * - Built-in functions: `builtin.count`, `system.builtin.count`, unqualified `count`
+ * - Session/temp functions: `session.my_func`, `system.session.my_func` (may or may not work)
+ * - Persistent functions: `catalog.db.func` (not currently supported, but not actively blocked)
+ *
+ * We only explicitly reject certain built-in functions that require special handling
+ * (lambdas, generators, etc.) via the UNSUPPORTED_FUNCTION_NAMES list.
+ *
+ * @param unresolvedFunction the unresolved function to check
+ * @return true if the function should be allowed to attempt resolution, false otherwise
+ */
+ private def checkUnresolvedFunction(unresolvedFunction: UnresolvedFunction) = {
+ val nameParts = unresolvedFunction.nameParts
+ val functionName = nameParts.last
+
+ // Only reject functions that are explicitly unsupported or not in the builtin registry
+ // for unqualified names. For qualified names, still reject unsupported function names
+ // (e.g. builtin.explode), but allow others to pass through for the resolver.
+ val shouldReject = if (nameParts.length == 1) {
+ // Unqualified: only allow if it's a known builtin (excluding unsupported ones)
+ ResolverGuard.UNSUPPORTED_FUNCTION_NAMES.contains(functionName) ||
+ !FunctionRegistry.functionSet.contains(
+ FunctionIdentifier(functionName.toLowerCase(Locale.ROOT))
+ )
+ } else {
+ // Qualified: reject if the function name is unsupported (e.g. builtin.explode);
Review Comment:
sorry I'm confused, we completely ignore the qualifiers 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.
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-54807][SQL] Allow qualified names for built-in and session functions [spark]
Posted by "srielau (via GitHub)" <gi...@apache.org>.
srielau commented on code in PR #53570:
URL: https://github.com/apache/spark/pull/53570#discussion_r2689091451
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/resolver/FunctionResolver.scala:
##########
@@ -172,11 +170,11 @@ class FunctionResolver(
/**
* Method used to determine whether the given function should be replaced with another one.
+ * Handles qualified function names (e.g., "count", "builtin.count", "system.builtin.count").
*/
private def isCount(unresolvedFunction: UnresolvedFunction): Boolean = {
!unresolvedFunction.isDistinct &&
- unresolvedFunction.nameParts.length == 1 &&
- unresolvedFunction.nameParts.head.toLowerCase(Locale.ROOT) == "count"
+ unresolvedFunction.nameParts.lastOption.exists(_.equalsIgnoreCase("count"))
Review Comment:
Fixed.
--
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-54807][SQL] Allow qualified names for built-in and session functions [spark]
Posted by "vladimirg-db (via GitHub)" <gi...@apache.org>.
vladimirg-db commented on code in PR #53570:
URL: https://github.com/apache/spark/pull/53570#discussion_r2712073937
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/resolver/ResolverGuard.scala:
##########
@@ -345,14 +345,42 @@ class ResolverGuard(catalogManager: CatalogManager) extends SQLConfHelper {
createNamedStruct.children.forall(checkExpression)
}
- private def checkUnresolvedFunction(unresolvedFunction: UnresolvedFunction) =
- unresolvedFunction.nameParts.size == 1 &&
- !ResolverGuard.UNSUPPORTED_FUNCTION_NAMES.contains(unresolvedFunction.nameParts.head) &&
- // UDFs are not supported
- FunctionRegistry.functionSet.contains(
- FunctionIdentifier(unresolvedFunction.nameParts.head.toLowerCase(Locale.ROOT))
- ) &&
- unresolvedFunction.children.forall(checkExpression)
+ private def checkUnresolvedFunction(unresolvedFunction: UnresolvedFunction) = {
Review Comment:
Please avoid the inline comments and create a holistic yet succinct method doc
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/resolver/ResolverGuard.scala:
##########
@@ -345,14 +345,42 @@ class ResolverGuard(catalogManager: CatalogManager) extends SQLConfHelper {
createNamedStruct.children.forall(checkExpression)
}
- private def checkUnresolvedFunction(unresolvedFunction: UnresolvedFunction) =
- unresolvedFunction.nameParts.size == 1 &&
- !ResolverGuard.UNSUPPORTED_FUNCTION_NAMES.contains(unresolvedFunction.nameParts.head) &&
- // UDFs are not supported
- FunctionRegistry.functionSet.contains(
- FunctionIdentifier(unresolvedFunction.nameParts.head.toLowerCase(Locale.ROOT))
- ) &&
- unresolvedFunction.children.forall(checkExpression)
+ private def checkUnresolvedFunction(unresolvedFunction: UnresolvedFunction) = {
+ val nameParts = unresolvedFunction.nameParts
+
+ // Only accept unqualified names or names explicitly qualified as builtin.
+ // Session/temporary functions are UDFs and not supported by single-pass analyzer.
+ // Persistent functions from external catalogs are also not supported.
+ val isBuiltinOrUnqualified = nameParts.length match {
+ case 1 =>
+ // Unqualified: "count" - check if it's a builtin
+ true
+ case 2 =>
+ // Two parts: must be "builtin.count"
+ nameParts.head.equalsIgnoreCase(CatalogManager.BUILTIN_NAMESPACE)
+ case 3 =>
+ // Three parts: must be "system.builtin.count"
+ nameParts(0).equalsIgnoreCase(CatalogManager.SYSTEM_CATALOG_NAME) &&
+ nameParts(1).equalsIgnoreCase(CatalogManager.BUILTIN_NAMESPACE)
+ case _ =>
+ // More than 3 parts is not valid
+ false
+ }
+
+ if (!isBuiltinOrUnqualified) {
+ // This is session.func, catalog.db.function, or invalid - not supported
+ false
Review Comment:
Why wouldn't we support these in single-pass analyzer?
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/resolver/ResolverGuard.scala:
##########
@@ -345,14 +345,42 @@ class ResolverGuard(catalogManager: CatalogManager) extends SQLConfHelper {
createNamedStruct.children.forall(checkExpression)
}
- private def checkUnresolvedFunction(unresolvedFunction: UnresolvedFunction) =
- unresolvedFunction.nameParts.size == 1 &&
- !ResolverGuard.UNSUPPORTED_FUNCTION_NAMES.contains(unresolvedFunction.nameParts.head) &&
- // UDFs are not supported
- FunctionRegistry.functionSet.contains(
- FunctionIdentifier(unresolvedFunction.nameParts.head.toLowerCase(Locale.ROOT))
- ) &&
- unresolvedFunction.children.forall(checkExpression)
+ private def checkUnresolvedFunction(unresolvedFunction: UnresolvedFunction) = {
+ val nameParts = unresolvedFunction.nameParts
+
+ // Only accept unqualified names or names explicitly qualified as builtin.
+ // Session/temporary functions are UDFs and not supported by single-pass analyzer.
+ // Persistent functions from external catalogs are also not supported.
+ val isBuiltinOrUnqualified = nameParts.length match {
Review Comment:
Please create a private method for this logic
--
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-54807][SQL] Allow qualified names for built-in and session functions [spark]
Posted by "gengliangwang (via GitHub)" <gi...@apache.org>.
gengliangwang commented on code in PR #53570:
URL: https://github.com/apache/spark/pull/53570#discussion_r2850353695
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/SessionCatalog.scala:
##########
@@ -2101,85 +2437,335 @@ class SessionCatalog(
}
/**
- * Look up the `ExpressionInfo` of the given function by name if it's a built-in or temp function.
+ * Handles view resolution context for temporary functions (both scalar and table-valued).
+ * When resolving a view, only returns the result if the function is explicitly referred
+ * by that view. Otherwise, tracks the function reference for future view creation.
+ *
+ * This generic helper works for both scalar functions (FunctionRegistry) and table-valued
+ * functions (TableFunctionRegistry) due to the type parameter T.
+ *
+ * @param name The function name (unqualified)
+ * @param result The result to wrap with view context handling
+ * @tparam T The result type (ExpressionInfo, Expression, or LogicalPlan)
+ * @return The result if visible in current context, None otherwise
+ */
+ private def handleViewContext[T](name: String, result: Option[T]): Option[T] =
+ result.filter { _ =>
+ val isResolvingView = AnalysisContext.get.catalogAndNamespace.nonEmpty
+ val referredTempFunctionNames = AnalysisContext.get.referredTempFunctionNames
+
+ if (isResolvingView) {
+ // When resolving a view, only return a temp function if it's referred by this view.
+ referredTempFunctionNames.contains(name)
+ } else {
+ // We are not resolving a view and the function is a temp one, add it to
+ // AnalysisContext so if a view is being created, it can be checked.
+ AnalysisContext.get.referredTempFunctionNames.add(name)
+ true
+ }
+ }
+
+ /**
+ * Generic helper for looking up functions with temp/builtin shadowing and view context.
+ * Checks temp function first (with TEMP_FUNCTION_DB database qualifier), then built-in
+ * (without database qualifier). For temp functions, applies view resolution context.
+ *
+ * @param name The function name (unqualified)
+ * @param registry The registry to search (FunctionRegistry or TableFunctionRegistry)
+ * @param checkBuiltinOperators Whether to check built-in operators first (scalar functions only)
+ * @tparam T The registry's type parameter (Expression for FunctionRegistry,
+ * LogicalPlan for TableFunctionRegistry)
+ * @return ExpressionInfo if function found, None otherwise
+ */
+ /**
+ * Looks up functions using PATH-based resolution.
+ * Searches through the resolution path (session then builtin) with view context handling.
+ *
+ * @param name The function name (unqualified).
+ * @param registry The registry to search (FunctionRegistry or TableFunctionRegistry).
+ * @param checkBuiltinOperators Whether to check built-in operators first (scalar functions only).
+ * @tparam T The registry's type parameter (Expression for FunctionRegistry,
+ * LogicalPlan for TableFunctionRegistry).
+ * @return ExpressionInfo if function found, None otherwise.
+ */
+ private def lookupFunctionWithShadowing[T](
+ name: String,
+ registry: FunctionRegistryBase[T],
+ checkBuiltinOperators: Boolean): Option[ExpressionInfo] = {
+
+ // Check built-in operators first (only for scalar functions).
+ val operatorResult = if (checkBuiltinOperators) {
+ FunctionRegistry.builtinOperators.get(name.toLowerCase(Locale.ROOT))
+ } else {
+ None
+ }
+
+ operatorResult.orElse {
+ // Use PATH-based resolution: iterate through namespaces until a match is found.
+ val path = resolutionPath()
+
+ // Use iterator for short-circuit evaluation (stops at first match).
+ path.iterator.flatMap { namespace =>
+ lookupInNamespace(namespace, name, registry)
+ }.nextOption()
+ }
+ }
+
+ /**
+ * Look up the `ExpressionInfo` of the given function by name.
+ * Resolution order follows the configured path (e.g. builtin then session);
+ * extension functions are stored in the builtin namespace.
* This only supports scalar functions.
*/
def lookupBuiltinOrTempFunction(name: String): Option[ExpressionInfo] = {
- FunctionRegistry.builtinOperators.get(name.toLowerCase(Locale.ROOT)).orElse {
- synchronized(lookupTempFuncWithViewContext(
- name, FunctionRegistry.builtin.functionExists, functionRegistry.lookupFunction))
+ lookupFunctionWithShadowing(name, functionRegistry, checkBuiltinOperators = true)
+ }
+
+ /**
+ * Look up the `ExpressionInfo` of the given function by name.
+ * Resolution order follows the configured path (e.g. builtin then session);
+ * extension functions are stored in the builtin namespace.
+ */
+ def lookupBuiltinOrTempTableFunction(name: String): Option[ExpressionInfo] = {
+ lookupFunctionWithShadowing(name, tableFunctionRegistry, checkBuiltinOperators = false)
+ }
+
+ /**
+ * Look up only builtin function (no temp).
+ */
+ def lookupBuiltinFunction(name: String): Option[ExpressionInfo] = {
+ val builtinIdentifier = FunctionIdentifier(format(name))
+ functionRegistry.lookupFunction(builtinIdentifier)
+ }
+
+ /**
+ * Look up only builtin table function (no temp).
+ */
+ def lookupBuiltinTableFunction(name: String): Option[ExpressionInfo] = {
+ val builtinIdentifier = FunctionIdentifier(format(name))
+ tableFunctionRegistry.lookupFunction(builtinIdentifier)
+ }
+
+ /**
+ * Look up only temp function (no builtin).
+ */
+ def lookupTempFunction(name: String): Option[ExpressionInfo] = {
+ val tempIdentifier = tempFunctionIdentifier(name)
+ synchronized(lookupTempFuncWithViewContext(
+ name,
+ // Return false if temp exists (not builtin)
+ ident => !functionRegistry.functionExists(tempIdentifier),
Review Comment:
This is confusing. The parameter is for `isBuiltIn`
--
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-54807][SQL] Allow qualified names for built-in and session functions [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #53570:
URL: https://github.com/apache/spark/pull/53570#discussion_r2870441275
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/SessionCatalog.scala:
##########
@@ -84,6 +94,224 @@ class SessionCatalog(
import SessionCatalog._
import CatalogTypes.TablePartitionSpec
+ // Marker database name for temporary functions to distinguish from builtin functions
+ /**
+ * Database qualifier used to store temporary functions in the function registry.
+ * Temporary functions use composite keys to coexist with builtin functions of the same name:
+ * - Builtin functions: FunctionIdentifier(name, None)
Review Comment:
```suggestion
* - Builtin functions: FunctionIdentifier(name, None, None)
```
--
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-54807][SQL] Allow qualified names for built-in and session functions [spark]
Posted by "srielau (via GitHub)" <gi...@apache.org>.
srielau commented on code in PR #53570:
URL: https://github.com/apache/spark/pull/53570#discussion_r2867698470
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/resolver/ResolverGuard.scala:
##########
@@ -345,14 +346,56 @@ class ResolverGuard(catalogManager: CatalogManager) extends SQLConfHelper {
createNamedStruct.children.forall(checkExpression)
}
- private def checkUnresolvedFunction(unresolvedFunction: UnresolvedFunction) =
- unresolvedFunction.nameParts.size == 1 &&
- !ResolverGuard.UNSUPPORTED_FUNCTION_NAMES.contains(unresolvedFunction.nameParts.head) &&
- // UDFs are not supported
- FunctionRegistry.functionSet.contains(
- FunctionIdentifier(unresolvedFunction.nameParts.head.toLowerCase(Locale.ROOT))
- ) &&
- unresolvedFunction.children.forall(checkExpression)
+ /**
+ * Checks if an unresolved function is supported by the single-pass analyzer.
+ *
+ * The single-pass analyzer supports built-in functions. For qualified function names,
+ * we allow them to pass through to the resolver, which will handle them appropriately.
+ * This includes:
+ * - Built-in functions: `builtin.count`, `system.builtin.count`, unqualified `count`
+ * - Session/temp functions: `session.my_func`, `system.session.my_func` (may or may not work)
+ * - Persistent functions: `catalog.db.func` (not currently supported, but not actively blocked)
+ *
+ * We only explicitly reject certain built-in functions that require special handling
+ * (lambdas, generators, etc.) via the UNSUPPORTED_FUNCTION_NAMES list.
+ *
+ * @param unresolvedFunction the unresolved function to check
+ * @return true if the function should be allowed to attempt resolution, false otherwise
+ */
+ private def checkUnresolvedFunction(unresolvedFunction: UnresolvedFunction) = {
+ val nameParts = unresolvedFunction.nameParts
+ val functionName = nameParts.last
+
+ // Only reject functions that are explicitly unsupported or not in the builtin registry
+ // for unqualified names. For qualified names, still reject unsupported function names
+ // (e.g. builtin.explode), but allow others to pass through for the resolver.
+ val shouldReject = if (nameParts.length == 1) {
+ // Unqualified: only allow if it's a known builtin (excluding unsupported ones)
+ ResolverGuard.UNSUPPORTED_FUNCTION_NAMES.contains(functionName) ||
+ !FunctionRegistry.functionSet.contains(
+ FunctionIdentifier(functionName.toLowerCase(Locale.ROOT))
+ )
+ } else {
+ // Qualified: reject if the function name is unsupported (e.g. builtin.explode);
Review Comment:
I'm narrowed it as discussed above
--
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-54807][SQL] Allow qualified names for built-in and session functions [spark]
Posted by "vladimirg-db (via GitHub)" <gi...@apache.org>.
vladimirg-db commented on code in PR #53570:
URL: https://github.com/apache/spark/pull/53570#discussion_r2675873589
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala:
##########
@@ -2077,25 +2077,25 @@ class Analyzer(
plan.resolveExpressionsWithPruning(_.containsAnyPattern(UNRESOLVED_FUNCTION)) {
case f @ UnresolvedFunction(nameParts, _, _, _, _, _, _) =>
- if (functionResolution.lookupBuiltinOrTempFunction(nameParts, Some(f)).isDefined) {
+ // Check cache first for persistent functions to avoid repeated external catalog lookups.
Review Comment:
Please reflect the changes in the `LookupFunctions` class scaladoc instead of adding the inline comments
##########
sql/core/src/test/resources/sql-tests/analyzer-results/identifier-clause-legacy.sql.out:
##########
@@ -991,41 +991,30 @@ org.apache.spark.sql.AnalysisException
-- !query
CREATE TEMPORARY FUNCTION IDENTIFIER('default.my' || 'DoubleAvg') AS 'test.org.apache.spark.sql.MyDoubleAvg'
Review Comment:
Can you please add new golden file tests specifically to test builtin/temp/persistent function name disambiguation?
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionResolution.scala:
##########
@@ -47,6 +47,40 @@ class FunctionResolution(
private val trimWarningEnabled = new AtomicBoolean(true)
+ /**
+ * Checks if a multi-part name is qualified with a specific namespace.
+ * Supports both 2-part (namespace.name) and 3-part (system.namespace.name) qualifications.
+ *
+ * @param nameParts The multi-part name to check
+ * @param namespace The namespace to check for (e.g., "builtin", "session")
+ * @return true if qualified with the given namespace
+ */
+ private def isQualifiedWithNamespace(nameParts: Seq[String], namespace: String): Boolean = {
Review Comment:
Propose to put functions in the order of their usage top-down, because it's a natural reading order (iow these private functions after the public `lookupBuiltinOrTempFunction`)
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionResolution.scala:
##########
@@ -89,17 +130,87 @@ class FunctionResolution(
}
}
+ /**
+ * Validates that a function exists and can be used in the given context.
+ * This is used by the LookupFunctions analyzer rule for early validation.
+ *
+ * @param nameParts The function name parts.
+ * @param node The UnresolvedFunction node for error reporting.
+ * @return true if the function is a builtin or temporary function, false if it's persistent.
+ */
+ def validateFunctionExistence(
+ nameParts: Seq[String],
+ node: UnresolvedFunction): Boolean = {
+
+ // Check if function exists as scalar function.
+ val existsAsScalar = lookupBuiltinOrTempFunction(nameParts, Some(node)).isDefined
+
+ if (existsAsScalar) {
+ // Function exists in scalar registry, can be used in scalar context.
+ return true // It's a builtin or temp function
+ }
+
+ // Check if function exists as table function.
+ val existsAsTable = lookupBuiltinOrTempTableFunction(nameParts).isDefined
+
+ if (existsAsTable) {
+ // Function exists ONLY in table registry - cannot be used in scalar context.
+ throw QueryCompilationErrors.notAScalarFunctionError(nameParts.mkString("."), node)
Review Comment:
The organization of this method is weird - sometimes it returns a boolean, and sometimes it throws an error. I propose to move error throwing outside.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionResolution.scala:
##########
@@ -89,17 +130,87 @@ class FunctionResolution(
}
}
+ /**
+ * Validates that a function exists and can be used in the given context.
+ * This is used by the LookupFunctions analyzer rule for early validation.
+ *
+ * @param nameParts The function name parts.
+ * @param node The UnresolvedFunction node for error reporting.
+ * @return true if the function is a builtin or temporary function, false if it's persistent.
+ */
+ def validateFunctionExistence(
Review Comment:
> true if the function is a builtin or temporary function, false if it's persistent.
Then it should be called `isBuiltinOrTemporaryFunction`
##########
sql/core/src/test/scala/org/apache/spark/sql/catalyst/analysis/FunctionQualificationSuite.scala:
##########
@@ -0,0 +1,745 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.sql.catalyst.analysis
+
+import org.apache.spark.sql.AnalysisException
+import org.apache.spark.sql.test.SharedSparkSession
+
+/**
+ * Test suite for function qualification with builtin and session namespaces.
+ * Tests qualified function references, qualified DDL, shadowing, and unified namespace resolution.
+ */
+class FunctionQualificationSuite extends SharedSparkSession {
+
+ // ==================== Qualified Function References ====================
+
+ test("unqualified builtin function works") {
Review Comment:
I think that everything tested here may be tested using a golden file. That would be easier and less code. Please convert to a golden file
--
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-54807][SQL] Allow qualified names for built-in and session functions [spark]
Posted by "gengliangwang (via GitHub)" <gi...@apache.org>.
gengliangwang commented on code in PR #53570:
URL: https://github.com/apache/spark/pull/53570#discussion_r2688989153
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/resolver/FunctionResolver.scala:
##########
@@ -172,11 +170,11 @@ class FunctionResolver(
/**
* Method used to determine whether the given function should be replaced with another one.
+ * Handles qualified function names (e.g., "count", "builtin.count", "system.builtin.count").
*/
private def isCount(unresolvedFunction: UnresolvedFunction): Boolean = {
!unresolvedFunction.isDistinct &&
- unresolvedFunction.nameParts.length == 1 &&
- unresolvedFunction.nameParts.head.toLowerCase(Locale.ROOT) == "count"
+ unresolvedFunction.nameParts.lastOption.exists(_.equalsIgnoreCase("count"))
Review Comment:
what if the input is "foo.bar.count"?
--
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-54807][SQL] Allow qualified names for built-in and session functions [spark]
Posted by "vladimirg-db (via GitHub)" <gi...@apache.org>.
vladimirg-db commented on code in PR #53570:
URL: https://github.com/apache/spark/pull/53570#discussion_r2685561930
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionRegistry.scala:
##########
@@ -78,10 +79,17 @@ trait FunctionRegistryBase[T] {
/* Create or replace a temporary function. */
final def createOrReplaceTempFunction(
name: String, builder: FunctionBuilder, source: String): Unit = {
- registerFunction(
- FunctionIdentifier(name),
- builder,
- source)
+ // Internal functions (source="internal") are NOT qualified with
+ // CatalogManager.SESSION_NAMESPACE database because they use a separate
+ // internal registry and are resolved differently
+ val identifier = if (source == "internal") {
+ FunctionIdentifier(name)
+ } else {
+ // Regular temporary functions are qualified with CatalogManager.SESSION_NAMESPACE
+ // to enable coexistence with builtin functions of the same name
+ FunctionIdentifier(name, Some(CatalogManager.SESSION_NAMESPACE))
Review Comment:
You can look for the cases where we call `nameParts.head` or `nameParts == Seq(...)`
--
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-54807][SQL] Allow qualified names for built-in and session functions [spark]
Posted by "gengliangwang (via GitHub)" <gi...@apache.org>.
gengliangwang commented on code in PR #53570:
URL: https://github.com/apache/spark/pull/53570#discussion_r2718233081
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala:
##########
@@ -2003,24 +2069,55 @@ class Analyzer(
plan.resolveExpressionsWithPruning(_.containsAnyPattern(UNRESOLVED_FUNCTION)) {
case f @ UnresolvedFunction(nameParts, _, _, _, _, _, _) =>
- if (functionResolution.lookupBuiltinOrTempFunction(nameParts, Some(f)).isDefined) {
+ // For builtin/temp functions, we can do a quick check without catalog lookup
+ val quickCheck = if (nameParts.size == 1) {
+ functionResolution.lookupBuiltinOrTempFunction(nameParts, Some(f))
+ } else if (FunctionResolution.maybeBuiltinFunctionName(nameParts) ||
+ FunctionResolution.maybeTempFunctionName(nameParts)) {
+ functionResolution.lookupBuiltinOrTempFunction(nameParts, Some(f))
+ } else {
+ None
+ }
+
+ if (quickCheck.isDefined) {
+ // It's a builtin or temp function - no need for catalog lookup or caching
f
} else {
+ // Might be a persistent function - compute full name and check cache first
val CatalogAndIdentifier(catalog, ident) =
relationResolution.expandIdentifier(nameParts)
- val fullName =
- normalizeFuncName((catalog.name +: ident.namespace :+ ident.name).toImmutableArraySeq)
+ val fullName = normalizeFuncName(
+ (catalog.name +: ident.namespace :+ ident.name).toImmutableArraySeq)
+
if (externalFunctionNameSet.contains(fullName)) {
- f
- } else if (catalog.asFunctionCatalog.functionExists(ident)) {
- externalFunctionNameSet.add(fullName)
+ // Already validated this function exists - skip lookup
f
} else {
- val catalogPath = (catalog.name() +: catalogManager.currentNamespace).mkString(".")
- throw QueryCompilationErrors.unresolvedRoutineError(
- nameParts,
- Seq("system.builtin", "system.session", catalogPath),
- f.origin)
+ // Not in cache - do full lookup to determine type
+ val functionType = functionResolution.lookupFunctionType(nameParts, Some(f))
+
+ functionType match {
+ case FunctionType.Builtin | FunctionType.Temporary =>
+ // This shouldn't happen since we checked above, but handle it
Review Comment:
If this is un-reachable, shall we add warning logs?
--
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-54807][SQL] Allow qualified names for built-in and session functions [spark]
Posted by "srielau (via GitHub)" <gi...@apache.org>.
srielau commented on PR #53570:
URL: https://github.com/apache/spark/pull/53570#issuecomment-3789984593
@gengliangwang @cloud-fan @vladimirg-db
I have addresses all comments, and also added a legacy config.
In terms of features I have forked of another virtual schema `extension`. Thsi is used exclusively fro fucntions registered as extensions.
It allows extensions to shadow builtins by default, whiel any other temporary functions (SQL or otherwise) remain in `session`, and get resolved after builtin
Under legacy more session pushed ahead of builtin. anyone relying on e.g. a scala UF shadowing a builtin can get there via config.
--
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-54807][SQL] Allow qualified names for built-in and session functions [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #53570:
URL: https://github.com/apache/spark/pull/53570#discussion_r2870441275
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/SessionCatalog.scala:
##########
@@ -84,6 +94,224 @@ class SessionCatalog(
import SessionCatalog._
import CatalogTypes.TablePartitionSpec
+ // Marker database name for temporary functions to distinguish from builtin functions
+ /**
+ * Database qualifier used to store temporary functions in the function registry.
+ * Temporary functions use composite keys to coexist with builtin functions of the same name:
+ * - Builtin functions: FunctionIdentifier(name, None)
Review Comment:
```suggestion
* - Builtin functions: FunctionIdentifier(name, None, None)
```
--
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-54807][SQL] Allow qualified names for built-in and session functions [spark]
Posted by "gengliangwang (via GitHub)" <gi...@apache.org>.
gengliangwang commented on code in PR #53570:
URL: https://github.com/apache/spark/pull/53570#discussion_r2718221228
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala:
##########
@@ -1828,23 +1879,33 @@ class Analyzer(
case _ => false
}
+ /**
+ * Checks if the given function name parts match the expected unqualified function name,
+ * regardless of whether it's qualified or not.
+ * Handles: "count", "builtin.count", "system.builtin.count", "session.count", etc.
+ */
+ private def matchesFunctionName(nameParts: Seq[String], expectedName: String): Boolean = {
+ nameParts.lastOption.exists(_.equalsIgnoreCase(expectedName))
Review Comment:
Let's also check the other name parts. Otherwise `foo.bar.count` is also valid
--
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-54807][SQL] Allow qualified names for built-in and session functions [spark]
Posted by "srielau (via GitHub)" <gi...@apache.org>.
srielau commented on code in PR #53570:
URL: https://github.com/apache/spark/pull/53570#discussion_r2720017502
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/resolver/ResolverGuard.scala:
##########
@@ -345,14 +345,42 @@ class ResolverGuard(catalogManager: CatalogManager) extends SQLConfHelper {
createNamedStruct.children.forall(checkExpression)
}
- private def checkUnresolvedFunction(unresolvedFunction: UnresolvedFunction) =
- unresolvedFunction.nameParts.size == 1 &&
- !ResolverGuard.UNSUPPORTED_FUNCTION_NAMES.contains(unresolvedFunction.nameParts.head) &&
- // UDFs are not supported
- FunctionRegistry.functionSet.contains(
- FunctionIdentifier(unresolvedFunction.nameParts.head.toLowerCase(Locale.ROOT))
- ) &&
- unresolvedFunction.children.forall(checkExpression)
+ private def checkUnresolvedFunction(unresolvedFunction: UnresolvedFunction) = {
+ val nameParts = unresolvedFunction.nameParts
+
+ // Only accept unqualified names or names explicitly qualified as builtin.
+ // Session/temporary functions are UDFs and not supported by single-pass analyzer.
+ // Persistent functions from external catalogs are also not supported.
+ val isBuiltinOrUnqualified = nameParts.length match {
+ case 1 =>
+ // Unqualified: "count" - check if it's a builtin
+ true
+ case 2 =>
+ // Two parts: must be "builtin.count"
+ nameParts.head.equalsIgnoreCase(CatalogManager.BUILTIN_NAMESPACE)
+ case 3 =>
+ // Three parts: must be "system.builtin.count"
+ nameParts(0).equalsIgnoreCase(CatalogManager.SYSTEM_CATALOG_NAME) &&
+ nameParts(1).equalsIgnoreCase(CatalogManager.BUILTIN_NAMESPACE)
+ case _ =>
+ // More than 3 parts is not valid
+ false
+ }
+
+ if (!isBuiltinOrUnqualified) {
+ // This is session.func, catalog.db.function, or invalid - not supported
+ false
Review Comment:
Yeah, that was FUD. Fixing it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
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-54807][SQL] Allow qualified names for built-in and session functions [spark]
Posted by "srielau (via GitHub)" <gi...@apache.org>.
srielau commented on PR #53570:
URL: https://github.com/apache/spark/pull/53570#issuecomment-3977499657
@cloud-fan
[design-function-qualification-waist.md](https://github.com/user-attachments/files/25632086/design-function-qualification-waist.md)
--
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-54807][SQL] Allow qualified names for built-in and session functions [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #53570:
URL: https://github.com/apache/spark/pull/53570#discussion_r2870463094
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/SessionCatalog.scala:
##########
@@ -84,6 +94,224 @@ class SessionCatalog(
import SessionCatalog._
import CatalogTypes.TablePartitionSpec
+ // Marker database name for temporary functions to distinguish from builtin functions
+ /**
+ * Database qualifier used to store temporary functions in the function registry.
+ * Temporary functions use composite keys to coexist with builtin functions of the same name:
+ * - Builtin functions: FunctionIdentifier(name, None)
+ * - Extension functions: FunctionIdentifier(name, None, None) (Treated as builtin)
+ * - Temp functions: FunctionIdentifier(name, Some(CatalogManager.SESSION_NAMESPACE))
+ * This allows all three to exist in the same registry without conflicts.
+ */
+ private val TEMP_FUNCTION_DB = CatalogManager.SESSION_NAMESPACE
+ private val EXTENSION_FUNCTION_DB = CatalogManager.EXTENSION_NAMESPACE
+
+ /**
+ * Creates a FunctionIdentifier for an extension function.
+ * Extension functions are treated as builtins and stored unqualified.
+ * This allows them to overwrite existing builtins if registered by power users.
+ *
+ * @param name The function name (unqualified)
+ * @return FunctionIdentifier without qualification (same as builtins)
+ */
+ private def extensionFunctionIdentifier(name: String): FunctionIdentifier =
+ FunctionIdentifier(format(name))
+
+ /**
+ * Creates a FunctionIdentifier for a temporary function with the TEMP_FUNCTION_DB database
+ * qualifier. This enables temporary functions to coexist with builtin functions of the same name.
+ *
+ * @param name The function name (unqualified)
+ * @return FunctionIdentifier with database = TEMP_FUNCTION_DB
+ */
+ private def tempFunctionIdentifier(name: String): FunctionIdentifier =
+ FunctionIdentifier(
+ format(name),
+ Some(TEMP_FUNCTION_DB),
+ Some(CatalogManager.SYSTEM_CATALOG_NAME))
+
+ /**
+ * Checks if a FunctionIdentifier represents an extension function by checking for the
+ * EXTENSION_FUNCTION_DB database qualifier.
+ *
+ * @param identifier The FunctionIdentifier to check
+ * @return true if this is an extension function identifier
+ */
+ private def isExtensionFunctionIdentifier(identifier: FunctionIdentifier): Boolean =
+ identifier.database.contains(EXTENSION_FUNCTION_DB)
+
+ /**
+ * Checks if a FunctionIdentifier represents a temporary function by checking for the
+ * TEMP_FUNCTION_DB database qualifier.
+ *
+ * @param identifier The FunctionIdentifier to check
+ * @return true if this is a temporary function identifier
+ */
+ private def isTempFunctionIdentifier(identifier: FunctionIdentifier): Boolean =
+ identifier.database.contains(TEMP_FUNCTION_DB)
+
+ // --------------------------------
+ // | PATH-Based Resolution |
+ // --------------------------------
+ // Functions are resolved by searching through an ordered PATH of namespaces.
+ // This provides a unified, data-driven resolution mechanism instead of hardcoded checks.
+
+ /**
+ * Function resolution PATH - ordered list of namespaces to search for unqualified functions.
+ * Each entry is a FunctionIdentifier representing a namespace (catalog + database).
+ * The funcName field is unused (empty string) as these represent namespace templates.
+ *
+ * Resolution order follows the configured path: builtin then session (default "second"),
+ * or session then builtin (legacy "first"), or builtin only ("last" - session tried after
+ * persistent in FunctionResolution). Extension functions are stored and resolved in the
+ * builtin namespace; they are not a separate path step.
+ *
+ * When resolving a view, the system.session namespace is included in the path, but
+ * handleViewContext filters to only return temporary functions that were referred to
+ * when the view was created.
+ *
+ * These identifiers are cached for performance since they're accessed frequently.
+ */
+ private val SESSION_NAMESPACE_TEMPLATE = FunctionIdentifier(
+ funcName = "",
+ database = Some(CatalogManager.SESSION_NAMESPACE),
+ catalog = Some(CatalogManager.SYSTEM_CATALOG_NAME))
+
+ private val BUILTIN_NAMESPACE_TEMPLATE = FunctionIdentifier(
+ funcName = "",
+ database = Some(CatalogManager.BUILTIN_NAMESPACE),
+ catalog = Some(CatalogManager.SYSTEM_CATALOG_NAME))
+
+ // The resolution path: builtin -> session (session "second")
+ private val RESOLUTION_PATH = Seq(
+ BUILTIN_NAMESPACE_TEMPLATE,
+ SESSION_NAMESPACE_TEMPLATE
+ )
+
+ // Legacy resolution path: session -> builtin (session "first")
+ private val LEGACY_RESOLUTION_PATH = Seq(
+ SESSION_NAMESPACE_TEMPLATE,
+ BUILTIN_NAMESPACE_TEMPLATE
+ )
+
+ // Session last: only builtin (session tried after persistent in FunctionResolution)
+ private val SESSION_LAST_RESOLUTION_PATH = Seq(BUILTIN_NAMESPACE_TEMPLATE)
+
+ /**
+ * Returns the resolution path for function lookup.
+ * @return Ordered sequence of namespace identifiers.
+ */
+ private def resolutionPath(): Seq[FunctionIdentifier] = {
+ conf.sessionFunctionResolutionOrder match {
+ case "first" => LEGACY_RESOLUTION_PATH
+ case "last" => SESSION_LAST_RESOLUTION_PATH
+ case _ => RESOLUTION_PATH // "second" (default)
+ }
+ }
+
+ /**
+ * Maps a namespace template to an actual storage identifier for a specific function.
+ * This handles the asymmetry between how builtins, extensions, and temp functions are stored.
+ *
+ * Storage conventions:
+ * - Builtin functions: FunctionIdentifier(name, None, None)
+ * - Extension functions: FunctionIdentifier(name, None, None) (Treated as builtin)
+ * - Temp functions: FunctionIdentifier(name, Some("session"), Some("system"))
+ * - Other: FunctionIdentifier(name, namespace.database, namespace.catalog)
+ *
+ * @param namespace The namespace template
+ * @param name The function name
+ * @return The actual identifier to use for registry lookup
+ */
+ private def namespaceToIdentifier(
+ namespace: FunctionIdentifier,
Review Comment:
```suggestion
template: FunctionIdentifier,
```
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/SessionCatalog.scala:
##########
@@ -84,6 +94,224 @@ class SessionCatalog(
import SessionCatalog._
import CatalogTypes.TablePartitionSpec
+ // Marker database name for temporary functions to distinguish from builtin functions
+ /**
+ * Database qualifier used to store temporary functions in the function registry.
+ * Temporary functions use composite keys to coexist with builtin functions of the same name:
+ * - Builtin functions: FunctionIdentifier(name, None)
+ * - Extension functions: FunctionIdentifier(name, None, None) (Treated as builtin)
+ * - Temp functions: FunctionIdentifier(name, Some(CatalogManager.SESSION_NAMESPACE))
+ * This allows all three to exist in the same registry without conflicts.
+ */
+ private val TEMP_FUNCTION_DB = CatalogManager.SESSION_NAMESPACE
+ private val EXTENSION_FUNCTION_DB = CatalogManager.EXTENSION_NAMESPACE
+
+ /**
+ * Creates a FunctionIdentifier for an extension function.
+ * Extension functions are treated as builtins and stored unqualified.
+ * This allows them to overwrite existing builtins if registered by power users.
+ *
+ * @param name The function name (unqualified)
+ * @return FunctionIdentifier without qualification (same as builtins)
+ */
+ private def extensionFunctionIdentifier(name: String): FunctionIdentifier =
+ FunctionIdentifier(format(name))
+
+ /**
+ * Creates a FunctionIdentifier for a temporary function with the TEMP_FUNCTION_DB database
+ * qualifier. This enables temporary functions to coexist with builtin functions of the same name.
+ *
+ * @param name The function name (unqualified)
+ * @return FunctionIdentifier with database = TEMP_FUNCTION_DB
+ */
+ private def tempFunctionIdentifier(name: String): FunctionIdentifier =
+ FunctionIdentifier(
+ format(name),
+ Some(TEMP_FUNCTION_DB),
+ Some(CatalogManager.SYSTEM_CATALOG_NAME))
+
+ /**
+ * Checks if a FunctionIdentifier represents an extension function by checking for the
+ * EXTENSION_FUNCTION_DB database qualifier.
+ *
+ * @param identifier The FunctionIdentifier to check
+ * @return true if this is an extension function identifier
+ */
+ private def isExtensionFunctionIdentifier(identifier: FunctionIdentifier): Boolean =
+ identifier.database.contains(EXTENSION_FUNCTION_DB)
+
+ /**
+ * Checks if a FunctionIdentifier represents a temporary function by checking for the
+ * TEMP_FUNCTION_DB database qualifier.
+ *
+ * @param identifier The FunctionIdentifier to check
+ * @return true if this is a temporary function identifier
+ */
+ private def isTempFunctionIdentifier(identifier: FunctionIdentifier): Boolean =
+ identifier.database.contains(TEMP_FUNCTION_DB)
+
+ // --------------------------------
+ // | PATH-Based Resolution |
+ // --------------------------------
+ // Functions are resolved by searching through an ordered PATH of namespaces.
+ // This provides a unified, data-driven resolution mechanism instead of hardcoded checks.
+
+ /**
+ * Function resolution PATH - ordered list of namespaces to search for unqualified functions.
+ * Each entry is a FunctionIdentifier representing a namespace (catalog + database).
+ * The funcName field is unused (empty string) as these represent namespace templates.
+ *
+ * Resolution order follows the configured path: builtin then session (default "second"),
+ * or session then builtin (legacy "first"), or builtin only ("last" - session tried after
+ * persistent in FunctionResolution). Extension functions are stored and resolved in the
+ * builtin namespace; they are not a separate path step.
+ *
+ * When resolving a view, the system.session namespace is included in the path, but
+ * handleViewContext filters to only return temporary functions that were referred to
+ * when the view was created.
+ *
+ * These identifiers are cached for performance since they're accessed frequently.
+ */
+ private val SESSION_NAMESPACE_TEMPLATE = FunctionIdentifier(
+ funcName = "",
+ database = Some(CatalogManager.SESSION_NAMESPACE),
+ catalog = Some(CatalogManager.SYSTEM_CATALOG_NAME))
+
+ private val BUILTIN_NAMESPACE_TEMPLATE = FunctionIdentifier(
+ funcName = "",
+ database = Some(CatalogManager.BUILTIN_NAMESPACE),
+ catalog = Some(CatalogManager.SYSTEM_CATALOG_NAME))
+
+ // The resolution path: builtin -> session (session "second")
+ private val RESOLUTION_PATH = Seq(
+ BUILTIN_NAMESPACE_TEMPLATE,
+ SESSION_NAMESPACE_TEMPLATE
+ )
+
+ // Legacy resolution path: session -> builtin (session "first")
+ private val LEGACY_RESOLUTION_PATH = Seq(
+ SESSION_NAMESPACE_TEMPLATE,
+ BUILTIN_NAMESPACE_TEMPLATE
+ )
+
+ // Session last: only builtin (session tried after persistent in FunctionResolution)
+ private val SESSION_LAST_RESOLUTION_PATH = Seq(BUILTIN_NAMESPACE_TEMPLATE)
+
+ /**
+ * Returns the resolution path for function lookup.
+ * @return Ordered sequence of namespace identifiers.
+ */
+ private def resolutionPath(): Seq[FunctionIdentifier] = {
+ conf.sessionFunctionResolutionOrder match {
+ case "first" => LEGACY_RESOLUTION_PATH
+ case "last" => SESSION_LAST_RESOLUTION_PATH
+ case _ => RESOLUTION_PATH // "second" (default)
+ }
+ }
+
+ /**
+ * Maps a namespace template to an actual storage identifier for a specific function.
+ * This handles the asymmetry between how builtins, extensions, and temp functions are stored.
+ *
+ * Storage conventions:
+ * - Builtin functions: FunctionIdentifier(name, None, None)
+ * - Extension functions: FunctionIdentifier(name, None, None) (Treated as builtin)
+ * - Temp functions: FunctionIdentifier(name, Some("session"), Some("system"))
+ * - Other: FunctionIdentifier(name, namespace.database, namespace.catalog)
+ *
+ * @param namespace The namespace template
+ * @param name The function name
+ * @return The actual identifier to use for registry lookup
+ */
+ private def namespaceToIdentifier(
+ namespace: FunctionIdentifier,
+ name: String): FunctionIdentifier = {
+ namespace.database match {
+ case Some(CatalogManager.SESSION_NAMESPACE) =>
Review Comment:
why this needs a special case? Exception for builtin functions, all others are just `namespace.copy(name = 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-54807][SQL] Allow qualified names for built-in and session functions [spark]
Posted by "gengliangwang (via GitHub)" <gi...@apache.org>.
gengliangwang commented on PR #53570:
URL: https://github.com/apache/spark/pull/53570#issuecomment-3986894569
I will merge this one once the CI passed. Thanks for the works @srielau
--
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-54807][SQL] Allow qualified names for built-in and session functions [spark]
Posted by "vladimirg-db (via GitHub)" <gi...@apache.org>.
vladimirg-db commented on code in PR #53570:
URL: https://github.com/apache/spark/pull/53570#discussion_r2685544675
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionRegistry.scala:
##########
@@ -78,10 +79,17 @@ trait FunctionRegistryBase[T] {
/* Create or replace a temporary function. */
final def createOrReplaceTempFunction(
name: String, builder: FunctionBuilder, source: String): Unit = {
- registerFunction(
- FunctionIdentifier(name),
- builder,
- source)
+ // Internal functions (source="internal") are NOT qualified with
+ // CatalogManager.SESSION_NAMESPACE database because they use a separate
+ // internal registry and are resolved differently
+ val identifier = if (source == "internal") {
+ FunctionIdentifier(name)
+ } else {
+ // Regular temporary functions are qualified with CatalogManager.SESSION_NAMESPACE
+ // to enable coexistence with builtin functions of the same name
+ FunctionIdentifier(name, Some(CatalogManager.SESSION_NAMESPACE))
Review Comment:
There is a number of places where the Analyzer has special logic for specific functions. These places expect that `UnresolvedFunction` to have a single-part name. Here's one example:
<img width="748" height="207" alt="image" src="https://github.com/user-attachments/assets/9eb57bb3-45fe-4e67-bb32-c5a1d9cef363" />
We need to find all those places and update them to handle cases when customers reference functions by fully-qualified names.
--
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-54807][SQL] Allow qualified names for built-in and session functions [spark]
Posted by "srielau (via GitHub)" <gi...@apache.org>.
srielau commented on code in PR #53570:
URL: https://github.com/apache/spark/pull/53570#discussion_r2688848674
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionRegistry.scala:
##########
@@ -78,10 +79,17 @@ trait FunctionRegistryBase[T] {
/* Create or replace a temporary function. */
final def createOrReplaceTempFunction(
name: String, builder: FunctionBuilder, source: String): Unit = {
- registerFunction(
- FunctionIdentifier(name),
- builder,
- source)
+ // Internal functions (source="internal") are NOT qualified with
+ // CatalogManager.SESSION_NAMESPACE database because they use a separate
+ // internal registry and are resolved differently
+ val identifier = if (source == "internal") {
+ FunctionIdentifier(name)
+ } else {
+ // Regular temporary functions are qualified with CatalogManager.SESSION_NAMESPACE
+ // to enable coexistence with builtin functions of the same name
+ FunctionIdentifier(name, Some(CatalogManager.SESSION_NAMESPACE))
Review Comment:
OK, I think I got these all fixed. What I did NOT fix are the functions in the form that require grammar rules.
(e.g. EXTRACT). These forms doe not actually accepty identifiers. so `extract`(month from current_date()) is not allowed to begin with.
Fixing that (if desired) is an orthogonal problem.
--
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-54807][SQL] Allow qualified names for built-in and session functions [spark]
Posted by "srielau (via GitHub)" <gi...@apache.org>.
srielau commented on code in PR #53570:
URL: https://github.com/apache/spark/pull/53570#discussion_r2867573891
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/resolver/ResolverGuard.scala:
##########
@@ -345,14 +346,56 @@ class ResolverGuard(catalogManager: CatalogManager) extends SQLConfHelper {
createNamedStruct.children.forall(checkExpression)
}
- private def checkUnresolvedFunction(unresolvedFunction: UnresolvedFunction) =
- unresolvedFunction.nameParts.size == 1 &&
- !ResolverGuard.UNSUPPORTED_FUNCTION_NAMES.contains(unresolvedFunction.nameParts.head) &&
- // UDFs are not supported
- FunctionRegistry.functionSet.contains(
- FunctionIdentifier(unresolvedFunction.nameParts.head.toLowerCase(Locale.ROOT))
- ) &&
- unresolvedFunction.children.forall(checkExpression)
+ /**
+ * Checks if an unresolved function is supported by the single-pass analyzer.
+ *
+ * The single-pass analyzer supports built-in functions. For qualified function names,
+ * we allow them to pass through to the resolver, which will handle them appropriately.
+ * This includes:
+ * - Built-in functions: `builtin.count`, `system.builtin.count`, unqualified `count`
+ * - Session/temp functions: `session.my_func`, `system.session.my_func` (may or may not work)
+ * - Persistent functions: `catalog.db.func` (not currently supported, but not actively blocked)
+ *
+ * We only explicitly reject certain built-in functions that require special handling
+ * (lambdas, generators, etc.) via the UNSUPPORTED_FUNCTION_NAMES list.
+ *
+ * @param unresolvedFunction the unresolved function to check
+ * @return true if the function should be allowed to attempt resolution, false otherwise
+ */
+ private def checkUnresolvedFunction(unresolvedFunction: UnresolvedFunction) = {
+ val nameParts = unresolvedFunction.nameParts
+ val functionName = nameParts.last
+
+ // Only reject functions that are explicitly unsupported or not in the builtin registry
+ // for unqualified names. For qualified names, still reject unsupported function names
+ // (e.g. builtin.explode), but allow others to pass through for the resolver.
+ val shouldReject = if (nameParts.length == 1) {
+ // Unqualified: only allow if it's a known builtin (excluding unsupported ones)
+ ResolverGuard.UNSUPPORTED_FUNCTION_NAMES.contains(functionName) ||
+ !FunctionRegistry.functionSet.contains(
+ FunctionIdentifier(functionName.toLowerCase(Locale.ROOT))
+ )
+ } else {
+ // Qualified: reject if the function name is unsupported (e.g. builtin.explode);
Review Comment:
This is the do-no-harm solution. It's how it works today.
Really, resolver guard logic should not kick in until after resolution, but I think that shoudl be thought through by the experts in single pass analyzer.
I could explicitly limit to unqualified and builtin. But that is still a partial solution because unqualified in the money case.
--
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-54807][SQL] Allow qualified names for built-in and session functions [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #53570:
URL: https://github.com/apache/spark/pull/53570#discussion_r2870463094
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/SessionCatalog.scala:
##########
@@ -84,6 +94,224 @@ class SessionCatalog(
import SessionCatalog._
import CatalogTypes.TablePartitionSpec
+ // Marker database name for temporary functions to distinguish from builtin functions
+ /**
+ * Database qualifier used to store temporary functions in the function registry.
+ * Temporary functions use composite keys to coexist with builtin functions of the same name:
+ * - Builtin functions: FunctionIdentifier(name, None)
+ * - Extension functions: FunctionIdentifier(name, None, None) (Treated as builtin)
+ * - Temp functions: FunctionIdentifier(name, Some(CatalogManager.SESSION_NAMESPACE))
+ * This allows all three to exist in the same registry without conflicts.
+ */
+ private val TEMP_FUNCTION_DB = CatalogManager.SESSION_NAMESPACE
+ private val EXTENSION_FUNCTION_DB = CatalogManager.EXTENSION_NAMESPACE
+
+ /**
+ * Creates a FunctionIdentifier for an extension function.
+ * Extension functions are treated as builtins and stored unqualified.
+ * This allows them to overwrite existing builtins if registered by power users.
+ *
+ * @param name The function name (unqualified)
+ * @return FunctionIdentifier without qualification (same as builtins)
+ */
+ private def extensionFunctionIdentifier(name: String): FunctionIdentifier =
+ FunctionIdentifier(format(name))
+
+ /**
+ * Creates a FunctionIdentifier for a temporary function with the TEMP_FUNCTION_DB database
+ * qualifier. This enables temporary functions to coexist with builtin functions of the same name.
+ *
+ * @param name The function name (unqualified)
+ * @return FunctionIdentifier with database = TEMP_FUNCTION_DB
+ */
+ private def tempFunctionIdentifier(name: String): FunctionIdentifier =
+ FunctionIdentifier(
+ format(name),
+ Some(TEMP_FUNCTION_DB),
+ Some(CatalogManager.SYSTEM_CATALOG_NAME))
+
+ /**
+ * Checks if a FunctionIdentifier represents an extension function by checking for the
+ * EXTENSION_FUNCTION_DB database qualifier.
+ *
+ * @param identifier The FunctionIdentifier to check
+ * @return true if this is an extension function identifier
+ */
+ private def isExtensionFunctionIdentifier(identifier: FunctionIdentifier): Boolean =
+ identifier.database.contains(EXTENSION_FUNCTION_DB)
+
+ /**
+ * Checks if a FunctionIdentifier represents a temporary function by checking for the
+ * TEMP_FUNCTION_DB database qualifier.
+ *
+ * @param identifier The FunctionIdentifier to check
+ * @return true if this is a temporary function identifier
+ */
+ private def isTempFunctionIdentifier(identifier: FunctionIdentifier): Boolean =
+ identifier.database.contains(TEMP_FUNCTION_DB)
+
+ // --------------------------------
+ // | PATH-Based Resolution |
+ // --------------------------------
+ // Functions are resolved by searching through an ordered PATH of namespaces.
+ // This provides a unified, data-driven resolution mechanism instead of hardcoded checks.
+
+ /**
+ * Function resolution PATH - ordered list of namespaces to search for unqualified functions.
+ * Each entry is a FunctionIdentifier representing a namespace (catalog + database).
+ * The funcName field is unused (empty string) as these represent namespace templates.
+ *
+ * Resolution order follows the configured path: builtin then session (default "second"),
+ * or session then builtin (legacy "first"), or builtin only ("last" - session tried after
+ * persistent in FunctionResolution). Extension functions are stored and resolved in the
+ * builtin namespace; they are not a separate path step.
+ *
+ * When resolving a view, the system.session namespace is included in the path, but
+ * handleViewContext filters to only return temporary functions that were referred to
+ * when the view was created.
+ *
+ * These identifiers are cached for performance since they're accessed frequently.
+ */
+ private val SESSION_NAMESPACE_TEMPLATE = FunctionIdentifier(
+ funcName = "",
+ database = Some(CatalogManager.SESSION_NAMESPACE),
+ catalog = Some(CatalogManager.SYSTEM_CATALOG_NAME))
+
+ private val BUILTIN_NAMESPACE_TEMPLATE = FunctionIdentifier(
+ funcName = "",
+ database = Some(CatalogManager.BUILTIN_NAMESPACE),
+ catalog = Some(CatalogManager.SYSTEM_CATALOG_NAME))
+
+ // The resolution path: builtin -> session (session "second")
+ private val RESOLUTION_PATH = Seq(
+ BUILTIN_NAMESPACE_TEMPLATE,
+ SESSION_NAMESPACE_TEMPLATE
+ )
+
+ // Legacy resolution path: session -> builtin (session "first")
+ private val LEGACY_RESOLUTION_PATH = Seq(
+ SESSION_NAMESPACE_TEMPLATE,
+ BUILTIN_NAMESPACE_TEMPLATE
+ )
+
+ // Session last: only builtin (session tried after persistent in FunctionResolution)
+ private val SESSION_LAST_RESOLUTION_PATH = Seq(BUILTIN_NAMESPACE_TEMPLATE)
+
+ /**
+ * Returns the resolution path for function lookup.
+ * @return Ordered sequence of namespace identifiers.
+ */
+ private def resolutionPath(): Seq[FunctionIdentifier] = {
+ conf.sessionFunctionResolutionOrder match {
+ case "first" => LEGACY_RESOLUTION_PATH
+ case "last" => SESSION_LAST_RESOLUTION_PATH
+ case _ => RESOLUTION_PATH // "second" (default)
+ }
+ }
+
+ /**
+ * Maps a namespace template to an actual storage identifier for a specific function.
+ * This handles the asymmetry between how builtins, extensions, and temp functions are stored.
+ *
+ * Storage conventions:
+ * - Builtin functions: FunctionIdentifier(name, None, None)
+ * - Extension functions: FunctionIdentifier(name, None, None) (Treated as builtin)
+ * - Temp functions: FunctionIdentifier(name, Some("session"), Some("system"))
+ * - Other: FunctionIdentifier(name, namespace.database, namespace.catalog)
+ *
+ * @param namespace The namespace template
+ * @param name The function name
+ * @return The actual identifier to use for registry lookup
+ */
+ private def namespaceToIdentifier(
+ namespace: FunctionIdentifier,
Review Comment:
```suggestion
template: FunctionIdentifier,
```
--
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-54807][SQL] Allow qualified names for built-in and session functions [spark]
Posted by "vladimirg-db (via GitHub)" <gi...@apache.org>.
vladimirg-db commented on code in PR #53570:
URL: https://github.com/apache/spark/pull/53570#discussion_r2873197074
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/resolver/ResolverGuard.scala:
##########
@@ -345,14 +347,46 @@ class ResolverGuard(catalogManager: CatalogManager) extends SQLConfHelper {
createNamedStruct.children.forall(checkExpression)
}
- private def checkUnresolvedFunction(unresolvedFunction: UnresolvedFunction) =
- unresolvedFunction.nameParts.size == 1 &&
- !ResolverGuard.UNSUPPORTED_FUNCTION_NAMES.contains(unresolvedFunction.nameParts.head) &&
- // UDFs are not supported
- FunctionRegistry.functionSet.contains(
- FunctionIdentifier(unresolvedFunction.nameParts.head.toLowerCase(Locale.ROOT))
- ) &&
- unresolvedFunction.children.forall(checkExpression)
+ /**
+ * Checks if an unresolved function is supported by the single-pass analyzer.
+ *
+ * The single-pass analyzer supports built-in functions. For qualified function names,
+ * we allow them to pass through to the resolver, which will handle them appropriately.
+ * This includes:
+ * - Built-in functions: `builtin.count`, `system.builtin.count`, unqualified `count`
+ * - Session/temp functions: `session.my_func`, `system.session.my_func` (may or may not work)
+ * - Persistent functions: `catalog.db.func` (not currently supported, but not actively blocked)
+ *
+ * We only explicitly reject certain built-in functions that require special handling
+ * (lambdas, generators, etc.) via the UNSUPPORTED_FUNCTION_NAMES list.
+ *
+ * @param unresolvedFunction the unresolved function to check
+ * @return true if the function should be allowed to attempt resolution, false otherwise
+ */
+ private def checkUnresolvedFunction(unresolvedFunction: UnresolvedFunction) = {
+ val nameParts = unresolvedFunction.nameParts
+ val functionName = nameParts.last
+
+ // Reject only when we know it's the builtin: unqualified or explicitly builtin-qualified
+ // (system.builtin.func or builtin.func). Any other qualification (session, catalog.db) we let
+ // through since it's not a forbidden builtin.
+ val shouldReject = if (nameParts.length == 1) {
+ // Unqualified: only allow if it's a known builtin (excluding unsupported ones)
+ ResolverGuard.UNSUPPORTED_FUNCTION_NAMES.contains(functionName) ||
+ !FunctionRegistry.functionSet.contains(
+ FunctionIdentifier(functionName.toLowerCase(Locale.ROOT))
+ )
+ } else if (FunctionResolution.sessionNamespaceKind(nameParts)
+ .contains(SessionCatalog.Builtin)) {
+ // Explicitly builtin-qualified (builtin.func, system.builtin.func): reject if unsupported
+ ResolverGuard.UNSUPPORTED_FUNCTION_NAMES.contains(functionName)
+ } else {
+ // Other qualification (session, catalog.db, etc.): allow through
+ false
Review Comment:
@srielau we should return `true` here, as UDFs are not supported yet
--
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-54807][SQL] Allow qualified names for built-in and session functions [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #53570:
URL: https://github.com/apache/spark/pull/53570#discussion_r2867246316
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala:
##########
@@ -1823,23 +1823,34 @@ class Analyzer(
case _ => false
}
+ /**
+ * Checks if the given function name parts match the expected builtin function name.
+ * This is used for special syntax transformations (e.g., COUNT(*) -> COUNT(1)) that
+ * should only apply to builtin functions, not to user-defined functions.
+ */
+ private def matchesFunctionName(nameParts: Seq[String], expectedName: String): Boolean = {
+ FunctionResolution.isUnqualifiedOrBuiltinFunctionName(nameParts, expectedName)
+ }
+
/**
* Expands the matching attribute.*'s in `child`'s output.
*/
def expandStarExpression(expr: Expression, child: LogicalPlan): Expression = {
expr.transformUp {
case f0: UnresolvedFunction if !f0.isDistinct &&
- f0.nameParts.map(_.toLowerCase(Locale.ROOT)) == Seq("count") &&
+ matchesFunctionName(f0.nameParts, "count") &&
isCountStarExpansionAllowed(f0.arguments) =>
// Transform COUNT(*) into COUNT(1).
- f0.copy(nameParts = Seq("count"), arguments = Seq(Literal(1)))
Review Comment:
not related to your PR, but why did we normalize the name "count" 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-54807][SQL] Allow qualified names for built-in and session functions [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #53570:
URL: https://github.com/apache/spark/pull/53570#issuecomment-3982002613
I don't think we need `EXTENSION_NAMESPACE`. The purpose of `SparkSessionExtensions` is to allow power users to overwrite built-in functions — they register functions with a `FunctionIdentifier` they fully control, and those functions simply replace the builtin at that key. There's no need for a separate namespace to distinguish them.
A few concrete issues with the current `Extension` kind:
1. **`extensionFunctionIdentifier(name)` produces `FunctionIdentifier(format(name))` — identical to builtins.** Extensions are stored at the exact same key as builtins, so the `Extension` kind has no storage-level distinction.
2. **`SparkSessionExtensions.registerFunctions` allows users to provide arbitrary `FunctionIdentifier` with database/catalog.** The PR silently strips qualification for unqualified extensions and keeps it as-is for qualified ones, but qualified extensions can never be reached via `extension.func` or `system.extension.func` since those only look up unqualified keys. The abstraction is incomplete.
3. **Every code path treats `Extension` and `Builtin` identically** — `resolveScalarFunction`, `resolveTableFunction`, `lookupFunctionType`, `ResolveCatalogs`, etc. all collapse them together. This adds branches and complexity with no behavioral difference.
Suggestion: remove `Extension` from `SessionFunctionKind`, remove `EXTENSION_NAMESPACE` from `CatalogManager`, and let extension functions continue to work as they do on master — they overwrite builtins at injection time, no special namespace needed.
---
_This comment was generated with [GitHub MCP](http://go/mcps)._
--
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-54807][SQL] Allow qualified names for built-in and session functions [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #53570:
URL: https://github.com/apache/spark/pull/53570#discussion_r2870494122
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionResolution.scala:
##########
@@ -19,51 +19,235 @@ package org.apache.spark.sql.catalyst.analysis
import java.util.concurrent.atomic.AtomicBoolean
+import scala.util.control.NonFatal
+
+import org.apache.spark.internal.Logging
import org.apache.spark.sql.AnalysisException
import org.apache.spark.sql.catalyst.FunctionIdentifier
+import org.apache.spark.sql.catalyst.analysis.FunctionRegistry
+import org.apache.spark.sql.catalyst.catalog.SessionCatalog
import org.apache.spark.sql.catalyst.expressions._
import org.apache.spark.sql.catalyst.expressions.aggregate._
import org.apache.spark.sql.catalyst.plans.logical.LogicalPlan
import org.apache.spark.sql.connector.catalog.{
CatalogManager,
+ CatalogNotFoundException,
+ CatalogV2Util,
LookupCatalog
}
+
+/**
+ * Represents the type/location of a function.
+ */
+sealed trait FunctionType
+object FunctionType {
+ /** Function is a built-in function in the builtin registry. */
+ case object Builtin extends FunctionType
+ /** Function is a temporary function in the session registry. */
+ case object Temporary extends FunctionType
+ /** Function is a persistent function in the external catalog. */
+ case object Persistent extends FunctionType
+ /** Function exists only as a table function (cannot be used in scalar context). */
+ case object TableOnly extends FunctionType
+ /** Function does not exist anywhere. */
+ case object NotFound extends FunctionType
+}
import org.apache.spark.sql.connector.catalog.CatalogV2Implicits._
import org.apache.spark.sql.connector.catalog.functions.{
AggregateFunction => V2AggregateFunction,
ScalarFunction,
UnboundFunction
}
import org.apache.spark.sql.errors.{DataTypeErrorsBase, QueryCompilationErrors}
+import org.apache.spark.sql.internal.SQLConf
import org.apache.spark.sql.internal.connector.V1Function
import org.apache.spark.sql.types._
class FunctionResolution(
override val catalogManager: CatalogManager,
relationResolution: RelationResolution)
- extends DataTypeErrorsBase with LookupCatalog {
+ extends DataTypeErrorsBase with LookupCatalog with Logging {
private val v1SessionCatalog = catalogManager.v1SessionCatalog
private val trimWarningEnabled = new AtomicBoolean(true)
- def resolveFunction(u: UnresolvedFunction): Expression = {
- withPosition(u) {
- resolveBuiltinOrTempFunction(u.nameParts, u.arguments, u).getOrElse {
+ /**
+ * Produces the ordered list of fully qualified candidate names for resolution.
+ *
+ * @param nameParts The function name parts.
+ * @return A sequence of fully qualified function names to attempt resolution with.
+ */
+ private def resolutionCandidates(nameParts: Seq[String]): Seq[Seq[String]] = {
+ if (nameParts.size == 1) {
+ val catalogPath = catalogManager.currentCatalog.name +: catalogManager.currentNamespace
+ val searchPath = SQLConf.get.resolutionSearchPath(catalogPath.toSeq)
+ 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 3 if FunctionResolution.sessionNamespaceKind(nameParts).isDefined =>
+ // Fully qualified system.builtin.func or system.session.func
+ Seq(nameParts)
+ case _ =>
+ Seq(nameParts)
+ }
+ }
+ }
+
+ private def resolveQualifiedFunction(
+ nameParts: Seq[String],
+ unresolvedFunc: UnresolvedFunction): Option[Expression] = {
+ if (nameParts.length == 3 &&
+ nameParts.head.equalsIgnoreCase(CatalogManager.SYSTEM_CATALOG_NAME)) {
+ // Try resolving as a session-namespace function (builtin, temp, extension)
+ FunctionResolution.sessionNamespaceKind(nameParts).flatMap { kind =>
+ val funcName = nameParts.last
+ val expr = v1SessionCatalog.resolveScalarFunction(kind, funcName, unresolvedFunc.arguments)
+ if (expr.isEmpty) {
+ if (v1SessionCatalog.lookupFunctionInfo(
+ SessionCatalog.Temp, funcName, tableFunction = true).isDefined) {
Review Comment:
it can be builtin 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.
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-54807][SQL] Allow qualified names for built-in and session functions [spark]
Posted by "srielau (via GitHub)" <gi...@apache.org>.
srielau commented on code in PR #53570:
URL: https://github.com/apache/spark/pull/53570#discussion_r2870583408
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionResolution.scala:
##########
@@ -19,51 +19,235 @@ package org.apache.spark.sql.catalyst.analysis
import java.util.concurrent.atomic.AtomicBoolean
+import scala.util.control.NonFatal
+
+import org.apache.spark.internal.Logging
import org.apache.spark.sql.AnalysisException
import org.apache.spark.sql.catalyst.FunctionIdentifier
+import org.apache.spark.sql.catalyst.analysis.FunctionRegistry
+import org.apache.spark.sql.catalyst.catalog.SessionCatalog
import org.apache.spark.sql.catalyst.expressions._
import org.apache.spark.sql.catalyst.expressions.aggregate._
import org.apache.spark.sql.catalyst.plans.logical.LogicalPlan
import org.apache.spark.sql.connector.catalog.{
CatalogManager,
+ CatalogNotFoundException,
+ CatalogV2Util,
LookupCatalog
}
+
+/**
+ * Represents the type/location of a function.
+ */
+sealed trait FunctionType
+object FunctionType {
+ /** Function is a built-in function in the builtin registry. */
+ case object Builtin extends FunctionType
+ /** Function is a temporary function in the session registry. */
+ case object Temporary extends FunctionType
+ /** Function is a persistent function in the external catalog. */
+ case object Persistent extends FunctionType
+ /** Function exists only as a table function (cannot be used in scalar context). */
+ case object TableOnly extends FunctionType
+ /** Function does not exist anywhere. */
+ case object NotFound extends FunctionType
+}
import org.apache.spark.sql.connector.catalog.CatalogV2Implicits._
import org.apache.spark.sql.connector.catalog.functions.{
AggregateFunction => V2AggregateFunction,
ScalarFunction,
UnboundFunction
}
import org.apache.spark.sql.errors.{DataTypeErrorsBase, QueryCompilationErrors}
+import org.apache.spark.sql.internal.SQLConf
import org.apache.spark.sql.internal.connector.V1Function
import org.apache.spark.sql.types._
class FunctionResolution(
override val catalogManager: CatalogManager,
relationResolution: RelationResolution)
- extends DataTypeErrorsBase with LookupCatalog {
+ extends DataTypeErrorsBase with LookupCatalog with Logging {
private val v1SessionCatalog = catalogManager.v1SessionCatalog
private val trimWarningEnabled = new AtomicBoolean(true)
- def resolveFunction(u: UnresolvedFunction): Expression = {
- withPosition(u) {
- resolveBuiltinOrTempFunction(u.nameParts, u.arguments, u).getOrElse {
+ /**
+ * Produces the ordered list of fully qualified candidate names for resolution.
+ *
+ * @param nameParts The function name parts.
+ * @return A sequence of fully qualified function names to attempt resolution with.
+ */
+ private def resolutionCandidates(nameParts: Seq[String]): Seq[Seq[String]] = {
+ if (nameParts.size == 1) {
+ val catalogPath = catalogManager.currentCatalog.name +: catalogManager.currentNamespace
+ val searchPath = SQLConf.get.resolutionSearchPath(catalogPath.toSeq)
+ 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 3 if FunctionResolution.sessionNamespaceKind(nameParts).isDefined =>
+ // Fully qualified system.builtin.func or system.session.func
+ Seq(nameParts)
+ case _ =>
+ Seq(nameParts)
+ }
+ }
+ }
+
+ private def resolveQualifiedFunction(
+ nameParts: Seq[String],
+ unresolvedFunc: UnresolvedFunction): Option[Expression] = {
+ if (nameParts.length == 3 &&
+ nameParts.head.equalsIgnoreCase(CatalogManager.SYSTEM_CATALOG_NAME)) {
+ // Try resolving as a session-namespace function (builtin, temp, extension)
+ FunctionResolution.sessionNamespaceKind(nameParts).flatMap { kind =>
+ val funcName = nameParts.last
+ val expr = v1SessionCatalog.resolveScalarFunction(kind, funcName, unresolvedFunc.arguments)
+ if (expr.isEmpty) {
+ if (v1SessionCatalog.lookupFunctionInfo(
+ SessionCatalog.Temp, funcName, tableFunction = true).isDefined) {
Review Comment:
simplified
--
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-54807][SQL] Allow qualified names for built-in and session functions [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #53570:
URL: https://github.com/apache/spark/pull/53570#issuecomment-3803644809
Some high-level comments:
1. `SparkSessionExtensions` allows power users to register functions with `FunctionIdentifier`, which can include database and catalog names, in addition to the function name. We have very little control here, and I don't think the `extension` namespace can be applied.
2. The PR description does not mention the design sketch, shall we give one? The function registry is keyed by `FunctionIdentifier`, which already supports functions with same names, as long as they have different qualifier. So the design sketch can be: function names are always qualified before putting into the function registry. When lookup functions, we qualify the function name differently and try multiple times, according to the priority order.
--
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-54807][SQL] Allow qualified names for built-in and session functions [spark]
Posted by "srielau (via GitHub)" <gi...@apache.org>.
srielau commented on code in PR #53570:
URL: https://github.com/apache/spark/pull/53570#discussion_r2689099543
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/command/functions.scala:
##########
@@ -139,11 +140,27 @@ case class DropFunctionCommand(
override def run(sparkSession: SparkSession): Seq[Row] = {
val catalog = sparkSession.sessionState.catalog
if (isTemp) {
- assert(identifier.database.isEmpty)
- if (FunctionRegistry.builtin.functionExists(identifier)) {
- throw QueryCompilationErrors.cannotDropBuiltinFuncError(identifier.funcName)
+ // Extract the function name, handling qualified names like "system.session.func"
+ val funcName = if (identifier.database.isDefined) {
+ // Qualified name - validate it's a valid temporary function namespace
+ val db = identifier.database.get
+ if (db != "session" && db != CatalogManager.SESSION_NAMESPACE) {
Review Comment:
Fixed.
--
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-54807][SQL] Allow qualified names for built-in and session functions [spark]
Posted by "gengliangwang (via GitHub)" <gi...@apache.org>.
gengliangwang commented on code in PR #53570:
URL: https://github.com/apache/spark/pull/53570#discussion_r2850272168
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/SessionCatalog.scala:
##########
@@ -2064,17 +2377,40 @@ class SessionCatalog(
* Returns whether it is a temporary function. If not existed, returns false.
*/
def isTemporaryFunction(name: FunctionIdentifier): Boolean = {
- // A temporary function is a function that has been registered in functionRegistry
- // without a database name, and is neither a built-in function nor a Hive function
- name.database.isEmpty && isRegisteredFunction(name) && !isBuiltinFunction(name)
+ // A temporary function is stored with database = TEMP_FUNCTION_DB
+ if (name.database.isEmpty) {
+ val tempIdent = tempFunctionIdentifier(name.funcName)
+ functionRegistry.functionExists(tempIdent) ||
+ tableFunctionRegistry.functionExists(tempIdent)
+ } else {
+ isTempFunctionIdentifier(name)
Review Comment:
should we also check
```
functionRegistry.functionExists(tempIdent) ||
tableFunctionRegistry.functionExists(tempIdent)
```
here?
The `isTempFunctionIdentifier` method checks only the identifier
--
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-54807][SQL] Allow qualified names for built-in and session functions [spark]
Posted by "gengliangwang (via GitHub)" <gi...@apache.org>.
gengliangwang commented on code in PR #53570:
URL: https://github.com/apache/spark/pull/53570#discussion_r2688963186
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/command/functions.scala:
##########
@@ -139,11 +140,27 @@ case class DropFunctionCommand(
override def run(sparkSession: SparkSession): Seq[Row] = {
val catalog = sparkSession.sessionState.catalog
if (isTemp) {
- assert(identifier.database.isEmpty)
- if (FunctionRegistry.builtin.functionExists(identifier)) {
- throw QueryCompilationErrors.cannotDropBuiltinFuncError(identifier.funcName)
+ // Extract the function name, handling qualified names like "system.session.func"
+ val funcName = if (identifier.database.isDefined) {
+ // Qualified name - validate it's a valid temporary function namespace
+ val db = identifier.database.get
+ if (db != "session" && db != CatalogManager.SESSION_NAMESPACE) {
Review Comment:
`CatalogManager.SESSION_NAMESPACE` is `session`, right? Seems duplicated. Also, is it case sensitive?
--
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-54807][SQL] Allow qualified names for built-in and session functions [spark]
Posted by "vladimirg-db (via GitHub)" <gi...@apache.org>.
vladimirg-db commented on code in PR #53570:
URL: https://github.com/apache/spark/pull/53570#discussion_r2740467187
##########
common/utils/src/main/resources/error/error-conditions.json:
##########
@@ -541,6 +541,13 @@
],
"sqlState" : "58030"
},
+ "CANNOT_SHADOW_BUILTIN_FUNCTION" : {
Review Comment:
I also remembered that we have parameterless function calls like `current_user` vs `current_user()`. I propose to not allow referencing them by the fully-qualified names to avoid introducing more complexity to the attribute resolution code, or to the Parser if `enforceReservedKeywords` is true.
--
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-54807][SQL] Allow qualified names for built-in and session functions [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #53570:
URL: https://github.com/apache/spark/pull/53570#discussion_r2870463746
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/SessionCatalog.scala:
##########
@@ -84,6 +94,224 @@ class SessionCatalog(
import SessionCatalog._
import CatalogTypes.TablePartitionSpec
+ // Marker database name for temporary functions to distinguish from builtin functions
+ /**
+ * Database qualifier used to store temporary functions in the function registry.
+ * Temporary functions use composite keys to coexist with builtin functions of the same name:
+ * - Builtin functions: FunctionIdentifier(name, None)
+ * - Extension functions: FunctionIdentifier(name, None, None) (Treated as builtin)
+ * - Temp functions: FunctionIdentifier(name, Some(CatalogManager.SESSION_NAMESPACE))
+ * This allows all three to exist in the same registry without conflicts.
+ */
+ private val TEMP_FUNCTION_DB = CatalogManager.SESSION_NAMESPACE
+ private val EXTENSION_FUNCTION_DB = CatalogManager.EXTENSION_NAMESPACE
+
+ /**
+ * Creates a FunctionIdentifier for an extension function.
+ * Extension functions are treated as builtins and stored unqualified.
+ * This allows them to overwrite existing builtins if registered by power users.
+ *
+ * @param name The function name (unqualified)
+ * @return FunctionIdentifier without qualification (same as builtins)
+ */
+ private def extensionFunctionIdentifier(name: String): FunctionIdentifier =
+ FunctionIdentifier(format(name))
+
+ /**
+ * Creates a FunctionIdentifier for a temporary function with the TEMP_FUNCTION_DB database
+ * qualifier. This enables temporary functions to coexist with builtin functions of the same name.
+ *
+ * @param name The function name (unqualified)
+ * @return FunctionIdentifier with database = TEMP_FUNCTION_DB
+ */
+ private def tempFunctionIdentifier(name: String): FunctionIdentifier =
+ FunctionIdentifier(
+ format(name),
+ Some(TEMP_FUNCTION_DB),
+ Some(CatalogManager.SYSTEM_CATALOG_NAME))
+
+ /**
+ * Checks if a FunctionIdentifier represents an extension function by checking for the
+ * EXTENSION_FUNCTION_DB database qualifier.
+ *
+ * @param identifier The FunctionIdentifier to check
+ * @return true if this is an extension function identifier
+ */
+ private def isExtensionFunctionIdentifier(identifier: FunctionIdentifier): Boolean =
+ identifier.database.contains(EXTENSION_FUNCTION_DB)
+
+ /**
+ * Checks if a FunctionIdentifier represents a temporary function by checking for the
+ * TEMP_FUNCTION_DB database qualifier.
+ *
+ * @param identifier The FunctionIdentifier to check
+ * @return true if this is a temporary function identifier
+ */
+ private def isTempFunctionIdentifier(identifier: FunctionIdentifier): Boolean =
+ identifier.database.contains(TEMP_FUNCTION_DB)
+
+ // --------------------------------
+ // | PATH-Based Resolution |
+ // --------------------------------
+ // Functions are resolved by searching through an ordered PATH of namespaces.
+ // This provides a unified, data-driven resolution mechanism instead of hardcoded checks.
+
+ /**
+ * Function resolution PATH - ordered list of namespaces to search for unqualified functions.
+ * Each entry is a FunctionIdentifier representing a namespace (catalog + database).
+ * The funcName field is unused (empty string) as these represent namespace templates.
+ *
+ * Resolution order follows the configured path: builtin then session (default "second"),
+ * or session then builtin (legacy "first"), or builtin only ("last" - session tried after
+ * persistent in FunctionResolution). Extension functions are stored and resolved in the
+ * builtin namespace; they are not a separate path step.
+ *
+ * When resolving a view, the system.session namespace is included in the path, but
+ * handleViewContext filters to only return temporary functions that were referred to
+ * when the view was created.
+ *
+ * These identifiers are cached for performance since they're accessed frequently.
+ */
+ private val SESSION_NAMESPACE_TEMPLATE = FunctionIdentifier(
+ funcName = "",
+ database = Some(CatalogManager.SESSION_NAMESPACE),
+ catalog = Some(CatalogManager.SYSTEM_CATALOG_NAME))
+
+ private val BUILTIN_NAMESPACE_TEMPLATE = FunctionIdentifier(
+ funcName = "",
+ database = Some(CatalogManager.BUILTIN_NAMESPACE),
+ catalog = Some(CatalogManager.SYSTEM_CATALOG_NAME))
+
+ // The resolution path: builtin -> session (session "second")
+ private val RESOLUTION_PATH = Seq(
+ BUILTIN_NAMESPACE_TEMPLATE,
+ SESSION_NAMESPACE_TEMPLATE
+ )
+
+ // Legacy resolution path: session -> builtin (session "first")
+ private val LEGACY_RESOLUTION_PATH = Seq(
+ SESSION_NAMESPACE_TEMPLATE,
+ BUILTIN_NAMESPACE_TEMPLATE
+ )
+
+ // Session last: only builtin (session tried after persistent in FunctionResolution)
+ private val SESSION_LAST_RESOLUTION_PATH = Seq(BUILTIN_NAMESPACE_TEMPLATE)
+
+ /**
+ * Returns the resolution path for function lookup.
+ * @return Ordered sequence of namespace identifiers.
+ */
+ private def resolutionPath(): Seq[FunctionIdentifier] = {
+ conf.sessionFunctionResolutionOrder match {
+ case "first" => LEGACY_RESOLUTION_PATH
+ case "last" => SESSION_LAST_RESOLUTION_PATH
+ case _ => RESOLUTION_PATH // "second" (default)
+ }
+ }
+
+ /**
+ * Maps a namespace template to an actual storage identifier for a specific function.
+ * This handles the asymmetry between how builtins, extensions, and temp functions are stored.
+ *
+ * Storage conventions:
+ * - Builtin functions: FunctionIdentifier(name, None, None)
+ * - Extension functions: FunctionIdentifier(name, None, None) (Treated as builtin)
+ * - Temp functions: FunctionIdentifier(name, Some("session"), Some("system"))
+ * - Other: FunctionIdentifier(name, namespace.database, namespace.catalog)
+ *
+ * @param namespace The namespace template
+ * @param name The function name
+ * @return The actual identifier to use for registry lookup
+ */
+ private def namespaceToIdentifier(
+ namespace: FunctionIdentifier,
+ name: String): FunctionIdentifier = {
+ namespace.database match {
+ case Some(CatalogManager.SESSION_NAMESPACE) =>
Review Comment:
why this needs a special case? Exception for builtin functions, all others are just `template.copy(name = 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-54807][SQL] Allow qualified names for built-in and session functions [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #53570:
URL: https://github.com/apache/spark/pull/53570#discussion_r2870472432
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/SessionCatalog.scala:
##########
@@ -1935,20 +2163,50 @@ class SessionCatalog(
overrideIfExists: Boolean,
functionBuilder: Option[FunctionBuilder] = None): Unit = {
val builder = functionBuilder.getOrElse(makeFunctionBuilder(funcDefinition))
- registerFunction(funcDefinition, overrideIfExists, functionRegistry, builder)
+ // Use composite keys for temporary functions, but not for persistent functions
+ val useComposite = funcDefinition.identifier.database.isEmpty
+ registerFunction(funcDefinition, overrideIfExists, functionRegistry, builder, useComposite)
}
private def registerFunction[T](
funcDefinition: CatalogFunction,
overrideIfExists: Boolean,
registry: FunctionRegistryBase[T],
- functionBuilder: FunctionRegistryBase[T]#FunctionBuilder): Unit = {
+ functionBuilder: FunctionRegistryBase[T]#FunctionBuilder,
+ useCompositeKey: Boolean): Unit = {
Review Comment:
why do we need this flag? we can infer it by `funcDefinition.identifier.database.isEmpty`
--
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-54807][SQL] Allow qualified names for built-in and session functions [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #53570:
URL: https://github.com/apache/spark/pull/53570#discussion_r2870495037
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionResolution.scala:
##########
@@ -19,51 +19,235 @@ package org.apache.spark.sql.catalyst.analysis
import java.util.concurrent.atomic.AtomicBoolean
+import scala.util.control.NonFatal
+
+import org.apache.spark.internal.Logging
import org.apache.spark.sql.AnalysisException
import org.apache.spark.sql.catalyst.FunctionIdentifier
+import org.apache.spark.sql.catalyst.analysis.FunctionRegistry
+import org.apache.spark.sql.catalyst.catalog.SessionCatalog
import org.apache.spark.sql.catalyst.expressions._
import org.apache.spark.sql.catalyst.expressions.aggregate._
import org.apache.spark.sql.catalyst.plans.logical.LogicalPlan
import org.apache.spark.sql.connector.catalog.{
CatalogManager,
+ CatalogNotFoundException,
+ CatalogV2Util,
LookupCatalog
}
+
+/**
+ * Represents the type/location of a function.
+ */
+sealed trait FunctionType
+object FunctionType {
+ /** Function is a built-in function in the builtin registry. */
+ case object Builtin extends FunctionType
+ /** Function is a temporary function in the session registry. */
+ case object Temporary extends FunctionType
+ /** Function is a persistent function in the external catalog. */
+ case object Persistent extends FunctionType
+ /** Function exists only as a table function (cannot be used in scalar context). */
+ case object TableOnly extends FunctionType
+ /** Function does not exist anywhere. */
+ case object NotFound extends FunctionType
+}
import org.apache.spark.sql.connector.catalog.CatalogV2Implicits._
import org.apache.spark.sql.connector.catalog.functions.{
AggregateFunction => V2AggregateFunction,
ScalarFunction,
UnboundFunction
}
import org.apache.spark.sql.errors.{DataTypeErrorsBase, QueryCompilationErrors}
+import org.apache.spark.sql.internal.SQLConf
import org.apache.spark.sql.internal.connector.V1Function
import org.apache.spark.sql.types._
class FunctionResolution(
override val catalogManager: CatalogManager,
relationResolution: RelationResolution)
- extends DataTypeErrorsBase with LookupCatalog {
+ extends DataTypeErrorsBase with LookupCatalog with Logging {
private val v1SessionCatalog = catalogManager.v1SessionCatalog
private val trimWarningEnabled = new AtomicBoolean(true)
- def resolveFunction(u: UnresolvedFunction): Expression = {
- withPosition(u) {
- resolveBuiltinOrTempFunction(u.nameParts, u.arguments, u).getOrElse {
+ /**
+ * Produces the ordered list of fully qualified candidate names for resolution.
+ *
+ * @param nameParts The function name parts.
+ * @return A sequence of fully qualified function names to attempt resolution with.
+ */
+ private def resolutionCandidates(nameParts: Seq[String]): Seq[Seq[String]] = {
+ if (nameParts.size == 1) {
+ val catalogPath = catalogManager.currentCatalog.name +: catalogManager.currentNamespace
+ val searchPath = SQLConf.get.resolutionSearchPath(catalogPath.toSeq)
+ 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 3 if FunctionResolution.sessionNamespaceKind(nameParts).isDefined =>
+ // Fully qualified system.builtin.func or system.session.func
+ Seq(nameParts)
+ case _ =>
+ Seq(nameParts)
+ }
+ }
+ }
+
+ private def resolveQualifiedFunction(
+ nameParts: Seq[String],
+ unresolvedFunc: UnresolvedFunction): Option[Expression] = {
+ if (nameParts.length == 3 &&
+ nameParts.head.equalsIgnoreCase(CatalogManager.SYSTEM_CATALOG_NAME)) {
+ // Try resolving as a session-namespace function (builtin, temp, extension)
+ FunctionResolution.sessionNamespaceKind(nameParts).flatMap { kind =>
+ val funcName = nameParts.last
+ val expr = v1SessionCatalog.resolveScalarFunction(kind, funcName, unresolvedFunc.arguments)
+ if (expr.isEmpty) {
+ if (v1SessionCatalog.lookupFunctionInfo(
+ SessionCatalog.Temp, funcName, tableFunction = true).isDefined) {
+ throw QueryCompilationErrors.notAScalarFunctionError(funcName, unresolvedFunc)
+ }
+ if (v1SessionCatalog.lookupBuiltinOrTempTableFunction(funcName).isDefined) {
Review Comment:
why check existence again? isn't `lookupFunctionInfo` sufficient?
--
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-54807][SQL] Allow qualified names for built-in and session functions [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #53570:
URL: https://github.com/apache/spark/pull/53570#discussion_r2870490904
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionResolution.scala:
##########
@@ -19,51 +19,235 @@ package org.apache.spark.sql.catalyst.analysis
import java.util.concurrent.atomic.AtomicBoolean
+import scala.util.control.NonFatal
+
+import org.apache.spark.internal.Logging
import org.apache.spark.sql.AnalysisException
import org.apache.spark.sql.catalyst.FunctionIdentifier
+import org.apache.spark.sql.catalyst.analysis.FunctionRegistry
+import org.apache.spark.sql.catalyst.catalog.SessionCatalog
import org.apache.spark.sql.catalyst.expressions._
import org.apache.spark.sql.catalyst.expressions.aggregate._
import org.apache.spark.sql.catalyst.plans.logical.LogicalPlan
import org.apache.spark.sql.connector.catalog.{
CatalogManager,
+ CatalogNotFoundException,
+ CatalogV2Util,
LookupCatalog
}
+
+/**
+ * Represents the type/location of a function.
+ */
+sealed trait FunctionType
+object FunctionType {
+ /** Function is a built-in function in the builtin registry. */
+ case object Builtin extends FunctionType
+ /** Function is a temporary function in the session registry. */
+ case object Temporary extends FunctionType
+ /** Function is a persistent function in the external catalog. */
+ case object Persistent extends FunctionType
+ /** Function exists only as a table function (cannot be used in scalar context). */
+ case object TableOnly extends FunctionType
+ /** Function does not exist anywhere. */
+ case object NotFound extends FunctionType
+}
import org.apache.spark.sql.connector.catalog.CatalogV2Implicits._
import org.apache.spark.sql.connector.catalog.functions.{
AggregateFunction => V2AggregateFunction,
ScalarFunction,
UnboundFunction
}
import org.apache.spark.sql.errors.{DataTypeErrorsBase, QueryCompilationErrors}
+import org.apache.spark.sql.internal.SQLConf
import org.apache.spark.sql.internal.connector.V1Function
import org.apache.spark.sql.types._
class FunctionResolution(
override val catalogManager: CatalogManager,
relationResolution: RelationResolution)
- extends DataTypeErrorsBase with LookupCatalog {
+ extends DataTypeErrorsBase with LookupCatalog with Logging {
private val v1SessionCatalog = catalogManager.v1SessionCatalog
private val trimWarningEnabled = new AtomicBoolean(true)
- def resolveFunction(u: UnresolvedFunction): Expression = {
- withPosition(u) {
- resolveBuiltinOrTempFunction(u.nameParts, u.arguments, u).getOrElse {
+ /**
+ * Produces the ordered list of fully qualified candidate names for resolution.
+ *
+ * @param nameParts The function name parts.
+ * @return A sequence of fully qualified function names to attempt resolution with.
+ */
+ private def resolutionCandidates(nameParts: Seq[String]): Seq[Seq[String]] = {
+ if (nameParts.size == 1) {
+ val catalogPath = catalogManager.currentCatalog.name +: catalogManager.currentNamespace
+ val searchPath = SQLConf.get.resolutionSearchPath(catalogPath.toSeq)
+ 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 3 if FunctionResolution.sessionNamespaceKind(nameParts).isDefined =>
Review Comment:
we can remove this case, it's the same as the default.
--
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-54807][SQL] Allow qualified names for built-in and session functions [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #53570:
URL: https://github.com/apache/spark/pull/53570#discussion_r2870505997
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionResolution.scala:
##########
@@ -73,70 +257,119 @@ class FunctionResolution(
}
def lookupBuiltinOrTempFunction(
- name: Seq[String],
- u: Option[UnresolvedFunction]): Option[ExpressionInfo] = {
- if (name.size == 1 && u.exists(_.isInternal)) {
- FunctionRegistry.internal.lookupFunction(FunctionIdentifier(name.head))
- } else if (name.size == 1) {
- v1SessionCatalog.lookupBuiltinOrTempFunction(name.head)
+ nameParts: Seq[String],
+ unresolvedFunc: Option[UnresolvedFunction]): Option[ExpressionInfo] = {
+ if (nameParts.size == 1 && unresolvedFunc.exists(_.isInternal)) {
+ FunctionRegistry.internal.lookupFunction(FunctionIdentifier(nameParts.head))
} else {
- None
+ FunctionResolution.sessionNamespaceKind(nameParts) match {
+ case Some(kind) =>
+ v1SessionCatalog.lookupFunctionInfo(kind, nameParts.last, tableFunction = false)
+ case None =>
+ if (nameParts.size == 1) {
+ v1SessionCatalog.lookupBuiltinOrTempFunction(nameParts.head)
+ } else {
+ None
+ }
+ }
}
}
- def lookupBuiltinOrTempTableFunction(name: Seq[String]): Option[ExpressionInfo] = {
- if (name.length == 1) {
- v1SessionCatalog.lookupBuiltinOrTempTableFunction(name.head)
- } else {
- None
+ def lookupBuiltinOrTempTableFunction(nameParts: Seq[String]): Option[ExpressionInfo] = {
+ FunctionResolution.sessionNamespaceKind(nameParts) match {
+ case Some(kind) =>
+ v1SessionCatalog.lookupFunctionInfo(kind, nameParts.last, tableFunction = true)
+ case None =>
+ if (nameParts.length == 1) {
+ v1SessionCatalog.lookupBuiltinOrTempTableFunction(nameParts.head)
+ } else {
+ None
+ }
}
}
- def resolveBuiltinOrTempFunction(
- name: Seq[String],
- arguments: Seq[Expression],
- u: UnresolvedFunction): Option[Expression] = {
- val expression = if (name.size == 1 && u.isInternal) {
- Option(FunctionRegistry.internal.lookupFunction(FunctionIdentifier(name.head), arguments))
- } else if (name.size == 1) {
- v1SessionCatalog.resolveBuiltinOrTempFunction(name.head, arguments)
- } else {
- None
+ /**
+ * Determines the type/location of a function (builtin, temporary, persistent, etc.).
+ * This is used by the LookupFunctions analyzer rule for early validation and optimization.
+ * This method only performs the lookup and classification - it does not throw errors.
+ *
+ * @param nameParts The function name parts.
+ * @param unresolvedFunc Optional UnresolvedFunction node for lookups that may need it.
+ * @return The type of the function (Builtin, Temporary, Persistent, TableOnly, or NotFound).
+ */
+ def lookupFunctionType(
+ nameParts: Seq[String],
+ unresolvedFunc: Option[UnresolvedFunction] = None): FunctionType = {
+
+ // Check if it's explicitly qualified as extension, builtin, or temp
+ FunctionResolution.sessionNamespaceKind(nameParts) match {
+ case Some(SessionCatalog.Extension) | Some(SessionCatalog.Builtin) =>
+ if (lookupBuiltinOrTempFunction(nameParts, unresolvedFunc).isDefined) {
Review Comment:
do we need to check existence when determining the `FunctionType`? The search path resolution will try next one if can't find the function with the current type.
--
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-54807][SQL] Allow qualified names for built-in and session functions [spark]
Posted by "srielau (via GitHub)" <gi...@apache.org>.
srielau commented on code in PR #53570:
URL: https://github.com/apache/spark/pull/53570#discussion_r2870569034
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/SessionCatalog.scala:
##########
@@ -84,6 +94,224 @@ class SessionCatalog(
import SessionCatalog._
import CatalogTypes.TablePartitionSpec
+ // Marker database name for temporary functions to distinguish from builtin functions
+ /**
+ * Database qualifier used to store temporary functions in the function registry.
+ * Temporary functions use composite keys to coexist with builtin functions of the same name:
+ * - Builtin functions: FunctionIdentifier(name, None)
+ * - Extension functions: FunctionIdentifier(name, None, None) (Treated as builtin)
+ * - Temp functions: FunctionIdentifier(name, Some(CatalogManager.SESSION_NAMESPACE))
+ * This allows all three to exist in the same registry without conflicts.
+ */
+ private val TEMP_FUNCTION_DB = CatalogManager.SESSION_NAMESPACE
+ private val EXTENSION_FUNCTION_DB = CatalogManager.EXTENSION_NAMESPACE
+
+ /**
+ * Creates a FunctionIdentifier for an extension function.
+ * Extension functions are treated as builtins and stored unqualified.
+ * This allows them to overwrite existing builtins if registered by power users.
+ *
+ * @param name The function name (unqualified)
+ * @return FunctionIdentifier without qualification (same as builtins)
+ */
+ private def extensionFunctionIdentifier(name: String): FunctionIdentifier =
+ FunctionIdentifier(format(name))
+
+ /**
+ * Creates a FunctionIdentifier for a temporary function with the TEMP_FUNCTION_DB database
+ * qualifier. This enables temporary functions to coexist with builtin functions of the same name.
+ *
+ * @param name The function name (unqualified)
+ * @return FunctionIdentifier with database = TEMP_FUNCTION_DB
+ */
+ private def tempFunctionIdentifier(name: String): FunctionIdentifier =
+ FunctionIdentifier(
+ format(name),
+ Some(TEMP_FUNCTION_DB),
+ Some(CatalogManager.SYSTEM_CATALOG_NAME))
+
+ /**
+ * Checks if a FunctionIdentifier represents an extension function by checking for the
+ * EXTENSION_FUNCTION_DB database qualifier.
+ *
+ * @param identifier The FunctionIdentifier to check
+ * @return true if this is an extension function identifier
+ */
+ private def isExtensionFunctionIdentifier(identifier: FunctionIdentifier): Boolean =
+ identifier.database.contains(EXTENSION_FUNCTION_DB)
+
+ /**
+ * Checks if a FunctionIdentifier represents a temporary function by checking for the
+ * TEMP_FUNCTION_DB database qualifier.
+ *
+ * @param identifier The FunctionIdentifier to check
+ * @return true if this is a temporary function identifier
+ */
+ private def isTempFunctionIdentifier(identifier: FunctionIdentifier): Boolean =
+ identifier.database.contains(TEMP_FUNCTION_DB)
+
+ // --------------------------------
+ // | PATH-Based Resolution |
+ // --------------------------------
+ // Functions are resolved by searching through an ordered PATH of namespaces.
+ // This provides a unified, data-driven resolution mechanism instead of hardcoded checks.
+
+ /**
+ * Function resolution PATH - ordered list of namespaces to search for unqualified functions.
+ * Each entry is a FunctionIdentifier representing a namespace (catalog + database).
+ * The funcName field is unused (empty string) as these represent namespace templates.
+ *
+ * Resolution order follows the configured path: builtin then session (default "second"),
+ * or session then builtin (legacy "first"), or builtin only ("last" - session tried after
+ * persistent in FunctionResolution). Extension functions are stored and resolved in the
+ * builtin namespace; they are not a separate path step.
+ *
+ * When resolving a view, the system.session namespace is included in the path, but
+ * handleViewContext filters to only return temporary functions that were referred to
+ * when the view was created.
+ *
+ * These identifiers are cached for performance since they're accessed frequently.
+ */
+ private val SESSION_NAMESPACE_TEMPLATE = FunctionIdentifier(
+ funcName = "",
+ database = Some(CatalogManager.SESSION_NAMESPACE),
+ catalog = Some(CatalogManager.SYSTEM_CATALOG_NAME))
+
+ private val BUILTIN_NAMESPACE_TEMPLATE = FunctionIdentifier(
+ funcName = "",
+ database = Some(CatalogManager.BUILTIN_NAMESPACE),
+ catalog = Some(CatalogManager.SYSTEM_CATALOG_NAME))
+
+ // The resolution path: builtin -> session (session "second")
+ private val RESOLUTION_PATH = Seq(
+ BUILTIN_NAMESPACE_TEMPLATE,
+ SESSION_NAMESPACE_TEMPLATE
+ )
+
+ // Legacy resolution path: session -> builtin (session "first")
+ private val LEGACY_RESOLUTION_PATH = Seq(
+ SESSION_NAMESPACE_TEMPLATE,
+ BUILTIN_NAMESPACE_TEMPLATE
+ )
+
+ // Session last: only builtin (session tried after persistent in FunctionResolution)
+ private val SESSION_LAST_RESOLUTION_PATH = Seq(BUILTIN_NAMESPACE_TEMPLATE)
+
+ /**
+ * Returns the resolution path for function lookup.
+ * @return Ordered sequence of namespace identifiers.
+ */
+ private def resolutionPath(): Seq[FunctionIdentifier] = {
+ conf.sessionFunctionResolutionOrder match {
+ case "first" => LEGACY_RESOLUTION_PATH
+ case "last" => SESSION_LAST_RESOLUTION_PATH
+ case _ => RESOLUTION_PATH // "second" (default)
+ }
+ }
+
+ /**
+ * Maps a namespace template to an actual storage identifier for a specific function.
+ * This handles the asymmetry between how builtins, extensions, and temp functions are stored.
+ *
+ * Storage conventions:
+ * - Builtin functions: FunctionIdentifier(name, None, None)
+ * - Extension functions: FunctionIdentifier(name, None, None) (Treated as builtin)
+ * - Temp functions: FunctionIdentifier(name, Some("session"), Some("system"))
+ * - Other: FunctionIdentifier(name, namespace.database, namespace.catalog)
+ *
+ * @param namespace The namespace template
+ * @param name The function name
+ * @return The actual identifier to use for registry lookup
+ */
+ private def namespaceToIdentifier(
+ namespace: FunctionIdentifier,
+ name: String): FunctionIdentifier = {
+ namespace.database match {
+ case Some(CatalogManager.SESSION_NAMESPACE) =>
Review Comment:
Non needed
--
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