andygrove commented on code in PR #4459:
URL: https://github.com/apache/datafusion-comet/pull/4459#discussion_r3708874153
##########
native/proto/src/proto/expr.proto:
##########
@@ -530,3 +531,22 @@ message JvmScalarUdf {
// Whether the result column may contain nulls.
bool return_nullable = 4;
}
+
+// Call to a user-supplied Rust UDF loaded from a cdylib.
+//
+// The native side resolves (library_path, name) against its loaded-library
+// cache, looks up the kernel by name, and invokes it through whichever ABI
+// flavor (C ABI / datafusion-ffi) the cdylib registered the kernel under.
+message RustUdfCall {
+ // Function name as registered through CometRustUDF.register on the JVM
+ // side; matched against names exposed by the cdylib.
+ string name = 1;
+ // Filesystem path of the cdylib.
+ string library_path = 2;
+ // Argument expressions, evaluated before invocation.
+ repeated Expr args = 3;
+ // Expected return type, declared at register time on the JVM side.
+ DataType return_type = 4;
Review Comment:
Revisiting this now that the ABI has settled, because my earlier answer
undersold what it does.
The return type *is* computed on demand.
`CometCScalarUdf::return_field(&self, args: &[Field])` is called with the
actual argument types and derives the output type per call site, so a kernel
has no fixed return type of its own. The test library now leans on that: a
single `echo_c` kernel serves all 19 types in the suite, from `Boolean` through
`decimal(10,2)` to `array<struct<a:int>>`, returning whatever it is handed.
What is fixed is the type declared to `CometRustUDF.register`, and that is a
Spark constraint rather than an ABI one: Spark needs a concrete `DataType` at
analysis time to plan the query and to install the catalog entry. So the
declaration is per-registration, not per-kernel. Re-registering the same
function under a different signature works, and the kernel computes the
matching return type each time. There is a test covering exactly that.
The proto field is therefore best read as "what Spark was told", not "the
only type this UDF can return". Comet now checks the two against each other at
planning time and fails with both types named if they disagree, which replaced
a bare DataFusion type assertion that used to fire partway through execution.
That check turned out to earn its keep immediately: Spark widens `decimal(10,2)
+ 0.25` to `decimal(11,2)`, and registering the un-widened type is an easy
mistake to make.
One wrinkle worth recording, since it also shaped the check. Spark carries
`containsNull` and struct field nullability inside the type, so `array<int>`
with `containsNull = false` converts to a `List` with a non-nullable child,
while the array actually delivered to the UDF has that child normalized to
nullable. The comparison erases nested nullability for that reason, but stays
strict about everything that changes how bytes are read: decimal precision and
scale, timestamp unit, and struct field names and order.
On extension types: agreed, and I have not tried to address it here. Variant
and Geometry would need the type representation in `expr.proto` to grow an
extension-type concept, which affects far more than this path, so it seems
right as a standalone change. I have noted it in the PR's follow-on list so it
does not get lost.
--
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]
