You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "timsaucer (via GitHub)" <gi...@apache.org> on 2024/10/14 13:01:53 UTC
[PR] Feature/ffi initial implementation [datafusion]
timsaucer opened a new pull request, #12920:
URL: https://github.com/apache/datafusion/pull/12920
## Which issue does this PR close?
This is to address part of https://github.com/apache/datafusion-python/issues/823 downstream but may have wider application than just python.
## Rationale for this change
This PR allows for registering table providers via a stable FFI. With this change it enables breaking the requirement for python providers to include all of datafusion-python and re-export it. With this change we can allow providers with different underlying datafusion versions to interoperate.
## What changes are included in this PR?
Adds support for `TableProvider` via FFI. In order to support this, it also includes `ExecutionPlan`, `SessionConfig`, `PlanProperties`, and `TableType`. As this gets used more, I expect we will want to expose other features but this gives an initial first implementation that solves an immediate need.
## Are these changes tested?
Some unit tests are provided. Additionally I did the following test:
I created a separate crate with the contents of `datafusion/ffi` so that I can test it against different versions of DataFusion by modifying the dependencies in Cargo.toml. Then I used this crate to build a test implementation of `datafusion-python` against DataFusion 42.0.0. I adjusted the test crate and built a test implementation of `delta-rs` against DataFusion 41.0.0. Then I registered the delta table in python against the session context. I was able to query the table with push down filters via this FFI interface even though the underlying DataFusion versions were different.
Additionally I ran memory leak checks against the provided unit tests and against running in python.
## Are there any user-facing changes?
This is not breaking, but a pure addition of a new `datafusion-ffi` library.
## Remaining Issues
- [ ] There is some inconsistency between the usage of `ExportedXYZ` and just using the raw `FFI_XYZ`. We should make the usage consistent across all struct types.
- [ ] Add documentation to explain the reasoning behind creating the data the way we do in the private data and foreign structs.
- [ ] Add documentation to explain more clearly the delineation between the `ExportedXYZ` and `ForeignXYZ`. It would probably be good to have a use case since which is "foreign" and which is "exported" can be complicated during some of the function calls.
- [ ] It would be *great* to demonstrate a C++ implementation linked against DataFusion rust. This might really open the doors for some implementations that are not feasible to convert to Rust.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] FFI initial implementation [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #12920:
URL: https://github.com/apache/datafusion/pull/12920#discussion_r1820767477
##########
datafusion/ffi/src/plan_properties.rs:
##########
@@ -0,0 +1,330 @@
+// 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.
+
+use std::{ffi::c_void, sync::Arc};
+
+use abi_stable::{
+ std_types::{
+ RResult::{self, RErr, ROk},
+ RStr, RVec,
+ },
+ StableAbi,
+};
+use arrow::{
+ datatypes::{Schema, SchemaRef},
+ ffi::FFI_ArrowSchema,
+};
+use datafusion::{
+ error::{DataFusionError, Result},
+ physical_expr::EquivalenceProperties,
+ physical_plan::{ExecutionMode, PlanProperties},
+ prelude::SessionContext,
+};
+use datafusion_proto::{
+ physical_plan::{
+ from_proto::{parse_physical_sort_exprs, parse_protobuf_partitioning},
+ to_proto::{serialize_partitioning, serialize_physical_sort_exprs},
+ DefaultPhysicalExtensionCodec,
+ },
+ protobuf::{Partitioning, PhysicalSortExprNodeCollection},
+};
+use log::error;
+use prost::Message;
+
+// TODO: should we just make ExecutionMode repr(C)?
+#[repr(C)]
+#[allow(non_camel_case_types)]
+#[derive(Clone, StableAbi)]
+pub enum FFI_ExecutionMode {
+ Bounded,
+ Unbounded,
+ PipelineBreaking,
+}
+
+impl From<ExecutionMode> for FFI_ExecutionMode {
+ fn from(value: ExecutionMode) -> Self {
+ match value {
+ ExecutionMode::Bounded => FFI_ExecutionMode::Bounded,
+ ExecutionMode::Unbounded => FFI_ExecutionMode::Unbounded,
+ ExecutionMode::PipelineBreaking => FFI_ExecutionMode::PipelineBreaking,
+ }
+ }
+}
+
+impl From<FFI_ExecutionMode> for ExecutionMode {
+ fn from(value: FFI_ExecutionMode) -> Self {
+ match value {
+ FFI_ExecutionMode::Bounded => ExecutionMode::Bounded,
+ FFI_ExecutionMode::Unbounded => ExecutionMode::Unbounded,
+ FFI_ExecutionMode::PipelineBreaking => ExecutionMode::PipelineBreaking,
+ }
+ }
+}
+
+#[repr(C)]
+#[derive(Debug, StableAbi)]
+pub struct WrappedSchema(#[sabi(unsafe_opaque_field)] pub FFI_ArrowSchema);
+
+impl From<SchemaRef> for WrappedSchema {
+ fn from(value: SchemaRef) -> Self {
+ let ffi_schema = match FFI_ArrowSchema::try_from(value.as_ref()) {
+ Ok(s) => s,
+ Err(e) => {
+ error!("Unable to convert DataFusion Schema to FFI_ArrowSchema in FFI_PlanProperties. {}", e);
+ FFI_ArrowSchema::empty()
+ }
+ };
+
+ WrappedSchema(ffi_schema)
+ }
+}
+
+impl From<WrappedSchema> for SchemaRef {
+ fn from(value: WrappedSchema) -> Self {
+ let schema = match Schema::try_from(&value.0) {
+ Ok(s) => s,
+ Err(e) => {
+ error!("Unable to convert from FFI_ArrowSchema to DataFusion Schema in FFI_PlanProperties. {}", e);
+ Schema::empty()
+ }
+ };
+ Arc::new(schema)
+ }
+}
+
+#[repr(C)]
+#[derive(Debug, StableAbi)]
+#[allow(missing_docs)]
+#[allow(non_camel_case_types)]
+pub struct FFI_PlanProperties {
Review Comment:
I will try and review https://github.com/apache/datafusion/pull/13136 later today
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] FFI initial implementation [datafusion]
Posted by "timsaucer (via GitHub)" <gi...@apache.org>.
timsaucer commented on code in PR #12920:
URL: https://github.com/apache/datafusion/pull/12920#discussion_r1818111708
##########
datafusion/ffi/src/plan_properties.rs:
##########
@@ -0,0 +1,330 @@
+// 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.
+
+use std::{ffi::c_void, sync::Arc};
+
+use abi_stable::{
+ std_types::{
+ RResult::{self, RErr, ROk},
+ RStr, RVec,
+ },
+ StableAbi,
+};
+use arrow::{
+ datatypes::{Schema, SchemaRef},
+ ffi::FFI_ArrowSchema,
+};
+use datafusion::{
+ error::{DataFusionError, Result},
+ physical_expr::EquivalenceProperties,
+ physical_plan::{ExecutionMode, PlanProperties},
+ prelude::SessionContext,
+};
+use datafusion_proto::{
+ physical_plan::{
+ from_proto::{parse_physical_sort_exprs, parse_protobuf_partitioning},
+ to_proto::{serialize_partitioning, serialize_physical_sort_exprs},
+ DefaultPhysicalExtensionCodec,
+ },
+ protobuf::{Partitioning, PhysicalSortExprNodeCollection},
+};
+use log::error;
+use prost::Message;
+
+// TODO: should we just make ExecutionMode repr(C)?
+#[repr(C)]
+#[allow(non_camel_case_types)]
+#[derive(Clone, StableAbi)]
+pub enum FFI_ExecutionMode {
+ Bounded,
+ Unbounded,
+ PipelineBreaking,
+}
+
+impl From<ExecutionMode> for FFI_ExecutionMode {
+ fn from(value: ExecutionMode) -> Self {
+ match value {
+ ExecutionMode::Bounded => FFI_ExecutionMode::Bounded,
+ ExecutionMode::Unbounded => FFI_ExecutionMode::Unbounded,
+ ExecutionMode::PipelineBreaking => FFI_ExecutionMode::PipelineBreaking,
+ }
+ }
+}
+
+impl From<FFI_ExecutionMode> for ExecutionMode {
+ fn from(value: FFI_ExecutionMode) -> Self {
+ match value {
+ FFI_ExecutionMode::Bounded => ExecutionMode::Bounded,
+ FFI_ExecutionMode::Unbounded => ExecutionMode::Unbounded,
+ FFI_ExecutionMode::PipelineBreaking => ExecutionMode::PipelineBreaking,
+ }
+ }
+}
+
+#[repr(C)]
+#[derive(Debug, StableAbi)]
+pub struct WrappedSchema(#[sabi(unsafe_opaque_field)] pub FFI_ArrowSchema);
+
+impl From<SchemaRef> for WrappedSchema {
+ fn from(value: SchemaRef) -> Self {
+ let ffi_schema = match FFI_ArrowSchema::try_from(value.as_ref()) {
+ Ok(s) => s,
+ Err(e) => {
+ error!("Unable to convert DataFusion Schema to FFI_ArrowSchema in FFI_PlanProperties. {}", e);
+ FFI_ArrowSchema::empty()
+ }
+ };
+
+ WrappedSchema(ffi_schema)
+ }
+}
+
+impl From<WrappedSchema> for SchemaRef {
+ fn from(value: WrappedSchema) -> Self {
+ let schema = match Schema::try_from(&value.0) {
+ Ok(s) => s,
+ Err(e) => {
+ error!("Unable to convert from FFI_ArrowSchema to DataFusion Schema in FFI_PlanProperties. {}", e);
+ Schema::empty()
+ }
+ };
+ Arc::new(schema)
+ }
+}
+
+#[repr(C)]
+#[derive(Debug, StableAbi)]
+#[allow(missing_docs)]
+#[allow(non_camel_case_types)]
+pub struct FFI_PlanProperties {
Review Comment:
@alamb I've added a separate PR https://github.com/apache/datafusion/pull/13136 for adding these to the protobuf message definitions. If you support that, then I'll be able to remove this exposure here and convert it over to serialized data. Actually, I think this is a much nicer approach since these are static values for a plan.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] FFI initial implementation [datafusion]
Posted by "timsaucer (via GitHub)" <gi...@apache.org>.
timsaucer commented on PR #12920:
URL: https://github.com/apache/datafusion/pull/12920#issuecomment-2445559200
Thank you for the reviews. I just did a quick rebase so the merge can go through. I'll get started on that end to end example in a different PR.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] FFI initial implementation [datafusion]
Posted by "timsaucer (via GitHub)" <gi...@apache.org>.
timsaucer commented on PR #12920:
URL: https://github.com/apache/datafusion/pull/12920#issuecomment-2439669272
Thanks for the feedback! I'll try to address the remaining concerns in the next few days.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] FFI initial implementation [datafusion]
Posted by "notfilippo (via GitHub)" <gi...@apache.org>.
notfilippo commented on code in PR #12920:
URL: https://github.com/apache/datafusion/pull/12920#discussion_r1809261663
##########
datafusion/ffi/Cargo.toml:
##########
@@ -0,0 +1,48 @@
+# 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.
+
+[package]
+name = "datafusion-ffi"
+description = "Foreign Function Interface implementation for DataFusion"
+readme = "README.md"
+version = { workspace = true }
+edition = { workspace = true }
+homepage = { workspace = true }
+repository = { workspace = true }
+license = { workspace = true }
+authors = { workspace = true }
+# Specify MSRV here as `cargo msrv` doesn't support workspace version
+rust-version = "1.76"
+
+[lints]
+workspace = true
+
+[lib]
+name = "datafusion_ffi"
+path = "src/lib.rs"
+
+[dependencies]
+abi_stable = "0.11.3"
+arrow = { workspace = true, features = ["ffi"] }
+async-ffi = { version = "0.5.0", features = ["abi_stable"] }
+async-trait = { workspace = true }
+datafusion = { workspace = true, default-features = true }
Review Comment:
IIUC saying `default-features = true` while in the workspace the dependency is declared as `default-features = false` (https://github.com/apache/datafusion/blob/701cb00e3b9b24b00908ad4a07ed01d68ccfa7c2/Cargo.toml#L95C1-L95C88) is the correct way of doing this 👍
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] FFI initial implementation [datafusion]
Posted by "timsaucer (via GitHub)" <gi...@apache.org>.
timsaucer commented on code in PR #12920:
URL: https://github.com/apache/datafusion/pull/12920#discussion_r1817895746
##########
datafusion/ffi/README.md:
##########
@@ -0,0 +1,81 @@
+<!---
+ 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.
+-->
+
+# `datafusion-ffi`: Apache DataFusion Foreign Function Interface
+
+This crate contains code to allow interoperability of Apache [DataFusion]
+with functions from other languages using a stable interface.
+
+See [API Docs] for details and examples.
+
+We expect this crate may be used by both sides of the FFI. This allows users
+to create modules that can interoperate with the necessity of using the same
+version of DataFusion. The driving use case has been the `datafusion-python`
+repository, but many other use cases may exist. We envision at least two
+use cases.
+
+1. `datafusion-python` which will use the FFI to provide external services such
+ as a `TableProvider` without needing to re-export the entire `datafusion-python`
+ code base. With `datafusion-ffi` these packages do not need `datafusion-python`
+ as a dependency at all.
+2. Users may want to create a modular interface that allows runtime loading of
+ libraries.
+
+## Struct Layout
+
+In this crate we have a variety of structs which closely mimic the behavior of
+their internal counterparts. In the following example, we will refer to the
+`TableProvider`, but the same pattern exists for other structs.
+
+Each of the exposted structs in this crate is provided with a variant prefixed
+with `Foreign`. This variant is designed to be used by the consumer of the
+foreign code. The `Foreign` structs should _never_ access the `private_data`
+fields. Instead they should only access the data returned through the function
+calls defined on the `FFI_` structs. The second purpose of the `Foreign`
+structs is to contain additional data that may be needed by the traits that
+are implemented on them. Some of these traits require borrowing data which
+can be far more convienent to be locally stored.
+
+For example, we have a struct `FFI_TableProvider` to give access to the
+`TableProvider` functions like `table_type()` and `scan()`. If we write a
+library that wishes to expose it's `TableProvider`, then we can access the
+private data that contains the Arc reference to the `TableProvider` via
+`FFI_TableProvider`. This data is local to the library.
+
+If we have a program that accesses a `TableProvider` via FFI, then it
+will use `ForeignTableProvider`. When using `ForeignTableProvider` we **must**
+not attempt to access the `private_data` field in `FFI_TableProvider`. If a
+user is testing locally, you may be able to successfully access this field, but
+it will only work if you are building against the exact same version of
+`DataFusion` for both libraries **and** the same compiler. It will not work
+in general.
+
+It is worth noting that which library is the `local` and which is `foreign`
Review Comment:
I was originally pushing for a C style API but based on some feedback on the discord decided to go with this crate that focuses on rust-rust interface with a *very* nice interface. I'll see what it would be like to try using C with it as a small test.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] FFI initial implementation [datafusion]
Posted by "timsaucer (via GitHub)" <gi...@apache.org>.
timsaucer commented on code in PR #12920:
URL: https://github.com/apache/datafusion/pull/12920#discussion_r1809249668
##########
datafusion/ffi/Cargo.toml:
##########
@@ -0,0 +1,48 @@
+# 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.
+
+[package]
+name = "datafusion-ffi"
+description = "Foreign Function Interface implementation for DataFusion"
+readme = "README.md"
+version = { workspace = true }
+edition = { workspace = true }
+homepage = { workspace = true }
+repository = { workspace = true }
+license = { workspace = true }
+authors = { workspace = true }
+# Specify MSRV here as `cargo msrv` doesn't support workspace version
+rust-version = "1.76"
+
+[lints]
+workspace = true
+
+[lib]
+name = "datafusion_ffi"
+path = "src/lib.rs"
+
+[dependencies]
+abi_stable = "0.11.3"
+arrow = { workspace = true, features = ["ffi"] }
+async-ffi = { version = "0.5.0", features = ["abi_stable"] }
+async-trait = { workspace = true }
+datafusion = { workspace = true, default-features = true }
Review Comment:
I'm no expert on cargo. If I just remove the `default-features = true` I will get all of the dependencies I need in my build. Is that sufficient? I removed it in my push right now.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] FFI initial implementation [datafusion]
Posted by "timsaucer (via GitHub)" <gi...@apache.org>.
timsaucer commented on code in PR #12920:
URL: https://github.com/apache/datafusion/pull/12920#discussion_r1809251882
##########
datafusion/ffi/Cargo.toml:
##########
@@ -0,0 +1,48 @@
+# 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.
+
+[package]
+name = "datafusion-ffi"
+description = "Foreign Function Interface implementation for DataFusion"
+readme = "README.md"
+version = { workspace = true }
+edition = { workspace = true }
+homepage = { workspace = true }
+repository = { workspace = true }
+license = { workspace = true }
+authors = { workspace = true }
+# Specify MSRV here as `cargo msrv` doesn't support workspace version
+rust-version = "1.76"
+
+[lints]
+workspace = true
+
+[lib]
+name = "datafusion_ffi"
+path = "src/lib.rs"
+
+[dependencies]
+abi_stable = "0.11.3"
+arrow = { workspace = true, features = ["ffi"] }
+async-ffi = { version = "0.5.0", features = ["abi_stable"] }
+async-trait = { workspace = true }
+datafusion = { workspace = true, default-features = true }
+datafusion-proto = { workspace = true }
+doc-comment = { workspace = true }
+futures = { workspace = true }
+prost = { workspace = true }
+tokio = { workspace = true }
Review Comment:
Correct! updated.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] FFI initial implementation [datafusion]
Posted by "timsaucer (via GitHub)" <gi...@apache.org>.
timsaucer commented on code in PR #12920:
URL: https://github.com/apache/datafusion/pull/12920#discussion_r1817895881
##########
datafusion/ffi/README.md:
##########
@@ -0,0 +1,81 @@
+<!---
+ 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.
+-->
+
+# `datafusion-ffi`: Apache DataFusion Foreign Function Interface
+
+This crate contains code to allow interoperability of Apache [DataFusion]
+with functions from other languages using a stable interface.
+
+See [API Docs] for details and examples.
+
+We expect this crate may be used by both sides of the FFI. This allows users
+to create modules that can interoperate with the necessity of using the same
+version of DataFusion. The driving use case has been the `datafusion-python`
+repository, but many other use cases may exist. We envision at least two
+use cases.
+
+1. `datafusion-python` which will use the FFI to provide external services such
+ as a `TableProvider` without needing to re-export the entire `datafusion-python`
+ code base. With `datafusion-ffi` these packages do not need `datafusion-python`
+ as a dependency at all.
+2. Users may want to create a modular interface that allows runtime loading of
+ libraries.
+
+## Struct Layout
Review Comment:
Ok, I'll probably move this over to TableProvider which is the most complex and have the other structs refer to it for more information.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] FFI initial implementation [datafusion]
Posted by "timsaucer (via GitHub)" <gi...@apache.org>.
timsaucer commented on PR #12920:
URL: https://github.com/apache/datafusion/pull/12920#issuecomment-2445268512
Adding issue https://github.com/apache/datafusion/issues/13175 to track additional documentation. I'd like to merge this in and work on that (I've assigned it to myself) so we can unblock `datafusion-python` and `delta-rs`
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] FFI initial implementation [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #12920:
URL: https://github.com/apache/datafusion/pull/12920#discussion_r1821437008
##########
datafusion/ffi/src/plan_properties.rs:
##########
@@ -0,0 +1,330 @@
+// 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.
+
+use std::{ffi::c_void, sync::Arc};
+
+use abi_stable::{
+ std_types::{
+ RResult::{self, RErr, ROk},
+ RStr, RVec,
+ },
+ StableAbi,
+};
+use arrow::{
+ datatypes::{Schema, SchemaRef},
+ ffi::FFI_ArrowSchema,
+};
+use datafusion::{
+ error::{DataFusionError, Result},
+ physical_expr::EquivalenceProperties,
+ physical_plan::{ExecutionMode, PlanProperties},
+ prelude::SessionContext,
+};
+use datafusion_proto::{
+ physical_plan::{
+ from_proto::{parse_physical_sort_exprs, parse_protobuf_partitioning},
+ to_proto::{serialize_partitioning, serialize_physical_sort_exprs},
+ DefaultPhysicalExtensionCodec,
+ },
+ protobuf::{Partitioning, PhysicalSortExprNodeCollection},
+};
+use log::error;
+use prost::Message;
+
+// TODO: should we just make ExecutionMode repr(C)?
+#[repr(C)]
+#[allow(non_camel_case_types)]
+#[derive(Clone, StableAbi)]
+pub enum FFI_ExecutionMode {
+ Bounded,
+ Unbounded,
+ PipelineBreaking,
+}
+
+impl From<ExecutionMode> for FFI_ExecutionMode {
+ fn from(value: ExecutionMode) -> Self {
+ match value {
+ ExecutionMode::Bounded => FFI_ExecutionMode::Bounded,
+ ExecutionMode::Unbounded => FFI_ExecutionMode::Unbounded,
+ ExecutionMode::PipelineBreaking => FFI_ExecutionMode::PipelineBreaking,
+ }
+ }
+}
+
+impl From<FFI_ExecutionMode> for ExecutionMode {
+ fn from(value: FFI_ExecutionMode) -> Self {
+ match value {
+ FFI_ExecutionMode::Bounded => ExecutionMode::Bounded,
+ FFI_ExecutionMode::Unbounded => ExecutionMode::Unbounded,
+ FFI_ExecutionMode::PipelineBreaking => ExecutionMode::PipelineBreaking,
+ }
+ }
+}
+
+#[repr(C)]
+#[derive(Debug, StableAbi)]
+pub struct WrappedSchema(#[sabi(unsafe_opaque_field)] pub FFI_ArrowSchema);
+
+impl From<SchemaRef> for WrappedSchema {
+ fn from(value: SchemaRef) -> Self {
+ let ffi_schema = match FFI_ArrowSchema::try_from(value.as_ref()) {
+ Ok(s) => s,
+ Err(e) => {
+ error!("Unable to convert DataFusion Schema to FFI_ArrowSchema in FFI_PlanProperties. {}", e);
+ FFI_ArrowSchema::empty()
+ }
+ };
+
+ WrappedSchema(ffi_schema)
+ }
+}
+
+impl From<WrappedSchema> for SchemaRef {
+ fn from(value: WrappedSchema) -> Self {
+ let schema = match Schema::try_from(&value.0) {
+ Ok(s) => s,
+ Err(e) => {
+ error!("Unable to convert from FFI_ArrowSchema to DataFusion Schema in FFI_PlanProperties. {}", e);
+ Schema::empty()
+ }
+ };
+ Arc::new(schema)
+ }
+}
+
+#[repr(C)]
+#[derive(Debug, StableAbi)]
+#[allow(missing_docs)]
+#[allow(non_camel_case_types)]
+pub struct FFI_PlanProperties {
Review Comment:
Ok now I think I understand the intricies more I think the approach in this PR is the better approach for the reasons I describe in https://github.com/apache/datafusion/pull/13136#pullrequestreview-2402924526
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] FFI initial implementation [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #12920:
URL: https://github.com/apache/datafusion/pull/12920#issuecomment-2424840850
I see this PR and plan to review it, hopefully today but likely tomorrow (Mon)
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] FFI initial implementation [datafusion]
Posted by "notfilippo (via GitHub)" <gi...@apache.org>.
notfilippo commented on code in PR #12920:
URL: https://github.com/apache/datafusion/pull/12920#discussion_r1807793949
##########
datafusion/ffi/Cargo.toml:
##########
@@ -0,0 +1,48 @@
+# 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.
+
+[package]
+name = "datafusion-ffi"
+description = "Foreign Function Interface implementation for DataFusion"
+readme = "README.md"
+version = { workspace = true }
+edition = { workspace = true }
+homepage = { workspace = true }
+repository = { workspace = true }
+license = { workspace = true }
+authors = { workspace = true }
+# Specify MSRV here as `cargo msrv` doesn't support workspace version
+rust-version = "1.76"
+
+[lints]
+workspace = true
+
+[lib]
+name = "datafusion_ffi"
+path = "src/lib.rs"
+
+[dependencies]
+abi_stable = "0.11.3"
+arrow = { workspace = true, features = ["ffi"] }
+async-ffi = { version = "0.5.0", features = ["abi_stable"] }
+async-trait = { workspace = true }
+datafusion = { workspace = true, default-features = true }
Review Comment:
Given the we are targeting FFI, I think not all default DataFusion features should be enabled by default when working the `datafusion-ffi` crate. Some of DataFusion's feature bring in some external native dependency (e.g. crypto functions) and they may conflict with the native setup of some binding implementations.
Instead I think that we should probably leave the choice of features to the user implementing the bindings and leave just the minimum set required for this crate to work.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] FFI initial implementation [datafusion]
Posted by "timsaucer (via GitHub)" <gi...@apache.org>.
timsaucer merged PR #12920:
URL: https://github.com/apache/datafusion/pull/12920
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] FFI initial implementation [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #12920:
URL: https://github.com/apache/datafusion/pull/12920#discussion_r1811062815
##########
datafusion/ffi/README.md:
##########
@@ -0,0 +1,81 @@
+<!---
+ 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.
+-->
+
+# `datafusion-ffi`: Apache DataFusion Foreign Function Interface
+
+This crate contains code to allow interoperability of Apache [DataFusion]
+with functions from other languages using a stable interface.
Review Comment:
It may also be good to point out that the ffi interface allows different *versions* of datafusion to interact with each other over stable interfaces in the intro
```suggestion
This crate contains code to allow interoperability of Apache [DataFusion]
with functions from other languages and/or versions using a stable interface.
```
##########
datafusion/ffi/src/plan_properties.rs:
##########
@@ -0,0 +1,330 @@
+// 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.
+
+use std::{ffi::c_void, sync::Arc};
+
+use abi_stable::{
+ std_types::{
+ RResult::{self, RErr, ROk},
+ RStr, RVec,
+ },
+ StableAbi,
+};
+use arrow::{
+ datatypes::{Schema, SchemaRef},
+ ffi::FFI_ArrowSchema,
+};
+use datafusion::{
+ error::{DataFusionError, Result},
+ physical_expr::EquivalenceProperties,
+ physical_plan::{ExecutionMode, PlanProperties},
+ prelude::SessionContext,
+};
+use datafusion_proto::{
+ physical_plan::{
+ from_proto::{parse_physical_sort_exprs, parse_protobuf_partitioning},
+ to_proto::{serialize_partitioning, serialize_physical_sort_exprs},
+ DefaultPhysicalExtensionCodec,
+ },
+ protobuf::{Partitioning, PhysicalSortExprNodeCollection},
+};
+use log::error;
+use prost::Message;
+
+// TODO: should we just make ExecutionMode repr(C)?
+#[repr(C)]
+#[allow(non_camel_case_types)]
+#[derive(Clone, StableAbi)]
+pub enum FFI_ExecutionMode {
+ Bounded,
+ Unbounded,
+ PipelineBreaking,
+}
+
+impl From<ExecutionMode> for FFI_ExecutionMode {
+ fn from(value: ExecutionMode) -> Self {
+ match value {
+ ExecutionMode::Bounded => FFI_ExecutionMode::Bounded,
+ ExecutionMode::Unbounded => FFI_ExecutionMode::Unbounded,
+ ExecutionMode::PipelineBreaking => FFI_ExecutionMode::PipelineBreaking,
+ }
+ }
+}
+
+impl From<FFI_ExecutionMode> for ExecutionMode {
+ fn from(value: FFI_ExecutionMode) -> Self {
+ match value {
+ FFI_ExecutionMode::Bounded => ExecutionMode::Bounded,
+ FFI_ExecutionMode::Unbounded => ExecutionMode::Unbounded,
+ FFI_ExecutionMode::PipelineBreaking => ExecutionMode::PipelineBreaking,
+ }
+ }
+}
+
+#[repr(C)]
+#[derive(Debug, StableAbi)]
+pub struct WrappedSchema(#[sabi(unsafe_opaque_field)] pub FFI_ArrowSchema);
+
+impl From<SchemaRef> for WrappedSchema {
+ fn from(value: SchemaRef) -> Self {
+ let ffi_schema = match FFI_ArrowSchema::try_from(value.as_ref()) {
+ Ok(s) => s,
+ Err(e) => {
+ error!("Unable to convert DataFusion Schema to FFI_ArrowSchema in FFI_PlanProperties. {}", e);
+ FFI_ArrowSchema::empty()
+ }
+ };
+
+ WrappedSchema(ffi_schema)
+ }
+}
+
+impl From<WrappedSchema> for SchemaRef {
+ fn from(value: WrappedSchema) -> Self {
+ let schema = match Schema::try_from(&value.0) {
+ Ok(s) => s,
+ Err(e) => {
+ error!("Unable to convert from FFI_ArrowSchema to DataFusion Schema in FFI_PlanProperties. {}", e);
+ Schema::empty()
+ }
+ };
+ Arc::new(schema)
+ }
+}
+
+#[repr(C)]
+#[derive(Debug, StableAbi)]
+#[allow(missing_docs)]
+#[allow(non_camel_case_types)]
+pub struct FFI_PlanProperties {
Review Comment:
Do we need to expose PlanProperties? This seems pretty low level and a large API surface area. Unless it is all needed I suggest we don't expose it at first and only do so when needed
##########
datafusion/ffi/src/table_provider.rs:
##########
@@ -0,0 +1,419 @@
+// 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.
+
+use std::{any::Any, ffi::c_void, sync::Arc};
+
+use abi_stable::{
+ std_types::{ROption, RResult, RString, RVec},
+ StableAbi,
+};
+use arrow::datatypes::SchemaRef;
+use async_ffi::{FfiFuture, FutureExt};
+use async_trait::async_trait;
+use datafusion::{
+ catalog::{Session, TableProvider},
+ datasource::TableType,
+ error::DataFusionError,
+ execution::session_state::SessionStateBuilder,
+ logical_expr::TableProviderFilterPushDown,
+ physical_plan::ExecutionPlan,
+ prelude::{Expr, SessionContext},
+};
+use datafusion_proto::{
+ logical_plan::{
+ from_proto::parse_exprs, to_proto::serialize_exprs, DefaultLogicalExtensionCodec,
+ },
+ protobuf::LogicalExprList,
+};
+use prost::Message;
+
+use crate::{
+ plan_properties::WrappedSchema,
+ session_config::ForeignSessionConfig,
+ table_source::{FFI_TableProviderFilterPushDown, FFI_TableType},
+};
+
+use super::{
+ execution_plan::{FFI_ExecutionPlan, ForeignExecutionPlan},
+ session_config::FFI_SessionConfig,
+};
+use datafusion::error::Result;
+
+/// A stable interface for creating a DataFusion TableProvider.
+#[repr(C)]
+#[derive(Debug, StableAbi)]
+#[allow(non_camel_case_types)]
+pub struct FFI_TableProvider {
+ pub schema: unsafe extern "C" fn(provider: &Self) -> WrappedSchema,
+ pub scan: unsafe extern "C" fn(
+ provider: &Self,
+ session_config: &FFI_SessionConfig,
+ projections: RVec<usize>,
+ filters_serialized: RVec<u8>,
Review Comment:
it would probably help to document what these types are (e.g. that this is a protobuf serialized bytes)
##########
datafusion/ffi/README.md:
##########
@@ -0,0 +1,81 @@
+<!---
+ 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.
+-->
+
+# `datafusion-ffi`: Apache DataFusion Foreign Function Interface
+
+This crate contains code to allow interoperability of Apache [DataFusion]
+with functions from other languages using a stable interface.
+
+See [API Docs] for details and examples.
+
+We expect this crate may be used by both sides of the FFI. This allows users
+to create modules that can interoperate with the necessity of using the same
+version of DataFusion. The driving use case has been the `datafusion-python`
+repository, but many other use cases may exist. We envision at least two
+use cases.
+
+1. `datafusion-python` which will use the FFI to provide external services such
+ as a `TableProvider` without needing to re-export the entire `datafusion-python`
+ code base. With `datafusion-ffi` these packages do not need `datafusion-python`
+ as a dependency at all.
+2. Users may want to create a modular interface that allows runtime loading of
+ libraries.
+
+## Struct Layout
Review Comment:
I suggest we move this discussion of code layout / struct naming into the rust code so it stays closer to the code and has less chance to get out of date
##########
datafusion/ffi/README.md:
##########
@@ -0,0 +1,81 @@
+<!---
+ 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.
+-->
+
+# `datafusion-ffi`: Apache DataFusion Foreign Function Interface
+
+This crate contains code to allow interoperability of Apache [DataFusion]
+with functions from other languages using a stable interface.
+
+See [API Docs] for details and examples.
+
+We expect this crate may be used by both sides of the FFI. This allows users
+to create modules that can interoperate with the necessity of using the same
+version of DataFusion. The driving use case has been the `datafusion-python`
+repository, but many other use cases may exist. We envision at least two
+use cases.
+
+1. `datafusion-python` which will use the FFI to provide external services such
+ as a `TableProvider` without needing to re-export the entire `datafusion-python`
+ code base. With `datafusion-ffi` these packages do not need `datafusion-python`
+ as a dependency at all.
+2. Users may want to create a modular interface that allows runtime loading of
+ libraries.
+
+## Struct Layout
+
+In this crate we have a variety of structs which closely mimic the behavior of
+their internal counterparts. In the following example, we will refer to the
+`TableProvider`, but the same pattern exists for other structs.
+
+Each of the exposted structs in this crate is provided with a variant prefixed
+with `Foreign`. This variant is designed to be used by the consumer of the
+foreign code. The `Foreign` structs should _never_ access the `private_data`
+fields. Instead they should only access the data returned through the function
+calls defined on the `FFI_` structs. The second purpose of the `Foreign`
+structs is to contain additional data that may be needed by the traits that
+are implemented on them. Some of these traits require borrowing data which
+can be far more convienent to be locally stored.
+
+For example, we have a struct `FFI_TableProvider` to give access to the
+`TableProvider` functions like `table_type()` and `scan()`. If we write a
+library that wishes to expose it's `TableProvider`, then we can access the
+private data that contains the Arc reference to the `TableProvider` via
+`FFI_TableProvider`. This data is local to the library.
+
+If we have a program that accesses a `TableProvider` via FFI, then it
+will use `ForeignTableProvider`. When using `ForeignTableProvider` we **must**
+not attempt to access the `private_data` field in `FFI_TableProvider`. If a
+user is testing locally, you may be able to successfully access this field, but
+it will only work if you are building against the exact same version of
+`DataFusion` for both libraries **and** the same compiler. It will not work
+in general.
+
+It is worth noting that which library is the `local` and which is `foreign`
Review Comment:
Maybe we should name this "DataFusion C API" or something instead of using the rust FFI term 🤔
##########
datafusion/ffi/README.md:
##########
@@ -0,0 +1,81 @@
+<!---
+ 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.
+-->
+
+# `datafusion-ffi`: Apache DataFusion Foreign Function Interface
+
+This crate contains code to allow interoperability of Apache [DataFusion]
+with functions from other languages using a stable interface.
+
+See [API Docs] for details and examples.
+
+We expect this crate may be used by both sides of the FFI. This allows users
+to create modules that can interoperate with the necessity of using the same
+version of DataFusion. The driving use case has been the `datafusion-python`
Review Comment:
I think this text is backwards -- it is "without the necessity". Maybe we could rephrase like
```suggestion
We expect this crate may be used by both sides of the FFI. This allows users
to create modules that can interoperate using different
versions of DataFusion. The driving use case has been the `datafusion-python`
```
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] FFI initial implementation [datafusion]
Posted by "timsaucer (via GitHub)" <gi...@apache.org>.
timsaucer commented on code in PR #12920:
URL: https://github.com/apache/datafusion/pull/12920#discussion_r1817895480
##########
datafusion/ffi/src/plan_properties.rs:
##########
@@ -0,0 +1,330 @@
+// 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.
+
+use std::{ffi::c_void, sync::Arc};
+
+use abi_stable::{
+ std_types::{
+ RResult::{self, RErr, ROk},
+ RStr, RVec,
+ },
+ StableAbi,
+};
+use arrow::{
+ datatypes::{Schema, SchemaRef},
+ ffi::FFI_ArrowSchema,
+};
+use datafusion::{
+ error::{DataFusionError, Result},
+ physical_expr::EquivalenceProperties,
+ physical_plan::{ExecutionMode, PlanProperties},
+ prelude::SessionContext,
+};
+use datafusion_proto::{
+ physical_plan::{
+ from_proto::{parse_physical_sort_exprs, parse_protobuf_partitioning},
+ to_proto::{serialize_partitioning, serialize_physical_sort_exprs},
+ DefaultPhysicalExtensionCodec,
+ },
+ protobuf::{Partitioning, PhysicalSortExprNodeCollection},
+};
+use log::error;
+use prost::Message;
+
+// TODO: should we just make ExecutionMode repr(C)?
+#[repr(C)]
+#[allow(non_camel_case_types)]
+#[derive(Clone, StableAbi)]
+pub enum FFI_ExecutionMode {
+ Bounded,
+ Unbounded,
+ PipelineBreaking,
+}
+
+impl From<ExecutionMode> for FFI_ExecutionMode {
+ fn from(value: ExecutionMode) -> Self {
+ match value {
+ ExecutionMode::Bounded => FFI_ExecutionMode::Bounded,
+ ExecutionMode::Unbounded => FFI_ExecutionMode::Unbounded,
+ ExecutionMode::PipelineBreaking => FFI_ExecutionMode::PipelineBreaking,
+ }
+ }
+}
+
+impl From<FFI_ExecutionMode> for ExecutionMode {
+ fn from(value: FFI_ExecutionMode) -> Self {
+ match value {
+ FFI_ExecutionMode::Bounded => ExecutionMode::Bounded,
+ FFI_ExecutionMode::Unbounded => ExecutionMode::Unbounded,
+ FFI_ExecutionMode::PipelineBreaking => ExecutionMode::PipelineBreaking,
+ }
+ }
+}
+
+#[repr(C)]
+#[derive(Debug, StableAbi)]
+pub struct WrappedSchema(#[sabi(unsafe_opaque_field)] pub FFI_ArrowSchema);
+
+impl From<SchemaRef> for WrappedSchema {
+ fn from(value: SchemaRef) -> Self {
+ let ffi_schema = match FFI_ArrowSchema::try_from(value.as_ref()) {
+ Ok(s) => s,
+ Err(e) => {
+ error!("Unable to convert DataFusion Schema to FFI_ArrowSchema in FFI_PlanProperties. {}", e);
+ FFI_ArrowSchema::empty()
+ }
+ };
+
+ WrappedSchema(ffi_schema)
+ }
+}
+
+impl From<WrappedSchema> for SchemaRef {
+ fn from(value: WrappedSchema) -> Self {
+ let schema = match Schema::try_from(&value.0) {
+ Ok(s) => s,
+ Err(e) => {
+ error!("Unable to convert from FFI_ArrowSchema to DataFusion Schema in FFI_PlanProperties. {}", e);
+ Schema::empty()
+ }
+ };
+ Arc::new(schema)
+ }
+}
+
+#[repr(C)]
+#[derive(Debug, StableAbi)]
+#[allow(missing_docs)]
+#[allow(non_camel_case_types)]
+pub struct FFI_PlanProperties {
Review Comment:
Unfortunately, I think we need to. The other option I could see would be to add it to the protobuf message definition and use that to transfer it back and forth.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] FFI initial implementation [datafusion]
Posted by "notfilippo (via GitHub)" <gi...@apache.org>.
notfilippo commented on code in PR #12920:
URL: https://github.com/apache/datafusion/pull/12920#discussion_r1807792175
##########
datafusion/ffi/Cargo.toml:
##########
@@ -0,0 +1,48 @@
+# 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.
+
+[package]
+name = "datafusion-ffi"
+description = "Foreign Function Interface implementation for DataFusion"
+readme = "README.md"
+version = { workspace = true }
+edition = { workspace = true }
+homepage = { workspace = true }
+repository = { workspace = true }
+license = { workspace = true }
+authors = { workspace = true }
+# Specify MSRV here as `cargo msrv` doesn't support workspace version
+rust-version = "1.76"
+
+[lints]
+workspace = true
+
+[lib]
+name = "datafusion_ffi"
+path = "src/lib.rs"
+
+[dependencies]
+abi_stable = "0.11.3"
+arrow = { workspace = true, features = ["ffi"] }
+async-ffi = { version = "0.5.0", features = ["abi_stable"] }
+async-trait = { workspace = true }
+datafusion = { workspace = true, default-features = true }
+datafusion-proto = { workspace = true }
+doc-comment = { workspace = true }
+futures = { workspace = true }
+prost = { workspace = true }
+tokio = { workspace = true }
Review Comment:
Maybe `tokio` is only needed for tests?
##########
datafusion/ffi/src/table_provider.rs:
##########
@@ -0,0 +1,416 @@
+// 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.
+
+use std::{any::Any, ffi::c_void, sync::Arc};
+
+use abi_stable::{
+ std_types::{ROption, RResult, RString, RVec},
+ StableAbi,
+};
+use arrow::{
+ datatypes::{Schema, SchemaRef},
+ ffi::FFI_ArrowSchema,
+};
+use async_ffi::{FfiFuture, FutureExt};
+use async_trait::async_trait;
+use datafusion::{
+ catalog::{Session, TableProvider},
+ datasource::TableType,
+ error::DataFusionError,
+ execution::session_state::SessionStateBuilder,
+ logical_expr::TableProviderFilterPushDown,
+ physical_plan::ExecutionPlan,
+ prelude::{Expr, SessionContext},
+};
+use datafusion_proto::{
+ logical_plan::{
+ from_proto::parse_exprs, to_proto::serialize_exprs, DefaultLogicalExtensionCodec,
+ },
+ protobuf::LogicalExprList,
+};
+use prost::Message;
+
+use crate::{
+ plan_properties::WrappedSchema,
+ session_config::ForeignSessionConfig,
+ table_source::{FFI_TableProviderFilterPushDown, FFI_TableType},
+};
+
+use super::{
+ execution_plan::{FFI_ExecutionPlan, ForeignExecutionPlan},
+ session_config::FFI_SessionConfig,
+};
+use datafusion::error::Result;
+
+/// A stable interface for creating a DataFusion TableProvider.
+#[repr(C)]
+#[derive(Debug, StableAbi)]
+#[allow(non_camel_case_types)]
+pub struct FFI_TableProvider {
+ pub schema: unsafe extern "C" fn(provider: &Self) -> WrappedSchema,
+ pub scan: unsafe extern "C" fn(
+ provider: &Self,
+ session_config: &FFI_SessionConfig,
+ projections: RVec<usize>,
+ filters_serialized: RVec<u8>,
+ limit: ROption<usize>,
+ ) -> FfiFuture<RResult<FFI_ExecutionPlan, RString>>,
+
+ pub table_type: unsafe extern "C" fn(provider: &Self) -> FFI_TableType,
+
+ pub supports_filters_pushdown:
+ unsafe extern "C" fn(
+ provider: &Self,
+ filters_serialized: RVec<u8>,
+ )
+ -> RResult<RVec<FFI_TableProviderFilterPushDown>, RString>,
+
+ pub clone: unsafe extern "C" fn(provider: &Self) -> Self,
+ pub release: unsafe extern "C" fn(arg: &mut Self),
+ pub private_data: *mut c_void,
+}
+
+unsafe impl Send for FFI_TableProvider {}
+unsafe impl Sync for FFI_TableProvider {}
+
+struct ProviderPrivateData {
+ provider: Arc<dyn TableProvider + Send>,
+}
+
+unsafe extern "C" fn schema_fn_wrapper(provider: &FFI_TableProvider) -> WrappedSchema {
+ let private_data = provider.private_data as *const ProviderPrivateData;
+ let provider = &(*private_data).provider;
+
+ // This does silently fail because TableProvider does not return a result.
+ // It expects schema to always pass.
+ let ffi_schema = FFI_ArrowSchema::try_from(provider.schema().as_ref())
+ .unwrap_or(FFI_ArrowSchema::empty());
+
+ WrappedSchema(ffi_schema)
+}
+
+unsafe extern "C" fn table_type_fn_wrapper(
+ provider: &FFI_TableProvider,
+) -> FFI_TableType {
+ let private_data = provider.private_data as *const ProviderPrivateData;
+ let provider = &(*private_data).provider;
+
+ provider.table_type().into()
+}
+
+fn supports_filters_pushdown_internal(
+ provider: &Arc<dyn TableProvider + Send>,
+ filters_serialized: &[u8],
+) -> Result<RVec<FFI_TableProviderFilterPushDown>> {
+ let default_ctx = SessionContext::new();
+ let codec = DefaultLogicalExtensionCodec {};
+
+ let filters = match filters_serialized.is_empty() {
+ true => vec![],
+ false => {
+ let proto_filters = LogicalExprList::decode(filters_serialized)
+ .map_err(|e| DataFusionError::Plan(e.to_string()))?;
+
+ parse_exprs(proto_filters.expr.iter(), &default_ctx, &codec)?
+ }
+ };
+ let filters_borrowed: Vec<&Expr> = filters.iter().collect();
+
+ let results: RVec<_> = provider
+ .supports_filters_pushdown(&filters_borrowed)?
+ .iter()
+ .map(|v| v.into())
+ .collect();
+
+ Ok(results)
+}
+
+unsafe extern "C" fn supports_filters_pushdown_fn_wrapper(
+ provider: &FFI_TableProvider,
+ filters_serialized: RVec<u8>,
+) -> RResult<RVec<FFI_TableProviderFilterPushDown>, RString> {
+ let private_data = provider.private_data as *const ProviderPrivateData;
+ let provider = &(*private_data).provider;
+
+ supports_filters_pushdown_internal(provider, &filters_serialized)
+ .map_err(|e| e.to_string().into())
+ .into()
+}
+
+unsafe extern "C" fn scan_fn_wrapper(
+ provider: &FFI_TableProvider,
+ session_config: &FFI_SessionConfig,
+ projections: RVec<usize>,
+ filters_serialized: RVec<u8>,
+ limit: ROption<usize>,
+) -> FfiFuture<RResult<FFI_ExecutionPlan, RString>> {
+ let private_data = provider.private_data as *mut ProviderPrivateData;
+ let internal_provider = &(*private_data).provider;
+ let session_config = session_config.clone();
+
+ async move {
+ let config = match ForeignSessionConfig::new(&session_config) {
+ Ok(c) => c,
+ Err(e) => return RResult::RErr(e.to_string().into()),
+ };
+ let session = SessionStateBuilder::new()
+ .with_default_features()
+ .with_config(config.0)
+ .build();
+ let ctx = SessionContext::new_with_state(session);
+
+ let filters = match filters_serialized.is_empty() {
+ true => vec![],
+ false => {
+ let default_ctx = SessionContext::new();
+ let codec = DefaultLogicalExtensionCodec {};
+
+ let proto_filters =
+ match LogicalExprList::decode(filters_serialized.as_ref()) {
+ Ok(f) => f,
+ Err(e) => return RResult::RErr(e.to_string().into()),
+ };
+
+ match parse_exprs(proto_filters.expr.iter(), &default_ctx, &codec) {
+ Ok(f) => f,
+ Err(e) => return RResult::RErr(e.to_string().into()),
+ }
+ }
+ };
+
+ let projections: Vec<_> = projections.into_iter().collect();
+ let maybe_projections = match projections.is_empty() {
+ true => None,
+ false => Some(&projections),
+ };
+
+ let plan = match internal_provider
+ .scan(&ctx.state(), maybe_projections, &filters, limit.into())
+ .await
+ {
+ Ok(p) => p,
+ Err(e) => return RResult::RErr(e.to_string().into()),
+ };
+
+ FFI_ExecutionPlan::new(plan, ctx.task_ctx())
+ .map_err(|e| e.to_string().into())
+ .into()
+ }
+ .into_ffi()
+}
+
+unsafe extern "C" fn release_fn_wrapper(provider: &mut FFI_TableProvider) {
+ let private_data = Box::from_raw(provider.private_data as *mut ProviderPrivateData);
+ drop(private_data);
+}
+
+unsafe extern "C" fn clone_fn_wrapper(provider: &FFI_TableProvider) -> FFI_TableProvider {
+ let old_private_data = provider.private_data as *const ProviderPrivateData;
+
+ let private_data = Box::into_raw(Box::new(ProviderPrivateData {
+ provider: Arc::clone(&(*old_private_data).provider),
+ })) as *mut c_void;
+
+ FFI_TableProvider {
+ schema: schema_fn_wrapper,
+ scan: scan_fn_wrapper,
+ table_type: table_type_fn_wrapper,
+ supports_filters_pushdown: supports_filters_pushdown_fn_wrapper,
+ clone: clone_fn_wrapper,
+ release: release_fn_wrapper,
+ private_data,
+ }
+}
+
+impl Drop for FFI_TableProvider {
+ fn drop(&mut self) {
+ unsafe { (self.release)(self) }
+ }
+}
+
+impl FFI_TableProvider {
+ /// Creates a new [`FFI_TableProvider`].
+ pub fn new(provider: Arc<dyn TableProvider + Send>) -> Self {
+ let private_data = Box::new(ProviderPrivateData { provider });
+
+ Self {
+ schema: schema_fn_wrapper,
+ scan: scan_fn_wrapper,
+ table_type: table_type_fn_wrapper,
+ supports_filters_pushdown: supports_filters_pushdown_fn_wrapper,
+ clone: clone_fn_wrapper,
+ release: release_fn_wrapper,
+ private_data: Box::into_raw(private_data) as *mut c_void,
+ }
+ }
+}
+
+/// This wrapper struct exists on the reciever side of the FFI interface, so it has
+/// no guarantees about being able to access the data in `private_data`. Any functions
+/// defined on this struct must only use the stable functions provided in
+/// FFI_TableProvider to interact with the foreign table provider.
+#[derive(Debug)]
+pub struct ForeignTableProvider(FFI_TableProvider);
+
+unsafe impl Send for ForeignTableProvider {}
+unsafe impl Sync for ForeignTableProvider {}
+
+impl ForeignTableProvider {
+ pub fn new(provider: &FFI_TableProvider) -> Self {
+ Self(provider.clone())
+ }
+}
+
+impl Clone for FFI_TableProvider {
+ fn clone(&self) -> Self {
+ unsafe { (self.clone)(self) }
+ }
+}
+
+#[async_trait]
+impl TableProvider for ForeignTableProvider {
+ fn as_any(&self) -> &dyn Any {
+ self
+ }
+
+ fn schema(&self) -> SchemaRef {
+ let schema = unsafe {
+ let wrapped_schema = (self.0.schema)(&self.0);
+ Schema::try_from(&wrapped_schema.0).unwrap_or(Schema::empty())
+ };
+ Arc::new(schema)
+ }
+
+ fn table_type(&self) -> TableType {
+ unsafe { (self.0.table_type)(&self.0).into() }
+ }
+
+ async fn scan(
+ &self,
+ session: &dyn Session,
+ projection: Option<&Vec<usize>>,
+ filters: &[Expr],
+ limit: Option<usize>,
+ ) -> Result<Arc<dyn ExecutionPlan>> {
+ let session_config = FFI_SessionConfig::new(session.config());
+
+ let projections: Option<RVec<usize>> =
+ projection.map(|p| p.iter().map(|v| v.to_owned()).collect());
+
+ let codec = DefaultLogicalExtensionCodec {};
+ let filter_list = LogicalExprList {
+ expr: serialize_exprs(filters, &codec)?,
+ };
+ let filters_serialized = filter_list.encode_to_vec().into();
+
+ let plan = unsafe {
+ let maybe_plan = (self.0.scan)(
+ &self.0,
+ &session_config,
+ projections.unwrap_or_default(),
+ filters_serialized,
+ limit.into(),
+ )
+ .await;
+
+ match maybe_plan {
+ RResult::ROk(p) => ForeignExecutionPlan::new(p)?,
+ RResult::RErr(_) => {
+ return Err(datafusion::error::DataFusionError::Internal(
+ "Unable to perform scan via FFI".to_string(),
+ ))
+ }
+ }
+ };
+
+ Ok(Arc::new(plan))
+ }
+
+ /// Tests whether the table provider can make use of a filter expression
+ /// to optimise data retrieval.
+ fn supports_filters_pushdown(
+ &self,
+ filter: &[&Expr],
+ ) -> Result<Vec<TableProviderFilterPushDown>> {
+ unsafe {
+ let codec = DefaultLogicalExtensionCodec {};
+
+ let expr_list = LogicalExprList {
+ expr: serialize_exprs(filter.iter().map(|f| f.to_owned()), &codec)?,
+ };
+ let serialized_filters = expr_list.encode_to_vec();
+
+ let pushdown_fn = self.0.supports_filters_pushdown;
+
+ let pushdowns = pushdown_fn(&self.0, serialized_filters.into());
+
+ match pushdowns {
+ RResult::ROk(p) => Ok(p.iter().map(|v| v.into()).collect()),
+ RResult::RErr(e) => Err(DataFusionError::Plan(e.to_string())),
+ }
Review Comment:
While supporting this is definitely a good idea I think that implementing `supports_filters_pushdown` should be made optional for providers which do not support any push down of filters in order to prevent a potentially expensive (and wasteful) rendering of the filters as protobuf.
##########
datafusion/ffi/src/execution_plan.rs:
##########
@@ -0,0 +1,373 @@
+// 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.
+
+use std::{ffi::c_void, pin::Pin, sync::Arc};
+
+use abi_stable::{
+ std_types::{RResult, RStr, RString, RVec},
+ StableAbi,
+};
+use arrow::ffi_stream::FFI_ArrowArrayStream;
+use datafusion::error::Result;
+use datafusion::{
+ error::DataFusionError,
+ execution::{SendableRecordBatchStream, TaskContext},
+ physical_plan::{DisplayAs, ExecutionPlan, PlanProperties},
+};
+
+use crate::{
+ plan_properties::{FFI_PlanProperties, ForeignPlanProperties},
+ record_batch_stream::FFI_RecordBatchStream,
+};
+
+#[repr(C)]
+#[derive(Debug, StableAbi)]
+pub struct WrappedArrayStream(#[sabi(unsafe_opaque_field)] pub FFI_ArrowArrayStream);
+
+#[repr(C)]
+#[derive(Debug, StableAbi)]
+#[allow(missing_docs)]
+#[allow(non_camel_case_types)]
+pub struct FFI_ExecutionPlan {
Review Comment:
I really like the fact that this change follows the same setup as arrow-rs's FFI. It makes it familiar to use.
##########
datafusion/ffi/src/record_batch_stream.rs:
##########
@@ -0,0 +1,190 @@
+// 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.
+
+use std::{ffi::c_void, sync::Arc, task::Poll};
+
+use abi_stable::{
+ std_types::{ROption, RResult, RString},
+ StableAbi,
+};
+use arrow::array::{Array, RecordBatch};
+use arrow::{
+ array::{make_array, StructArray},
+ datatypes::{Schema, SchemaRef},
+ ffi::{from_ffi, to_ffi, FFI_ArrowArray, FFI_ArrowSchema},
+};
+use async_ffi::{ContextExt, FfiContext, FfiPoll};
+use datafusion::error::Result;
+use datafusion::{
+ error::DataFusionError,
+ execution::{RecordBatchStream, SendableRecordBatchStream},
+};
+use futures::{Stream, TryStreamExt};
+
+#[repr(C)]
+#[derive(Debug, StableAbi)]
+pub struct WrappedArray {
+ #[sabi(unsafe_opaque_field)]
+ array: FFI_ArrowArray,
+
+ schema: WrappedSchema,
+}
+
+#[repr(C)]
+#[derive(Debug, StableAbi)]
+pub struct WrappedSchema(#[sabi(unsafe_opaque_field)] FFI_ArrowSchema);
+
+impl From<SchemaRef> for WrappedSchema {
+ fn from(value: SchemaRef) -> Self {
+ let schema = FFI_ArrowSchema::try_from(value.as_ref());
+ WrappedSchema(schema.unwrap_or(FFI_ArrowSchema::empty()))
Review Comment:
I don't really like that this silently fails but it seems like the only error it could realistically encounter is https://github.com/apache/arrow-rs/blob/1666a4d7b237370c167e2b90b45d1354e28f500a/arrow-schema/src/ffi.rs#L745-L747. This happens in other places in the code as well.
##########
datafusion/ffi/Cargo.toml:
##########
@@ -0,0 +1,48 @@
+# 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.
+
+[package]
+name = "datafusion-ffi"
+description = "Foreign Function Interface implementation for DataFusion"
+readme = "README.md"
+version = { workspace = true }
+edition = { workspace = true }
+homepage = { workspace = true }
+repository = { workspace = true }
+license = { workspace = true }
+authors = { workspace = true }
+# Specify MSRV here as `cargo msrv` doesn't support workspace version
+rust-version = "1.76"
+
+[lints]
+workspace = true
+
+[lib]
+name = "datafusion_ffi"
+path = "src/lib.rs"
+
+[dependencies]
+abi_stable = "0.11.3"
+arrow = { workspace = true, features = ["ffi"] }
+async-ffi = { version = "0.5.0", features = ["abi_stable"] }
+async-trait = { workspace = true }
+datafusion = { workspace = true, default-features = true }
Review Comment:
Given the we are targeting FFI, I think not all features should be enabled by default when working the `datafusion-ffi` crate. Some of DataFusion's feature bring in some external native dependency (e.g. crypto functions) and they may conflict with the native setup of some binding implementations.
Instead I think that we should probably leave the choice of features to the user implementing the bindings and leave just the minimum set required for this crate to work.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] FFI initial implementation [datafusion]
Posted by "notfilippo (via GitHub)" <gi...@apache.org>.
notfilippo commented on code in PR #12920:
URL: https://github.com/apache/datafusion/pull/12920#discussion_r1807795673
##########
datafusion/ffi/src/table_provider.rs:
##########
@@ -0,0 +1,416 @@
+// 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.
+
+use std::{any::Any, ffi::c_void, sync::Arc};
+
+use abi_stable::{
+ std_types::{ROption, RResult, RString, RVec},
+ StableAbi,
+};
+use arrow::{
+ datatypes::{Schema, SchemaRef},
+ ffi::FFI_ArrowSchema,
+};
+use async_ffi::{FfiFuture, FutureExt};
+use async_trait::async_trait;
+use datafusion::{
+ catalog::{Session, TableProvider},
+ datasource::TableType,
+ error::DataFusionError,
+ execution::session_state::SessionStateBuilder,
+ logical_expr::TableProviderFilterPushDown,
+ physical_plan::ExecutionPlan,
+ prelude::{Expr, SessionContext},
+};
+use datafusion_proto::{
+ logical_plan::{
+ from_proto::parse_exprs, to_proto::serialize_exprs, DefaultLogicalExtensionCodec,
+ },
+ protobuf::LogicalExprList,
+};
+use prost::Message;
+
+use crate::{
+ plan_properties::WrappedSchema,
+ session_config::ForeignSessionConfig,
+ table_source::{FFI_TableProviderFilterPushDown, FFI_TableType},
+};
+
+use super::{
+ execution_plan::{FFI_ExecutionPlan, ForeignExecutionPlan},
+ session_config::FFI_SessionConfig,
+};
+use datafusion::error::Result;
+
+/// A stable interface for creating a DataFusion TableProvider.
+#[repr(C)]
+#[derive(Debug, StableAbi)]
+#[allow(non_camel_case_types)]
+pub struct FFI_TableProvider {
+ pub schema: unsafe extern "C" fn(provider: &Self) -> WrappedSchema,
+ pub scan: unsafe extern "C" fn(
+ provider: &Self,
+ session_config: &FFI_SessionConfig,
+ projections: RVec<usize>,
+ filters_serialized: RVec<u8>,
+ limit: ROption<usize>,
+ ) -> FfiFuture<RResult<FFI_ExecutionPlan, RString>>,
+
+ pub table_type: unsafe extern "C" fn(provider: &Self) -> FFI_TableType,
+
+ pub supports_filters_pushdown:
+ unsafe extern "C" fn(
+ provider: &Self,
+ filters_serialized: RVec<u8>,
+ )
+ -> RResult<RVec<FFI_TableProviderFilterPushDown>, RString>,
+
+ pub clone: unsafe extern "C" fn(provider: &Self) -> Self,
+ pub release: unsafe extern "C" fn(arg: &mut Self),
+ pub private_data: *mut c_void,
+}
+
+unsafe impl Send for FFI_TableProvider {}
+unsafe impl Sync for FFI_TableProvider {}
+
+struct ProviderPrivateData {
+ provider: Arc<dyn TableProvider + Send>,
+}
+
+unsafe extern "C" fn schema_fn_wrapper(provider: &FFI_TableProvider) -> WrappedSchema {
+ let private_data = provider.private_data as *const ProviderPrivateData;
+ let provider = &(*private_data).provider;
+
+ // This does silently fail because TableProvider does not return a result.
+ // It expects schema to always pass.
+ let ffi_schema = FFI_ArrowSchema::try_from(provider.schema().as_ref())
+ .unwrap_or(FFI_ArrowSchema::empty());
+
+ WrappedSchema(ffi_schema)
+}
+
+unsafe extern "C" fn table_type_fn_wrapper(
+ provider: &FFI_TableProvider,
+) -> FFI_TableType {
+ let private_data = provider.private_data as *const ProviderPrivateData;
+ let provider = &(*private_data).provider;
+
+ provider.table_type().into()
+}
+
+fn supports_filters_pushdown_internal(
+ provider: &Arc<dyn TableProvider + Send>,
+ filters_serialized: &[u8],
+) -> Result<RVec<FFI_TableProviderFilterPushDown>> {
+ let default_ctx = SessionContext::new();
+ let codec = DefaultLogicalExtensionCodec {};
+
+ let filters = match filters_serialized.is_empty() {
+ true => vec![],
+ false => {
+ let proto_filters = LogicalExprList::decode(filters_serialized)
+ .map_err(|e| DataFusionError::Plan(e.to_string()))?;
+
+ parse_exprs(proto_filters.expr.iter(), &default_ctx, &codec)?
+ }
+ };
+ let filters_borrowed: Vec<&Expr> = filters.iter().collect();
+
+ let results: RVec<_> = provider
+ .supports_filters_pushdown(&filters_borrowed)?
+ .iter()
+ .map(|v| v.into())
+ .collect();
+
+ Ok(results)
+}
+
+unsafe extern "C" fn supports_filters_pushdown_fn_wrapper(
+ provider: &FFI_TableProvider,
+ filters_serialized: RVec<u8>,
+) -> RResult<RVec<FFI_TableProviderFilterPushDown>, RString> {
+ let private_data = provider.private_data as *const ProviderPrivateData;
+ let provider = &(*private_data).provider;
+
+ supports_filters_pushdown_internal(provider, &filters_serialized)
+ .map_err(|e| e.to_string().into())
+ .into()
+}
+
+unsafe extern "C" fn scan_fn_wrapper(
+ provider: &FFI_TableProvider,
+ session_config: &FFI_SessionConfig,
+ projections: RVec<usize>,
+ filters_serialized: RVec<u8>,
+ limit: ROption<usize>,
+) -> FfiFuture<RResult<FFI_ExecutionPlan, RString>> {
+ let private_data = provider.private_data as *mut ProviderPrivateData;
+ let internal_provider = &(*private_data).provider;
+ let session_config = session_config.clone();
+
+ async move {
+ let config = match ForeignSessionConfig::new(&session_config) {
+ Ok(c) => c,
+ Err(e) => return RResult::RErr(e.to_string().into()),
+ };
+ let session = SessionStateBuilder::new()
+ .with_default_features()
+ .with_config(config.0)
+ .build();
+ let ctx = SessionContext::new_with_state(session);
+
+ let filters = match filters_serialized.is_empty() {
+ true => vec![],
+ false => {
+ let default_ctx = SessionContext::new();
+ let codec = DefaultLogicalExtensionCodec {};
+
+ let proto_filters =
+ match LogicalExprList::decode(filters_serialized.as_ref()) {
+ Ok(f) => f,
+ Err(e) => return RResult::RErr(e.to_string().into()),
+ };
+
+ match parse_exprs(proto_filters.expr.iter(), &default_ctx, &codec) {
+ Ok(f) => f,
+ Err(e) => return RResult::RErr(e.to_string().into()),
+ }
+ }
+ };
+
+ let projections: Vec<_> = projections.into_iter().collect();
+ let maybe_projections = match projections.is_empty() {
+ true => None,
+ false => Some(&projections),
+ };
+
+ let plan = match internal_provider
+ .scan(&ctx.state(), maybe_projections, &filters, limit.into())
+ .await
+ {
+ Ok(p) => p,
+ Err(e) => return RResult::RErr(e.to_string().into()),
+ };
+
+ FFI_ExecutionPlan::new(plan, ctx.task_ctx())
+ .map_err(|e| e.to_string().into())
+ .into()
+ }
+ .into_ffi()
+}
+
+unsafe extern "C" fn release_fn_wrapper(provider: &mut FFI_TableProvider) {
+ let private_data = Box::from_raw(provider.private_data as *mut ProviderPrivateData);
+ drop(private_data);
+}
+
+unsafe extern "C" fn clone_fn_wrapper(provider: &FFI_TableProvider) -> FFI_TableProvider {
+ let old_private_data = provider.private_data as *const ProviderPrivateData;
+
+ let private_data = Box::into_raw(Box::new(ProviderPrivateData {
+ provider: Arc::clone(&(*old_private_data).provider),
+ })) as *mut c_void;
+
+ FFI_TableProvider {
+ schema: schema_fn_wrapper,
+ scan: scan_fn_wrapper,
+ table_type: table_type_fn_wrapper,
+ supports_filters_pushdown: supports_filters_pushdown_fn_wrapper,
+ clone: clone_fn_wrapper,
+ release: release_fn_wrapper,
+ private_data,
+ }
+}
+
+impl Drop for FFI_TableProvider {
+ fn drop(&mut self) {
+ unsafe { (self.release)(self) }
+ }
+}
+
+impl FFI_TableProvider {
+ /// Creates a new [`FFI_TableProvider`].
+ pub fn new(provider: Arc<dyn TableProvider + Send>) -> Self {
+ let private_data = Box::new(ProviderPrivateData { provider });
+
+ Self {
+ schema: schema_fn_wrapper,
+ scan: scan_fn_wrapper,
+ table_type: table_type_fn_wrapper,
+ supports_filters_pushdown: supports_filters_pushdown_fn_wrapper,
+ clone: clone_fn_wrapper,
+ release: release_fn_wrapper,
+ private_data: Box::into_raw(private_data) as *mut c_void,
+ }
+ }
+}
+
+/// This wrapper struct exists on the reciever side of the FFI interface, so it has
+/// no guarantees about being able to access the data in `private_data`. Any functions
+/// defined on this struct must only use the stable functions provided in
+/// FFI_TableProvider to interact with the foreign table provider.
+#[derive(Debug)]
+pub struct ForeignTableProvider(FFI_TableProvider);
+
+unsafe impl Send for ForeignTableProvider {}
+unsafe impl Sync for ForeignTableProvider {}
+
+impl ForeignTableProvider {
+ pub fn new(provider: &FFI_TableProvider) -> Self {
+ Self(provider.clone())
+ }
+}
+
+impl Clone for FFI_TableProvider {
+ fn clone(&self) -> Self {
+ unsafe { (self.clone)(self) }
+ }
+}
+
+#[async_trait]
+impl TableProvider for ForeignTableProvider {
+ fn as_any(&self) -> &dyn Any {
+ self
+ }
+
+ fn schema(&self) -> SchemaRef {
+ let schema = unsafe {
+ let wrapped_schema = (self.0.schema)(&self.0);
+ Schema::try_from(&wrapped_schema.0).unwrap_or(Schema::empty())
+ };
+ Arc::new(schema)
+ }
+
+ fn table_type(&self) -> TableType {
+ unsafe { (self.0.table_type)(&self.0).into() }
+ }
+
+ async fn scan(
+ &self,
+ session: &dyn Session,
+ projection: Option<&Vec<usize>>,
+ filters: &[Expr],
+ limit: Option<usize>,
+ ) -> Result<Arc<dyn ExecutionPlan>> {
+ let session_config = FFI_SessionConfig::new(session.config());
+
+ let projections: Option<RVec<usize>> =
+ projection.map(|p| p.iter().map(|v| v.to_owned()).collect());
+
+ let codec = DefaultLogicalExtensionCodec {};
+ let filter_list = LogicalExprList {
+ expr: serialize_exprs(filters, &codec)?,
+ };
+ let filters_serialized = filter_list.encode_to_vec().into();
+
+ let plan = unsafe {
+ let maybe_plan = (self.0.scan)(
+ &self.0,
+ &session_config,
+ projections.unwrap_or_default(),
+ filters_serialized,
+ limit.into(),
+ )
+ .await;
+
+ match maybe_plan {
+ RResult::ROk(p) => ForeignExecutionPlan::new(p)?,
+ RResult::RErr(_) => {
+ return Err(datafusion::error::DataFusionError::Internal(
+ "Unable to perform scan via FFI".to_string(),
+ ))
+ }
+ }
+ };
+
+ Ok(Arc::new(plan))
+ }
+
+ /// Tests whether the table provider can make use of a filter expression
+ /// to optimise data retrieval.
+ fn supports_filters_pushdown(
+ &self,
+ filter: &[&Expr],
+ ) -> Result<Vec<TableProviderFilterPushDown>> {
+ unsafe {
+ let codec = DefaultLogicalExtensionCodec {};
+
+ let expr_list = LogicalExprList {
+ expr: serialize_exprs(filter.iter().map(|f| f.to_owned()), &codec)?,
+ };
+ let serialized_filters = expr_list.encode_to_vec();
+
+ let pushdown_fn = self.0.supports_filters_pushdown;
+
+ let pushdowns = pushdown_fn(&self.0, serialized_filters.into());
+
+ match pushdowns {
+ RResult::ROk(p) => Ok(p.iter().map(|v| v.into()).collect()),
+ RResult::RErr(e) => Err(DataFusionError::Plan(e.to_string())),
+ }
Review Comment:
While supporting this is definitely a good idea I think that implementing `supports_filters_pushdown` should be made optional for providers which do not support any push down of filters, in order to prevent a potentially expensive (and wasteful) rendering of the filters as protobuf.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] FFI initial implementation [datafusion]
Posted by "notfilippo (via GitHub)" <gi...@apache.org>.
notfilippo commented on code in PR #12920:
URL: https://github.com/apache/datafusion/pull/12920#discussion_r1807786121
##########
datafusion/ffi/src/record_batch_stream.rs:
##########
@@ -0,0 +1,190 @@
+// 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.
+
+use std::{ffi::c_void, sync::Arc, task::Poll};
+
+use abi_stable::{
+ std_types::{ROption, RResult, RString},
+ StableAbi,
+};
+use arrow::array::{Array, RecordBatch};
+use arrow::{
+ array::{make_array, StructArray},
+ datatypes::{Schema, SchemaRef},
+ ffi::{from_ffi, to_ffi, FFI_ArrowArray, FFI_ArrowSchema},
+};
+use async_ffi::{ContextExt, FfiContext, FfiPoll};
+use datafusion::error::Result;
+use datafusion::{
+ error::DataFusionError,
+ execution::{RecordBatchStream, SendableRecordBatchStream},
+};
+use futures::{Stream, TryStreamExt};
+
+#[repr(C)]
+#[derive(Debug, StableAbi)]
+pub struct WrappedArray {
+ #[sabi(unsafe_opaque_field)]
+ array: FFI_ArrowArray,
+
+ schema: WrappedSchema,
+}
+
+#[repr(C)]
+#[derive(Debug, StableAbi)]
+pub struct WrappedSchema(#[sabi(unsafe_opaque_field)] FFI_ArrowSchema);
+
+impl From<SchemaRef> for WrappedSchema {
+ fn from(value: SchemaRef) -> Self {
+ let schema = FFI_ArrowSchema::try_from(value.as_ref());
+ WrappedSchema(schema.unwrap_or(FFI_ArrowSchema::empty()))
Review Comment:
I don't really like that this silently fails but it seems like the only error it could realistically encounter is https://github.com/apache/arrow-rs/blob/1666a4d7b237370c167e2b90b45d1354e28f500a/arrow-schema/src/ffi.rs#L745-L747.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] FFI initial implementation [datafusion]
Posted by "timsaucer (via GitHub)" <gi...@apache.org>.
timsaucer commented on code in PR #12920:
URL: https://github.com/apache/datafusion/pull/12920#discussion_r1809343800
##########
datafusion/ffi/src/record_batch_stream.rs:
##########
@@ -0,0 +1,190 @@
+// 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.
+
+use std::{ffi::c_void, sync::Arc, task::Poll};
+
+use abi_stable::{
+ std_types::{ROption, RResult, RString},
+ StableAbi,
+};
+use arrow::array::{Array, RecordBatch};
+use arrow::{
+ array::{make_array, StructArray},
+ datatypes::{Schema, SchemaRef},
+ ffi::{from_ffi, to_ffi, FFI_ArrowArray, FFI_ArrowSchema},
+};
+use async_ffi::{ContextExt, FfiContext, FfiPoll};
+use datafusion::error::Result;
+use datafusion::{
+ error::DataFusionError,
+ execution::{RecordBatchStream, SendableRecordBatchStream},
+};
+use futures::{Stream, TryStreamExt};
+
+#[repr(C)]
+#[derive(Debug, StableAbi)]
+pub struct WrappedArray {
+ #[sabi(unsafe_opaque_field)]
+ array: FFI_ArrowArray,
+
+ schema: WrappedSchema,
+}
+
+#[repr(C)]
+#[derive(Debug, StableAbi)]
+pub struct WrappedSchema(#[sabi(unsafe_opaque_field)] FFI_ArrowSchema);
+
+impl From<SchemaRef> for WrappedSchema {
+ fn from(value: SchemaRef) -> Self {
+ let schema = FFI_ArrowSchema::try_from(value.as_ref());
+ WrappedSchema(schema.unwrap_or(FFI_ArrowSchema::empty()))
Review Comment:
I completely agree and I had planned to go back and address this, so thank you for bringing it up. I just pushed a commit that generates an error in the log.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] FFI initial implementation [datafusion]
Posted by "notfilippo (via GitHub)" <gi...@apache.org>.
notfilippo commented on code in PR #12920:
URL: https://github.com/apache/datafusion/pull/12920#discussion_r1807786121
##########
datafusion/ffi/src/record_batch_stream.rs:
##########
@@ -0,0 +1,190 @@
+// 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.
+
+use std::{ffi::c_void, sync::Arc, task::Poll};
+
+use abi_stable::{
+ std_types::{ROption, RResult, RString},
+ StableAbi,
+};
+use arrow::array::{Array, RecordBatch};
+use arrow::{
+ array::{make_array, StructArray},
+ datatypes::{Schema, SchemaRef},
+ ffi::{from_ffi, to_ffi, FFI_ArrowArray, FFI_ArrowSchema},
+};
+use async_ffi::{ContextExt, FfiContext, FfiPoll};
+use datafusion::error::Result;
+use datafusion::{
+ error::DataFusionError,
+ execution::{RecordBatchStream, SendableRecordBatchStream},
+};
+use futures::{Stream, TryStreamExt};
+
+#[repr(C)]
+#[derive(Debug, StableAbi)]
+pub struct WrappedArray {
+ #[sabi(unsafe_opaque_field)]
+ array: FFI_ArrowArray,
+
+ schema: WrappedSchema,
+}
+
+#[repr(C)]
+#[derive(Debug, StableAbi)]
+pub struct WrappedSchema(#[sabi(unsafe_opaque_field)] FFI_ArrowSchema);
+
+impl From<SchemaRef> for WrappedSchema {
+ fn from(value: SchemaRef) -> Self {
+ let schema = FFI_ArrowSchema::try_from(value.as_ref());
+ WrappedSchema(schema.unwrap_or(FFI_ArrowSchema::empty()))
Review Comment:
I don't really like that this silently fails but it seems like the only error it could realistically encounter is https://github.com/apache/arrow-rs/blob/1666a4d7b237370c167e2b90b45d1354e28f500a/arrow-schema/src/ffi.rs#L745-L747.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] FFI initial implementation [datafusion]
Posted by "timsaucer (via GitHub)" <gi...@apache.org>.
timsaucer commented on code in PR #12920:
URL: https://github.com/apache/datafusion/pull/12920#discussion_r1809250118
##########
datafusion/ffi/src/table_provider.rs:
##########
@@ -0,0 +1,416 @@
+// 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.
+
+use std::{any::Any, ffi::c_void, sync::Arc};
+
+use abi_stable::{
+ std_types::{ROption, RResult, RString, RVec},
+ StableAbi,
+};
+use arrow::{
+ datatypes::{Schema, SchemaRef},
+ ffi::FFI_ArrowSchema,
+};
+use async_ffi::{FfiFuture, FutureExt};
+use async_trait::async_trait;
+use datafusion::{
+ catalog::{Session, TableProvider},
+ datasource::TableType,
+ error::DataFusionError,
+ execution::session_state::SessionStateBuilder,
+ logical_expr::TableProviderFilterPushDown,
+ physical_plan::ExecutionPlan,
+ prelude::{Expr, SessionContext},
+};
+use datafusion_proto::{
+ logical_plan::{
+ from_proto::parse_exprs, to_proto::serialize_exprs, DefaultLogicalExtensionCodec,
+ },
+ protobuf::LogicalExprList,
+};
+use prost::Message;
+
+use crate::{
+ plan_properties::WrappedSchema,
+ session_config::ForeignSessionConfig,
+ table_source::{FFI_TableProviderFilterPushDown, FFI_TableType},
+};
+
+use super::{
+ execution_plan::{FFI_ExecutionPlan, ForeignExecutionPlan},
+ session_config::FFI_SessionConfig,
+};
+use datafusion::error::Result;
+
+/// A stable interface for creating a DataFusion TableProvider.
+#[repr(C)]
+#[derive(Debug, StableAbi)]
+#[allow(non_camel_case_types)]
+pub struct FFI_TableProvider {
+ pub schema: unsafe extern "C" fn(provider: &Self) -> WrappedSchema,
+ pub scan: unsafe extern "C" fn(
+ provider: &Self,
+ session_config: &FFI_SessionConfig,
+ projections: RVec<usize>,
+ filters_serialized: RVec<u8>,
+ limit: ROption<usize>,
+ ) -> FfiFuture<RResult<FFI_ExecutionPlan, RString>>,
+
+ pub table_type: unsafe extern "C" fn(provider: &Self) -> FFI_TableType,
+
+ pub supports_filters_pushdown:
+ unsafe extern "C" fn(
+ provider: &Self,
+ filters_serialized: RVec<u8>,
+ )
+ -> RResult<RVec<FFI_TableProviderFilterPushDown>, RString>,
+
+ pub clone: unsafe extern "C" fn(provider: &Self) -> Self,
+ pub release: unsafe extern "C" fn(arg: &mut Self),
+ pub private_data: *mut c_void,
+}
+
+unsafe impl Send for FFI_TableProvider {}
+unsafe impl Sync for FFI_TableProvider {}
+
+struct ProviderPrivateData {
+ provider: Arc<dyn TableProvider + Send>,
+}
+
+unsafe extern "C" fn schema_fn_wrapper(provider: &FFI_TableProvider) -> WrappedSchema {
+ let private_data = provider.private_data as *const ProviderPrivateData;
+ let provider = &(*private_data).provider;
+
+ // This does silently fail because TableProvider does not return a result.
+ // It expects schema to always pass.
+ let ffi_schema = FFI_ArrowSchema::try_from(provider.schema().as_ref())
+ .unwrap_or(FFI_ArrowSchema::empty());
+
+ WrappedSchema(ffi_schema)
+}
+
+unsafe extern "C" fn table_type_fn_wrapper(
+ provider: &FFI_TableProvider,
+) -> FFI_TableType {
+ let private_data = provider.private_data as *const ProviderPrivateData;
+ let provider = &(*private_data).provider;
+
+ provider.table_type().into()
+}
+
+fn supports_filters_pushdown_internal(
+ provider: &Arc<dyn TableProvider + Send>,
+ filters_serialized: &[u8],
+) -> Result<RVec<FFI_TableProviderFilterPushDown>> {
+ let default_ctx = SessionContext::new();
+ let codec = DefaultLogicalExtensionCodec {};
+
+ let filters = match filters_serialized.is_empty() {
+ true => vec![],
+ false => {
+ let proto_filters = LogicalExprList::decode(filters_serialized)
+ .map_err(|e| DataFusionError::Plan(e.to_string()))?;
+
+ parse_exprs(proto_filters.expr.iter(), &default_ctx, &codec)?
+ }
+ };
+ let filters_borrowed: Vec<&Expr> = filters.iter().collect();
+
+ let results: RVec<_> = provider
+ .supports_filters_pushdown(&filters_borrowed)?
+ .iter()
+ .map(|v| v.into())
+ .collect();
+
+ Ok(results)
+}
+
+unsafe extern "C" fn supports_filters_pushdown_fn_wrapper(
+ provider: &FFI_TableProvider,
+ filters_serialized: RVec<u8>,
+) -> RResult<RVec<FFI_TableProviderFilterPushDown>, RString> {
+ let private_data = provider.private_data as *const ProviderPrivateData;
+ let provider = &(*private_data).provider;
+
+ supports_filters_pushdown_internal(provider, &filters_serialized)
+ .map_err(|e| e.to_string().into())
+ .into()
+}
+
+unsafe extern "C" fn scan_fn_wrapper(
+ provider: &FFI_TableProvider,
+ session_config: &FFI_SessionConfig,
+ projections: RVec<usize>,
+ filters_serialized: RVec<u8>,
+ limit: ROption<usize>,
+) -> FfiFuture<RResult<FFI_ExecutionPlan, RString>> {
+ let private_data = provider.private_data as *mut ProviderPrivateData;
+ let internal_provider = &(*private_data).provider;
+ let session_config = session_config.clone();
+
+ async move {
+ let config = match ForeignSessionConfig::new(&session_config) {
+ Ok(c) => c,
+ Err(e) => return RResult::RErr(e.to_string().into()),
+ };
+ let session = SessionStateBuilder::new()
+ .with_default_features()
+ .with_config(config.0)
+ .build();
+ let ctx = SessionContext::new_with_state(session);
+
+ let filters = match filters_serialized.is_empty() {
+ true => vec![],
+ false => {
+ let default_ctx = SessionContext::new();
+ let codec = DefaultLogicalExtensionCodec {};
+
+ let proto_filters =
+ match LogicalExprList::decode(filters_serialized.as_ref()) {
+ Ok(f) => f,
+ Err(e) => return RResult::RErr(e.to_string().into()),
+ };
+
+ match parse_exprs(proto_filters.expr.iter(), &default_ctx, &codec) {
+ Ok(f) => f,
+ Err(e) => return RResult::RErr(e.to_string().into()),
+ }
+ }
+ };
+
+ let projections: Vec<_> = projections.into_iter().collect();
+ let maybe_projections = match projections.is_empty() {
+ true => None,
+ false => Some(&projections),
+ };
+
+ let plan = match internal_provider
+ .scan(&ctx.state(), maybe_projections, &filters, limit.into())
+ .await
+ {
+ Ok(p) => p,
+ Err(e) => return RResult::RErr(e.to_string().into()),
+ };
+
+ FFI_ExecutionPlan::new(plan, ctx.task_ctx())
+ .map_err(|e| e.to_string().into())
+ .into()
+ }
+ .into_ffi()
+}
+
+unsafe extern "C" fn release_fn_wrapper(provider: &mut FFI_TableProvider) {
+ let private_data = Box::from_raw(provider.private_data as *mut ProviderPrivateData);
+ drop(private_data);
+}
+
+unsafe extern "C" fn clone_fn_wrapper(provider: &FFI_TableProvider) -> FFI_TableProvider {
+ let old_private_data = provider.private_data as *const ProviderPrivateData;
+
+ let private_data = Box::into_raw(Box::new(ProviderPrivateData {
+ provider: Arc::clone(&(*old_private_data).provider),
+ })) as *mut c_void;
+
+ FFI_TableProvider {
+ schema: schema_fn_wrapper,
+ scan: scan_fn_wrapper,
+ table_type: table_type_fn_wrapper,
+ supports_filters_pushdown: supports_filters_pushdown_fn_wrapper,
+ clone: clone_fn_wrapper,
+ release: release_fn_wrapper,
+ private_data,
+ }
+}
+
+impl Drop for FFI_TableProvider {
+ fn drop(&mut self) {
+ unsafe { (self.release)(self) }
+ }
+}
+
+impl FFI_TableProvider {
+ /// Creates a new [`FFI_TableProvider`].
+ pub fn new(provider: Arc<dyn TableProvider + Send>) -> Self {
+ let private_data = Box::new(ProviderPrivateData { provider });
+
+ Self {
+ schema: schema_fn_wrapper,
+ scan: scan_fn_wrapper,
+ table_type: table_type_fn_wrapper,
+ supports_filters_pushdown: supports_filters_pushdown_fn_wrapper,
+ clone: clone_fn_wrapper,
+ release: release_fn_wrapper,
+ private_data: Box::into_raw(private_data) as *mut c_void,
+ }
+ }
+}
+
+/// This wrapper struct exists on the reciever side of the FFI interface, so it has
+/// no guarantees about being able to access the data in `private_data`. Any functions
+/// defined on this struct must only use the stable functions provided in
+/// FFI_TableProvider to interact with the foreign table provider.
+#[derive(Debug)]
+pub struct ForeignTableProvider(FFI_TableProvider);
+
+unsafe impl Send for ForeignTableProvider {}
+unsafe impl Sync for ForeignTableProvider {}
+
+impl ForeignTableProvider {
+ pub fn new(provider: &FFI_TableProvider) -> Self {
+ Self(provider.clone())
+ }
+}
+
+impl Clone for FFI_TableProvider {
+ fn clone(&self) -> Self {
+ unsafe { (self.clone)(self) }
+ }
+}
+
+#[async_trait]
+impl TableProvider for ForeignTableProvider {
+ fn as_any(&self) -> &dyn Any {
+ self
+ }
+
+ fn schema(&self) -> SchemaRef {
+ let schema = unsafe {
+ let wrapped_schema = (self.0.schema)(&self.0);
+ Schema::try_from(&wrapped_schema.0).unwrap_or(Schema::empty())
+ };
+ Arc::new(schema)
+ }
+
+ fn table_type(&self) -> TableType {
+ unsafe { (self.0.table_type)(&self.0).into() }
+ }
+
+ async fn scan(
+ &self,
+ session: &dyn Session,
+ projection: Option<&Vec<usize>>,
+ filters: &[Expr],
+ limit: Option<usize>,
+ ) -> Result<Arc<dyn ExecutionPlan>> {
+ let session_config = FFI_SessionConfig::new(session.config());
+
+ let projections: Option<RVec<usize>> =
+ projection.map(|p| p.iter().map(|v| v.to_owned()).collect());
+
+ let codec = DefaultLogicalExtensionCodec {};
+ let filter_list = LogicalExprList {
+ expr: serialize_exprs(filters, &codec)?,
+ };
+ let filters_serialized = filter_list.encode_to_vec().into();
+
+ let plan = unsafe {
+ let maybe_plan = (self.0.scan)(
+ &self.0,
+ &session_config,
+ projections.unwrap_or_default(),
+ filters_serialized,
+ limit.into(),
+ )
+ .await;
+
+ match maybe_plan {
+ RResult::ROk(p) => ForeignExecutionPlan::new(p)?,
+ RResult::RErr(_) => {
+ return Err(datafusion::error::DataFusionError::Internal(
+ "Unable to perform scan via FFI".to_string(),
+ ))
+ }
+ }
+ };
+
+ Ok(Arc::new(plan))
+ }
+
+ /// Tests whether the table provider can make use of a filter expression
+ /// to optimise data retrieval.
+ fn supports_filters_pushdown(
+ &self,
+ filter: &[&Expr],
+ ) -> Result<Vec<TableProviderFilterPushDown>> {
+ unsafe {
+ let codec = DefaultLogicalExtensionCodec {};
+
+ let expr_list = LogicalExprList {
+ expr: serialize_exprs(filter.iter().map(|f| f.to_owned()), &codec)?,
+ };
+ let serialized_filters = expr_list.encode_to_vec();
+
+ let pushdown_fn = self.0.supports_filters_pushdown;
+
+ let pushdowns = pushdown_fn(&self.0, serialized_filters.into());
+
+ match pushdowns {
+ RResult::ROk(p) => Ok(p.iter().map(|v| v.into()).collect()),
+ RResult::RErr(e) => Err(DataFusionError::Plan(e.to_string())),
+ }
Review Comment:
Added in recent push.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] FFI initial implementation [datafusion]
Posted by "timsaucer (via GitHub)" <gi...@apache.org>.
timsaucer commented on PR #12920:
URL: https://github.com/apache/datafusion/pull/12920#issuecomment-2418184711
In response to https://github.com/apache/datafusion/issues/12357 I believe this PR should go into the core DataFusion because it does allow for extension of core features and enabling of runtime loading of libraries. However the driving case is `datafusion-python` so I would be comfortable moving it either there or to a `datafusion-contrib` repository.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] FFI initial implementation [datafusion]
Posted by "timsaucer (via GitHub)" <gi...@apache.org>.
timsaucer commented on code in PR #12920:
URL: https://github.com/apache/datafusion/pull/12920#discussion_r1817895563
##########
datafusion/ffi/src/table_provider.rs:
##########
@@ -0,0 +1,419 @@
+// 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.
+
+use std::{any::Any, ffi::c_void, sync::Arc};
+
+use abi_stable::{
+ std_types::{ROption, RResult, RString, RVec},
+ StableAbi,
+};
+use arrow::datatypes::SchemaRef;
+use async_ffi::{FfiFuture, FutureExt};
+use async_trait::async_trait;
+use datafusion::{
+ catalog::{Session, TableProvider},
+ datasource::TableType,
+ error::DataFusionError,
+ execution::session_state::SessionStateBuilder,
+ logical_expr::TableProviderFilterPushDown,
+ physical_plan::ExecutionPlan,
+ prelude::{Expr, SessionContext},
+};
+use datafusion_proto::{
+ logical_plan::{
+ from_proto::parse_exprs, to_proto::serialize_exprs, DefaultLogicalExtensionCodec,
+ },
+ protobuf::LogicalExprList,
+};
+use prost::Message;
+
+use crate::{
+ plan_properties::WrappedSchema,
+ session_config::ForeignSessionConfig,
+ table_source::{FFI_TableProviderFilterPushDown, FFI_TableType},
+};
+
+use super::{
+ execution_plan::{FFI_ExecutionPlan, ForeignExecutionPlan},
+ session_config::FFI_SessionConfig,
+};
+use datafusion::error::Result;
+
+/// A stable interface for creating a DataFusion TableProvider.
+#[repr(C)]
+#[derive(Debug, StableAbi)]
+#[allow(non_camel_case_types)]
+pub struct FFI_TableProvider {
+ pub schema: unsafe extern "C" fn(provider: &Self) -> WrappedSchema,
+ pub scan: unsafe extern "C" fn(
+ provider: &Self,
+ session_config: &FFI_SessionConfig,
+ projections: RVec<usize>,
+ filters_serialized: RVec<u8>,
Review Comment:
I've pushed updated documentation where I describe each of the protobuf byte parameters.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] FFI initial implementation [datafusion]
Posted by "notfilippo (via GitHub)" <gi...@apache.org>.
notfilippo commented on code in PR #12920:
URL: https://github.com/apache/datafusion/pull/12920#discussion_r1809261663
##########
datafusion/ffi/Cargo.toml:
##########
@@ -0,0 +1,48 @@
+# 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.
+
+[package]
+name = "datafusion-ffi"
+description = "Foreign Function Interface implementation for DataFusion"
+readme = "README.md"
+version = { workspace = true }
+edition = { workspace = true }
+homepage = { workspace = true }
+repository = { workspace = true }
+license = { workspace = true }
+authors = { workspace = true }
+# Specify MSRV here as `cargo msrv` doesn't support workspace version
+rust-version = "1.76"
+
+[lints]
+workspace = true
+
+[lib]
+name = "datafusion_ffi"
+path = "src/lib.rs"
+
+[dependencies]
+abi_stable = "0.11.3"
+arrow = { workspace = true, features = ["ffi"] }
+async-ffi = { version = "0.5.0", features = ["abi_stable"] }
+async-trait = { workspace = true }
+datafusion = { workspace = true, default-features = true }
Review Comment:
IIUC removing `default-features = true` while in the workspace the dependency is declared as `default-features = false` (https://github.com/apache/datafusion/blob/701cb00e3b9b24b00908ad4a07ed01d68ccfa7c2/Cargo.toml#L95C1-L95C88) is the correct way of doing this 👍
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] FFI initial implementation [datafusion]
Posted by "notfilippo (via GitHub)" <gi...@apache.org>.
notfilippo commented on PR #12920:
URL: https://github.com/apache/datafusion/pull/12920#issuecomment-2439680504
After you are done I'll gladly give it another look! 🚀
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org