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

feat: add mysql impl #26

Merged
merged 13 commits into from
Feb 22, 2023
Merged

feat: add mysql impl #26

merged 13 commits into from
Feb 22, 2023

Conversation

dust1
Copy link
Contributor

@dust1 dust1 commented Jan 16, 2023

Which issue does this PR close?

Closes #15

Rationale for this change

Added a default database implementation based on mysql

What changes are included in this PR?

A default impl module has been added to append internal database implementations

Are there any user-facing changes?

If the target database is mysql, users do not need to implement the Database.
I tried to consider the possibility of adding other database implementations, So you can use this method to create default implementations of various databases:

pub async fn build_default_database<T>(config: DatabaseConnConfig) -> Result<Box<dyn Database>>
where
    T: DatabaseBuilder,
{
    let builder = T::default();
    builder.build(config).await
}

Its name is now _build_default_database because I need to get it through make clippy.

If you want to add new database implementations, We can adapt this method by implementing MysqlDatabaseBuilder and Database

How does this change test

I ran it through my local database and it worked:
select * from test;

id,name,age,
1,Bob,25,
2,Ben,41

delete from test where id = 1

affected_rows: 1

@CLAassistant
Copy link

CLAassistant commented Jan 16, 2023

CLA assistant check
All committers have signed the CLA.

@waynexia
Copy link
Member

I plan to review it today or tomorrow

@dust1
Copy link
Contributor Author

dust1 commented Jan 16, 2023

I plan to review it today or tomorrow

It's still a little bit unimplemented, but I'll submit it later or later in the evening😥

@waynexia
Copy link
Member

I plan to review it today or tomorrow

It's still a little bit unimplemented, but I'll submit it later or later in the evening😥

Sorry I didn't notice this is a draft PR on my phone... Nooo means to push, take your own pace 🫣

@jiacai2050 jiacai2050 added the enhancement New feature or request label Feb 3, 2023
@dust1 dust1 marked this pull request as ready for review February 20, 2023 08:40
@dust1
Copy link
Contributor Author

dust1 commented Feb 20, 2023

Sorry it took so long😢

@@ -10,9 +10,13 @@ repository = "https://github.com/CeresDB/sqlness"
license = "Apache-2.0"
description = "SQL integration test harness"

[dev-dependencies]
tokio-test = "*"

[dependencies]
async-trait = "0.1"
derive_builder = "0.11"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make this dep optional, and add a feature to enable this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it like this?

tokio-test = {version = "0.4.2", optional = true}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you also need a feature to enable this. Something like this

[features]
default = []
mysql = ["dep:mysql"]

Cargo.toml Outdated
@@ -10,9 +10,13 @@ repository = "https://github.com/CeresDB/sqlness"
license = "Apache-2.0"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Delete Cargo.lock, it shouldn't be tracked in git.

#28

src/config.rs Outdated Show resolved Hide resolved
}
}

pub async fn _build_default_database<T>(config: DatabaseConnConfig) -> Result<Box<dyn Database>>
Copy link
Member

@jiacai2050 jiacai2050 Feb 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this method is not used, I think we remove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method is the one I used to create the default database implementation.

let mysql_db = build_default_database::<MysqlDatabaseBuilder>(config)?;

But right now sqlness doesn't need it anywhere

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have on clear idea on when will this be used, so I suggest remove this method for now.

use crate::{config::DatabaseConnConfig, error::Result, Database, SqlnessError};

#[async_trait]
pub trait DatabaseBuilder: Send + Sync + Default {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this trait depend on Default?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As with build_default_database above, I need Default to be compatible with other database implementations

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still can't understand why this is required.

Mysql builder should have no relation with other DB.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's what I think. If additional database support is added later, the Builder can be implemented like this:

pub struct PostgreSQLBuilder;

#[async_trait]
impl DatabaseBuilder for PostgreSQLBuilder {
    async fn build(&self, config: DatabaseConnConfig) -> Result<Box<dyn Database>> {
        todo!()
    }
}

We can then create a default implementation of PostgreSQL like this:

let pg_db = build_default_database::<PostgreSQLBuilder>()

Maybe I should delete them now😂

@jiacai2050
Copy link
Member

@dust1 Please sign CLA first.

@jiacai2050
Copy link
Member

The general impl looks fine to me now, I will append some minor changes before merge.

Copy link
Member

@jiacai2050 jiacai2050 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jiacai2050 jiacai2050 changed the title add: init mysql impl feat: add mysql impl Feb 22, 2023
@jiacai2050 jiacai2050 added this pull request to the merge queue Feb 22, 2023
Merged via the queue into CeresDB:main with commit 8f56d30 Feb 22, 2023
@dust1 dust1 deleted the default-mysql branch February 22, 2023 03:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add default database implementation
4 participants