gh-516: support coexistence of SimpleExpr and QueryStatement #572
Conversation
src/func.rs
Outdated
| /// Query::select() | ||
| /// .from(Char::Table) | ||
| /// .expr(Func::max(Expr::col(Char::SizeW))) | ||
| /// .to_owned() |
src/func.rs
Outdated
| /// .from(Char::Table) | ||
| /// .expr(Func::max(Expr::col(Char::SizeW))) | ||
| /// .to_owned() | ||
| /// .into(), |
There was a problem hiding this comment.
I would rather be more conservative on this type conversion. Would a method into_sub_query_expr with an explicit intent better for this use case?
There was a problem hiding this comment.
Sorry for the slow reply.
Yes, perhaps into_sub_query_expr would be better (perhaps even better something like into_scalar_subquery_expr and implementing a separate ScalarSubQueryStatement)
But doesn't it create confusion that for coalesce SelectStatement needs to be explicitly converted to SubQueryStatement, and all other functions need to pass SelectStatement when called
Seems like this would require redoing the whole api or am I complicating things and it's simpler?
src/expr.rs
Outdated
| @@ -2163,6 +2163,12 @@ impl Expression for SimpleExpr { | |||
| } | |||
| } | |||
|
|
|||
| impl From<SelectStatement> for SimpleExpr { | |||
There was a problem hiding this comment.
What I mean is we remove this From and instead have a concrete impl into_sub_query_expr in SelectStatement.
What I am concerning is that there may be multiple ways of converting SelectStatement into SimpleExpr, or may be can have additional options.
That'd be more future proof.
| @@ -416,15 +420,15 @@ impl Func { | |||
| /// | |||
| /// assert_eq!( | |||
| /// query.to_string(MysqlQueryBuilder), | |||
| /// r#"SELECT COALESCE(`size_w`, `size_h`, 12) FROM `character`"# | |||
| /// r#"SELECT COALESCE((SELECT MAX(`size_w`) FROM `character`), `size_h`, 12) FROM `character`"# | |||
There was a problem hiding this comment.
And btw, can you add a new test case instead? For better test coverage.
There was a problem hiding this comment.
What do you mean by a new test case? I added test cases, insert_coalesce for example. The current test case involves checking for a subquery in coalesce
|
@alphavector hello! Any updates? |
|
Sorry for the long reply... |
| @@ -37,6 +37,10 @@ pub enum SimpleExpr { | |||
| Constant(Value), | |||
| } | |||
|
|
|||
| pub trait IntoSimpleExpr { | |||
| fn into_sub_query_expr(self) -> SimpleExpr; | |||
There was a problem hiding this comment.
@alphavector hello! Naming is confusing me a little bit)
@tyt2y3 what do you think?
pub trait IntoSimpleExpr {
fn into_simple_expr(self) -> SimpleExpr;
}There was a problem hiding this comment.
Actually, there is only one impl for this trait. May be we can simply make it an inherit method?
Took the liberty of implementing this feature to better understand the project
Closes: #516