andygrove commented on PR #4459:
URL:
https://github.com/apache/datafusion-comet/pull/4459#issuecomment-5481132557
Thanks @comphead, all four P1s were real. Merged current `main` and pushed
fixes for each; @paleolimbot's point about `new_impl` and @wForget's two are in
this round as well, answered on their threads.
### P1: the `Mutex` spans the whole batch (702f952d1)
Removed, not narrowed. Your read of the consequence was right, and it is
worse than the guard being wider than `new_impl` needs: it is one lock per UDF
per **process**, and every task in an executor shares one `ImportedCScalarUdf`
through the cache in `cache.rs`, so N tasks over `add_one_c(id)` collapsed to
roughly one core while the equivalent built-in scaled.
No lock is needed at all, which is @paleolimbot's observation on the thread
above. `new_impl` and `function_name` already take `*const CometCScalarKernel`,
and the SDK's exporter only reads through it: `c_factory_new_impl` clones an
`Arc` and writes a fresh `CometCScalarKernelImpl` into the caller's `out`.
Everything mutable is that per-call impl, which never leaves the stack frame
that built it. So the property held and the ABI simply had not said so. It now
says so on `CometCScalarKernel`, on the `new_impl` field, on the
`CometCScalarUdf` trait, and in the user guide, because a kernel written
against a permissive host and later run without a lock is the failure this
invites.
Pinned by `concurrent_invocations_share_one_adapter`: eight threads through
one shared adapter on distinct batches, so a kernel leaking state between
concurrent calls returns another thread's answers. I did not add a wall-clock
scaling assertion, since a timing threshold in CI is a flake waiting to happen;
the correctness test plus the absence of a lock is what I would rather rely on.
### P1: `TimestampType` docs omit the `Some("UTC")` tag (f8538b2a7)
Correct, and your diagnosis of why the tests could not catch it is exactly
right: `echo_c` returns `args[0].data_type().clone()` and so inherits Comet's
own tag, which made the contract invisible.
- the table now reads `Timestamp(Microsecond, Some("UTC"))` and
`Timestamp(Microsecond, None)`, with a paragraph saying the tag is the only
difference between the two Spark types, that it is compared exactly, and that
the values are the same microseconds either way
- the mismatch error now names the axes that must match exactly, including
both timestamp mappings, so the user does not have to open Comet's source to
find out which one rejected them
- `make_ts_utc_c` and `make_ts_naive_c` in `comet-test-udfs`, one test
asserting both succeed against their matching declarations and one asserting
the untagged UDF declared as `TimestampType` fails with a message that names
the timezone
### P1: the `MapType` field-name contract (f8538b2a7)
Also correct, and this one I fixed rather than documented.
`MapBuilder::new(None, ..)` gives `entries` / `keys` / `values` against Comet's
`entries` / `key` / `value`, and rejecting a UDF over that is rejecting it for
a spelling. Those names are positional in the Arrow columnar format, and
Comet's own `CometMapVector` reads the entries struct by index, never by name.
So the comparison now normalizes list and map child field names before
comparing, alongside the nested-nullability erasure that was already there.
Struct field names stay strict, since those are part of the Spark type and are
how a caller addresses the result.
`make_map_c` builds its output through the default `MapBuilder` and a test
asserts the values come back. I checked it is not vacuous by restoring strict
name comparison, and it fails with precisely the message you predicted:
```
native UDF 'make_map_c' was registered as returning
Map("entries": non-null Struct("key": non-null Utf8, "value": Int32),
unsorted)
but its return_field reports
Map("entries": non-null Struct("keys": non-null Utf8, "values": Int32),
unsorted)
```
### P1: placement in `WHERE`, join, `GROUP BY`, window (d211a1add)
Added all four. All pass natively as-is, so this is coverage against
regression rather than a fix.
I leaned on the catalog stub instead of asserting on the plan string: if
Spark evaluates the UDF itself the stub throws, so a fallback fails the test
rather than passing with the right answer computed on the JVM. That is a
stronger guarantee than a plan match, since it also catches a fallback that
leaves the operator name intact.
### Merge and follow-ups
`main` merged in aac789c2d. Three of the four conflicts were the two
PR-build workflows and `native/Cargo.toml`, all additive; `Cargo.lock` was
regenerated.
Two issues filed from this round, both raised by @wForget: #5597 (derive the
return type from the library rather than making the caller declare it) and
#5596 (align with SPARK-55278). On the SPIP: it is out-of-process over gRPC
where this path is in-process over the Arrow C Data Interface, so it does not
replace this one, but the registration vocabulary is the piece worth borrowing
before this API has users, and #5596 writes up where the two surfaces will have
to meet.
Suite is 57 passing / 1 ignored, `cargo test`, clippy, fmt, spotless and
scalastyle clean. The PR description is updated, including the naming that went
stale after the `CometRustUDF` to `CometNativeUDF` rename.
--
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]
