search

alexandreyc / adbc-rs

Rust implementation of Arrow Database Connectivity (ADBC)
Apache License 2.0
3 stars 0 forks source link

Very cool! #1

Open paleolimbot opened 3 months ago

paleolimbot commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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.