feat : ABI upgrade from abi_stabby to stabby since abi_stable is no longer maintained#21030
feat : ABI upgrade from abi_stabby to stabby since abi_stable is no longer maintained#21030coderfender wants to merge 13 commits intoapache:mainfrom
Conversation
|
Linking Tim's PR here #21025 |
coderfender
changed the title
|
|
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:
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
force-pushed
the
feat_migrate_ffi_to_stabby
branch
from
7dea35a to
9834a59
Compare
| FFIResult, rvec_wrapped_to_vec_datatype, vec_datatype_to_rvec_wrapped, | ||
| }; | ||
| use crate::volatility::FFI_Volatility; | ||
| use crate::volatility::FfiVolatility; |
There was a problem hiding this comment.
The naming convention I used here was to match Arrow's FFI convention.
There was a problem hiding this comment.
Thank you for the comment @timsaucer . I had to rename these structs to fix clippy errors . Do you think we should stick with clippy recommended naming convention or we could revert to use arrow's convention and ignore clippy warnings ?
There was a problem hiding this comment.
We had a clippy override in the repo before. I wonder if it's getting lost in the update?
There was a problem hiding this comment.
I was under the impression. Let me see if I can get it fixed
There was a problem hiding this comment.
If we can get clippy to ignore it for everything in datafusion/ffi/src I think that would be ideal. What do you think? Do you have a strong preference on naming?
There was a problem hiding this comment.
I think now is a good time to make it clippy complaint given that we are making a major change . However, if the goal is to keep it consistent with Arrow's notation I can go ahead and revert the naming convention
datafusion/ffi/src/util.rs
Outdated
| use stabby::string::String as StabbyString; | ||
| use stabby::vec::Vec as StabbyVec; |
There was a problem hiding this comment.
Minor: Recommend naming SString and SVec to be a bit less distracting.
There was a problem hiding this comment.
@timsaucer thank you. I had to resort to clear naming to avoid mistakes with the crate upgrade. Now that the code compiles, I will push a commit to make the naming shorter
|
Merged with main and see some referenced older package. Working on fixing it to use |
|
Updated cargo format , changed table_provider_module to use the direct function pointer call instead of accessor method pattern used in abi_stable |
coderfender
force-pushed
the
feat_migrate_ffi_to_stabby
branch
from
70a5478 to
d6194b8
Compare
|
Testing python bindings now with the new ABI |
|
The df python bindings rendered through new stabby implementations seem to be working on my local machine |
2 participants
Which issue does this PR close?
generational-arenafrom project #20863Rationale for this change
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?