Skip to content

feat : ABI upgrade from abi_stabby to stabby since abi_stable is no longer maintained#21030

Open
coderfender wants to merge 37 commits intoapache:mainfrom
coderfender:feat_migrate_ffi_to_stabby
Open

feat : ABI upgrade from abi_stabby to stabby since abi_stable is no longer maintained#21030
coderfender wants to merge 37 commits intoapache:mainfrom
coderfender:feat_migrate_ffi_to_stabby

Conversation

@coderfender
Copy link
Copy Markdown
Contributor

@coderfender coderfender commented Mar 18, 2026 •
edited
Loading

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

  • Replace abi_stable types with stabby equivalents:
    • RVec -> stabby::vec::Vec
    • RString -> stabby::string::String
  • Add custom FFI_Option and FFI_Result types since we have raw pointers and self referential pointers which can't implement IStable trait needed for stabby
  • Rename macros: rresult → sresult, rresult_return → sresult_return
  • Fix library path detection for debug/release builds in the example loader
  • Update README

Are these changes tested?

Yes . With df python bindings on DF 52 release

Are there any user-facing changes?

@coderfender
Copy link
Copy Markdown
Contributor Author

coderfender commented Mar 18, 2026 •
edited
Loading

Linking Tim's PR here #21025

@coderfender coderfender changed the title feat : ABI change from abi_stabby to stabby feat : ABI upgrade from abi_stabby to stabby since abi_stable is no longer maintained Mar 18, 2026
@github-actions github-actions Bot added the ffi Changes to the ffi crate label Mar 19, 2026
@coderfender
Copy link
Copy Markdown
Contributor Author

