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

issues-969 Replace sea-query-driver to sea-query-binder #970

Conversation

ikrivosheev
Copy link
Member

@ikrivosheev ikrivosheev commented Aug 12, 2022

PR Info

Adds

  • Replace sea-query-driver to sea-query-binder

Cargo.toml Outdated Show resolved Hide resolved
Copy link
Member

@billy1624 billy1624 left a comment

Choose a reason for hiding this comment

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

Hey @ikrivosheev, maybe we could get SeaQL/sea-query#416 done then turn our heads back to this?

Btw... I notice a breaking change in the binder of SQLite. The old haviour is to convert rust decimal and big decimal into f64 for binding. However, the new behaviour will simply panic.

thread 'main' panicked at 'Sqlite doesn't support decimal arguments', /home/runner/.cargo/git/checkouts/sea-query-4258bc96379dd872/50ee12e/sea-query-binder/src/sqlx_sqlite.rs:98:21

https://github.com/SeaQL/sea-orm/runs/7913388359?check_suite_focus=true#step:5:300

Comment on lines 218 to 224
pub(crate) fn sqlx_query(stmt: &Statement) -> sqlx::query::Query<'_, Postgres, SqlxValues> {
if let Some(values) = &stmt.values {
query = bind_query(query, values);
sqlx::query_with(&stmt.sql, SqlxValues(values.clone()))
} else {
sqlx::query_with(&stmt.sql, SqlxValues(Values(Vec::new())))
}
query
}
Copy link
Member

Choose a reason for hiding this comment

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

Btw... I'm curious if we could avoid the clone by taking ownership of the stmt: Statement?

Copy link
Member Author

Choose a reason for hiding this comment

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

I cannot take ownership of the stmt: Statement, because after sqlx_query we needed to send metrics:

#[instrument(level = "trace")]
pub async fn query_all(&self, stmt: Statement) -> Result<Vec<QueryResult>, DbErr> {
    debug_print!("{}", stmt);

    let query = sqlx_query(&stmt);
    if let Ok(conn) = &mut self.pool.acquire().await {
        crate::metric::metric!(self.metric_callback, &stmt, {
            match query.fetch_all(conn).await {
                Ok(rows) => Ok(rows.into_iter().map(|r| r.into()).collect()),
                Err(err) => Err(sqlx_error_to_query_err(err)),
            }
        })
    } else {
        Err(DbErr::Query(
            "Failed to acquire connection from pool.".to_owned(),
        ))
    }
}

Copy link
Member

Choose a reason for hiding this comment

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

Right!

We can change pub statement: &'a crate::Statement into pub statement: String but then, worse still we need to to_string on Statement ahead of metrics!.

  • sea-orm/src/metric.rs

    Lines 8 to 17 in d532deb

    #[derive(Debug)]
    /// Query execution infos
    pub struct Info<'a> {
    /// Query executiuon duration
    pub elapsed: Duration,
    /// Query data
    pub statement: &'a crate::Statement,
    /// Query execution failed
    pub failed: bool,
    }

So, I'll support as-is - cloning stmt: Statement. Later in time, when we're optimizing for performance, We can move metric utility to compile time, i.e. provide a wrapper to existing connection. So that we don't have to clone Statement even when metric is turned off.

CC @tyt2y3

Copy link
Member Author

Choose a reason for hiding this comment

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

Can't do it anyway, because:

pub fn query_with<'q, DB, A>(sql: &'q str, arguments: A) -> Query<'q, DB, A> ...

query_with doesnot take ownership of the query...

Copy link
Member Author

Choose a reason for hiding this comment

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

I can make PR for add support QueryOwned into sqlx and this would help us.

@ikrivosheev
Copy link
Member Author

ikrivosheev commented Aug 26, 2022

@billy1624 thank you for review. I will return to this PR after merge PRs in SeaQuery.

@ikrivosheev ikrivosheev changed the base branch from master to sea-query-v0.27 September 8, 2022 09:24
@ikrivosheev ikrivosheev force-pushed the feature/issues-969_sea-query-binder branch from bf50cda to ecbd7c2 Compare September 8, 2022 09:57
@ikrivosheev ikrivosheev merged commit 52531db into SeaQL:sea-query-v0.27 Sep 9, 2022
@ikrivosheev ikrivosheev deleted the feature/issues-969_sea-query-binder branch September 9, 2022 08:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Support sea-query-binder
7 participants