FFI initial implementation #12920

timsaucer · 2024-10-14T13:01:51Z

Which issue does this PR close?

This is to address part of apache/datafusion-python#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.

timsaucer · 2024-10-17T00:02:30Z

In response to #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.

alamb · 2024-10-20T10:57:33Z

I see this PR and plan to review it, hopefully today but likely tomorrow (Mon)

notfilippo

Amazing work. I've left a few comments but the idea is great and I definitely support it. I've been working on some experimental FFI bindings for Go myself and I would love to swap them for this upstream implementation at some point.

I should also mention that I have ScalarUDFs working on my barebones FFI bindings that I would love to clean up and contribute upstream when this lands.

datafusion/ffi/src/record_batch_stream.rs

datafusion/ffi/Cargo.toml

datafusion/ffi/src/table_provider.rs

datafusion/ffi/src/execution_plan.rs

alamb

I think this is a really neat idea -- thank you @timsaucer

I am not super familiar with the FFI interfaces in general, but the idea makes sense to me. I don't review this entire PR for safety -- if you could reduce the API surface area and add comments, I will do so however.

My suggestions are to:

Try and reduce the initial API surface as much as possible (e.g. do we need to expose plan properties?)
Try and improve the comments (I left some suggestions)
Mark the feature as "BETA" or something and hint that the interfaces may not be stable until we work through all the details
File a ticke to track follow on work

Some obvious follow on work in my mind would be:

Add an end to end test (perhaps in a similar vein to https://github.com/apache/datafusion/tree/main/datafusion/wasmtest) that shows thi working
Add a section to the "library users guide" about using this API: https://datafusion.apache.org/library-user-guide/index.html

alamb · 2024-10-22T16:40:52Z

datafusion/ffi/README.md

+This crate contains code to allow interoperability of Apache [DataFusion]
+with functions from other languages using a stable interface.


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

Suggested change

This crate contains code to allow interoperability of Apache [DataFusion]

with functions from other languages using a stable interface.

This crate contains code to allow interoperability of Apache [DataFusion]

with functions from other languages and/or versions using a stable interface.

alamb · 2024-10-22T16:41:29Z

datafusion/ffi/README.md

+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`


I think this text is backwards -- it is "without the necessity". Maybe we could rephrase like

Suggested change

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`

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`

alamb · 2024-10-22T16:45:27Z

datafusion/ffi/README.md

+2. Users may want to create a modular interface that allows runtime loading of
+   libraries.
+
+## Struct Layout


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

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.

alamb · 2024-10-22T17:53:46Z

datafusion/ffi/src/plan_properties.rs

+#[derive(Debug, StableAbi)]
+#[allow(missing_docs)]
+#[allow(non_camel_case_types)]
+pub struct FFI_PlanProperties {


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

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.

@alamb I've added a separate PR #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.

I will try and review #13136 later today

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 #13136 (review)

alamb · 2024-10-22T17:55:45Z

datafusion/ffi/src/table_provider.rs

+        provider: &Self,
+        session_config: &FFI_SessionConfig,
+        projections: RVec<usize>,
+        filters_serialized: RVec<u8>,


it would probably help to document what these types are (e.g. that this is a protobuf serialized bytes)

I've pushed updated documentation where I describe each of the protobuf byte parameters.

alamb · 2024-10-22T18:00:38Z

datafusion/ffi/README.md

+`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`


Maybe we should name this "DataFusion C API" or something instead of using the rust FFI term 🤔

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.

timsaucer · 2024-10-26T17:40:50Z

Thanks for the feedback! I'll try to address the remaining concerns in the next few days.

notfilippo · 2024-10-26T18:12:19Z

After you are done I'll gladly give it another look! 🚀

alamb

Thank you @timsaucer -- I think this is a great first step towards the FFI interface . I do think it will be more valuable with additional documentation and examples

One cool thing to do would be to make a delta-rs FFI binding (and maybe other providers in https://github.com/datafusion-contrib/datafusion-table-providers) -- FYI @phillipleblanc

The key benefit here would be that that the providers be locked into the same DataFusion version as used by the consumer

timsaucer · 2024-10-29T20:29:08Z

Adding issue #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

…s to avoid extra work for some providers

…one parameter

timsaucer · 2024-10-30T00:22:33Z

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.

timsaucer changed the title ~~Feature/ffi initial implementation~~ FFI initial implementation Oct 14, 2024

timsaucer marked this pull request as ready for review October 16, 2024 23:58

notfilippo approved these changes Oct 20, 2024

View reviewed changes

alamb mentioned this pull request Oct 21, 2024

Oct 21, 2024: This week in DataFusion #13035

Closed

4 tasks

alamb reviewed Oct 22, 2024

View reviewed changes

timsaucer mentioned this pull request Oct 27, 2024

Add physical plan properties to protobuf definition #13136

Closed

alamb mentioned this pull request Oct 29, 2024

Oct 28, 2024: This week in DataFusion #13167

Closed

3 tasks

alamb approved these changes Oct 29, 2024

View reviewed changes

timsaucer mentioned this pull request Oct 29, 2024

Expand FFI documentation #13175

Closed

3 tasks

timsaucer added 16 commits October 29, 2024 20:21

Initial commit of FFI table provider code

4bb3d58

Add table type

4d31722

Make struct pub

d5f541e

Implementing supports_filters_pushdown

afeb439

Move plan properties over to its own file

7fb77da

Adding release function

3ab0f9f

Adding release functions to additional structs

c616227

Resolve memory leaks

461b4a9

Rename ForeignExecutionPlan for consistency

0408a9b

Resolving memory leak issues

7922d8f

Remove debug statements. Create runtime for block_on operations

afd06be

Switching over to stable abi and async-ffi

dafc982

Make consistent the use of Foreign and FFI on struct names

ff4d2e4

Apply prettier

1dbca58

Format for linter

7761c84

Add doc-comment

d0f8f88

timsaucer added 9 commits October 29, 2024 20:21

Add option to specify table provider does not support pushdown filter…

6b5227e

…s to avoid extra work for some providers

Remove setting default features in cargo file

400f45a

Tokio only needed for unit tests

8b220cd

Provide log errors rather than failing silently on schema requests

8d0f86d

Set default features for datafusion to false in ffi crate

9c01f75

Using TryFrom or From instead of implementing new when there is only …

61f44ae

…one parameter

Move arrow wrappers into their own file

1576520

Add documentation

790f454

Small adjustment to documentation

bf626f4

timsaucer force-pushed the feature/ffi-initial-implementation branch from 486bf06 to bf626f4 Compare October 30, 2024 00:21

timsaucer added 2 commits October 29, 2024 20:25

Add license text

bb47819

Fix unnecessary qualification

71ae880

timsaucer mentioned this pull request Oct 30, 2024

Example: FFI Table Provider as dynamic module loading #13183

Merged

taplo format

011340c

timsaucer merged commit 538e0d1 into apache:main Oct 30, 2024
26 checks passed

timsaucer deleted the feature/ffi-initial-implementation branch October 31, 2024 12:04

corleyma mentioned this pull request Oct 31, 2024

Expose PyIceberg table as PyArrow Dataset apache/iceberg-python#30

Open

This was referenced Nov 5, 2024

Nov 5. 2024: This week in DataFusion #13265

Open

[DISCUSSION] 2024 Q4 / 2025 Q1 Roadmap #13274

Open

timsaucer mentioned this pull request Nov 8, 2024

Add protobuf codecs for FFI physical plans #13309

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FFI initial implementation #12920

FFI initial implementation #12920

timsaucer commented Oct 14, 2024 •

edited

Loading

timsaucer commented Oct 17, 2024

alamb commented Oct 20, 2024

notfilippo left a comment

alamb left a comment

alamb Oct 22, 2024

alamb Oct 22, 2024

alamb Oct 22, 2024

timsaucer Oct 26, 2024

alamb Oct 22, 2024

timsaucer Oct 26, 2024

timsaucer Oct 27, 2024

alamb Oct 29, 2024

alamb Oct 29, 2024

alamb Oct 22, 2024

timsaucer Oct 26, 2024

alamb Oct 22, 2024

timsaucer Oct 26, 2024

timsaucer commented Oct 26, 2024

notfilippo commented Oct 26, 2024

alamb left a comment

timsaucer commented Oct 29, 2024

timsaucer commented Oct 30, 2024

		This crate contains code to allow interoperability of Apache [DataFusion]
		with functions from other languages using a stable interface.

FFI initial implementation #12920

FFI initial implementation #12920

Conversation

timsaucer commented Oct 14, 2024 • edited Loading

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Remaining Issues

timsaucer commented Oct 17, 2024

alamb commented Oct 20, 2024

notfilippo left a comment

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timsaucer commented Oct 26, 2024

notfilippo commented Oct 26, 2024

alamb left a comment

Choose a reason for hiding this comment

timsaucer commented Oct 29, 2024

timsaucer commented Oct 30, 2024

timsaucer commented Oct 14, 2024 •

edited

Loading