-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Make BoolExpressionMethods not always return a Nullable<bool> #2597
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
Merged
Merged
Changes from 6 commits
Commits
Show all changes
12 commits
Select commit
Hold shift + click to select a range
09f83eb
Make BoolExpressionMethods not always return a nullable bool
Ten0 21fb9c5
Revert impl `AsExpression<Nullable<T>> for Expression<SqlType=T>`
Ten0 0ed7566
Use default generic type for And and get rid of unnecessary type aliases
Ten0 9f4d83c
Forgotten NOT operator declaration revert
Ten0 564e68e
fix compile test
Ten0 3486fc3
revert to simpler impl AsExpression<Nullable<ST>> for Option<T>
Ten0 0ec6a0c
fix fmt
Ten0 4267f2b
add missing markdown codeblock close on `and` and `or`
Ten0 df8b5e5
Make prefix_operator return types depend on the type of the original
Ten0 367d7c0
Merge remote-tracking branch 'upstream/master' into fix_2589
Ten0 d106e4c
accept trybuild diffs (after merge introduced trybuild)
Ten0 203be0d
Fix diesel_tests/query_to_sql_equality after master merge
Ten0 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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'm not sure if that should only accept non nullable values. I think accepting both
BoolandNullable<Bool>here is more meaningful. On the other hand that could potentially lead to situations where rustc cannot infer the correct type forSTfromAsExpressionhere.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.
Hmm that looks like kind of the same problem as
and/orno? Where if we enforce that it takes anAsExpression<Nullable<Bool>>we'll never be able to make it return a non-nullable bool?Additionally, not all expressions of SqlType
BoolimplementAsExpression<Nullable<Bool>>anymore, because otherwise this creates type inference issues withand/or: #2597 (comment)Hence the following does not compile anymore if we do this:
not(name.eq("Sean")(from tests)Finally, I believe the behavior of
notonNullable<Bool>is a bit of a trap in thatNULLvalues result inNULL, so they do not change "truthiness" with regards to e.g. afilter, which may lead to nasty bugs in case this kind of confusion is made and the compiler does not prevent it.So overall I believe we should keep this behavior where we only accept non-nullable values here.
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 had encountered a situation where I want to store the same. Is there a way we can specify that?
Uh oh!
There was an error while loading. Please reload this page.
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.
We must accept both variants (
BoolandNullable<Bool>) here otherwise it's not possible to write queries likenot(foo.and(bar))or evennot(foo.eq(bar))anymore in cases one of the arguments isNullable<Bool>Edit: @pksunkara Seems like you've added your comment while I typed mine. Yes we need this method to automatically accept both variants, otherwise we would restrict quite a lot of possible queries using
not.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.
Not sure what you mean by "the same".
So I thought it maybe wasn't a good idea to have
notsupport this implicitly, as this may easily lead to nasty bugs:Example:
Where
foo=trueandbar=NULL, the lines won't show, becausenotwon't change the "truthiness" of the expression, which is a bit counter-intuitive.Also, our codebase is rather large and we have zero such patterns.
Given that, do you still believe it's important for
notto support this? (and we couldn't be happy with a separatenullable_not?)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 still believe that
notshould support that as this is just the behavior of the underlying SQL operator. I feel that we had this discussion already about a few operators (for exampleeqin another context). Every time the final answer was: We follow the semantic of the underlying SQL operator as we do not try to hide complexity at this level of abstraction. Hiding this complexity can be done in a crate build on top of diesel, providing it owns set of operators that is not based on SQL in my opinion.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 agree. Done in df8b5e5.