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.

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

Very cool! #1

Open
paleolimbot opened this issue Apr 4, 2024 · 5 comments
Open

Very cool! #1

paleolimbot opened this issue Apr 4, 2024 · 5 comments

Comments

@paleolimbot
Copy link

Just a note that I think this is great! I'm not sure if you are there yet, but I have very much wanted to write drivers in Rust and am excited to see where this goes.

If you're ever interested in contributing this or something similar to apache/arrow-adbc, feel free to ping me and I would be happy to help!

@alexandreyc
Copy link
Owner

Thanks for the kind words!

My plan is to next implement a native driver (probably SQLite, but it can be anything really) to exercise and validate the API. Then, I will implement the driver exporter (the previous driver will also be helpful to test the driver exporter).

I would be glad to contribute this to the official ADBC repo! Ideally, I would prefer to merge this into apache/arrow-adbc before proceeding any further, in order to receive feedback on the API and a review of the entire codebase.

@paleolimbot
Copy link
Author

Nice! I just saw your PR from earlier this year and I see we didn't review it 😬 .

I'm not really a rust person but I did implement a minimal driver framework in C++ at one point ( https://github.com/apache/arrow-adbc/blob/main/c/driver/common/driver_base.h ) with some tests ( https://github.com/apache/arrow-adbc/blob/main/c/driver/common/driver_test.cc ). Once you're comfortable here that your design will work, I wonder if a good initial contribution would be something similar (a small driver framework + a driver that does nothing except get/set options + a Rust test suite that ensures all of those methods get called)...maybe followed by exporting to C (it sounds like this is your plan already!).

It should be said that big PRs are also welcome (and also that I don't speak for everybody except me as somebody excited to write drivers in Rust!). You could also make one big PR and use it as a scratch space and then make some smaller PRs based on discussion or comments from the big PR...I just mention the driver framework bit because I think I can help review that and/or help contribute tests from the C side.

@alexandreyc
Copy link
Owner

No worries for the previous PR. I started by rebasing and reworking it but I wasn't fully satisfied so I decided to rewrite it from scratch.

My C++ is rusty (no pun intended) but according to my understanding, what you describe as "minimal driver framework" is what I call "public abstract API" and it consists of lib.rs, error.rs and options.rs.

Here is a proposed plan:

  1. First PR with the public abstract API along with a dummy driver (and tests).
  2. Second PR with the driver manager and FFI bindings (it's already implemented and tested against SQLite and PostgreSQL C drivers)
  3. Third PR with the driver exporter (not yet implemented)

@paleolimbot
Copy link
Author

My C++ is rusty (no pun intended)

Pun appreciated!

Here is a proposed plan

That sounds great! Our main problem is that the intersection of people who care about ADBC and people who are able to review Rust properly is very small. Again, any/all PRs are welcome (they help because we can point people who ask about ADBC + Rust, which happens on a fairly regular basis, to them), but it's easier for us to solicit reviews on your behalf if the PRs are smaller/more focused. It sounds like that's your plan!

@alexandreyc
Copy link
Owner

alexandreyc commented Apr 5, 2024

I will take one or two more weeks to implement the driver exporter in this repo. The goal is to fully validate the API before opening any PR in the upstream repo. Then I will start the series of PRs previously mentioned.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants