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: [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]
