#23267 Warn when Orderby is silently ignored when using Distinct #24160
Conversation
This file contains 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
smitpatel
reviewed
Feb 22, 2021
test/EFCore.Specification.Tests/Query/InheritanceQueryTestBase.cs
Outdated
Show resolved
Hide resolved
smitpatel
reviewed
Feb 22, 2021
| @@ -4410,7 +4410,7 @@ public virtual Task Correlated_collections_with_Distinct(bool async) | |||
| { | |||
| 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()), | |||
There was a problem hiding this comment.
Please revert this change and apply Skip(0) after OrderBy to continue generating LeftJoin.
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
test/EFCore.Cosmos.FunctionalTests/Query/InheritanceQueryCosmosTest.cs
Outdated
Show resolved
Hide resolved
|
@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. |
|
@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
555d137
to
e19947a
Compare
smitpatel
reviewed
Mar 5, 2021
| @@ -765,6 +765,10 @@ | |||
| <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"> | |||
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 |
There was a problem hiding this comment.
DistinctAfterOrderByWithoutRowLimitingOperatorWarning
smitpatel
reviewed
Mar 12, 2021
| @@ -765,6 +765,10 @@ | |||
| <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"> | |||
| <value>The query uses the 'Distinct' operator after a 'OrderBy' operator. This may lead to unpredictable results.</value> | |||
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
| @@ -910,7 +914,7 @@ | |||
| <comment>Debug CoreEventId.RequiredAttributeOnSkipNavigation string string</comment> | |||
| </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> | |||
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
e2f012d
to
6e92cda
Compare
|
@smitpatel please review the changes in 6e92cdaf779e809954efb43c5de085d1d9bab841 |
smitpatel
approved these changes
Mar 15, 2021
|
@yesmey - Thank you for contribution. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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