Skip to content

issues-118 Add SubQuery operator#379

Merged
tyt2y3 merged 6 commits intoSeaQL:masterfrom
ikrivosheev:feature/issues-118_exists
Sep 17, 2022
Merged

issues-118 Add SubQuery operator#379
tyt2y3 merged 6 commits intoSeaQL:masterfrom
ikrivosheev:feature/issues-118_exists

Conversation

@ikrivosheev
Copy link
Copy Markdown
Contributor

@ikrivosheev ikrivosheev commented Jul 4, 2022 •
edited
Loading

PR Info

Adds

  • SubQuery operators: EXISTS, ALL, ANY, SOME

@ikrivosheev ikrivosheev requested review from billy1624 and tyt2y3 July 4, 2022 17:48
@ikrivosheev ikrivosheev force-pushed the feature/issues-118_exists branch from 9688f49 to 80e82d4 Compare July 7, 2022 15:01
@ikrivosheev
Copy link
Copy Markdown
Contributor Author

ikrivosheev commented Jul 17, 2022 •
edited
Loading

@billy1624 @tyt2y3 can you review?) I have done.

This was referenced Aug 12, 2022
Closed
Closed
billy1624
billy1624 requested changes Aug 18, 2022
View reviewed changes
Copy link
Copy Markdown
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, thanks!!

Based on the docs and experiment result. Seems that SQLite only support EXISTS. The other three ANY, SOME and ALL are not supported.

Also, it'd be best if we have doc test for subquery operators to be used in condition. Such as...

SELECT s1 FROM t1 WHERE s1 > ANY (SELECT s1 FROM t2);
SELECT s1 FROM t1 WHERE s1 <> ANY  (SELECT s1 FROM t2);
SELECT s1 FROM t1 WHERE s1 <> SOME (SELECT s1 FROM t2);

@ikrivosheev
Copy link
Copy Markdown
Contributor Author

ikrivosheev commented Aug 20, 2022 •
edited
Loading

@billy1624 @tyt2y3 hello! I have small question. Expr::ne have signature: pub fn ne<V: Into<Value>>(self, v: V) -> SimpleExpr. If i rewrite this method to: pub fn ne<V: Into<SimpleExpr>>(self, v: V) -> SimpleExpr will not be so convenient to use for simple queries and it is breaking change.
Should I add new method?

What is the best way to name new methods? For example:

  1. ne_expr?
  2. expr_ne?
  3. Something else?)

@ikrivosheev ikrivosheev requested a review from billy1624 August 20, 2022 15:35
@billy1624
Copy link
Copy Markdown
Member

billy1624 commented Aug 29, 2022

Hey @ikrivosheev, good suggestions!! I do think we need eq_expr and ne_expr inside Expr. That's definitely nice to have.

@ikrivosheev
Copy link
Copy Markdown
Contributor Author

ikrivosheev commented Aug 31, 2022

Hey @ikrivosheev, good suggestions!! I do think we need eq_expr and ne_expr inside Expr. That's definitely nice to have.

@billy1624 hello! Done

billy1624
billy1624 approved these changes Sep 2, 2022
View reviewed changes
Copy link
Copy Markdown
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.

Thanks!! @ikrivosheev

@ikrivosheev
Copy link
Copy Markdown
Contributor Author

ikrivosheev commented Sep 2, 2022

Hmm, @billy1624, I think about new method... Maybe is better impl From<T> for SimpleExpr, where T is all types for Value? And after that we don`t need new method. What do you think?

@ikrivosheev
Copy link
Copy Markdown
Contributor Author

ikrivosheev commented Sep 7, 2022

Hmm, @billy1624, I think about new method... Maybe is better impl From<T> for SimpleExpr, where T is all types for Value? And after that we don`t need new method. What do you think?

@tyt2y3 what do you think about it?

@ikrivosheev ikrivosheev force-pushed the feature/issues-118_exists branch from c9da3a1 to fb8bb71 Compare September 8, 2022 18:19
@ikrivosheev
Copy link
Copy Markdown
Contributor Author

ikrivosheev commented Sep 8, 2022

@billy1624 @tyt2y3 done! I add impl<T> From<T> for SimpleExpr and everything works without the new method!

@ikrivosheev
Copy link
Copy Markdown
Contributor Author

ikrivosheev commented Sep 14, 2022

@billy1624 @tyt2y3 can you review this PR?

tyt2y3
tyt2y3 reviewed Sep 17, 2022
View reviewed changes
src/expr.rs
Comment on lines +2265 to +2267
impl<T> From<T> for SimpleExpr
where
T: Into<Value>,
Copy link
Copy Markdown
Member

@tyt2y3 tyt2y3 Sep 17, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Interesting... I was thinking of the potential drawbacks of this blanket impl but could not think of one.
This should improve ergonomics a lot in the end.

Copy link
Copy Markdown
Member

@billy1624 billy1624 Sep 20, 2022

Choose a reason for hiding this comment

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

This is amazing!

@tyt2y3 tyt2y3 merged commit 7f31ee8 into SeaQL:master Sep 17, 2022
@ikrivosheev ikrivosheev deleted the feature/issues-118_exists branch September 17, 2022 09:36
@ikrivosheev ikrivosheev mentioned this pull request Sep 20, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tyt2y3 tyt2y3 tyt2y3 approved these changes

@billy1624 billy1624 Awaiting requested review from billy1624

Assignees

@ikrivosheev ikrivosheev

Labels

None yet

Projects

SeaQL Dev Tracker
Archived in project

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Support EXISTS (subquery) operator

3 participants

@ikrivosheev @billy1624 @tyt2y3