feat(rust/drivermgr): start rust driver_manager #416

wjones127 · 2023-02-04T21:30:09Z

No description provided.

wjones127 · 2023-02-08T06:05:57Z

I'm still figuring out the Windows CI, but otherwise this is ready for review. I recommend reading the files in the following order:

rust/src/lib.rs
rust/src/error.rs
rust/src/interface.rs
rust/src/ffi.rs
rust/src/driver_manager.rs
rust/tests/test_driver_manager.rs

wjones127 · 2023-02-09T06:15:29Z

Rust CI is successful on my branch: https://github.com/wjones127/arrow-adbc/actions/runs/4131461920
🚀

wjones127 · 2023-02-13T16:25:30Z

I am working on a follow up that adds tests with Miri (#446). As part of that I've found there are some memory leaks and use-after-free errors in this code. Once I'm done with #446 I can backport them into this PR.

wjones127 · 2023-02-13T16:28:38Z

rust/src/driver_manager.rs

+//! [AdbcConnection] should not be used across multiple threads. Driver
+//! implementations do not guarantee connection APIs are safe to call from
+//! multiple threads, unless calls are carefully sequenced. So instead of using
+//! the same connection across multiple threads, create a connection for each
+//! thread. [AdbcConnectionBuilder] is [core::marker::Send], so it can be moved
+//! to a new thread before initialized into an [AdbcConnection]. [AdbcConnection]
+//! holds it's inner data in a [std::rc::Rc], so it is also cheaply copyable.


I was thinking just last night that we might want to implement async execute methods, which would involve invoking with spawn_blocking(). That might mean we need to make AdbcConnection Send, but not 100% sure yet. Maybe something for a follow up.

I've wanted to implement async interfaces in general. That'll require some thought between Flight RPC, and Rust/C++/Go. (I want to make the C-level ABI in a way that all three languages can expose their native concurrency models efficiently, and import others' models efficiently. But that means I need to really go study up on how those models work.) I'd appreciate any thoughts you and @zeroshade have.

e.g.: gRPC C++ uses a callback-based model. UCX (in C) uses a polling-based model. There's precedent like kqueue/epoll, and of course the new io_uring (don't know if that's relevant).

There's also Python's model to consider (as a consumer), and probably we'd want to anticipate things like OpenTelemetry (or generally: non-stack-frame, non-thread based 'task' context). And eventually, whenever I get around to JNI bindings, Java's model as well (which, with virtual threading, is also getting more complicated/interesting).

Oh, and consideration of whether we need an async version of C Data Interface...

FWIW the Rust model is described well here: https://rust-lang.github.io/async-book/02_execution/02_future.html.

But for now I was simply thinking that Rust driver manager could wrap the blocking calls into C drivers with tokio::spawn_blocking(), which sends the call to another thread to execute. This is what Rust async libraries do for filesystem interaction on systems that don't provide a native async interface. And then native Rust drivers could always be async. But maybe there is value in having both async and blocking versions of execute() in the Rust API. Something I'll try to get feedback from the Rust community on once I have a proposal for the ADBC Rust API.

I thought about this in the R package too. In the Arrow R package we always execute "cancellable arrow stuff" on another thread (via an Executor) and wait for that future to complete. That relies on Arrow's async API and cancellation signal handlers, but it would be a huge asset for the driver manager to handle this (nothing worse than accidentally executing a huge query without the ability to cancel).

Cool. If I can find some spare time...

I think async interfaces in general throughout our ecosystem are going to be more important as they get adopted more by developers (C++ is, well, coming along, but Go/mostly Rust/sorta Python have generally mature concurrency stories nowadays)

lidavidm

Generally looks good, thank you!

I had some questions, but my Rust is rather out of date so hopefully it makes sense

lidavidm · 2023-02-24T16:28:01Z

.github/workflows/rust.yml

+        run: cargo fmt -- --check
+      - name: Clippy
+        run: cargo clippy --tests
+      - name: Build Driver SQLite (Windows)


You may or may not want to integrate with the main pipeline, which will build all these artifacts once so you can just download them from other jobs.

For now it seems clear enough while separate. But if we rely on any more artifacts from the other build, then I think we should merge.

lidavidm · 2023-02-24T16:28:34Z

rust/Cargo.toml

+
+[package]
+name = "arrow-adbc"
+version = "0.1.0"


nit: version needs to be bumped (also, we'll have to add this to the release scripts)

Good point. I'll need to look at what needs to be done for release.

lidavidm · 2023-02-24T16:29:02Z

rust/Cargo.toml

+arrow = { version = "32.0.0", features = ["ffi"], default_features = false}
+libloading = "0.7"
+
+# TODO: support arrow2 with a non-default feature.


Given the upcoming merger - should we target arrow2 directly if that is going to be the primary implementation?

lidavidm · 2023-02-24T16:30:49Z

rust/src/error.rs

+///
+/// Can be used in combination with [check_err] when implementing ADBC FFI
+/// functions.
+pub trait AdbcError {


Hmm. Instead of requiring boxing everywhere, what about having a concrete error type and just converting to/from the FFI version as necessary?

Also in general, I think it'd be better to compartmentalize the FFI bits the same way Go does, instead of assuming FFI as a fundamental part of the implementation

I agree with this now. When I was thinking more FFI-centric this made a bit more sense. I have refactored to use a concrete struct in #478.

lidavidm · 2023-02-24T16:31:24Z

rust/src/interface.rs

+/// Databases hold state shared by multiple connections. This typically means
+/// configuration and caches. For in-memory databases, it provides a place to
+/// hold ownership of the in-memory database.
+pub trait DatabaseApi {


nit: is it typical to suffix traits with 'Api'?

This was mostly to differentiate between the AdbcDatabase in the driver_manager module and this trait. But given we want to be less FFI-centric, I renamed the driver_manager AdbcDatabase -> DriverDatabase and renamed this one AdbcDatabase.

wjones127 · 2023-02-26T23:16:16Z

@lidavidm I think I'll halt this PR in favor of #478. I'd like to get feedback from the Rust community on the Rust API first, then go ahead with the driver manager and C interface implementation modules.

lidavidm · 2023-02-27T15:31:35Z

Sounds good to me. Thanks Will!

lidavidm · 2024-07-01T00:37:10Z

I think this has been superseded?

wjones127 force-pushed the rust-driver-manager branch 5 times, most recently from 0dc016f to 6f6a5f9 Compare February 4, 2023 22:46

wjones127 mentioned this pull request Feb 4, 2023

ADBC FFI types and possible abstraction apache/arrow-rs#3540

Open

wjones127 force-pushed the rust-driver-manager branch 18 times, most recently from c6e3c8e to 1492bc2 Compare February 8, 2023 05:58

wjones127 marked this pull request as ready for review February 8, 2023 06:06

wjones127 mentioned this pull request Feb 8, 2023

Start new ADBC crate apache/arrow-rs#3582

Closed

4 tasks

wjones127 force-pushed the rust-driver-manager branch from 1492bc2 to 3ad0dea Compare February 9, 2023 03:15

wjones127 mentioned this pull request Feb 9, 2023

feat(rust/implement): create driver implementation utilities #446

Closed

wjones127 force-pushed the rust-driver-manager branch from 3ad0dea to 747b9e2 Compare February 9, 2023 03:31

wjones127 force-pushed the rust-driver-manager branch 6 times, most recently from a50c6f5 to 157d304 Compare February 9, 2023 05:31

wip: write rust driver_manager

1fb6999

wjones127 force-pushed the rust-driver-manager branch from 157d304 to 1fb6999 Compare February 9, 2023 06:04

wjones127 commented Feb 13, 2023

View reviewed changes

lidavidm reviewed Feb 24, 2023

View reviewed changes

wjones127 marked this pull request as draft February 26, 2023 23:11

eitsupi mentioned this pull request Jan 4, 2024

PRQL query engine PRQL/prql#3825

Open

alexandreyc mentioned this pull request Jan 29, 2024

feat(rust): add driver manager and ability to expose Rust driver as C driver #1496

Closed

cocoa-xu mentioned this pull request Feb 10, 2024

[WIP] Adding support for deltalake file elixir-explorer/explorer#843

Closed

alexandreyc mentioned this pull request Apr 15, 2024

Rust: complete implementation #1723

Closed

wjones127 closed this Sep 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(rust/drivermgr): start rust driver_manager #416

feat(rust/drivermgr): start rust driver_manager #416

wjones127 commented Feb 4, 2023

wjones127 commented Feb 8, 2023 •

edited

Loading

wjones127 commented Feb 9, 2023

wjones127 commented Feb 13, 2023

wjones127 Feb 13, 2023

lidavidm Feb 23, 2023

lidavidm Feb 23, 2023

wjones127 Feb 23, 2023

paleolimbot Feb 24, 2023

lidavidm Feb 24, 2023

lidavidm left a comment

lidavidm Feb 24, 2023

wjones127 Feb 26, 2023

lidavidm Feb 24, 2023

wjones127 Feb 26, 2023

lidavidm Feb 24, 2023

lidavidm Feb 24, 2023

wjones127 Feb 26, 2023

lidavidm Feb 24, 2023

wjones127 Feb 26, 2023

wjones127 commented Feb 26, 2023

lidavidm commented Feb 27, 2023

lidavidm commented Jul 1, 2024

feat(rust/drivermgr): start rust driver_manager #416

feat(rust/drivermgr): start rust driver_manager #416

Conversation

wjones127 commented Feb 4, 2023

wjones127 commented Feb 8, 2023 • edited Loading

wjones127 commented Feb 9, 2023

wjones127 commented Feb 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lidavidm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wjones127 commented Feb 26, 2023

lidavidm commented Feb 27, 2023

lidavidm commented Jul 1, 2024

wjones127 commented Feb 8, 2023 •

edited

Loading