mkleen opened a new issue, #25374:
URL: https://github.com/apache/datafusion/issues/25374
### Is your feature request related to a problem or challenge?
#25288 adds `AggregateUDFImpl::distinct_handling`
(`datafusion/expr/src/udaf.rs:958`), which lets an
aggregate declare whether `DISTINCT` can change its result, and
`EliminateAggregateDistinct` uses that
to drop the modifier from duplicate-insensitive functions.
`ForeignAggregateUDF` (`datafusion/ffi/src/udaf/mod.rs:471`) does not
forward the new method, and
`FFI_AggregateUDF` (`datafusion/ffi/src/udaf/mod.rs:59`) has no field to
carry it, so every UDAF loaded
over FFI falls back to the default `DistinctHandling::Sensitive`.
The effect is that the new API silently does nothing across the FFI
boundary. A third-party `min`-like
or set-valued aggregate that declares `DistinctHandling::Insensitive` still
gets the full
`SingleDistinctToGroupBy` treatment — a per-group hash set and an extra
grouping stage that only
deduplicates input the function was going to ignore — while the identical
function registered
in-process does not. Results stay correct either way; the optimization is
just unreachable.
### Describe the solution you'd like
Carry the tag through the vtable the way `order_sensitivity` already does:
1. Add `FFI_DistinctHandling` (`#[repr(C)]`) with `From` impls in both
directions, alongside
`FFI_AggregateOrderSensitivity` at `datafusion/ffi/src/udaf/mod.rs:628`.
2. Add a `distinct_handling` fn pointer to `FFI_AggregateUDF` and a
`distinct_handling_fn_wrapper`,
mirroring `order_sensitivity_fn_wrapper`
(`datafusion/ffi/src/udaf/mod.rs:328`).
3. Implement `distinct_handling` on `ForeignAggregateUDF` as
`unsafe { (self.udaf.distinct_handling)(&self.udaf).into() }`.
4. Add a round-trip test over every variant, like
`test_round_trip_all_order_sensitivities`
(`datafusion/ffi/src/udaf/mod.rs:864`), plus a `ForeignAggregateUDF` test
asserting a foreign UDAF
that declares `Insensitive` reports `Insensitive`.
One wrinkle worth deciding: `DistinctHandling` is `#[non_exhaustive]`, so
the `From<FFI_DistinctHandling>`
conversion needs a policy for a variant added by a newer library on the
other side of the boundary.
Mapping anything unrecognized to `Sensitive` keeps it conservative — that is
the default, and it only
ever costs an optimization, never correctness.
### Describe alternatives you've considered
Leaving it as is. FFI aggregates keep working and return correct results;
they just never benefit from
`EliminateAggregateDistinct`. The cost is that the two registration paths
behave differently for the
same function, which is surprising and hard to notice.
--
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]
