-
Notifications
You must be signed in to change notification settings - Fork 165
feat(rust/core): add async traits for Driver, Database, Connection, Statement #3714
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
base: main
Are you sure you want to change the base?
Conversation
|
CC @felipecrv @mbrobbel too |
a52ee39 to
ad1f184
Compare
mbrobbel
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
felipecrv
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once the code in blocking is moved back to the current location I can give another review.
@lidavidm do we really need everything to be a Future, even get/set options?
| @@ -0,0 +1,793 @@ | |||
| // Licensed to the Apache Software Foundation (ASF) under one | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can skip creating the blocking namespace altogether and keep the symbols where they were. "Blocking" has bad connotations and I don't think a blocking database API is as bad as the name "blocking" suggests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can skip creating the
blockingnamespace altogether and keep the symbols where they were. "Blocking" has bad connotations and I don't think a blocking database API is as bad as the name "blocking" suggests.
I’m a bit confused. @mbrobbel suggests that the async traits should be used by default. To me, this means they should be at the root of the crate, with the corresponding sync version in a separate module.
I don’t see anything wrong with using the namespace blocking. I relied on existing crates. For example reqwest
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
async being the default for database APIs is a bad idea IMO. Database connections are stateful objects that accumulates statements in transactions. Async may re-order actual execution of the queries on the connection. A world of pain.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made the sync traits the default, but I won’t move them back to the crate root. Let them stay in the sync mod.
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 |
…or DataFusion driver Signed-off-by: if0ne <[email protected]>
Signed-off-by: if0ne <[email protected]>
Signed-off-by: if0ne <[email protected]>
Signed-off-by: if0ne <[email protected]>
…d sync wrappers Signed-off-by: if0ne <[email protected]>
Signed-off-by: if0ne <[email protected]>
61963f8 to
c838f22
Compare
Signed-off-by: if0ne <[email protected]>
Signed-off-by: if0ne <[email protected]>
Signed-off-by: if0ne <[email protected]>
Signed-off-by: if0ne <[email protected]>
Signed-off-by: if0ne <[email protected]>
Signed-off-by: if0ne <[email protected]>
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.