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

Jump to bottom

Function::Coalesce not support the coexistence of SimpleExpr and QueryStatement #516

Open
bingryan opened this issue Nov 9, 2022 · 3 comments · May be fixed by #572
Open

Function::Coalesce not support the coexistence of SimpleExpr and QueryStatement #516

bingryan opened this issue Nov 9, 2022 · 3 comments · May be fixed by #572
Assignees
@ikrivosheev
Labels
good first issue Good for newcomers
Milestone
0.29.x

Comments

@bingryan
Copy link

bingryan commented Nov 9, 2022 •
edited
Loading

Test language=MySQL

Description

I found it difficult to handle the logic that Function::Coalesce supports the coexistence of SimpleExpr and QueryStatement

The following logic is correct

create table bakery (
	id bigint,
	name varchar(255),
	profit_margin	bigint
)


create table cake{
	id bigint,
	name varchar(255),
	price bigint,
	bakery_id bigint,
	gluten_free bigint,
	serial bigint
}

mysql query:

select
	name,
	bakery,
	EXISTS ( SELECT 1 FROM `cake` WHERE `id` = 1 ) AS `exist`,
	-- Coalesce not only support SimpleExpr, logic is correct
	COALESCE (
		-- QueryStatement
		( SELECT count(*) FROM `cake` WHERE `bakery_id` = 27 ), 
		-- SimpleExpr, select or const 
		(SELECT 0),
		0 
	) AS `count` 
from bakery
where id = 27

but the existing logic does not support the coexistence of SimpleExpr and QueryStatement

sea-query/src/func.rs

Lines 430 to 435 in 28cf151

pub fn coalesce<I>(args: I) -> FunctionCall
where
I: IntoIterator<Item = SimpleExpr>,
{
FunctionCall::new(Function::Coalesce).args(args)
}

@bingryan bingryan changed the title Function::Coalesce not only support SimpleExpr Function::Coalesce not support the coexistence of SimpleExpr and QueryStatement Nov 9, 2022
@billy1624
Copy link
Member

Hey @bingryan, thanks for the suggestions!! I think we can make Func::coalesce takes iterator of Into<SimpleExpr>.

impl Func {
    pub fn coalesce<I, T>(args: I) -> FunctionCall
    where
        I: IntoIterator<Item = T>,
        T: Into<SimpleExpr>,
    {
        FunctionCall::new(Function::Coalesce).args(args)
    }
}

Which also requires changing

impl FunctionCall {
    pub fn args<I, T>(mut self, args: I) -> Self
    where
        I: IntoIterator<Item = T>,
        T: Into<SimpleExpr>,
    {
        self.args = args.into_iter().map(Into::into).collect();
        self
    }
}

And implementing From<SelectStatement> for SimpleExpr

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

Sounds like a good plan? @ikrivosheev

@ikrivosheev
Copy link
Member

@billy1624 I think we need only last one:

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

And should work. We cannot do:

impl Func {
    pub fn coalesce<I, T>(args: I) -> FunctionCall
    where
        I: IntoIterator<Item = T>,
        T: Into<SimpleExpr>,
    {
        FunctionCall::new(Function::Coalesce).args(args)
    }
}

because after this we cannot do: Func::coalesce(Query::select(), 1).

@billy1624
Copy link
Member

Right. User still need Into::into anyways

Func::coalesce(vec![Query::select().into(), 1.into()])

@ikrivosheev ikrivosheev added this to the 0.28.x milestone Nov 25, 2022
@ikrivosheev ikrivosheev added the good first issue Good for newcomers label Nov 29, 2022
@ikrivosheev ikrivosheev modified the milestones: 0.28.x, 0.29.x Dec 13, 2022
@ikrivosheev ikrivosheev self-assigned this Dec 13, 2022
alphavector added a commit to alphavector/sea-query that referenced this issue Jan 2, 2023
@alphavector
SeaQLgh-516: support coexistence of SimpleExpr and QueryStatement
f8a2cfa
@alphavector alphavector linked a pull request Jan 2, 2023 that will close this issue
Open
alphavector added a commit to alphavector/sea-query that referenced this issue Jan 3, 2023
@alphavector
SeaQLgh-516: fix formatting
6dc6fc9
alphavector added a commit to alphavector/sea-query that referenced this issue Feb 25, 2023
@alphavector
SeaQLgh-516: call .take() instead of .to_owned()
bdd64a0
alphavector added a commit to alphavector/sea-query that referenced this issue Apr 27, 2023
@alphavector
SeaQLgh-516: use into_sub_query_expr instead of From
e5f0b64
alphavector added a commit to alphavector/sea-query that referenced this issue Apr 27, 2023
@alphavector
SeaQLgh-516: minor fix
e88fc4f
alphavector added a commit to alphavector/sea-query that referenced this issue Apr 27, 2023
@alphavector
SeaQLgh-516: reorder the tests
0c79402
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@ikrivosheev ikrivosheev

Labels
good first issue Good for newcomers
Projects
SeaQL Dev Tracker
Status: Triage
Milestone
0.29.x
Development

Successfully merging a pull request may close this issue.

gh-516: support coexistence of SimpleExpr and QueryStatement alphavector/sea-query
3 participants
@ikrivosheev @billy1624 @bingryan