timsaucer commented on code in PR #21030:
URL: https://github.com/apache/datafusion/pull/21030#discussion_r3132562075
##########
datafusion/ffi/README.md:
##########
@@ -65,14 +65,39 @@ to work across Rust libraries. In general, you can use
Rust's [FFI] to
operate across different programming languages, but that is not the design
intent of this crate. Instead, we are using external crates that provide
stable interfaces that closely mirror the Rust native approach. To learn more
-about this approach see the [abi_stable] and [async-ffi] crates.
+about this approach see the [stabby] and [async-ffi] crates.
If you have a library in another language that you wish to interface to
DataFusion the recommendation is to create a Rust wrapper crate to interface
with your library and then to connect it to DataFusion using this crate.
Alternatively, you could use [bindgen] to interface directly to the [FFI]
provided
by this crate, but that is currently not supported.
+## Stabby Usage
+
+This crate uses [stabby] for ABI-stable types like `stabby::string::String` and
+`stabby::vec::Vec`. We chose stabby because [abi_stable] is no longer actively
+maintained.
Review Comment:
This is a backwards facing statement. Future developers aren't going to be
interested in the fact that abi_stable is not maintained. You can jut cut this
last sentence.
##########
datafusion/ffi/README.md:
##########
@@ -65,14 +65,39 @@ to work across Rust libraries. In general, you can use
Rust's [FFI] to
operate across different programming languages, but that is not the design
intent of this crate. Instead, we are using external crates that provide
stable interfaces that closely mirror the Rust native approach. To learn more
-about this approach see the [abi_stable] and [async-ffi] crates.
+about this approach see the [stabby] and [async-ffi] crates.
If you have a library in another language that you wish to interface to
DataFusion the recommendation is to create a Rust wrapper crate to interface
with your library and then to connect it to DataFusion using this crate.
Alternatively, you could use [bindgen] to interface directly to the [FFI]
provided
by this crate, but that is currently not supported.
+## Stabby Usage
+
+This crate uses [stabby] for ABI-stable types like `stabby::string::String` and
+`stabby::vec::Vec`. We chose stabby because [abi_stable] is no longer actively
+maintained.
+
+We intentionally use `#[repr(C)]` for our struct definitions instead of
stabby's
+`#[stabby::stabby]` macro. The reason is that stabby's `IStable` trait
(required
+by the macro) demands that all inner types also implement `IStable`. This
creates
+challenges for our use case:
+
+1. **Arrow types**: Arrow's FFI types like `FFI_ArrowSchema` do not implement
+ `IStable`, and adding such implementations would be laborious and
error-prone.
+
+2. **Self-referential function pointers**: Many of our FFI structs contain
+ function pointers that reference `&Self`, which complicates `IStable`
+ implementations.
Review Comment:
I disagree with this. The stabby approach would be to make these stabby
traits that mirror the non-ffi traits rather than implement them as function
pointers on a stabby object. This part I actually find to be quite clean if you
go through the process of changing it all over.
##########
datafusion/ffi/README.md:
##########
@@ -65,14 +65,39 @@ to work across Rust libraries. In general, you can use
Rust's [FFI] to
operate across different programming languages, but that is not the design
intent of this crate. Instead, we are using external crates that provide
stable interfaces that closely mirror the Rust native approach. To learn more
-about this approach see the [abi_stable] and [async-ffi] crates.
+about this approach see the [stabby] and [async-ffi] crates.
If you have a library in another language that you wish to interface to
DataFusion the recommendation is to create a Rust wrapper crate to interface
with your library and then to connect it to DataFusion using this crate.
Alternatively, you could use [bindgen] to interface directly to the [FFI]
provided
by this crate, but that is currently not supported.
+## Stabby Usage
+
+This crate uses [stabby] for ABI-stable types like `stabby::string::String` and
+`stabby::vec::Vec`. We chose stabby because [abi_stable] is no longer actively
+maintained.
+
+We intentionally use `#[repr(C)]` for our struct definitions instead of
stabby's
+`#[stabby::stabby]` macro. The reason is that stabby's `IStable` trait
(required
+by the macro) demands that all inner types also implement `IStable`. This
creates
+challenges for our use case:
+
+1. **Arrow types**: Arrow's FFI types like `FFI_ArrowSchema` do not implement
+ `IStable`, and adding such implementations would be laborious and
error-prone.
+
+2. **Self-referential function pointers**: Many of our FFI structs contain
+ function pointers that reference `&Self`, which complicates `IStable`
+ implementations.
+
+3. **FFI_Option and FFIResult**: For similar reasons, we provide our own
+ `FFI_Option<T>` and `FFIResult<T>` types using `#[repr(C, u8)]` instead
+ of stabby's `Option` and `Result`, which require inner types to be
`IStable`.
Review Comment:
```suggestion
3. **FFI_Option and FFIResult**: We provide our own
`FFI_Option<T>` and `FFIResult<T>` types using `#[repr(C, u8)]` because
stabby's `Option` and `Result` require inner types to be `IStable`.
```
##########
datafusion/ffi/README.md:
##########
@@ -65,14 +65,39 @@ to work across Rust libraries. In general, you can use
Rust's [FFI] to
operate across different programming languages, but that is not the design
intent of this crate. Instead, we are using external crates that provide
stable interfaces that closely mirror the Rust native approach. To learn more
-about this approach see the [abi_stable] and [async-ffi] crates.
+about this approach see the [stabby] and [async-ffi] crates.
If you have a library in another language that you wish to interface to
DataFusion the recommendation is to create a Rust wrapper crate to interface
with your library and then to connect it to DataFusion using this crate.
Alternatively, you could use [bindgen] to interface directly to the [FFI]
provided
by this crate, but that is currently not supported.
+## Stabby Usage
+
+This crate uses [stabby] for ABI-stable types like `stabby::string::String` and
+`stabby::vec::Vec`. We chose stabby because [abi_stable] is no longer actively
+maintained.
+
+We intentionally use `#[repr(C)]` for our struct definitions instead of
stabby's
+`#[stabby::stabby]` macro. The reason is that stabby's `IStable` trait
(required
+by the macro) demands that all inner types also implement `IStable`. This
creates
+challenges for our use case:
Review Comment:
One of the reasons I think the stabby macros are difficult is that it
*greatly* increases build time due to the heavy macro use and the fact that we
have so many interdependent types.
Also I found that creating stabby dyn pointers for the trait objects led to
a more complex code patterns with very little added benefit. Instead I think we
use stabby for it's convenient structs.
--
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]
