Brijesh-Thakkar opened a new pull request, #22705:
URL: https://github.com/apache/datafusion/pull/22705
## Which issue does this PR close?
Closes #22332
## Rationale for this change
`FFI_WindowUDF` was silently dropping producer overrides of four defaulted
`WindowUDFImpl` methods. Any plugin that overrode `expressions()`,
`simplify()`, `reverse_expr()`, or `documentation()` would have those
overrides ignored on the consumer side, with the trait defaults running
instead. This could change window-function semantics (e.g. `expressions()`
selects which physical expressions reach the `PartitionEvaluator`) or
silently suppress planner-level optimizations (`simplify`, `reverse_expr`).
## What changes are included in this PR?
- **`documentation`** — new `unsafe extern "C"` fn pointer returning
`*const Documentation` (null = `None`); consumer reconstructs via
`ptr.as_ref()`
- **`expressions`** — new fn pointer taking `FFI_ExpressionArgs` and
returning `SVec<FFI_PhysicalExpr>`; a new sibling module
`udwf/expression_args.rs` provides `FFI_ExpressionArgs` /
`ForeignExpressionArgs` to carry `Vec<Arc<dyn PhysicalExpr>>` and
`Vec<FieldRef>` across the boundary
- **`simplify`** — closures cannot cross FFI, so simplification executes
on the producer side: the consumer serializes the `WindowFunction` to
protobuf bytes, calls the fn pointer, and deserializes the returned
`Expr`; the producer wrapper deserializes, calls the inner `simplify`
closure, and returns the serialized result
- **`reverse_expr`** — new `FFI_ReversedUDWF` enum (`#[repr(C, u8)]`,
matching the existing ABI-stable enum pattern in this crate) with
variants `Identical`, `NotSupported`, and `Reversed(FFI_WindowUDF)`;
the `Reversed` variant uses the round-trip downcast optimization to
avoid double-wrapping when the inner UDF is already a `ForeignWindowUDF`
**Known limitation:** `ForeignWindowUDF::simplify()` currently uses
`DefaultLogicalExtensionCodec` internally. UDWFs whose expressions require
a custom codec to round-trip are not yet supported through the simplify
path and are tracked as a follow-up.
## Are these changes tested?
Yes. For each of the four methods:
- **Unit tests** in `datafusion/ffi/src/udwf/mod.rs` cover both the
local-bypass path (marker IDs match, downcasts to concrete type) and
the forced-foreign path (`mock_foreign_marker_id` set, full FFI path
exercised)
- **Integration test** in `datafusion/ffi/tests/ffi_udwf.rs` covers
`documentation` across the crate boundary
- All 104 existing unit tests and all integration tests continue to pass
## Are there any user-facing changes?
`FFI_WindowUDF` struct layout has changed — this is an ABI-breaking change
for anyone linking against the struct directly. The PR targets `main` only
with no backport, per the `datafusion-ffi` crate convention for layout
changes.
--
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]
