[SPARK-54807][SQL] Allow qualified names for built-in and session functions

[srielau]

design_sketch.md

What changes were proposed in this pull request?

• Allow reference of built in functions with qualifiers builtin or system.builtin and temporary functions as session or system.session. Functions registered as extensionz can be qualified with system.extension or extension.

• Cleaned up APIs to resolve functions to prep for configurable path

• Register builtin extension, and session functions with qualified names, so they can co-exist

• Fix a bug that allowed session functions with the same name to co-exist as table and scalar functions
  design_sketch.md

Why are the changes needed?

This portion of work allows users to excplicitly pick a builtin or temporary function, the same way they would pick a persisted function by fully qualifying it.
This increases security.
WIth this we now have a fixed order:
extension -> builtin -> session -> current schema for function resolution.

In follow on work we plan to allow the priority of function resolution to be configurable, for example to push temporary functions after built-ins or even after persisted functions.
Ultimately we aim for proper SQL Standard PATH support where a user can add "libraries" of functions to the path.

Does this PR introduce any user-facing change?

You can now reference builtin functions such as concat with builtin.concat or system.builtin.concat. Teh same for temporary functions which can be qualified as session, or system.session.

How was this patch tested?

A new suite: functionQualificationSuite.scala has been added

Was this patch authored or co-authored using generative AI tooling?

Yes: Claude Sonnet

[cloud-fan]

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. Furthermore, we should not let end users know the concept of extension functions. The power users may want to do it silently.

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.

[cloud-fan]

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:

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:

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:

} 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.

[gengliangwang]

I will merge this one once the CI passed. Thanks for the works @srielau

[MaxGekk]

Hi @srielau @cloud-fan @gengliangwang @vladimirg-db, a heads-up on a performance regression introduced by this change (SPARK-54807), tracked as SPARK-57758.

Before this PR, an unqualified built-in function (count, sum, coalesce, ...) resolved with a single in-memory registry lookup. Now FunctionResolution.resolveFunction/resolveTableFunction builds an ordered candidate search path for every UnresolvedFunction: it reads the AnalysisContext/CatalogManager, reads spark.sql.functionResolution.sessionOrder, allocates the search-path and candidate Seqs, and iterates candidates doing a name-kind parse plus a registry lookup. None of it is memoized, so it runs for every function node, on every analysis pass.

The impact is amplified under Spark Connect, which re-analyzes the whole (growing) plan on every AnalyzePlan call, so the per-function overhead is paid repeatedly and analysis time regresses multi-fold. Execution is unaffected.

I opened #56869 to fix it: memoize the resolution path per analysis pass and add a built-in-only fast path for single-part names (gated so it only short-circuits when system.builtin precedes system.session, preserving the configurable order). Resolution semantics are unchanged.

Would appreciate your review. Thanks!