Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
FFI initial implementation #12920
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
FFI initial implementation #12920
Changes from all commits
4bb3d58
4d31722
d5f541e
afeb439
7fb77da
3ab0f9f
c616227
461b4a9
0408a9b
7922d8f
afd06be
dafc982
ff4d2e4
1dbca58
7761c84
d0f8f88
6b5227e
400f45a
8b220cd
8d0f86d
9c01f75
61f44ae
1576520
790f454
bf626f4
bb47819
71ae880
011340c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this text is backwards -- it is "without the necessity". Maybe we could rephrase like
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should name this "DataFusion C API" or something instead of using the rust FFI term 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.