coderfender commented Mar 19, 2026

  1. Removed abi_stable package dependency

  2. Replaced all abi_stable calls with stabby equivalents

  3. Used Vec<K,V> since stabby doesnt have HashMap support
    TODO :

  4. Work on WrappedArray / native arrow types integration (Original impl through ABI just assumed safe implementation through sabi. However, stabby's answer is a little different)

  5. Refactor / remove async-ffi module ( IMHO, this can be done in a separate followup PR)

@timsaucer
Copy link
Copy Markdown
Member

timsaucer commented Mar 19, 2026

One of the things I've been thinking about here is doing some scale testing of performance, which I haven't done on the FFI crate really. I was thinking we could do something along the lines of using https://github.com/datafusion-contrib/datafusion-tpch to generate table providers at different scale factors. Then it would seem we could have a series of tests:

  1. Pure rust with no FFI work.
  2. Pure rust but using two modules and passing table provider via FFI.
  3. Expose table provider to python and test with datafusion-python.

The thing I like about doing this is that we would be able to see the impacts of each of the layers between the code, ideally going from 2->3 having near zero impact.

For such a test I would think about setting up a stream, reading in and dumping the data as fast as possible.

Since this is orthogonal to the actual FFI work you're proposing I might try setting this up on a test repo.

@coderfender coderfender force-pushed the feat_migrate_ffi_to_stabby branch from 7dea35a to 9834a59 Compare March 20, 2026 12:57
timsaucer
timsaucer reviewed Mar 20, 2026
View reviewed changes
Comment thread datafusion/ffi/src/udf/mod.rs Outdated
timsaucer
timsaucer reviewed Mar 20, 2026
View reviewed changes
Comment thread datafusion/ffi/src/util.rs Outdated
@coderfender
Copy link
Copy Markdown
Contributor Author

coderfender commented Mar 20, 2026

Merged with main and see some referenced older package. Working on fixing it to use stabby

@coderfender
Copy link
Copy Markdown
Contributor Author

coderfender commented Mar 20, 2026

Updated cargo format , changed table_provider_module to use the direct function pointer call instead of accessor method pattern used in abi_stable

@coderfender coderfender force-pushed the feat_migrate_ffi_to_stabby branch from 70a5478 to d6194b8 Compare March 20, 2026 23:11
@coderfender coderfender marked this pull request as ready for review March 20, 2026 23:21
@coderfender
Copy link
Copy Markdown
Contributor Author

coderfender commented Mar 21, 2026

Testing python bindings now with the new ABI

@coderfender
Copy link
Copy Markdown
Contributor Author

coderfender commented Mar 21, 2026

The df python bindings rendered through new stabby implementations seem to be working on my local machine

timsaucer
timsaucer reviewed Mar 24, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@timsaucer timsaucer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really nice work on this. I've put in a few questions. I think it's really great that the vast majority of the changes are pretty simple and have 1-1 replacement.

Comment thread datafusion-examples/examples/ffi/ffi_example_table_provider/src/lib.rs
Comment thread datafusion-examples/examples/ffi/ffi_module_interface/src/lib.rs
Comment thread datafusion-examples/examples/ffi/ffi_module_loader/src/main.rs
Comment thread datafusion/ffi/src/proto/logical_extension_codec.rs Outdated
Comment thread datafusion/ffi/src/udf/mod.rs Outdated
Comment thread datafusion/ffi/src/insert_op.rs Outdated
Comment thread datafusion/ffi/src/plan_properties.rs Outdated
Comment thread datafusion/ffi/src/table_source.rs Outdated
Comment thread datafusion/ffi/src/util.rs Outdated
Comment thread datafusion/ffi/src/volatility.rs Outdated
@coderfender
Copy link
Copy Markdown
Contributor Author

coderfender commented Mar 24, 2026

thank you for the comments @timsaucer . I will work on the comments and push a commit shortly

@coderfender coderfender force-pushed the feat_migrate_ffi_to_stabby branch from d63326d to 48926f9 Compare March 25, 2026 04:39
@coderfender
Copy link
Copy Markdown
Contributor Author

coderfender commented Apr 18, 2026

Thank you for the update @timsaucer . Let me go ahead and update the docs to better reflect our approach here

timsaucer
timsaucer reviewed Apr 22, 2026
View reviewed changes
Comment thread datafusion/ffi/src/util.rs Outdated
pub type FFIResult<T> = RResult<T, RString>;
/// back. These are converted back and forth using the `df_result`, `sresult`,
/// and `sresult_return` macros.
pub type FFIResult<T> = FFI_Result<T, SString>;
Copy link
Copy Markdown
Member

@timsaucer timsaucer Apr 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having both a FFIResult<T> and a FFI_Result<T, SString> is confusing. Can we merge down to one?

Copy link
Copy Markdown
Contributor Author

@coderfender coderfender Apr 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great point ! sure

Copy link
Copy Markdown
Contributor Author

@coderfender coderfender Apr 22, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that there is only String based error usage across migration, we might not even need <T,E> and could consolidate this into just FFIResult<T>

Copy link
Copy Markdown
Member

@timsaucer timsaucer Apr 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, FFI_Result<T> to be consistent with the rest of the repo

Copy link
Copy Markdown
Contributor Author

@coderfender coderfender Apr 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Absolutely! I made changes to have one result used and exposed outside

Comment thread datafusion/ffi/README.md
@coderfender coderfender force-pushed the feat_migrate_ffi_to_stabby branch from a4e5903 to 71f1fde Compare April 22, 2026 20:24
@coderfender coderfender force-pushed the feat_migrate_ffi_to_stabby branch from 0ea3355 to 287ef29 Compare April 23, 2026 04:52
@coderfender coderfender force-pushed the feat_migrate_ffi_to_stabby branch from 8ae10c0 to 1aa4823 Compare April 23, 2026 06:50
@coderfender
Copy link
Copy Markdown
Contributor Author

coderfender commented Apr 23, 2026

CI is now green

@coderfender
Copy link
Copy Markdown
Contributor Author

coderfender commented Apr 23, 2026

  1. Merged FFI_Result<T, E>and FFIResult<T> into single FFIResult
  2. Updated test files to use direct function pointer syntax to match stabby's adaptation (abi_stable's provides an option while we directly use plain #[repr(C)] struct with stabby in our custom result / wrapper)
  3. Other fixes rebasing with main

timsaucer
timsaucer reviewed Apr 23, 2026
View reviewed changes
Comment thread datafusion/ffi/README.md Outdated
Comment thread datafusion/ffi/README.md Outdated
Comment thread datafusion/ffi/README.md Outdated
Comment thread datafusion/ffi/README.md Outdated
@coderfender
Copy link
Copy Markdown
Contributor Author

coderfender commented Apr 23, 2026

Updated readme.md per review comments.

timsaucer
timsaucer reviewed Apr 25, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@timsaucer timsaucer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking very good. Minor recommendation since this wasn't a reason for using the repr(c) vs macro but rather an outcome of the decision.

Comment thread datafusion/ffi/README.md
timsaucer
timsaucer reviewed Apr 25, 2026
View reviewed changes
Comment thread datafusion/ffi/src/ffi_option.rs Outdated
@coderfender
Copy link
Copy Markdown
Contributor Author

coderfender commented Apr 25, 2026 •
edited
Loading

Thank you . I updated the trait names per review comments

@timsaucer
Copy link
Copy Markdown
Member

timsaucer commented Apr 25, 2026

Looks like a couple of accidentally commited json files?

@coderfender
Copy link
Copy Markdown
Contributor Author

coderfender commented Apr 25, 2026

Thank you for the review . It seems like some of benchmark JSONs from my other pr got pulled in here. Let me remove them and update here

@coderfender coderfender force-pushed the feat_migrate_ffi_to_stabby branch from 5e2f303 to 99f089c Compare April 26, 2026 01:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@timsaucer timsaucer timsaucer left review comments

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

ffi Changes to the ffi crate

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Remove generational-arena from project

2 participants

@coderfender @timsaucer