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

Add is / is_not to Expr #348

Merged
merged 3 commits into from
Jun 27, 2022
Merged

Add is / is_not to Expr #348

merged 3 commits into from
Jun 27, 2022

Conversation

Sytten
Copy link
Contributor

@Sytten Sytten commented Jun 7, 2022

For Sqlite, they are the same as eq and neq but treat NULL differently.
I put them behind a flag since I don't think other DBMS allow random values with those operators.

Adds

  • IS and IS NOT operators for Sqlite

@ikrivosheev
Copy link
Member

ikrivosheev commented Jun 7, 2022

@Sytten thank you for the PR!
I don't understand... You add a doc test for Postgres backend and for MySQL backend but hide is under the feature flag: backend-sqlite. How should this work?

@Sytten
Copy link
Contributor Author

Sytten commented Jun 7, 2022

Right I will remove it, I did a stupid copy paste

@ikrivosheev
Copy link
Member

ikrivosheev commented Jun 8, 2022

@Sytten as I know in postgresql we can use IS TRUE for example. Also we didnot want to add feature flag to Expr.

@tyt2y3
Copy link
Member

tyt2y3 commented Jun 8, 2022

@Sytten as I know in postgresql we can use IS TRUE for example. Also we didnot want to add feature flag to Expr.

Agreed

@Sytten
Copy link
Contributor Author

Sytten commented Jun 8, 2022

So how do we wish to proceed? Just leave it open for any backend?

@tyt2y3
Copy link
Member

tyt2y3 commented Jun 9, 2022

Yes. We don't feature guard the frontend (unless a feature is only relevant to one backend), because it's only determined when building the query.

@Sytten
Copy link
Contributor Author

Sytten commented Jun 13, 2022

@tyt2y3 @ikrivosheev I made the changes

Copy link
Member

@ikrivosheev ikrivosheev left a comment

Choose a reason for hiding this comment

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

@Sytten thank you! LGTM!

@ikrivosheev ikrivosheev requested review from tyt2y3 and billy1624 June 14, 2022 07:44
@tyt2y3 tyt2y3 merged commit 1877ef2 into SeaQL:master Jun 27, 2022
@tyt2y3
Copy link
Member

tyt2y3 commented Jun 27, 2022

Thank you all

tyt2y3 pushed a commit that referenced this pull request Jun 27, 2022
* Add is / is_not to Expr

* Change example and remove sqlite limitation

* Change more examples
@Sytten Sytten deleted the feat/is_is_not branch July 13, 2023 14:02
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.

3 participants