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

Replace trait objects with generics #64

Open
mversic opened this issue Jul 25, 2020 · 2 comments
Open

Replace trait objects with generics #64

mversic opened this issue Jul 25, 2020 · 2 comments

Comments

@mversic
Copy link

mversic commented Jul 25, 2020

I really get sore eyes when I see Box<dyn Something> in rust code. It's not an idiomatic rust to use them, they bypass type safety and they hurt performance.

I understand that the API is oriented around trait objects so as to be able to link your compiled program against any driver at runtime. There certainly are cases where people would want to build apps that are compiled-once and which work with any database at runtime, but there are also cases where people would want to use RDBC API for accessing just one particular database. It's obvious that in the second case performance is harmed, and more importantly, type safety is lost. Also, API in the current state doesn't allow for downcasting or any other method of accessing concrete types which I found to be of use, although rarely, for some driver specific behavior when using JDBC. I think backdoors are always a nice to have feature in any API.

I would like to propose an alternative which would include best of both worlds. This means that API would use generics instead of trait objects like this:

pub trait Driver: Sync + Send {
    type C: Connection
    fn connect(&self, url: &str) -> Result<Self::C>;
}
pub trait Connection {
    type S: Statement;
    fn create(&mut self, sql: &str) -> Result<Self::S>;
    fn prepare(&mut self, sql: &str) -> Result<Self::S>;
}
pub trait Statement {
    type RS: ResultSet;
    fn execute_query(&mut self, params: &[Value]) -> Result<RS>;
    fn execute_update(&mut self, params: &[Value]) -> Result<u64>;
}

This API would support native compilation for any driver used, but the limitation is that the driver would have to be known in compile time. To handle that limitation, the trick is to make a driver-agnostic concrete implementation of the API which would wrap trait objects, it would hide the boxed API and expose it as generic. This way the user would only have to choose whether he wants his code compiled against native implementation or driver-agnostic implementation of the API.

PS: Compiling this project is broken. It complains that it cannot compile sqlparser

@aloucks
Copy link

aloucks commented Sep 18, 2020

Another approach would be to follow what futures did with the Waker and have the concept of a RawConnection that is constructed from a RawConnectionVTable. The same treatment could be applied to Statement, ResultSet, etc.. This should also eliminate the need to box all of the return types.

For reference:
https://doc.rust-lang.org/beta/std/task/struct.RawWaker.html

@nmusatti
Copy link

In most cases DBMS operations are so expensive that the cost of a few dynamic dispatched calls is going to be negligible. There is indeed value in being able to interface different databases not only without changing the code, but also without recompiling. My daily job involves such an application, written in Java. In my opinion this should be the focus of RDBC, especially given that, as far as I can tell, sqlx already does a decent job of implementing the approach suggested by @mversic. To be more precise: I'd provide the generic wrapper as primary API, and the wrapped, specific trait implementation as access points to vendor specific functionality. From the little I saw the vtable based approach seems to be needlessly complex.
Both ODBC and JDBC allow for binary plugins, but I don't think that's a goal worth pursuing, except that commercial driver vendors might not be too happy to have to ship their source code. While I don't think RDBC should directly support commercial DBMS's, it should certainly allow for external drivers.

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

3 participants