You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@gluten.apache.org by "kevinwilfong (via GitHub)" <gi...@apache.org> on 2026/09/22 23:36:45 UTC
[PR] [VL] Let a UDF/UDAF library declare a function by name [gluten]
kevinwilfong opened a new pull request, #13105:
URL: https://github.com/apache/gluten/pull/13105
## What changes are proposed in this pull request?
A UdfEntry restates a signature that the registered Velox function has already declared. The two are written separately and can disagree, and a function can only be called with the type combinations the library thought to list. For a UDAF the intermediate type is restated too, and a wrong one stays invisible until a partial aggregation exchanges a state the other side cannot read.
To improve on this, this change adds a second kind of entry that carries only a name. RegistryUdfEntry and RegistryUdafEntry are declared through their own symbol pairs (getNumRegistryUdf / getRegistryUdfEntries and the UDAF equivalents), so existing libraries keep their layout and need no rebuild. The library still registers the function through registerUdf().
When a call to such a function is planned, UDFResolver asks Velox to bind the actual argument types: resolveFunction for a scalar, and a SignatureBinder pass over the registered aggregate signatures for a UDAF, which yields the return and intermediate types from the same bound signature. Results are memoized per (name, argument types), since this runs once per call site during analysis on the driver, where the libraries are already loaded.
This is worth doing for any function, and it is the only practical option for one whose signature carries type variables -- array(T) -> T, or (K, V) -> map(K,V) -- where a UdfEntry means one entry per type combination.
Names declared this way join UDFNames / UDAFNames, so the existing offload gates -- VeloxHiveUDFTransformer, getFunctionDescriptions, HashAggregateExecTransformer and the AggregateRel validator -- pick them up unchanged. A UdfEntry still wins over a by-name declaration of the same name, so a library can pin one call shape by hand and leave the rest to Velox. Binding is exact: Velox does not support coercion for signatures carrying type variables, so udfAllowTypeConversion does not extend to them, and a call that binds to nothing falls back to the JVM as before.
## How was this patch tested?
**`UDFResolverSuite`** — the resolver in isolation, no native library loaded (4 new):
- a UDF and a UDAF declared by name alone land in `UDFNames` / `UDAFNames`, so they reach
the existing offload gates
- an unregistered name is still reported as unsupported. `getUdfExpression` used to fail
inside `UDFMap.getOrElse`; a by-name declaration has no `UDFMap` entry, so the miss is now
caught after binding and both paths are pinned
**`VeloxUdfSuiteLocal`** — against `libmyudf` / `libmyudaf` loaded through
`spark.gluten.sql.columnar.backend.velox.udfLibraryPaths` (6 new). This exercises the whole
path: declaration read from the `.so`, registration over JNI, binding against the Velox
registry, and the resulting plan.
- `myudf_map_cardinality` on `map<string,double>`: offloads to `ProjectExecTransformer` and
resolves a `bigint` return type, with no signature ever stated to Gluten
- the same on `map<string,array<bigint>>` — a nested value type, the shape an enumerated
list of signatures would have to spell out
- a call on an `array` argument binds to nothing and is rejected; the test walks the cause
chain and requires the resolver's own `GlutenNotSupportException`, so it cannot pass on an
unrelated analysis failure
- `myudaf_arbitrary` resolves both its return type and its aggregation buffer from the bound
signature, over `bigint`, `string` and a nested map, and reports a wrong-arity call
- a grouped aggregation over `myudaf_arbitrary` splits into partial and final stages around a
shuffle, asserting two `HashAggregateExecTransformer` nodes with a `ShuffleExchangeLike`
between them and the correct answers
## Was this patch authored or co-authored using generative AI tooling?
co-authored with Claude Opus 5
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL] Let a UDF/UDAF library declare a function by name [gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #13105:
URL: https://github.com/apache/gluten/pull/13105#issuecomment-5898557414
Run Gluten Clickhouse CI on x86
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL] Let a UDF/UDAF library declare a function by name [gluten]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #13105:
URL: https://github.com/apache/gluten/pull/13105#discussion_r4098338817
##########
backends-velox/src/test/scala/org/apache/gluten/expression/UDFResolverSuite.scala:
##########
@@ -74,4 +76,32 @@ class UDFResolverSuite extends AnyFunSuite with BeforeAndAfterEach {
UDFResolver.UDFNames ++= Seq("b_udf", "a_udf")
assert(describedNames() == Seq("a_udf", "b_udf"))
}
+
+ // Looking up an unregistered name used to fail inside UDFMap.getOrElse. A function declared
+ // by name alone has no UDFMap entry, so the lookup no longer throws there and the miss has to
+ // be caught after binding instead.
+ test("an unregistered udf is not supported") {
+ intercept[GlutenNotSupportException] {
+ UDFResolver.getUdfExpression("not_registered", "not_registered")(Seq(Literal(1)))
+ }
+ }
+
+ test("an unregistered udaf is not supported") {
+ intercept[GlutenNotSupportException] {
+ UDFResolver.getUdafExpression("not_registered")(Seq(Literal(1)))
+ }
+ }
+
+ // A function declared by name alone is offloaded through the same UDFNames / UDAFNames gates
+ // as one with a stated signature; only where its types come from differs.
+ test("a udf declared by name alone is registered") {
+ UDFResolver.registerRegistryUDF("myudf_map_cardinality")
+ assert(UDFResolver.UDFNames.contains("myudf_map_cardinality"))
+ assert(describedNames() == Seq("myudf_map_cardinality"))
+ }
+
+ test("a udaf declared by name alone is registered") {
+ UDFResolver.registerRegistryUDAF("myudaf_arbitrary")
Review Comment:
This test mutates both the JVM-global UDAFNames and the private RegistryUDAFNames set, but beforeEach/afterEach only saves and restores UDFNames. The registry name therefore remains registered after this test, so later tests can attempt native resolution for myudaf_arbitrary without a library and become order-dependent. Extend the fixture cleanup (or otherwise isolate this registration) before adding this test.
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL] Let a UDF/UDAF library declare a function by name [gluten]
Posted by "kevinwilfong (via GitHub)" <gi...@apache.org>.
kevinwilfong commented on code in PR #13105:
URL: https://github.com/apache/gluten/pull/13105#discussion_r4098473448
##########
backends-velox/src/test/scala/org/apache/gluten/expression/UDFResolverSuite.scala:
##########
@@ -74,4 +76,32 @@ class UDFResolverSuite extends AnyFunSuite with BeforeAndAfterEach {
UDFResolver.UDFNames ++= Seq("b_udf", "a_udf")
assert(describedNames() == Seq("a_udf", "b_udf"))
}
+
+ // Looking up an unregistered name used to fail inside UDFMap.getOrElse. A function declared
+ // by name alone has no UDFMap entry, so the lookup no longer throws there and the miss has to
+ // be caught after binding instead.
+ test("an unregistered udf is not supported") {
+ intercept[GlutenNotSupportException] {
+ UDFResolver.getUdfExpression("not_registered", "not_registered")(Seq(Literal(1)))
+ }
+ }
+
+ test("an unregistered udaf is not supported") {
+ intercept[GlutenNotSupportException] {
+ UDFResolver.getUdafExpression("not_registered")(Seq(Literal(1)))
+ }
+ }
+
+ // A function declared by name alone is offloaded through the same UDFNames / UDAFNames gates
+ // as one with a stated signature; only where its types come from differs.
+ test("a udf declared by name alone is registered") {
+ UDFResolver.registerRegistryUDF("myudf_map_cardinality")
+ assert(UDFResolver.UDFNames.contains("myudf_map_cardinality"))
+ assert(describedNames() == Seq("myudf_map_cardinality"))
+ }
+
+ test("a udaf declared by name alone is registered") {
+ UDFResolver.registerRegistryUDAF("myudaf_arbitrary")
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL] Let a UDF/UDAF library declare a function by name [gluten]
Posted by "kevinwilfong (via GitHub)" <gi...@apache.org>.
kevinwilfong commented on PR #13105:
URL: https://github.com/apache/gluten/pull/13105#issuecomment-5900460599
> @kevinwilfong Thanks for proposing this change and it overall looks good. IIUC the current gap for the proposed approach vs current approach lies in that declare-by-name doesn't support type conversion.
>
> The major concern from my side is that keeping both approaches makes things harder for users. They need to understand the difference between using `UdfEntry` and declare-by-name, and remember that type conversion only works with the UdfEntry.
>
> I haven't looked into details but maybe we can use the helper function `resolveFunctionWithCoercions` in velox and implement the same cast rule for spark. If so, the current approach can be fully replaced by the declare-by-name.
@marin-ma Thanks for taking a look! I agree I would love to see this be the default instead of supporting both ways of registering functions, however I do worry that it represents a substantial change in behavior that will break existing customers.
I dug through it a bit and it seems like in most cases Spark has already taken care of type coercion by the time we've gotten to Gluten's UDF/UDAF resolution, so we should probably be doing an exact match or risk accidentally introducing behavior differences. The exceptions are HiveSimpleUDF and UDAFs wrapped by GenericUDAFBridge which rely on Hive ObjectInspectors to do the coercion at UDF runtime.
So it looks like we need to support Hive ObjectInspector type coercion rules rather than Spark's for those cases.
I went ahead and implemented that using resolveFunctionWithCoercions. There's a couple known gaps:
1) Decimal and Unknown (probably not relevant) aren't supported in VeloxToSubstrait so we can't coerce to types containing these or functions returning these
2) Velox doesn't currently have a way to say we don't support coercing complex types
I'm happy to address those here or in follow ups.
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL] Let a UDF/UDAF library declare a function by name [gluten]
Posted by "zhztheplayer (via GitHub)" <gi...@apache.org>.
zhztheplayer commented on code in PR #13105:
URL: https://github.com/apache/gluten/pull/13105#discussion_r4090992871
##########
backends-velox/src/main/scala/org/apache/spark/sql/expression/UDFResolver.scala:
##########
@@ -386,14 +422,62 @@ object UDFResolver extends Logging {
}
}
+ /**
+ * Resolves the types of a call by binding 'argTypes' against the signatures the library
+ * registered with Velox for 'name'. Returns the resolved types, or None if nothing binds.
+ *
+ * Binding is exact: no cast is injected to make a call fit. Velox does not support coercion for
+ * signatures carrying type variables, and where one is in play "cast the arguments until they
+ * bind" has too many answers to pick from.
+ */
+ private def resolveFromRegistry(
+ name: String,
+ argTypes: Seq[DataType],
+ isAggregate: Boolean): RegistryResolution = {
+ def resolve(): RegistryResolution = {
+ // Velox types carry no nullability, so it is not part of the question
+ // being asked here.
+ val argTypeNodes = argTypes.map(t => ConverterUtils.getTypeNode(t, nullable = true))
+ val serialized = TypeBuilder.makeStruct(false, argTypeNodes.asJava).toProtobuf.toByteArray
+
+ val resolved = if (isAggregate) {
+ UdfJniWrapper.resolveUdafTypes(name, serialized)
+ } else {
+ UdfJniWrapper.resolveUdfType(name, serialized)
+ }
+
+ if (resolved == null) {
+ logDebug(
+ s"No Velox signature of $name binds to " +
+ s"${argTypes.map(_.simpleString).mkString(", ")}.")
+ None
+ } else if (isAggregate) {
+ // {returnType, intermediateType}, as one struct.
+ val fields = ConverterUtils
+ .parseFromBytes(resolved)
+ .dataType
+ .asInstanceOf[StructType]
+ .fields
+ Some(fields.map(f => ExpressionType(f.dataType, f.nullable)).toSeq)
+ } else {
+ Some(Seq(ConverterUtils.parseFromBytes(resolved)))
+ }
+ }
+
+ registryResolutions.computeIfAbsent((name, argTypes), _ => resolve())
Review Comment:
Can the code be strengthened, in case the same name is registered as both an UDF and an UDAF? E.g., should we have both `registryUdfResolutions` and `registryUdafResolutions`?
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL] Let a UDF/UDAF library declare a function by name [gluten]
Posted by "kevinwilfong (via GitHub)" <gi...@apache.org>.
kevinwilfong commented on PR #13105:
URL: https://github.com/apache/gluten/pull/13105#issuecomment-5824220496
> > Maybe an off-topic, but I also noticed the bare native UDAFs are not registered to Spark function registry, since `getFunctionDescriptions` only handles the native UDFs. Is there any tech blocker against registering UDAFs as well? Or it was left unsupported on purpose?
>
> This is in regards to #13016 ? No, I don't think there's any technical limitations preventing us from registering UDAFs as well, I can put up a PR for that.
>
> Note that the change in this PR is somewhat orthogonal, my main motivation here was allowing us to simplify Velox UDFs with corresponding Spark implementations, though registering Velox-ony UDFs in the manner that PR allows should benefit in the same way.
Filed https://github.com/apache/gluten/pull/13124 for this
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL] Let a UDF/UDAF library declare a function by name [gluten]
Posted by "kevinwilfong (via GitHub)" <gi...@apache.org>.
kevinwilfong commented on code in PR #13105:
URL: https://github.com/apache/gluten/pull/13105#discussion_r4098235443
##########
backends-velox/src/main/scala/org/apache/spark/sql/expression/UDFResolver.scala:
##########
@@ -386,14 +422,62 @@ object UDFResolver extends Logging {
}
}
+ /**
+ * Resolves the types of a call by binding 'argTypes' against the signatures the library
+ * registered with Velox for 'name'. Returns the resolved types, or None if nothing binds.
+ *
+ * Binding is exact: no cast is injected to make a call fit. Velox does not support coercion for
+ * signatures carrying type variables, and where one is in play "cast the arguments until they
+ * bind" has too many answers to pick from.
+ */
+ private def resolveFromRegistry(
+ name: String,
+ argTypes: Seq[DataType],
+ isAggregate: Boolean): RegistryResolution = {
+ def resolve(): RegistryResolution = {
+ // Velox types carry no nullability, so it is not part of the question
+ // being asked here.
+ val argTypeNodes = argTypes.map(t => ConverterUtils.getTypeNode(t, nullable = true))
+ val serialized = TypeBuilder.makeStruct(false, argTypeNodes.asJava).toProtobuf.toByteArray
+
+ val resolved = if (isAggregate) {
+ UdfJniWrapper.resolveUdafTypes(name, serialized)
+ } else {
+ UdfJniWrapper.resolveUdfType(name, serialized)
+ }
+
+ if (resolved == null) {
+ logDebug(
+ s"No Velox signature of $name binds to " +
+ s"${argTypes.map(_.simpleString).mkString(", ")}.")
+ None
+ } else if (isAggregate) {
+ // {returnType, intermediateType}, as one struct.
+ val fields = ConverterUtils
+ .parseFromBytes(resolved)
+ .dataType
+ .asInstanceOf[StructType]
+ .fields
+ Some(fields.map(f => ExpressionType(f.dataType, f.nullable)).toSeq)
+ } else {
+ Some(Seq(ConverterUtils.parseFromBytes(resolved)))
+ }
+ }
+
+ registryResolutions.computeIfAbsent((name, argTypes), _ => resolve())
Review Comment:
Good call, fixed, I separated registryUdfResolutions and registryUdafResolutions and the resolvers.
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL] Let a UDF/UDAF library declare a function by name [gluten]
Posted by "zhztheplayer (via GitHub)" <gi...@apache.org>.
zhztheplayer commented on PR #13105:
URL: https://github.com/apache/gluten/pull/13105#issuecomment-5810307068
cc @marin-ma
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL] Let a UDF/UDAF library declare a function by name [gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #13105:
URL: https://github.com/apache/gluten/pull/13105#issuecomment-5896360734
Run Gluten Clickhouse CI on x86
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL] Let a UDF/UDAF library declare a function by name [gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #13105:
URL: https://github.com/apache/gluten/pull/13105#issuecomment-5900357072
Run Gluten Clickhouse CI on x86
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL] Let a UDF/UDAF library declare a function by name [gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #13105:
URL: https://github.com/apache/gluten/pull/13105#issuecomment-5897575185
Run Gluten Clickhouse CI on x86
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL] Let a UDF/UDAF library declare a function by name [gluten]
Posted by "kevinwilfong (via GitHub)" <gi...@apache.org>.
kevinwilfong commented on code in PR #13105:
URL: https://github.com/apache/gluten/pull/13105#discussion_r4139061321
##########
backends-velox/src/main/scala/org/apache/spark/sql/expression/UDFResolver.scala:
##########
@@ -386,14 +430,80 @@ object UDFResolver extends Logging {
}
}
+ // Velox types carry no nullability, so it is not part of the question being asked here.
+ private def encodeArgTypes(argTypes: Seq[DataType]): Array[Byte] = {
+ val argTypeNodes = argTypes.map(t => ConverterUtils.getTypeNode(t, nullable = true))
+ TypeBuilder.makeStruct(false, argTypeNodes.asJava).toProtobuf.toByteArray
+ }
+
+ private def logNoBind(name: String, argTypes: Seq[DataType]): Unit =
+ logDebug(
+ s"No Velox signature of $name binds to ${argTypes.map(_.simpleString).mkString(", ")}.")
+
+ /**
+ * Resolves the return type of a scalar call by binding 'argTypes' against the signatures the
+ * library registered with Velox for 'name'. Returns None if nothing binds.
+ *
+ * Binding is exact: no cast is injected to make a call fit. Velox does not support coercion for
+ * signatures carrying type variables, and where one is in play "cast the arguments until they
+ * bind" has too many answers to pick from.
+ */
+ private def resolveUdfFromRegistry(
Review Comment:
Done
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL] Let a UDF/UDAF library declare a function by name [gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #13105:
URL: https://github.com/apache/gluten/pull/13105#issuecomment-5899971904
Run Gluten Clickhouse CI on x86
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL] Let a UDF/UDAF library declare a function by name [gluten]
Posted by "kevinwilfong (via GitHub)" <gi...@apache.org>.
kevinwilfong commented on PR #13105:
URL: https://github.com/apache/gluten/pull/13105#issuecomment-5822004929
> Maybe an off-topic, but I also noticed the bare native UDAFs are not registered to Spark function registry, since `getFunctionDescriptions` only handles the native UDFs. Is there any tech blocker against registering UDAFs as well? Or it was left unsupported on purpose?
This is in regards to https://github.com/apache/gluten/pull/13016 ? No, I don't think there's any technical limitations preventing us from registering UDAFs as well, I can put up a PR for that.
Note that the change in this PR is somewhat orthogonal, my main motivation here was allowing us to simplify Velox UDFs with corresponding Spark implementations, though registering Velox-ony UDFs in the manner that PR allows should benefit in the same way.
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL] Let a UDF/UDAF library declare a function by name [gluten]
Posted by "marin-ma (via GitHub)" <gi...@apache.org>.
marin-ma commented on code in PR #13105:
URL: https://github.com/apache/gluten/pull/13105#discussion_r4103239776
##########
backends-velox/src/main/scala/org/apache/spark/sql/expression/UDFResolver.scala:
##########
@@ -386,14 +430,80 @@ object UDFResolver extends Logging {
}
}
+ // Velox types carry no nullability, so it is not part of the question being asked here.
+ private def encodeArgTypes(argTypes: Seq[DataType]): Array[Byte] = {
+ val argTypeNodes = argTypes.map(t => ConverterUtils.getTypeNode(t, nullable = true))
+ TypeBuilder.makeStruct(false, argTypeNodes.asJava).toProtobuf.toByteArray
+ }
+
+ private def logNoBind(name: String, argTypes: Seq[DataType]): Unit =
+ logDebug(
+ s"No Velox signature of $name binds to ${argTypes.map(_.simpleString).mkString(", ")}.")
+
+ /**
+ * Resolves the return type of a scalar call by binding 'argTypes' against the signatures the
+ * library registered with Velox for 'name'. Returns None if nothing binds.
+ *
+ * Binding is exact: no cast is injected to make a call fit. Velox does not support coercion for
+ * signatures carrying type variables, and where one is in play "cast the arguments until they
+ * bind" has too many answers to pick from.
+ */
+ private def resolveUdfFromRegistry(
Review Comment:
Should we catch any exceptions thrown from `resolveUdfFromRegistry` and re-throw `GlutenNotSupportException` to allow fallback? Same for `resolveUdafFromRegistry`
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL] Let a UDF/UDAF library declare a function by name [gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #13105:
URL: https://github.com/apache/gluten/pull/13105#issuecomment-5900129247
Run Gluten Clickhouse CI on x86
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL] Let a UDF/UDAF library declare a function by name [gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #13105:
URL: https://github.com/apache/gluten/pull/13105#issuecomment-5899813506
Run Gluten Clickhouse CI on x86
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL] Let a UDF/UDAF library declare a function by name [gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #13105:
URL: https://github.com/apache/gluten/pull/13105#issuecomment-5900563291
Run Gluten Clickhouse CI on x86
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL] Let a UDF/UDAF library declare a function by name [gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #13105:
URL: https://github.com/apache/gluten/pull/13105#issuecomment-5896710827
Run Gluten Clickhouse CI on x86
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL] Let a UDF/UDAF library declare a function by name [gluten]
Posted by "zhztheplayer (via GitHub)" <gi...@apache.org>.
zhztheplayer commented on code in PR #13105:
URL: https://github.com/apache/gluten/pull/13105#discussion_r4090992871
##########
backends-velox/src/main/scala/org/apache/spark/sql/expression/UDFResolver.scala:
##########
@@ -386,14 +422,62 @@ object UDFResolver extends Logging {
}
}
+ /**
+ * Resolves the types of a call by binding 'argTypes' against the signatures the library
+ * registered with Velox for 'name'. Returns the resolved types, or None if nothing binds.
+ *
+ * Binding is exact: no cast is injected to make a call fit. Velox does not support coercion for
+ * signatures carrying type variables, and where one is in play "cast the arguments until they
+ * bind" has too many answers to pick from.
+ */
+ private def resolveFromRegistry(
+ name: String,
+ argTypes: Seq[DataType],
+ isAggregate: Boolean): RegistryResolution = {
+ def resolve(): RegistryResolution = {
+ // Velox types carry no nullability, so it is not part of the question
+ // being asked here.
+ val argTypeNodes = argTypes.map(t => ConverterUtils.getTypeNode(t, nullable = true))
+ val serialized = TypeBuilder.makeStruct(false, argTypeNodes.asJava).toProtobuf.toByteArray
+
+ val resolved = if (isAggregate) {
+ UdfJniWrapper.resolveUdafTypes(name, serialized)
+ } else {
+ UdfJniWrapper.resolveUdfType(name, serialized)
+ }
+
+ if (resolved == null) {
+ logDebug(
+ s"No Velox signature of $name binds to " +
+ s"${argTypes.map(_.simpleString).mkString(", ")}.")
+ None
+ } else if (isAggregate) {
+ // {returnType, intermediateType}, as one struct.
+ val fields = ConverterUtils
+ .parseFromBytes(resolved)
+ .dataType
+ .asInstanceOf[StructType]
+ .fields
+ Some(fields.map(f => ExpressionType(f.dataType, f.nullable)).toSeq)
+ } else {
+ Some(Seq(ConverterUtils.parseFromBytes(resolved)))
+ }
+ }
+
+ registryResolutions.computeIfAbsent((name, argTypes), _ => resolve())
Review Comment:
Can the code be strengthened, in case the same name is registered as both an UDF and an UDAF? E.g., should we have both `registryUdfResolutions` and `registryUdafResolutions`, or simply throw error for duplication?
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org