feat(rust/core): add async traits for Driver, Database, Connection, Statement#3714
feat(rust/core): add async traits for Driver, Database, Connection, Statement#3714if0ne wants to merge 14 commits intoapache:mainfrom
Conversation
|
CC @felipecrv @mbrobbel too |
a52ee39 to
ad1f184
Compare
mbrobbel
left a comment
There was a problem hiding this comment.
For the Send bound stuff I would suggest using https://docs.rs/trait-variant/0.1.2/trait_variant/ (as suggested in https://blog.rust-lang.org/2023/12/21/async-fn-rpit-in-traits/#async-fn-in-public-traits).
For async vs sync I would suggest using the async trait as the default and provided sync helper traits and structs. But the truth is; there is no elegant solution available today for this.
|
|
2888512 to
61963f8
Compare
I made the |
|
Even get_option may involve I/O in some cases (e.g. if the driver has to run a query to fetch some information), unfortunately |
61963f8 to
c838f22
Compare
There was a problem hiding this comment.
Does arrow-rs not already have a trait for this?
There was a problem hiding this comment.
I see that arrow-rs does not implement ArrowAsyncDeviceStreamHandler etc yet. I think we need to figure this out in arrow-rs
There was a problem hiding this comment.
After the merge, I’ll get back to this PR
There was a problem hiding this comment.
Merged (at least the trait). We can bump to the next version when available (possibly via feature? Or maybe splitting async traits into a new crate?)
There was a problem hiding this comment.
I will split the current PR into three pull requests:
- Moving the existing structures into a separate module
- Adding sendable async traits as an
asyncfeature - Adding bridge structures, which will probably also be under the
asyncfeature
There was a problem hiding this comment.
I think there needs to be explanation somewhere of what "local" means (traits that are not Send?)
Additionally, does it make sense to have non-Send variants? It's not clear to me that that's useful (when would a connection be limited to a single thread?)
There was a problem hiding this comment.
This is about flexibility. I think that, in practical terms, most drivers will implement the async trait with a Send bound. But someone might want to implement an async Rust driver that uses an FFI library to connect to a specialized database.
There was a problem hiding this comment.
Is it actually worth optimizing for that flexibility, though? (Versus, say, expecting that such a very specialized database would instead manage this internally?)
There was a problem hiding this comment.
Because as it stands, this means duplicating every method/trait
There was a problem hiding this comment.
IMO it’s not worth supporting the non-Send variant (maybe someday in the future, when the crate stabilizes and reaches versions > 1.0.0).
Personally, I was planning to use adbc for the wasm target, and as far as I understand, all the necessary structures are Send.
There was a problem hiding this comment.
FYI: I found another driver (which seems to be fairly recent): https://github.com/marconae/exarrow-rs
There was a problem hiding this comment.
The sedona-db implementation is unused to my knowledge so don't worry about any compatibility issues there!
There was a problem hiding this comment.
@marconae sorry for the random ping, but would you like to weigh in on async ADBC interfaces? 🙂
There was a problem hiding this comment.
This is about flexibility.
We might end up offering too much flexibility in a standard that is supposed to make database access more uniform. If a database doesn't allow its handles to move between threads, it's the responsibility of the ADBC driver wrapping these database APIs to fix for that rather than bubbling up these restrictions to Rust users of ADBC via the type system which basically means a rewrite of the entire application depending on the ADBC driver that it uses.
There was a problem hiding this comment.
@lidavidm thx for including me. I had to go async, because I am fetching data from an async web-socket protocol. Maybe the my pattern is a bit unusual. For me it was fine to work against a synchronous library and wrap it.
…or DataFusion driver Signed-off-by: if0ne <pavel.agafonov.al@gmail.com>
Signed-off-by: if0ne <pavel.agafonov.al@gmail.com>
Signed-off-by: if0ne <pavel.agafonov.al@gmail.com>
Signed-off-by: if0ne <pavel.agafonov.al@gmail.com>
…d sync wrappers Signed-off-by: if0ne <pavel.agafonov.al@gmail.com>
Signed-off-by: if0ne <pavel.agafonov.al@gmail.com>
Signed-off-by: if0ne <pavel.agafonov.al@gmail.com>
Signed-off-by: if0ne <pavel.agafonov.al@gmail.com>
Signed-off-by: if0ne <pavel.agafonov.al@gmail.com>
Signed-off-by: if0ne <pavel.agafonov.al@gmail.com>
65929ed to
4996555
Compare
Hi, I’m back again. This is a continuation of this PR: #3712
I’ve returned with a draft of the asynchronous traits. They fully mirror the synchronous API (with a couple of differences in input arguments), adding
Future + Sendas the return type.As further development, I can propose:
Abstract away from async executors and introduce an
AsyncExecutortrait with a singleblock_onmethod. Inadbc_core, add helper structures likeSyncDriverWrapper<E: AsyncExecutor, D: AsyncDriver>and others, and implement the corresponding synchronous traits for them.Add some procedural macro magic to automatically implement the synchronous traits by generating the code described above.