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

[join-lateral] #224

Merged
merged 1 commit into from
Jan 16, 2022
Merged

[join-lateral] #224

merged 1 commit into from
Jan 16, 2022

Conversation

rex-remind101
Copy link
Contributor

added support for JOIN LATERAL as specified in MySQL and Postgres #201

Comment on lines +1098 to +1101
/// assert_eq!(
/// query.to_string(SqliteQueryBuilder),
/// r#"SELECT `name` FROM `font` LEFT JOIN LATERAL (SELECT `id` FROM `glyph`) AS `sub_glyph` ON `font`.`id` = `sub_glyph`.`id`"#
/// );
Copy link
Member

Choose a reason for hiding this comment

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

JOIN LATERAL seems not supported by SQLite. We shouldn't generate invalid select statement

Copy link
Contributor Author

Choose a reason for hiding this comment

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

how do i error on sqlite within the builder?

Copy link
Member

Choose a reason for hiding this comment

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

We could panic! when building the select statement for SQLite. Is this be reasonable? @tyt2y3 It's better to let the user know explicitly something went wrong early.

Comment on lines +1120 to +1129
pub fn join_lateral_subquery<T, C>(
&mut self,
join: JoinType,
query: SelectStatement,
alias: T,
condition: C,
) -> &mut Self
where
T: IntoIden,
C: IntoCondition,
Copy link
Member

Choose a reason for hiding this comment

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

I'm thinking this method could simply be named join_lateral. As it's assumed to take a (sub)query as argument. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yup, that makes sense 👍

@tyt2y3 tyt2y3 changed the base branch from master to join-lateral January 16, 2022 13:35
@tyt2y3
Copy link
Member

tyt2y3 commented Jan 16, 2022

I think the lateral should be made part of JoinType though.
Edit: after reading the MySQL docs I think you are right.

@tyt2y3 tyt2y3 merged commit 12c1292 into SeaQL:join-lateral Jan 16, 2022
@rex-remind101
Copy link
Contributor Author

I noticed this was merged into it's own branch. Has any further work been done on it? Should I make another pr with those changes or is there a different direction we're heading in?

@rex-remind101 rex-remind101 deleted the join-lateral branch January 18, 2022 18:02
@rex-remind101
Copy link
Contributor Author

@billy1624 hello, any thoughts?

@billy1624
Copy link
Member

I noticed this was merged into it's own branch. Has any further work been done on it? Should I make another pr with those changes or is there a different direction we're heading in?

It was later cherry picked into master, see 12c1292

@rex-remind101
Copy link
Contributor Author

thank you, do i need to follow up with the above adjustments you suggested? if so, i'm still not sure where to insert the panic! for slqite, thanks

@billy1624
Copy link
Member

thank you, do i need to follow up with the above adjustments you suggested? if so, i'm still not sure where to insert the panic! for slqite, thanks

I can open a PR for that :P

@billy1624
Copy link
Member

thank you, do i need to follow up with the above adjustments you suggested? if so, i'm still not sure where to insert the panic! for slqite, thanks

Hey @rex-remind101, I have open an PR (#243) for this

@billy1624 billy1624 linked an issue Mar 14, 2022 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No Left Join Lateral
3 participants