alamb commented on code in PR #22816:
URL: https://github.com/apache/datafusion/pull/22816#discussion_r3390344153
##########
datafusion/ffi/src/udaf/groups_accumulator.rs:
##########
@@ -195,21 +194,14 @@ unsafe extern "C" fn merge_batch_fn_wrapper(
accumulator: &mut FFI_GroupsAccumulator,
values: SVec<WrappedArray>,
group_indices: SVec<usize>,
- opt_filter: FFI_Option<WrappedArray>,
Review Comment:
I think this is supposed to be a stable ABI (so different versions of
DataFusion can use the same table provider). However, I am not sure if this
actually works in practice. Thus I think maybe we should keep this FFI
interface the same (but ignore the parameter) ?
@timsaucer can you comment about if/when it is ok to change the FFI APIs?
I didn't see anything in the FFI readme that addressed if/when API changes
are allowed to the FFI interfaces.
https://github.com/apache/datafusion/blob/af7904f6358095b5a16d179213bce82c6d812bb1/datafusion/ffi/README.md#L22
Says
> This crate contains code to allow interoperability of [Apache DataFusion]
with
functions from other libraries and/or DataFusion versions using a stable
interface.
But that doesn't specify if it is supposed to remain stable across major
versions too 🤔
--
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]
