#23267 Warn when Orderby is silently ignored when using Distinct#24160
Merged
Conversation
smitpatel
reviewed
Feb 22, 2021
smitpatel
reviewed
Feb 22, 2021
| return AssertQuery( | ||
| async, | ||
| ss => ss.Set<Squad>().OrderBy(s => s.Name).Select(s => s.Members.OrderBy(m => m.Nickname).Distinct()), | ||
| ss => ss.Set<Squad>().OrderBy(s => s.Name).Select(s => s.Members.Distinct().OrderBy(m => m.Nickname).ToList()), |
Contributor
There was a problem hiding this comment.
Please revert this change and apply Skip(0) after OrderBy to continue generating LeftJoin.
Contributor
Author
There was a problem hiding this comment.
It still generates a OUTER APPLY after the change. Any idea how to solve it it?
smitpatel
reviewed
Feb 22, 2021
Contributor
|
@yesmey - Please hold on making any changes to PR. This doesn't feel exactly the warning we want to show. We will discuss with team what exactly we want to implement. Please monitor the tracking issue for any updates. |
Contributor
|
@yesmey - Please refer to design meeting notes on original issue and update the PR accordingly. |
yesmey
force-pushed
the
orderby-before-distinct
branch
2 times, most recently
from
March 4, 2021 15:57
555d137 to
e19947a
Compare
smitpatel
reviewed
Mar 5, 2021
| <value>The query uses the 'First'/'FirstOrDefault' operator without 'OrderBy' and filter operators. This may lead to unpredictable results.</value> | ||
| <comment>Warning CoreEventId.FirstWithoutOrderByAndFilterWarning</comment> | ||
| </data> | ||
| <data name="LogDistinctAfterOrderBy" xml:space="preserve"> |
Contributor
There was a problem hiding this comment.
@dotnet/efteam - For exception message review
smitpatel
reviewed
Mar 12, 2021
| /// This event is in the <see cref="DbLoggerCategory.Query" /> category. | ||
| /// </para> | ||
| /// </summary> | ||
| public static readonly EventId DistinctAfterOrderByWarning |
Contributor
There was a problem hiding this comment.
DistinctAfterOrderByWithoutRowLimitingOperatorWarning
smitpatel
reviewed
Mar 12, 2021
| <comment>Warning CoreEventId.FirstWithoutOrderByAndFilterWarning</comment> | ||
| </data> | ||
| <data name="LogDistinctAfterOrderBy" xml:space="preserve"> | ||
| <value>The query uses the 'Distinct' operator after a 'OrderBy' operator. This may lead to unpredictable results.</value> |
Contributor
There was a problem hiding this comment.
Suggested change
| <value>The query uses the 'Distinct' operator after a 'OrderBy' operator. This may lead to unpredictable results.</value> | |
| <value>The query uses the 'Distinct' operator after applying an ordering. If there are any row limiting operation used before `Distinct` and after ordering then ordering will be used for it. Ordering(s) will be erased after `Distinct` and results afterwards would be unordered.</value> |
smitpatel
reviewed
Mar 12, 2021
| </data> | ||
| <data name="LogRowLimitingOperationWithoutOrderBy" xml:space="preserve"> | ||
| <value>The query uses a row limiting operator ('Skip'/'Take') without an 'OrderBy' operator. This may lead to unpredictable results.</value> | ||
| <value>The query uses a row limiting operator ('Skip'/'Take') without an 'OrderBy' operator. This may lead to unpredictable results. If the 'Distinct' operator is used, then make sure to use the 'OrderBy' operator after 'Distinct' as the order would otherwise get erased.</value> |
Contributor
There was a problem hiding this comment.
Suggested change
| <value>The query uses a row limiting operator ('Skip'/'Take') without an 'OrderBy' operator. This may lead to unpredictable results. If the 'Distinct' operator is used, then make sure to use the 'OrderBy' operator after 'Distinct' as the order would otherwise get erased.</value> | |
| <value>The query uses a row limiting operator ('Skip'/'Take') without an 'OrderBy' operator. This may lead to unpredictable results. If the 'Distinct' operator is used after `OrderBy`, then make sure to use the 'OrderBy' operator after 'Distinct' as the ordering would otherwise get erased.</value> |
Revert subquery_oftype test modifications. Only warn when Limit/Offset is not specified Update text in LogRowLimitingOperationWithoutOrderBy
yesmey
force-pushed
the
orderby-before-distinct
branch
from
March 14, 2021 11:39
e2f012d to
6e92cda
Compare
Contributor
Author
|
@smitpatel please review the changes in 6e92cdaf779e809954efb43c5de085d1d9bab841 |
smitpatel
approved these changes
Mar 15, 2021
Contributor
|
@yesmey - Thank you for contribution. |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Implement a warning for when using OrderBy before Distinct. The OrderBy will silently get ignored or cause unintentional behavior.
The reason and design behind this change can be read in #23267
In simple terms, the definition of Enumerable.Distinct() is that it returns an unordered result...
... so when using a Distinct after a OrderBy - the previously specified order will get ignored