cloud-fan commented on code in PR #59697:
URL: https://github.com/apache/spark/pull/59697#discussion_r4346067541
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/SessionCatalog.scala:
##########
@@ -2167,12 +2203,49 @@ class SessionCatalog(
if (funcDefinition.isUserDefinedFunction) {
UserDefinedFunction.fromCatalogFunction(funcDefinition,
parser).toExpressionInfo
} else {
- makeExprInfoForHiveFunction(funcDefinition)
+ makeExprInfoForClassFunction(funcDefinition)
}
}
registry.registerFunction(identToRegister, resolvedInfo, functionBuilder)
}
+ /**
+ * Builds the [[ExpressionInfo]] of a class-based function (`CREATE FUNCTION
... AS 'class'`).
+ * A DataSource V2 function reports its own description, which requires
instantiating it; if the
+ * class is not on the classpath yet (its `USING` resources are only loaded
on first invocation)
+ * or cannot be instantiated, fall back to the class-name-only info, so that
like a Hive UDF, a
+ * broken function only fails when invoked. Other classes are checked
without being initialized.
+ */
+ private def makeExprInfoForClassFunction(func: CatalogFunction):
ExpressionInfo = {
+ if (Utils.classIsLoadableAndAssignableFrom(func.className,
classOf[UnboundFunction])) {
+ try {
+ val clazz = Utils.classForName(func.className)
+ makeExprInfoForV2Function(func, newV2FunctionInstance(clazz,
func.identifier))
+ } catch {
+ case NonFatal(_) => makeExprInfoForHiveFunction(func)
Review Comment:
**Non-blocking (P2):** This fallback only catches `NonFatal`, and `NonFatal`
treats `LinkageError` as fatal. The `classIsLoadableAndAssignableFrom` check at
line 2220 also sits outside the `try`, and its `Try` has the same gap. Before
this PR these metadata paths never touched the class. Now
`lookupPersistentFunction` and the eager info in `loadPersistentScalarFunction`
load it, and for a V2 class they also initialize and construct it. So a
persistent function whose class has a throwing static initializer
(`ExceptionInInitializerError`, then `NoClassDefFoundError: Could not
initialize class`) or a missing superclass (`NoClassDefFoundError`) now breaks
`DESCRIBE FUNCTION`, `spark.catalog.getFunction`, Thrift `getFunctions`, and
`spark.catalog.listFunctions()` for the whole database with a raw
`java.lang.Error`. That contradicts the fail-only-on-invoke behavior the PR
promises.
There is also an ordering regression. The eager info at line 2796 is
computed before `builderFactory` calls `loadFunctionResources`. Suppose the
class is already visible (say its jar was added earlier) but its superclass or
a static-initializer dependency only exists in a jar from this function's
`USING` clause. Then the lookup throws before the resources are ever loaded, so
the function can't be resolved in the session at all. The merge target loaded
resources first and resolved it fine. For a V2 class, initializing it this
early can also leave it permanently erroneous in the classloader.
**Recommended change:** Make class-function metadata construction safe for
broken classes and keep it from initializing a class before that function's
USING resources are loaded. In makeExprInfoForClassFunction, treat a
LinkageError from the loadability check, class initialization or construction
like any other non-fatal failure, and fall back to the class-name-only info. On
the metadata-only paths (DESCRIBE lookup and the eager info built before first
invocation), do not load or instantiate the class of a function that declares
USING resources; report the class-name-only info until first invocation loads
the resources and registers the function with its full info.
**Why this works:** With LinkageError in the fallback at the single info
producer, no metadata consumer can receive an escaping class-loading Error.
With the class left untouched for resource-bearing functions until
registration, the merge-target order is restored: USING resources load before
the class initializes. A dependency that only exists in those resources is then
present when the static initializer runs, and the shared classloader never
records a class as erroneous because of premature initialization.
**Scope:** Narrow the class-function ExpressionInfo builder in
SessionCatalog so its fallback covers linkage failures and it does not touch a
function's class on metadata paths before that function's USING resources are
loaded. Add tests for broken-class metadata operations and for USING-resource
ordering.
**Compatibility:** Hive UDF metadata stays class-name-only and Hive UDF
invocation is unchanged. DESCRIBE for a classpath V2 function without USING
resources still reports its description before first use.
**Risks:** Catching LinkageError must not also swallow VirtualMachineError
or other fatal throwables; those must keep propagating. A V2 function that
declares USING resources but whose class happens to be visible already will now
show class-name-only info until it is first invoked, so any expectation of an
earlier description for such functions must follow the accepted timing.
**Constraints:** On the invocation path, a function's USING resources must
still load before its class is initialized, as in the merge target.
External-catalog calls stay the same as for a Hive UDF.
**Exclusions:**
- Changing the shared class-loadability utility for its other callers.
- The codegen classloader question for magic-method functions loaded from
jars, which awaits the author's decision.
**Success:** DESCRIBE FUNCTION, spark.catalog.listFunctions,
spark.catalog.getFunction and JDBC getFunctions succeed with class-name-only
info for a persistent class-based function whose class fails to link or
initialize, and only invoking that function fails. No metadata lookup loads or
initializes the class of a persistent function before that function's USING
resources are loaded. A persistent V2 function without USING resources whose
class is on the classpath still shows its description in DESCRIBE before first
use.
**Verification:**
- **Regression:** For a persistent V2 function whose class has a throwing
static initializer, listing user functions, the catalog listing and lookup
APIs, and DESCRIBE FUNCTION EXTENDED all succeed with class-name-only info,
while invoking the function fails with an error.
- **Regression:** For a persistent function whose class is already visible
but whose superclass or static-initializer dependency exists only in a jar
listed in the function's USING clause, describing the function before first use
does not initialize the class, and the first invocation succeeds after the
resources load.
- **Behavior:** A persistent V2 function with no USING resources and a
classpath class reports its description in DESCRIBE before it is ever invoked,
and a USING JAR V2 function reports its description after first invocation.
- **Compatibility:** Existing persistent Hive UDFs keep class-name-only
metadata and resolve and run exactly as before, including when their USING
resources are loaded only on first invocation.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryCompilationErrors.scala:
##########
@@ -1442,6 +1442,17 @@ private[sql] object QueryCompilationErrors extends
QueryErrorsBase with Compilat
new FunctionAlreadyExistsException(func.nameParts)
}
+ def cannotInstantiateFunctionClassError(
+ className: String, func: FunctionIdentifier, cause: Throwable):
Throwable = {
+ new AnalysisException(
+ errorClass = "CANNOT_INSTANTIATE_FUNCTION_CLASS",
+ messageParameters = Map(
+ "className" -> className,
+ "functionName" -> toSQLId(func.toString),
+ "msg" -> Option(cause.getMessage).getOrElse("")),
Review Comment:
**Nit (P3):** `<msg>` comes from `cause.getMessage`, but the most likely
failure here is a public no-arg constructor that throws.
`Constructor.newInstance` wraps that in `InvocationTargetException`, whose
message is null. The user then sees `... please make sure it has a public
no-arg constructor. ` with nothing after it. That points at the wrong cause and
drops the real one; JDBC, Thrift and Connect clients only get this text.
Unwrapping the target exception in `newV2FunctionInstance`, and wording the
template so it also covers a constructor that throws, would make the message
accurate. The 'class without a public no-arg constructor' test only checks the
condition and `className` (and only the condition on the persistent path), so
it wouldn't catch this.
**Verification:**
- **Regression:** Registering a temporary V2 function, and first invoking a
persistent one, whose public no-arg constructor throws yields
CANNOT_INSTANTIATE_FUNCTION_CLASS whose message parameter contains the
constructor's own failure message.
- **Behavior:** A V2 class without a usable no-arg constructor yields
CANNOT_INSTANTIATE_FUNCTION_CLASS with every message parameter asserted,
including the function name for both the temporary and the persistent
registration.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/SessionCatalog.scala:
##########
@@ -2076,14 +2079,47 @@ class SessionCatalog(
* Constructs a [[FunctionBuilder]] based on the provided function metadata.
*/
private def makeFunctionBuilder(func: CatalogFunction): FunctionBuilder = {
+ val clazz = loadFunctionClass(func)
+ if (isV2FunctionClass(clazz)) {
+ // A DataSource V2 function: bind it to the arguments at each call site.
An aggregate
+ // function is returned as a bare `V2Aggregator`, which the caller wraps
into an
+ // `AggregateExpression` carrying DISTINCT / FILTER. This check goes
first so that a class
+ // also implementing a Hive UDF interface, for other engines sharing the
metastore, runs as
+ // a V2 function in Spark.
+ val unbound = newV2FunctionInstance(clazz, func.identifier)
+ (input: Seq[Expression]) => V2ExpressionUtils.bindFunction(unbound,
input)
Review Comment:
**Nit (P3):** The builder returns the raw `bindFunction` result
(`ApplyFunctionExpression`, `StaticInvoke`/`Invoke`, or a bare `V2Aggregator`)
without a `FUNC_ALIAS` tag, and none of these nodes overrides `prettyName`. So
when `validateFunction` rejects a clause on this V1 path, the error names the
node rather than the function. For example, `SELECT strlen(DISTINCT 'a')` on a
session-catalog V2 scalar reports `applyfunctionexpression` (or
`staticinvoke`/`invoke` for a magic method), and `iavg(x) IGNORE NULLS` reports
`v2aggregator`. FILTER and WITHIN GROUP behave the same way. Hive UDF wrappers
on this path override `prettyName` with the function name, and the
FunctionCatalog path passes `unbound.name()`, so the same class reports a
different name depending on where it is registered. The suite has no negative
test for these clauses.
Tagging the returned expression with `FunctionRegistry.FUNC_ALIAS` set to
the registered name would fix the errors. Note that it also changes the default
output column names of the `produceResult` and aggregate shapes, which render
through `prettyName`. The magic-method shapes render through their own
`toString`, so their column names stay as they are.
**Verification:**
- **Regression:** Temporary and persistent session-catalog V2 functions,
both scalar (magic-method and produceResult) and aggregate, reject each
unsupported clause with an error whose function-name parameter is the
registered function name rather than an internal node name.
- **Compatibility:** Unsupported-clause and type-check errors for V2
functions resolved through a FunctionCatalog keep their current function names
and SQL text.
- **Behavior:** Positive session-catalog V2 calls of each shape
(magic-method scalar, produceResult scalar, and aggregate with DISTINCT and
FILTER) return the same results after tagging, and their output column names
are asserted, so the column-name change for the produceResult and aggregate
shapes is deliberate.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
