timsaucer commented on code in PR #21030:
URL: https://github.com/apache/datafusion/pull/21030#discussion_r2976545756
##########
datafusion-examples/examples/ffi/ffi_example_table_provider/src/lib.rs:
##########
@@ -56,11 +55,10 @@ extern "C" fn construct_simple_table_provider(
FFI_TableProvider::new_with_ffi_codec(Arc::new(table_provider), true,
None, codec)
}
-#[export_root_module]
+#[unsafe(no_mangle)]
Review Comment:
Should we use `#[stabby::export]` in all the places we
have`#[unsafe(no_mangle)]`?
##########
datafusion-examples/examples/ffi/ffi_module_interface/src/lib.rs:
##########
@@ -15,36 +15,17 @@
// specific language governing permissions and limitations
// under the License.
-use abi_stable::{
- StableAbi, declare_root_module_statics,
- library::{LibraryError, RootModule},
- package_version_strings,
- sabi_types::VersionStrings,
-};
use datafusion_ffi::proto::logical_extension_codec::FFI_LogicalExtensionCodec;
use datafusion_ffi::table_provider::FFI_TableProvider;
-#[repr(C)]
-#[derive(StableAbi)]
-#[sabi(kind(Prefix(prefix_ref = TableProviderModuleRef)))]
/// This struct defines the module interfaces. It is to be shared by
/// both the module loading program and library that implements the
/// module. It is possible to move this definition into the loading
/// program and reference it in the modules, but this example shows
/// how a user may wish to separate these concerns.
+#[repr(C)]
Review Comment:
Similarly should we use `#[stabby::stabby]` instead of `#[repr(C)]`
throughout?
##########
datafusion-examples/examples/ffi/ffi_module_loader/src/main.rs:
##########
@@ -18,28 +18,47 @@
use std::sync::Arc;
use datafusion::{
+ datasource::TableProvider,
error::{DataFusionError, Result},
+ execution::TaskContextProvider,
prelude::SessionContext,
};
-
-use abi_stable::library::{RootModule, development_utils::compute_library_path};
-use datafusion::datasource::TableProvider;
-use datafusion::execution::TaskContextProvider;
use datafusion_ffi::proto::logical_extension_codec::FFI_LogicalExtensionCodec;
-use ffi_module_interface::TableProviderModuleRef;
+use ffi_module_interface::TableProviderModule;
#[tokio::main]
async fn main() -> Result<()> {
// Find the location of the library. This is specific to the build
environment,
// so you will need to change the approach here based on your use case.
- let target: &std::path::Path = "../../../../target/".as_ref();
- let library_path = compute_library_path::<TableProviderModuleRef>(target)
- .map_err(|e| DataFusionError::External(Box::new(e)))?;
+ let lib_prefix = if cfg!(target_os = "windows") {
+ ""
+ } else {
+ "lib"
+ };
+ let lib_ext = if cfg!(target_os = "macos") {
+ "dylib"
+ } else if cfg!(target_os = "windows") {
+ "dll"
+ } else {
+ "so"
+ };
+
+ let library_path = format!(
+
"../../../../target/debug/{lib_prefix}ffi_example_table_provider.{lib_ext}"
+ );
Review Comment:
I think this will break for CI or release builds.
##########
datafusion/ffi/src/ffi_option.rs:
##########
@@ -0,0 +1,133 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! FFI-safe Option and Result types that do not require `IStable` bounds.
+//!
+//! stabby's `Option<T>` and `Result<T, E>` require `T: IStable` for niche
+//! optimization. Many of our FFI structs contain self-referential function
+//! pointers and cannot implement `IStable`. These simple `#[repr(C)]` types
+//! provide the same FFI-safe semantics without that constraint.
+
+/// An FFI-safe option type.
+#[repr(C, u8)]
+#[derive(Debug, Clone)]
+pub enum FfiOption<T> {
Review Comment:
Also let's stick with the same casing convention as throughout the rest of
the crate
##########
datafusion/ffi/src/ffi_option.rs:
##########
@@ -0,0 +1,133 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! FFI-safe Option and Result types that do not require `IStable` bounds.
+//!
+//! stabby's `Option<T>` and `Result<T, E>` require `T: IStable` for niche
+//! optimization. Many of our FFI structs contain self-referential function
+//! pointers and cannot implement `IStable`. These simple `#[repr(C)]` types
+//! provide the same FFI-safe semantics without that constraint.
+
+/// An FFI-safe option type.
+#[repr(C, u8)]
+#[derive(Debug, Clone)]
+pub enum FfiOption<T> {
+ Some(T),
+ None,
+}
+
+impl<T> From<Option<T>> for FfiOption<T> {
+ fn from(opt: Option<T>) -> Self {
+ match opt {
+ Some(v) => FfiOption::Some(v),
+ None => FfiOption::None,
+ }
+ }
+}
+
+impl<T> From<FfiOption<T>> for Option<T> {
+ fn from(opt: FfiOption<T>) -> Self {
+ match opt {
+ FfiOption::Some(v) => Some(v),
+ FfiOption::None => None,
+ }
+ }
+}
+
+impl<T> FfiOption<T> {
+ pub fn as_ref(&self) -> Option<&T> {
+ match self {
+ FfiOption::Some(v) => Some(v),
+ FfiOption::None => None,
+ }
+ }
+
+ pub fn map<U, F: FnOnce(T) -> U>(self, f: F) -> FfiOption<U> {
+ match self {
+ FfiOption::Some(v) => FfiOption::Some(f(v)),
+ FfiOption::None => FfiOption::None,
+ }
+ }
+
+ pub fn into_option(self) -> Option<T> {
+ self.into()
+ }
+}
+
+/// An FFI-safe result type.
+#[repr(C, u8)]
+#[derive(Debug, Clone)]
+pub enum FfiResult<T, E> {
Review Comment:
Same, let's keep casing convention of the crate constant
##########
datafusion/ffi/src/ffi_option.rs:
##########
@@ -0,0 +1,133 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! FFI-safe Option and Result types that do not require `IStable` bounds.
+//!
+//! stabby's `Option<T>` and `Result<T, E>` require `T: IStable` for niche
+//! optimization. Many of our FFI structs contain self-referential function
+//! pointers and cannot implement `IStable`. These simple `#[repr(C)]` types
+//! provide the same FFI-safe semantics without that constraint.
+
+/// An FFI-safe option type.
+#[repr(C, u8)]
+#[derive(Debug, Clone)]
+pub enum FfiOption<T> {
Review Comment:
I'm guessing you need this because you can't use stabby's Option unless
everything inside it is marked as IStable?
##########
datafusion/ffi/src/table_source.rs:
##########
@@ -15,49 +15,47 @@
// specific language governing permissions and limitations
// under the License.
-use abi_stable::StableAbi;
use datafusion_expr::{TableProviderFilterPushDown, TableType};
/// FFI safe version of [`TableProviderFilterPushDown`].
-#[repr(C)]
-#[derive(StableAbi)]
-pub enum FFI_TableProviderFilterPushDown {
+#[repr(u8)]
+pub enum FfiTableProviderFilterPushDown {
Review Comment:
Lets not rename this if we don't have to
##########
datafusion/ffi/src/volatility.rs:
##########
@@ -15,18 +15,17 @@
// specific language governing permissions and limitations
// under the License.
-use abi_stable::StableAbi;
use datafusion_expr::Volatility;
-#[repr(C)]
-#[derive(Debug, StableAbi, Clone)]
-pub enum FFI_Volatility {
+#[repr(u8)]
+#[derive(Debug, Clone)]
+pub enum FfiVolatility {
Review Comment:
Lets not rename this if we don't have to
##########
datafusion-examples/examples/ffi/ffi_module_interface/src/lib.rs:
##########
@@ -15,36 +15,17 @@
// specific language governing permissions and limitations
// under the License.
-use abi_stable::{
- StableAbi, declare_root_module_statics,
- library::{LibraryError, RootModule},
- package_version_strings,
- sabi_types::VersionStrings,
-};
use datafusion_ffi::proto::logical_extension_codec::FFI_LogicalExtensionCodec;
use datafusion_ffi::table_provider::FFI_TableProvider;
-#[repr(C)]
-#[derive(StableAbi)]
-#[sabi(kind(Prefix(prefix_ref = TableProviderModuleRef)))]
/// This struct defines the module interfaces. It is to be shared by
/// both the module loading program and library that implements the
/// module. It is possible to move this definition into the loading
/// program and reference it in the modules, but this example shows
/// how a user may wish to separate these concerns.
+#[repr(C)]
Review Comment:
Ok, now I understand the depth of the changes to get conformity to stabby. I
think your proposal to use only repr(C) is a good one. Could you add a note
into the crate's readme to explain how and why we are using stabby?
##########
datafusion/ffi/src/execution_plan.rs:
##########
@@ -112,35 +113,35 @@ unsafe extern "C" fn properties_fn_wrapper(
unsafe extern "C" fn children_fn_wrapper(
plan: &FFI_ExecutionPlan,
-) -> RVec<FFI_ExecutionPlan> {
- let runtime = plan.runtime();
- let plan = plan.inner();
-
- let children: Vec<_> = plan
- .children()
- .into_iter()
- .map(|child| FFI_ExecutionPlan::new(Arc::clone(child),
runtime.clone()))
- .collect();
+) -> SVec<FFI_ExecutionPlan> {
+ unsafe {
+ let private_data = plan.private_data as *const
ExecutionPlanPrivateData;
+ let plan = &(*private_data).plan;
+ let runtime = &(*private_data).runtime;
Review Comment:
Why remove the helper function usage?
##########
datafusion/ffi/src/execution_plan.rs:
##########
@@ -112,35 +113,35 @@ unsafe extern "C" fn properties_fn_wrapper(
unsafe extern "C" fn children_fn_wrapper(
plan: &FFI_ExecutionPlan,
-) -> RVec<FFI_ExecutionPlan> {
- let runtime = plan.runtime();
- let plan = plan.inner();
-
- let children: Vec<_> = plan
- .children()
- .into_iter()
- .map(|child| FFI_ExecutionPlan::new(Arc::clone(child),
runtime.clone()))
- .collect();
+) -> SVec<FFI_ExecutionPlan> {
+ unsafe {
+ let private_data = plan.private_data as *const
ExecutionPlanPrivateData;
+ let plan = &(*private_data).plan;
+ let runtime = &(*private_data).runtime;
- children.into()
+ plan.children()
+ .into_iter()
+ .map(|child| FFI_ExecutionPlan::new(Arc::clone(child),
runtime.clone()))
+ .collect()
+ }
}
unsafe extern "C" fn with_new_children_fn_wrapper(
plan: &FFI_ExecutionPlan,
- children: RVec<FFI_ExecutionPlan>,
+ children: SVec<FFI_ExecutionPlan>,
) -> FFIResult<FFI_ExecutionPlan> {
let runtime = plan.runtime();
- let plan = Arc::clone(plan.inner());
- let children = rresult_return!(
- children
- .iter()
- .map(<Arc<dyn ExecutionPlan>>::try_from)
- .collect::<Result<Vec<_>>>()
- );
+ let inner_plan = Arc::clone(plan.inner());
+
+ let children: Result<Vec<Arc<dyn ExecutionPlan>>> = children
+ .iter()
+ .map(<Arc<dyn ExecutionPlan>>::try_from)
+ .collect();
- let new_plan = rresult_return!(plan.with_new_children(children));
+ let children = rresult_return!(children);
Review Comment:
annoying but we probably need to rename `rresult_return` to `sresult_return`
##########
datafusion/ffi/src/proto/logical_extension_codec.rs:
##########
@@ -185,86 +187,86 @@ unsafe extern "C" fn try_encode_table_provider_fn_wrapper(
&mut bytes
));
- RResult::ROk(bytes.into())
+ FfiResult::Ok(bytes.into_iter().collect())
}
unsafe extern "C" fn try_decode_udf_fn_wrapper(
codec: &FFI_LogicalExtensionCodec,
- name: RStr,
- buf: RSlice<u8>,
+ name: SStr,
+ buf: SSlice<u8>,
) -> FFIResult<FFI_ScalarUDF> {
let codec = codec.inner();
let udf = rresult_return!(codec.try_decode_udf(name.as_str(),
buf.as_ref()));
let udf = FFI_ScalarUDF::from(udf);
- RResult::ROk(udf)
+ FfiResult::Ok(udf)
}
unsafe extern "C" fn try_encode_udf_fn_wrapper(
codec: &FFI_LogicalExtensionCodec,
node: FFI_ScalarUDF,
-) -> FFIResult<RVec<u8>> {
+) -> FFIResult<SVec<u8>> {
let codec = codec.inner();
let node: Arc<dyn ScalarUDFImpl> = (&node).into();
let node = ScalarUDF::new_from_shared_impl(node);
let mut bytes = Vec::new();
rresult_return!(codec.try_encode_udf(&node, &mut bytes));
- RResult::ROk(bytes.into())
+ FfiResult::Ok(bytes.into_iter().collect())
}
unsafe extern "C" fn try_decode_udaf_fn_wrapper(
codec: &FFI_LogicalExtensionCodec,
- name: RStr,
- buf: RSlice<u8>,
+ name: SStr,
+ buf: SSlice<u8>,
) -> FFIResult<FFI_AggregateUDF> {
let codec_inner = codec.inner();
let udaf = rresult_return!(codec_inner.try_decode_udaf(name.into(),
buf.as_ref()));
let udaf = FFI_AggregateUDF::from(udaf);
- RResult::ROk(udaf)
+ FfiResult::Ok(udaf)
}
unsafe extern "C" fn try_encode_udaf_fn_wrapper(
codec: &FFI_LogicalExtensionCodec,
node: FFI_AggregateUDF,
-) -> FFIResult<RVec<u8>> {
+) -> FFIResult<SVec<u8>> {
let codec = codec.inner();
let udaf: Arc<dyn AggregateUDFImpl> = (&node).into();
let udaf = AggregateUDF::new_from_shared_impl(udaf);
let mut bytes = Vec::new();
rresult_return!(codec.try_encode_udaf(&udaf, &mut bytes));
- RResult::ROk(bytes.into())
+ FfiResult::Ok(bytes.into_iter().collect())
}
unsafe extern "C" fn try_decode_udwf_fn_wrapper(
codec: &FFI_LogicalExtensionCodec,
- name: RStr,
- buf: RSlice<u8>,
+ name: SStr,
+ buf: SSlice<u8>,
) -> FFIResult<FFI_WindowUDF> {
let codec = codec.inner();
let udwf = rresult_return!(codec.try_decode_udwf(name.into(),
buf.as_ref()));
let udwf = FFI_WindowUDF::from(udwf);
- RResult::ROk(udwf)
+ FfiResult::Ok(udwf)
}
unsafe extern "C" fn try_encode_udwf_fn_wrapper(
codec: &FFI_LogicalExtensionCodec,
node: FFI_WindowUDF,
-) -> FFIResult<RVec<u8>> {
+) -> FFIResult<SVec<u8>> {
let codec = codec.inner();
let udwf: Arc<dyn WindowUDFImpl> = (&node).into();
let udwf = WindowUDF::new_from_shared_impl(udwf);
let mut bytes = Vec::new();
rresult_return!(codec.try_encode_udwf(&udwf, &mut bytes));
- RResult::ROk(bytes.into())
+ FfiResult::Ok(bytes.into_iter().collect())
Review Comment:
I'm guessing abi_stable had an implementation for RVec<u8> that doesn't
exist for SVec<u8>?
##########
datafusion/ffi/src/plan_properties.rs:
##########
@@ -228,30 +229,30 @@ impl From<FFI_Boundedness> for Boundedness {
}
/// FFI safe version of [`EmissionType`].
-#[repr(C)]
-#[derive(Clone, StableAbi)]
-pub enum FFI_EmissionType {
+#[repr(u8)]
+#[derive(Clone)]
+pub enum FfiEmissionType {
Review Comment:
Lets not rename this if we don't have to
##########
datafusion/ffi/src/insert_op.rs:
##########
@@ -15,34 +15,32 @@
// specific language governing permissions and limitations
// under the License.
-use abi_stable::StableAbi;
use datafusion_expr::logical_plan::dml::InsertOp;
/// FFI safe version of [`InsertOp`].
-#[repr(C)]
-#[derive(StableAbi)]
-pub enum FFI_InsertOp {
+#[repr(u8)]
+pub enum FFiInsertOp {
Review Comment:
Let's not rename this if we don't have to
##########
datafusion/ffi/src/util.rs:
##########
@@ -17,59 +17,63 @@
use std::sync::Arc;
-use abi_stable::std_types::{RResult, RString, RVec};
use arrow::datatypes::{DataType, Field};
use arrow::ffi::FFI_ArrowSchema;
use arrow_schema::FieldRef;
+use stabby::string::String as SString;
+use stabby::vec::Vec as SVec;
use crate::arrow_wrappers::WrappedSchema;
+// Re-export for convenience
+pub use crate::ffi_option::{FfiOption, FfiResult};
+
/// Convenience type for results passed through the FFI boundary. Since the
/// `DataFusionError` enum is complex and little value is gained from creating
/// a FFI safe variant of it, we convert errors to strings when passing results
/// back. These are converted back and forth using the `df_result`, `rresult`,
/// and `rresult_return` macros.
-pub type FFIResult<T> = RResult<T, RString>;
+pub type FFIResult<T> = FfiResult<T, SString>;
Review Comment:
Seems not helpful to have both `FFIREsult` and `FfiResult`.
##########
datafusion/ffi/src/udf/mod.rs:
##########
@@ -45,7 +45,7 @@ use crate::expr::columnar_value::FFI_ColumnarValue;
use crate::util::{
FFIResult, rvec_wrapped_to_vec_datatype, vec_datatype_to_rvec_wrapped,
};
-use crate::volatility::FFI_Volatility;
+use crate::volatility::FfiVolatility;
Review Comment:
Lets not changing the naming since it will cause downstream additional churn
--
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]
