-
-
Notifications
You must be signed in to change notification settings - Fork 194
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
issues-485 Fix syntax error #505
issues-485 Fix syntax error #505
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should add a test with the negation of an empty any/all?
tests/mysql/query.rs
Outdated
|
||
assert_eq!(statement, r#"SELECT `id` FROM `glyph`"#); | ||
assert_eq!(statement, r#"SELECT `id` FROM `glyph` WHERE FALSE"#); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there's some unfortunate interaction between 2 features here:
- Collapsing an empty any/all into a boolean
- Not adding an any/all that's empty.
This should be any(TRUE, FALSE)
, so TRUE
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is reasonable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update: I am trying to understand why it outputs FALSE
here:
I imagine the expected output to be:
SELECT `id` FROM `glyph` WHERE TRUE OR FALSE
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah found it
Agreed. Actually, I think we should remove the behaviour |
Just got bitten by this bug again, with an |
b6f44db
to
8a6448a
Compare
@nitnelave Can you go over this again and may be add some tests as you see fit? |
@tyt2y3 I think, we can compress conditional when we add it. |
I imagine |
I'm of the opinion that the query builder should not try to be too smart, since that's the role of the DB. They'll have no problem with an extra "OR FALSE" in the query. |
@nitnelave @tyt2y3 ok. |
c4cb627
to
993a174
Compare
PR Info
Bug Fixes
Condition