Copilot commented on code in PR #13016:
URL: https://github.com/apache/gluten/pull/13016#discussion_r4017562622
##########
backends-velox/src/main/scala/org/apache/gluten/backendsapi/velox/VeloxRuleApi.scala:
##########
@@ -67,6 +68,11 @@ object VeloxRuleApi {
if (BackendsApiManager.getSettings.supportAppendDataExec()) {
injector.injectPlannerStrategy(SparkShimLoader.getSparkShims.getRewriteCreateTableAsSelect(_))
}
+
+ // Makes a UDF loaded from `udfLibraryPaths` resolvable by its own name.
Injected through
+ // SparkInjector so InjectorControl turns a call made while Gluten is
disabled into an
+ // analysis-time error rather than a failure at execution.
+ UDFResolver.getFunctionDescriptions.foreach(injector.injectFunction)
Review Comment:
The added suite only inspects the descriptions returned by
`getFunctionDescriptions`; it does not verify that `injectFunction` makes a
plain-name UDF resolvable and executable through a SparkSession, nor that the
wrapped function fails when Gluten is disabled. A regression in this wiring
would therefore pass all new tests. Please add Spark-level coverage using the
existing native-UDF test setup, with the required test placed under
`gluten-ut/`.
##########
backends-velox/src/main/scala/org/apache/spark/sql/expression/UDFResolver.scala:
##########
@@ -338,6 +341,35 @@ object UDFResolver extends Logging {
.toBoolean
}
+ /**
+ * One Spark function per loaded UDF whose name contains no dot. A dotted
name is a Hive UDF class
+ * name, which VeloxHiveUDFTransformer already resolves, so it is skipped
here.
+ *
+ * A name is also skipped when it collides with a Spark built-in: the names
are unqualified, so
+ * injecting one would redirect that built-in to a native implementation
with possibly different
+ * semantics for every query on the session.
+ */
+ def getFunctionDescriptions: Seq[FunctionDescription] = {
+ val (shadowing, injectable) = UDFNames.toSeq
+ .filterNot(_.contains("."))
+ .sorted
+ .partition(name =>
FunctionRegistry.builtin.functionExists(FunctionIdentifier(name)))
Review Comment:
Spark function identifiers are case-insensitive, but this code groups only
exact strings and injects one builder for each. If a library exports both `Foo`
and `foo`, Spark normalizes both registrations to one entry; the later builder
wins, so calls to one native function invoke the other closure and can fail
because `UDFMap` is keyed by the original case. Reject/skip case-insensitive
duplicates (or define a deterministic mapping) before injection and cover that
case.
##########
docs/developers/VeloxUDF.md:
##########
@@ -192,6 +192,18 @@ VeloxColumnarToRow
+- Scan hive spark_catalog.default.tbl [col1#11], HiveTableRelation
[`spark_catalog`.`default`.`tbl`,
org.apache.hadoop.hive.serde2.lazy.LazySimpleSerDe, Data Cols: [col1#11],
Partition Cols: []]
```
+## Natively Only UDF Registration
+
+This is an alternative to the registration described above, for a UDF that is
implemented only in Velox and has no Java counterpart.
+
+A UDF whose registered name contains no dot is added to the session's function
registry under that name. It needs no matching Hive UDF class, no jar on the
classpath, and no `CREATE TEMPORARY FUNCTION` — register it under a name with
no dot, such as `my_udf`, and call it directly:
Review Comment:
This sentence is too broad: `getFunctionDescriptions` explicitly skips names
that collide with Spark built-ins, so a native UDF named `abs` is not added
under its own name and `select abs(...)` still calls Spark's built-in. Document
the non-built-in requirement 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: [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]
