Re: [PR] feat: custom Rust UDFs via arrow-ffi [experimental] [datafusion-comet]

["andygrove (via GitHub)" <gi...@apache.org>]

andygrove commented on PR #4459:
URL: https://github.com/apache/datafusion-comet/pull/4459#issuecomment-5811556433

   Following up on my self-review above. Everything in it is now addressed:
   
   - **Result type check (f8d273eef).** `c_kernel_execute` compares the array `invoke` returned with the type `return_field` declared, using `equals_datatype`, and fails the call if they differ. The Int32-declared-as-Int64 case now fails with `invoke returned Int32 but return_field declared Int64` instead of returning garbage. SDK test added.
   - **Map and list child names in the plan (f9dd01b48).** I kept accepting arrow-rs's names rather than going back to strict names. The planner now promises DataFusion the kernel's type with list and map children renamed to `item` / `entries` / `key` / `value`, and the adapter relabels each result to match with a cast that reuses the buffers. Anything beyond a naming difference is an error, not a cast. The new test combines `make_map_c` with `map_from_arrays` in both branch orders of `if`. With the planner change reverted it fails with `column types must match schema types`, and with it the test passes.
   - **Library lifetime (f9dd01b48).** `ImportedCScalarUdf` holds an `Arc<Library>` declared after `kernel`. Details, and the correction to my earlier reply, are on the `Arc<CometCScalarKernel>` thread.
   - **`rust_udfs.md` (69702a5d2).**
     - The example uses `arrow = "59"` and pins `tag`, with a note on the E0053 error a mismatched arrow produces.
     - The panic section says containment needs `panic = "unwind"`.
     - The reload advice says a rename is safe but copying over a loaded library in place can crash the executors and the driver.
   - **`comet-udf-sdk` crate docs (f8d273eef)** no longer claim cross-release compatibility or C/C++ support.
   - **Tests (970c12a7f).** A two-argument, order-sensitive `sub_c`, literal arguments in each position both end to end and in the adapter tests, and the `if` case above.
   - **`CometNativeUdfSignatureException`** is removed.
   - **`inputTypes` (6998df990)** is now enforced. A call with other argument types is refused at planning time rather than cast. Reasoning is on @wForget's thread.
   - **Follow-ups filed:** #6174 (test coverage for empty batches, slices and dictionaries), #6175 (config gate), #6176 (library distribution) and #6177 (arity cap and `registerAll`). The description now links them and reflects the current CI tiers.
   
   Locally, `CometNativeUdfSuite` passes (61 tests, 1 ignored), as do the 22 native `c_udf` tests and the 5 SDK tests, and clippy is clean with `-D warnings`. The new test that drops a `LoadedLibrary` only means something on Linux, so this CI run is its first real check.
   
   @paleolimbot @mbutrovich @comphead @wForget, this is ready for another look when you have time.
   


-- 
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: github-unsubscribe@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org