Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat(rust): add the driver exporter #1756

Merged
merged 33 commits into from
Apr 30, 2024
Merged

feat(rust): add the driver exporter #1756

merged 33 commits into from
Apr 30, 2024

Conversation

alexandreyc
Copy link
Contributor

@alexandreyc alexandreyc commented Apr 24, 2024 •
edited by lidavidm
Loading

Third PR for the Rust implementation containing the driver exporter which allows to automatically create a C-compatible driver from a native Rust driver.

@github-actions github-actions bot added this to the ADBC Libraries 1.0.0 milestone Apr 24, 2024
@alexandreyc
Copy link
Contributor Author

I'm not sure why the CI is broken... Maybe a cache invalidation issue?

CC @mbrobbel

@mbrobbel
Copy link
Contributor

I'm not sure why the CI is broken... Maybe a cache invalidation issue?

CC @mbrobbel

Looks like actions/runner-images#9732.

@alexandreyc
Copy link
Contributor Author

Well, I've tried to relaunch the pipeline but it's still failing... @lidavidm can you clear the cache?

@lidavidm
Copy link
Member

#1764, but I still see that cargo isn't found despite having what is supposed to be the latest image version

@lidavidm
Copy link
Member

For now I'm just not going to run this on M1 and we can add it back later

@lidavidm
Copy link
Member

@alexandreyc if you rebase, it should work 🤞

@alexandreyc
Copy link
Contributor Author

It works! Thanks!

mbrobbel
mbrobbel reviewed Apr 25, 2024
View reviewed changes
Copy link
Contributor

@mbrobbel mbrobbel left a comment

Choose a reason for hiding this comment

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

Thanks! I started reviewing this.

rust/core/src/driver_exporter.rs Outdated Show resolved Hide resolved
rust/core/src/driver_exporter.rs Outdated Show resolved Hide resolved
rust/core/src/driver_exporter.rs Outdated Show resolved Hide resolved
rust/core/src/driver_exporter.rs Outdated Show resolved Hide resolved
rust/core/src/lib.rs Outdated Show resolved Hide resolved
rust/core/src/driver_exporter.rs Outdated Show resolved Hide resolved
rust/core/src/driver_exporter.rs Show resolved Hide resolved
rust/core/src/driver_exporter.rs Show resolved Hide resolved
rust/core/src/driver_exporter.rs Outdated Show resolved Hide resolved
rust/core/src/driver_exporter.rs Outdated Show resolved Hide resolved
@alexandreyc
Update to make driver_exporter module private
df85b8b
@alexandreyc
Add exporting dummy driver
72da2fa
@alexandreyc
Update to use trait instead of function for building ffi driver
0b7ce0a
@alexandreyc
Update to use std::ptr::copy_nonoverlapping instead of `str::ptr::c…
ba10d4c 
…opy`
@alexandreyc
Add null checks for copy_string and copy_bytes
c248900
@alexandreyc
Update release_ffi_driver to return error when double releasing
580bad0
@alexandreyc
Remove useless bounds on exported types
3376c76
@alexandreyc
Update ExportedDatabase and ExportedConnection to enums
77ccb7b
@alexandreyc
Add safety comments to copy_bytes and copy_string
6eccba3
@alexandreyc
Update copy_string to improve length calculation
7348489
@alexandreyc
Update to avoid panicking in check_err
d5c034f
@alexandreyc
Improve documentation for init function naming
b7e01e9
@alexandreyc
Improve documentation of export_driver macro
dee721f
@alexandreyc
Update ExportedStatement to tuple struct
97f4e4b
mbrobbel
mbrobbel reviewed Apr 29, 2024
View reviewed changes
rust/core/src/driver_exporter.rs Outdated Show resolved Hide resolved
rust/core/src/driver_exporter.rs Outdated Show resolved Hide resolved
rust/core/src/driver_exporter.rs Outdated Show resolved Hide resolved
rust/core/src/driver_exporter.rs Outdated Show resolved Hide resolved
rust/core/src/driver_exporter.rs Outdated Show resolved Hide resolved
rust/core/src/driver_exporter.rs Outdated Show resolved Hide resolved
rust/core/src/driver_exporter.rs Outdated Show resolved Hide resolved
rust/core/src/driver_exporter.rs Outdated Show resolved Hide resolved
rust/core/src/driver_exporter.rs Outdated Show resolved Hide resolved
rust/core/src/driver_exporter.rs Show resolved Hide resolved
@alexandreyc
Copy link
Contributor Author

Thanks @mbrobbel for your thorough review! I think I've taken into account all your comments.

I've pushed a last commit to better differentiate between broken internal invariant (panicking) and API misuse (erroring)

mbrobbel
mbrobbel approved these changes Apr 30, 2024
View reviewed changes
Copy link
Contributor

@mbrobbel mbrobbel left a comment

Choose a reason for hiding this comment

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

Thanks for working through all those comments so quickly. On to the next PR!

rust/core/src/options.rs Outdated Show resolved Hide resolved
@alexandreyc
Copy link
Contributor Author

@lidavidm I think this PR can be merged

@lidavidm lidavidm merged commit 608a1e4 into apache:main Apr 30, 2024
5 checks passed
cocoa-xu pushed a commit to meowcraft-dev/arrow-adbc that referenced this pull request May 8, 2024
@alexandreyc @cocoa-xu
feat(rust): add the driver exporter (apache#1756)
4c3fc1c 
Third PR for the Rust implementation containing the driver exporter
which allows to automatically create a C-compatible driver from a native
Rust driver.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mbrobbel mbrobbel mbrobbel approved these changes

@lidavidm lidavidm lidavidm approved these changes

@wjones127 wjones127 Awaiting requested review from wjones127 wjones127 is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
ADBC Libraries 12
Development

Successfully merging this pull request may close these issues.
3 participants
@alexandreyc @mbrobbel @lidavidm