-
Notifications
You must be signed in to change notification settings - Fork 0
4283: feat: custom Rust UDFs [experimental] [skip ci] #56
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
9f51ebc
05770a5
c4a59fe
97b22e6
e0d7669
275fc86
fea30b5
b345d61
54652a2
4bfbc2d
51cf22d
d058ee8
88a6925
fa3ebf8
e824d82
b4bec8c
174003c
5adb3e9
c8f3f66
797b026
acd738d
62a948d
894c507
2da5056
1900f48
a8f8eca
4f18e39
6a548ba
6fbde43
f6d420b
548b7a5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,42 @@ | ||
| /* | ||
| * Licensed to the Apache Software Foundation (ASF) under one | ||
| * or more contributor license agreements. See the NOTICE file | ||
| * distributed with this work for additional information | ||
| * regarding copyright ownership. The ASF licenses this file | ||
| * to you under the Apache License, Version 2.0 (the | ||
| * "License"); you may not use this file except in compliance | ||
| * with the License. You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, | ||
| * software distributed under the License is distributed on an | ||
| * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
| * KIND, either express or implied. See the License for the | ||
| * specific language governing permissions and limitations | ||
| * under the License. | ||
| */ | ||
|
|
||
| package org.apache.comet.udf; | ||
|
|
||
| import org.apache.comet.NativeBase; | ||
|
|
||
| /** JNI bridge for driver-side Rust UDF library validation. */ | ||
| public final class CometRustUdfBridge extends NativeBase { | ||
| private CometRustUdfBridge() {} | ||
|
|
||
| /** | ||
| * Validate that {@code libraryPath} loads, exposes a UDF named {@code expectedName}, and return a | ||
| * JSON description of that UDF. Throws RuntimeException on any error. | ||
| * | ||
| * <p>The returned JSON has the form: {@code | ||
| * {"name":"add_one","args":["Int64"],"return_type":"Int64","volatility":0}} | ||
| */ | ||
| public static native String validateLibrary(String libraryPath, String expectedName); | ||
|
|
||
| /** | ||
| * Return a JSON array describing every UDF exposed by {@code libraryPath}. Each element has the | ||
| * same shape as the return value of {@link #validateLibrary}. | ||
| */ | ||
| public static native String listUdfs(String libraryPath); | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,210 @@ | ||
| /* | ||
| * Licensed to the Apache Software Foundation (ASF) under one | ||
| * or more contributor license agreements. See the NOTICE file | ||
| * distributed with this work for additional information | ||
| * regarding copyright ownership. The ASF licenses this file | ||
| * to you under the Apache License, Version 2.0 (the | ||
| * "License"); you may not use this file except in compliance | ||
| * with the License. You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, | ||
| * software distributed under the License is distributed on an | ||
| * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
| * KIND, either express or implied. See the License for the | ||
| * specific language governing permissions and limitations | ||
| * under the License. | ||
| */ | ||
|
|
||
| package org.apache.comet.udf | ||
|
|
||
| import scala.util.Try | ||
|
|
||
| import org.apache.spark.sql.SparkSession | ||
| import org.apache.spark.sql.expressions.UserDefinedFunction | ||
| import org.apache.spark.sql.functions.udf | ||
| import org.apache.spark.sql.types.{BinaryType, BooleanType, ByteType, DataType, DoubleType, FloatType, IntegerType, LongType, ShortType, StringType} | ||
|
|
||
| import com.fasterxml.jackson.databind.ObjectMapper | ||
| import com.fasterxml.jackson.databind.node.{ArrayNode, ObjectNode} | ||
|
|
||
| /** | ||
| * Public entry point for registering Rust scalar UDFs with Comet. | ||
| * | ||
| * See `docs/source/user-guide/latest/custom-rust-udfs.md` for an end-to-end walkthrough. | ||
| */ | ||
| object CometRustUDF { | ||
|
|
||
| /** Spark conf key under which registered Rust UDF entries are propagated to executors. */ | ||
| val RUST_UDFS_CONF_KEY = "spark.comet.rustUdfs" | ||
|
|
||
| private val mapper: ObjectMapper = new ObjectMapper() | ||
|
|
||
| /** | ||
| * Register a single Rust UDF with an explicit signature. | ||
| * | ||
| * Validates the library on the driver, throws if loading fails or the declared signature | ||
| * differs from what the library reports. On success: a stub Spark catalog UDF is registered (so | ||
| * SQL/DataFrame reference checks pass), the driver-side registry is updated, and the conf key | ||
| * `spark.comet.rustUdfs` is updated so executors see the registration. | ||
| */ | ||
| def register( | ||
| spark: SparkSession, | ||
| name: String, | ||
| libraryPath: String, | ||
| inputTypes: Seq[DataType], | ||
| returnType: DataType, | ||
| deterministic: Boolean = true): Unit = { | ||
| val described = describeOne(libraryPath, name) | ||
| requireSignatureMatch(name, described, inputTypes, returnType) | ||
| installCatalogStub(spark, name, inputTypes, returnType, deterministic) | ||
| val meta = RustUdfMetadata(libraryPath, inputTypes, returnType, deterministic) | ||
| CometRustUdfRegistry.instance.register(name, meta) | ||
| propagateConf(spark) | ||
| } | ||
|
|
||
| /** | ||
| * Register every UDF exposed by `libraryPath`, using each UDF's discovered signature. Returns | ||
| * the list of names registered. | ||
| */ | ||
| def registerAll(spark: SparkSession, libraryPath: String): Seq[String] = { | ||
| val descs = describeAll(libraryPath) | ||
| descs.map { d => | ||
| val args = d.args.map(parseSparkType) | ||
| val ret = parseSparkType(d.returnType) | ||
| val deterministic = d.volatility != 2 // Volatile = non-deterministic | ||
| installCatalogStub(spark, d.name, args, ret, deterministic) | ||
| val meta = RustUdfMetadata(libraryPath, args, ret, deterministic) | ||
| CometRustUdfRegistry.instance.register(d.name, meta) | ||
| propagateConf(spark) | ||
| d.name | ||
| } | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. registerAll breaks on struct typesHigh Severity
Additional Locations (1)Reviewed by Cursor Bugbot for commit 548b7a5. Configure here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. registerAll leaves partial registrationsMedium Severity
Reviewed by Cursor Bugbot for commit 548b7a5. Configure here.
Comment on lines
+73
to
+82
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Calling val names = descs.map { d =>
val args = d.args.map(parseSparkType)
val ret = parseSparkType(d.returnType)
val deterministic = d.volatility != 2 // Volatile = non-deterministic
installCatalogStub(spark, d.name, args, ret, deterministic)
val meta = RustUdfMetadata(libraryPath, args, ret, deterministic)
CometRustUdfRegistry.instance.register(d.name, meta)
d.name
}
propagateConf(spark)
names |
||
| } | ||
|
|
||
| // -------- internals -------- | ||
|
|
||
| private case class Described( | ||
| name: String, | ||
| args: Seq[String], | ||
| returnType: String, | ||
| volatility: Int) | ||
|
|
||
| private def describeOne(libraryPath: String, name: String): Described = { | ||
| val json = | ||
| invokeBridge(() => CometRustUdfBridge.validateLibrary(libraryPath, name), libraryPath) | ||
| parseDescribed(json) | ||
| } | ||
|
|
||
| private def describeAll(libraryPath: String): Seq[Described] = { | ||
| val json = invokeBridge(() => CometRustUdfBridge.listUdfs(libraryPath), libraryPath) | ||
| val arr = mapper.readTree(json) | ||
| require(arr.isArray, s"listUdfs returned non-array JSON: $json") | ||
| val out = scala.collection.mutable.ArrayBuffer[Described]() | ||
| val it = arr.elements() | ||
| while (it.hasNext) out += parseDescribed(it.next().toString) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| out.toSeq | ||
| } | ||
|
|
||
| private def invokeBridge(call: () => String, libraryPath: String): String = { | ||
| Try(call()).recover { case t: Throwable => throw classifyNativeError(libraryPath, t) }.get | ||
| } | ||
|
|
||
| private def parseDescribed(json: String): Described = { | ||
| val node = mapper.readTree(json).asInstanceOf[ObjectNode] | ||
| val argsArr = node.get("args").asInstanceOf[ArrayNode] | ||
| val args = scala.collection.mutable.ArrayBuffer[String]() | ||
| val it = argsArr.elements() | ||
| while (it.hasNext) args += it.next().asText() | ||
| Described( | ||
| name = node.get("name").asText(), | ||
| args = args.toSeq, | ||
| returnType = node.get("return_type").asText(), | ||
| volatility = node.get("volatility").asInt()) | ||
| } | ||
|
|
||
| private def classifyNativeError(libraryPath: String, t: Throwable): RuntimeException = { | ||
| val m = Option(t.getMessage).getOrElse("") | ||
| if (m.contains("ABI") || m.contains("missing required symbol")) { | ||
| new CometRustUdfAbiException(m) | ||
| } else if (m.contains("not found in")) { | ||
| new java.util.NoSuchElementException(m) | ||
| } else { | ||
| new CometRustUdfLoadException(s"failed to load $libraryPath: $m", t) | ||
| } | ||
| } | ||
|
|
||
| private def requireSignatureMatch( | ||
| name: String, | ||
| d: Described, | ||
| declaredArgs: Seq[DataType], | ||
| declaredReturn: DataType): Unit = { | ||
| val nativeArgs = d.args.map(parseSparkType) | ||
| val nativeReturn = parseSparkType(d.returnType) | ||
| if (nativeArgs != declaredArgs) { | ||
| throw new CometRustUdfSignatureException( | ||
| s"UDF '$name' arg types: native=$nativeArgs, declared=$declaredArgs") | ||
| } | ||
| if (nativeReturn != declaredReturn) { | ||
| throw new CometRustUdfSignatureException( | ||
| s"UDF '$name' return type: native=$nativeReturn, declared=$declaredReturn") | ||
| } | ||
| } | ||
|
|
||
| /** Parse a Comet-emitted Arrow type string into a Spark DataType. */ | ||
| private def parseSparkType(s: String): DataType = s match { | ||
| case "Boolean" => BooleanType | ||
| case "Int8" => ByteType | ||
| case "Int16" => ShortType | ||
| case "Int32" => IntegerType | ||
| case "Int64" => LongType | ||
| case "Float32" => FloatType | ||
| case "Float64" => DoubleType | ||
| case "Utf8" | "LargeUtf8" => StringType | ||
| case "Binary" | "LargeBinary" => BinaryType | ||
| case other => | ||
| // For complex/nested types: attempt to parse via Spark's DDL parser. | ||
| // The mapping is approximate for v1 — primitives are exact; | ||
| // nested types may need refinement in a follow-up. | ||
| DataType.fromDDL(other) | ||
|
Comment on lines
+165
to
+169
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| } | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing unsigned and date mappingsMedium Severity
Additional Locations (1)Reviewed by Cursor Bugbot for commit 548b7a5. Configure here.
Comment on lines
+137
to
+170
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🌐 Web query:
💡 Result: Spark’s DataType.fromDDL(String ddl) accepts a Spark SQL schema DDL string that describes data types (including nested ones) using Spark’s DDL syntax. For StructType specifically, it’s a comma-separated list of field definitions, where each field definition is “name TYPE” (examples show field names in backticks, e.g. “ Citations:
🏁 Script executed: #!/bin/bash
set -euo pipefail
# Locate Comet-emitted arrow type strings usage/production
rg -n "parseSparkType\\(" -S common/src/main/scala/org/apache/comet/udf/CometRustUDF.scala
rg -n "UDF '.*' arg types|CometRustUdfSignatureException|returnType|args\\.map\\(parseSparkType\\)" -S common/src/main/scala/org/apache/comet/udf/CometRustUDF.scala
# Search for "to_string" or "DataType" string emission on Rust side if present in repo
rg -n "to_string\\(\\)|Display|fromDDL\\(|STRUCT<|ARRAY<|MAP<|LargeUtf8|LargeBinary|Dictionary\\(" -S .
# Find any code that builds the type strings for UDF descriptors
rg -n "native.*type|declaredReturn|declaredArgs|descriptor.*type|signature.*type|returnType" -S .Repository: martin-augment/datafusion-comet Length of output: 50388 🏁 Script executed: #!/bin/bash
set -euo pipefail
# Locate Comet-emitted arrow type strings usage/production
rg -n "parseSparkType\\(" -S common/src/main/scala/org/apache/comet/udf/CometRustUDF.scala
rg -n "CometRustUdfSignatureException|returnType|args\\.map\\(parseSparkType\\)|declaredArgs|declaredReturn" -S common/src/main/scala/org/apache/comet/udf/CometRustUDF.scala
# Search for where native/arrow type strings are produced/serialized
rg -n "to_string\\(\\)|DataType::to_string|FromStr|Display\\s*\\+|parse.*type|type_string|arrow.*type|struct\\(|list\\(|map\\(" -S .
# Search for spark DDL parsing usage
rg -n "fromDDL\\(" -S .
# Print nearby definitions for parseSparkType and requireSignatureMatch
sed -n '1,220p' common/src/main/scala/org/apache/comet/udf/CometRustUDF.scalaRepository: martin-augment/datafusion-comet Length of output: 50388 🏁 Script executed: #!/bin/bash
set -euo pipefail
FILE="common/src/main/scala/org/apache/comet/udf/CometRustUDF.scala"
# Show the relevant sections around requireSignatureMatch/parseSparkType and where `d` is parsed.
sed -n '1,220p' "$FILE" | sed -n '1,220p'
sed -n '220,420p' "$FILE"
# Locate the Described type definition and any JSON parsing for d.args / d.returnType
rg -n "Described" "$FILE"
rg -n "return_type|args\\b" "$FILE"
rg -n "node\\.get\\(\"return_type\"\\)|node\\.get\\(\"args\"\\)" "$FILE"Repository: martin-augment/datafusion-comet Length of output: 9327 🏁 Script executed: #!/bin/bash
set -euo pipefail
FILE="common/src/main/scala/org/apache/comet/udf/CometRustUDF.scala"
# Narrowly inspect around the signature parsing and where `d` is constructed
sed -n '90,220p' "$FILE"
sed -n '220,340p' "$FILE"
# Find the `Described` type definition and how `d.args` / `d.returnType` are parsed/constructed
rg -n "type Described|case class Described|class Described|Described\\b" "$FILE"
rg -n "return_type|args\\b" "$FILE"
rg -n "node\\.get\\(\"return_type\"\\)|node\\.get\\(\"args\"\\)" "$FILE"Repository: martin-augment/datafusion-comet Length of output: 5856 🏁 Script executed: #!/bin/bash
set -euo pipefail
rg -n "\"return_type\"|return_type\\\"|returnType\"|listUdfs\\(|validateLibrary\\(|CometRustUdfBridge" -S native common spark .Repository: martin-augment/datafusion-comet Length of output: 254 🏁 Script executed: #!/bin/bash
set -euo pipefail
# Search for the bridge implementation (Scala/JNI) that produces the JSON consumed by CometRustUDF.scala
rg -n "CometRustUdfBridge" -S .
rg -n "\"return_type\"" -S .
rg -n "\"args\"" -S common native spark . || true
# Also search for listUdfs/validateLibrary symbols
rg -n "listUdfs|validateLibrary" -S .
# If there are Rust functions, narrow further
rg -n "return_type|listUdfs|validateLibrary" -S native .Repository: martin-augment/datafusion-comet Length of output: 25136 🏁 Script executed: #!/bin/bash
set -euo pipefail
sed -n '1,140p' native/core/src/comet_rust_udf_bridge.rsRepository: martin-augment/datafusion-comet Length of output: 3851 🏁 Script executed: #!/bin/bash
set -euo pipefail
rg -n "return_type" native/core/src/comet_rust_udf_bridge.rs
rg -n "\"args\"|args" native/core/src/comet_rust_udf_bridge.rs
sed -n '1,120p' native/comet-udf-sdk/src/types.rs
sed -n '150,280p' native/comet-udf-sdk/src/types.rs || trueRepository: martin-augment/datafusion-comet Length of output: 10015 Complex Rust UDF signatures won’t round-trip through this parser.
Fix by implementing a real Arrow→Spark 🤖 Prompt for AI Agents |
||
|
|
||
| private def installCatalogStub( | ||
| spark: SparkSession, | ||
| name: String, | ||
| inputTypes: Seq[DataType], | ||
| returnType: DataType, | ||
| deterministic: Boolean): Unit = { | ||
| val arity = inputTypes.size | ||
| val u: UserDefinedFunction = arity match { | ||
| case 0 => | ||
| udf(() => throw new CometRustUdfNotEvaluatedException(name), returnType) | ||
| case 1 => | ||
| udf((_: Any) => throw new CometRustUdfNotEvaluatedException(name), returnType) | ||
| case 2 => | ||
| udf((_: Any, _: Any) => throw new CometRustUdfNotEvaluatedException(name), returnType) | ||
| case 3 => | ||
| udf( | ||
| (_: Any, _: Any, _: Any) => throw new CometRustUdfNotEvaluatedException(name), | ||
| returnType) | ||
| case 4 => | ||
| udf( | ||
| (_: Any, _: Any, _: Any, _: Any) => throw new CometRustUdfNotEvaluatedException(name), | ||
| returnType) | ||
| case n => | ||
| throw new IllegalArgumentException( | ||
| s"Rust UDF '$name' arity $n not supported by stub. Reduce arity " + | ||
| s"or open a feature request to extend stub coverage.") | ||
| } | ||
| val finalUdf = if (deterministic) u else u.asNondeterministic() | ||
| spark.udf.register(name, finalUdf) | ||
| } | ||
|
|
||
| private def propagateConf(spark: SparkSession): Unit = { | ||
| val snapshot = CometRustUdfRegistry.instance.snapshot | ||
| val entries = snapshot.toSeq.map { case (name, meta) => | ||
| mapper.writeValueAsString(java.util.Map.of("name", name, "libraryPath", meta.libraryPath)) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
val map = new java.util.HashMap[String, String]()
map.put("name", name)
map.put("libraryPath", meta.libraryPath)
mapper.writeValueAsString(map) |
||
| } | ||
| spark.conf.set(RUST_UDFS_CONF_KEY, entries.mkString(";")) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Severity: low 🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage. |
||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,50 @@ | ||
| /* | ||
| * Licensed to the Apache Software Foundation (ASF) under one | ||
| * or more contributor license agreements. See the NOTICE file | ||
| * distributed with this work for additional information | ||
| * regarding copyright ownership. The ASF licenses this file | ||
| * to you under the Apache License, Version 2.0 (the | ||
| * "License"); you may not use this file except in compliance | ||
| * with the License. You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, | ||
| * software distributed under the License is distributed on an | ||
| * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
| * KIND, either express or implied. See the License for the | ||
| * specific language governing permissions and limitations | ||
| * under the License. | ||
| */ | ||
|
|
||
| package org.apache.comet.udf | ||
|
|
||
| import org.apache.comet.{CometNativeException, CometRuntimeException} | ||
|
|
||
| /** Thrown when a Rust UDF dynamic library cannot be opened. */ | ||
| class CometRustUdfLoadException(msg: String, cause: Throwable = null) | ||
| extends CometNativeException(msg) { | ||
| if (cause != null) initCause(cause) | ||
| } | ||
|
|
||
| /** | ||
| * Thrown when a Rust UDF library exposes the wrong ABI version or is missing required symbols. | ||
| */ | ||
| class CometRustUdfAbiException(msg: String) extends CometNativeException(msg) | ||
|
|
||
| /** | ||
| * Thrown when the declared signature does not match what the library reports via | ||
| * comet_udf_describe. | ||
| */ | ||
| class CometRustUdfSignatureException(msg: String) extends CometRuntimeException(msg) | ||
|
|
||
| /** | ||
| * Thrown by the catalog stub if a registered Rust UDF is invoked on the JVM (which means Comet's | ||
| * plan rule did not replace it). | ||
| */ | ||
| class CometRustUdfNotEvaluatedException(name: String) | ||
| extends CometRuntimeException( | ||
| s"Rust UDF '$name' must run inside Comet native execution; the JVM " + | ||
| s"stub was invoked, which means Comet did not replace this expression " + | ||
| s"with a native call. Check that Comet is enabled for the operator " + | ||
| s"hosting this expression.") |


There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't let registration override or collapse the library's volatility.
register()validates only the types and then trusts the caller'sdeterministicflag.registerAll()similarly reduces the descriptor'svolatilitytod.volatility != 2. That loses information from the native contract and allows the JVM side to plan a UDF with different determinism than the library reported. Preserve the descriptor's volatility end-to-end and reject mismatches instead of downgrading it here.Also applies to: 76-80
🤖 Prompt for AI Agents