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.

Sign up for GitHub

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

gh-516: support coexistence of SimpleExpr and QueryStatement #572

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

gh-516: support coexistence of SimpleExpr and QueryStatement #572

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
f8a2cfa
gh-516: support coexistence of SimpleExpr and QueryStatement
alphavector Jan 2, 2023
6dc6fc9
gh-516: fix formatting
alphavector Jan 3, 2023
bdd64a0
gh-516: call `.take()` instead of `.to_owned()`
alphavector Feb 25, 2023
e5f0b64
gh-516: use into_sub_query_expr instead of From
alphavector Apr 27, 2023
e88fc4f
gh-516: minor fix
alphavector Apr 27, 2023
2e24c0c
Merge remote-tracking branch 'upstream/master' into feature/coalesce_…
alphavector Apr 27, 2023
0c79402
gh-516: reorder the tests
alphavector Apr 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions src/expr.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2163,6 +2163,12 @@ impl Expression for SimpleExpr {
}
}

impl From<SelectStatement> for SimpleExpr {
Copy link
Member

@tyt2y3 tyt2y3 Feb 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

fn from(sel: SelectStatement) -> Self {
SimpleExpr::SubQuery(None, Box::new(sel.into_sub_query_statement()))
}
}

impl SimpleExpr {
/// Negates an expression with `NOT`.
///
Expand Down
12 changes: 8 additions & 4 deletions src/func.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -407,7 +407,11 @@ impl Func {
///
/// let query = Query::select()
/// .expr(Func::coalesce([
/// Expr::col(Char::SizeW).into(),
/// Query::select()
/// .from(Char::Table)
/// .expr(Func::max(Expr::col(Char::SizeW)))
/// .take()
/// .into(),
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Author

Choose a reason for hiding this comment

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

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?

/// Expr::col(Char::SizeH).into(),
/// Expr::val(12).into(),
/// ]))
Expand All @@ -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`"#
Copy link
Member

Choose a reason for hiding this comment

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

And btw, can you add a new test case instead? For better test coverage.

Copy link
Author

Choose a reason for hiding this comment

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

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

/// );
/// assert_eq!(
/// query.to_string(PostgresQueryBuilder),
/// r#"SELECT COALESCE("size_w", "size_h", 12) FROM "character""#
/// r#"SELECT COALESCE((SELECT MAX("size_w") FROM "character"), "size_h", 12) FROM "character""#
/// );
/// assert_eq!(
/// query.to_string(SqliteQueryBuilder),
/// r#"SELECT COALESCE("size_w", "size_h", 12) FROM "character""#
/// r#"SELECT COALESCE((SELECT MAX("size_w") FROM "character"), "size_h", 12) FROM "character""#
/// );
/// ```
pub fn coalesce<I>(args: I) -> FunctionCall
Expand Down
47 changes: 47 additions & 0 deletions tests/mysql/query.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1005,6 +1005,27 @@ fn select_58() {
);
}

#[test]
fn select_coalesce() {
let query = Query::select()
.expr(Func::coalesce([
Query::select()
.from(Char::Table)
.expr(Func::max(Expr::col(Character::Id)))
.to_owned()
.into(),
1.into(),
Value::Bool(None).into(),
]))
.from(Char::Table)
.to_owned();

assert_eq!(
query.to_string(MysqlQueryBuilder),
r#"SELECT COALESCE((SELECT MAX(`id`) FROM `character`), 1, NULL) FROM `character`"#
);
}

#[test]
#[allow(clippy::approx_constant)]
fn insert_2() {
Expand Down Expand Up @@ -1272,6 +1293,32 @@ fn insert_on_conflict_4() {
);
}

#[test]
fn insert_coalesce() {
assert_eq!(Query::insert()
.into_table(Glyph::Table)
.columns([Glyph::Image, Glyph::Aspect])
.values_panic([
"04108048005887010020060000204E0180400400".into(),
Func::coalesce([Query::select()
.from(Glyph::Table)
.expr(Func::max(Expr::col(Glyph::Aspect)))
.take()
.into(),
1.into(),
Value::Bool(None).into(),
])
.into(),
])
.to_string(MysqlQueryBuilder),
[
r#"INSERT INTO `glyph` (`image`, `aspect`)"#,
r#"VALUES ('04108048005887010020060000204E0180400400', COALESCE((SELECT MAX(`aspect`) FROM `glyph`), 1, NULL))"#,
]
.join(" ")
);
}

#[test]
fn update_1() {
assert_eq!(
Expand Down
50 changes: 49 additions & 1 deletion tests/postgres/query.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
use super::*;
use pretty_assertions::assert_eq;
use sea_query::extension::postgres::PgBinOper;
use sea_query::Value;

#[test]
fn select_1() {
Expand Down Expand Up @@ -103,7 +104,7 @@ fn select_8() {
.from(Char::Table)
.left_join(
Font::Table,
Expr::col((Char::Table, Char::FontId)).equals((Font::Table, Font::Id))
Expr::col((Char::Table, Char::FontId)).equals((Font::Table, Font::Id)),
)
.to_string(PostgresQueryBuilder),
r#"SELECT "character" FROM "character" LEFT JOIN "font" ON "character"."font_id" = "font"."id""#
Expand Down Expand Up @@ -1095,6 +1096,27 @@ fn select_62() {
);
}

#[test]
fn select_coalesce() {
let query = Query::select()
.expr(Func::coalesce([
Query::select()
.from(Char::Table)
.expr(Func::count(Expr::col(Character::Id)))
.to_owned()
.into(),
1.into(),
Value::Bool(None).into(),
]))
.from(Char::Table)
.to_owned();

assert_eq!(
query.to_string(PostgresQueryBuilder),
r#"SELECT COALESCE((SELECT COUNT("id") FROM "character"), 1, NULL) FROM "character""#
);
}

#[test]
#[allow(clippy::approx_constant)]
fn insert_2() {
Expand Down Expand Up @@ -1396,6 +1418,32 @@ fn insert_returning_specific_columns() {
);
}

#[test]
fn insert_coalesce() {
assert_eq!(Query::insert()
.into_table(Glyph::Table)
.columns([Glyph::Image, Glyph::Aspect])
.values_panic([
"04108048005887010020060000204E0180400400".into(),
Func::coalesce([Query::select()
.from(Glyph::Table)
.expr(Func::max(Expr::col(Glyph::Aspect)))
.take()
.into(),
1.into(),
Value::Bool(None).into(),
])
.into(),
])
.to_string(PostgresQueryBuilder),
[
r#"INSERT INTO "glyph" ("image", "aspect")"#,
r#"VALUES ('04108048005887010020060000204E0180400400', COALESCE((SELECT MAX("aspect") FROM "glyph"), 1, NULL))"#,
]
.join(" ")
);
}

#[test]
fn update_1() {
assert_eq!(
Expand Down
47 changes: 47 additions & 0 deletions tests/sqlite/query.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1041,6 +1041,27 @@ fn cast_json_field_bin_oper() {
);
}

#[test]
fn select_coalesce() {
let query = Query::select()
.expr(Func::coalesce([
Query::select()
.from(Char::Table)
.expr(Func::max(Expr::col(Character::Id)))
.to_owned()
.into(),
1.into(),
Value::Bool(None).into(),
]))
.from(Char::Table)
.to_owned();

assert_eq!(
query.to_string(SqliteQueryBuilder),
r#"SELECT COALESCE((SELECT MAX("id") FROM "character"), 1, NULL) FROM "character""#
);
}

#[test]
#[allow(clippy::approx_constant)]
fn insert_2() {
Expand Down Expand Up @@ -1333,6 +1354,32 @@ fn insert_returning_specific_columns() {
);
}

#[test]
fn insert_coalesce() {
assert_eq!(Query::insert()
.into_table(Glyph::Table)
.columns([Glyph::Image, Glyph::Aspect])
.values_panic([
"04108048005887010020060000204E0180400400".into(),
Func::coalesce([Query::select()
.from(Glyph::Table)
.expr(Func::max(Expr::col(Glyph::Aspect)))
.take()
.into(),
1.into(),
Value::Bool(None).into(),
])
.into(),
])
.to_string(SqliteQueryBuilder),
[
r#"INSERT INTO "glyph" ("image", "aspect")"#,
r#"VALUES ('04108048005887010020060000204E0180400400', COALESCE((SELECT MAX("aspect") FROM "glyph"), 1, NULL))"#,
]
.join(" ")
);
}

#[test]
fn update_1() {
assert_eq!(
Expand Down