andygrove commented on PR #4459: URL: https://github.com/apache/datafusion-comet/pull/4459#issuecomment-5173658202
Thanks for the review @paleolimbot. The PR has changed quite a bit since you looked, so here is a summary of where it stands. It is also rebased onto current `main`, which turned out to be relevant to the biggest change. ### The datafusion-ffi flavor is gone This is the one I want to flag directly, because you were positive about that implementation and it is the thing I removed. Carrying two public ABIs would have meant two permanent compatibility promises, so I picked one, and the rebase is what decided it. Merging current `main` moved Comet from DataFusion 53 to 54, which removed `as_any` from `ScalarUDFImpl`. Every user library built against the `datafusion-ffi` flavor would have needed a source edit and a recompile. The C ABI needed no change at all, because no DataFusion type appears in it. Comet tracks DataFusion closely and upgrades often, so that break would recur on a cadence UDF authors neither control nor can opt out of, and it would land on whoever upgrades Comet rather than on us. So the surviving ABI is the arrow-only one. `comet-udf-sdk` now has no DataFusion dependency at all: a user's cdylib pulls in `arrow` and nothing else. The tradeoff we accept is the smaller authoring surface, and if that becomes the binding constraint the answer is to widen the C ABI deliberately rather than adopt a version-pinned one. The rationale is recorded in the crate docs so it does not have to be rediscovered. ### Panic containment at every FFI entry point Previously only `execute` was guarded. A panic escaping any of the other `extern "C"` functions aborts the process, which for Comet means killing the executor JVM and losing every task on it, not just the query that used the UDF. Since user UDF code is arbitrary and panicking is idiomatic Rust, panics are now caught at the boundary and reported through the same `get_last_error` channel as a returned `Err`. The query fails; the executor survives. Covered by SDK unit tests and by end-to-end tests that drive genuinely panicking UDFs through Spark. ### Full type coverage, including complex types Arguments and return values now cover every non-nested Spark type plus `ArrayType`, `MapType` and `StructType` with arbitrary nesting, nulls preserved in both directions. No ABI change was needed for any of it, since the FFI surface is type-agnostic and already carries child arrays, but nothing had demonstrated it beyond `Int64`. Two test UDFs cover the matrix: `echo_c` returns its argument unchanged, which checks that each type survives the round trip with its nulls, and `stringify_c` renders any input as strings, which forces the UDF to actually decode the values rather than hand the array straight back. ### Return type validation Comet now checks the type declared to `CometRustUDF.register` against what the kernel's `return_field` reports, at planning time, naming both types when they disagree. This replaced a bare DataFusion type assertion that used to fire partway through execution. More on the semantics in my reply on the `return_type` thread. ### User guide Added `docs/source/user-guide/latest/rust_udfs.md`, which describes the feature as experimental and is explicit about the current limitations: the library must already exist on every executor at the given path since Comet does not distribute it, and loading a UDF library means running unsandboxed native code in the executor process. ### Still outstanding Listed in the PR description under "Follow-on work". The ones I would most want a reviewer's opinion on: the suite is not yet wired into CI, so the ABI has never been exercised against a Linux `.so`; there is no config key gating library loading, so an operator cannot disable the feature or restrict which paths may be loaded; and nothing distributes the library to executors. -- 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]
