Make sort order consistent in split queries #34097
Conversation
There was a problem hiding this comment.
Thanks for this PR @ascott18, and sorry it took so long to get reviewed.
This looks correct to me, and a good fix; I'm not sure why the identifier orderings were added late (after the pushdown) - hopefully that was just an oversight and not for a good reason (this area of the code was written by a team member who has left since, and it's one place where we don't have perfect expertise at the moment).
@ascott18 can you please rebase on latest main and see the comment about removing the dead warning? Otherwise I'm good with merging this for 9.0 - @maumar @cincuranet would you mind giving this a look in case I've missed something?
| @@ -24,14 +24,64 @@ public virtual void Check_all_tests_overridden() | |||
| => TestHelpers.AssertAllMethodsOverridden(GetType()); | |||
|
|
|||
| public override async Task Include_collection_skip_take_no_order_by(bool async) | |||
| => Assert.Equal( | |||
| SqlServerStrings.SplitQueryOffsetWithoutOrderBy, | |||
There was a problem hiding this comment.
Unless I'm mistaken, this warning can no longer be generated, since we ensure that the identifier is always injected as the ordering, both into the main query and into the split one (outer and subquery). So I think the warning and the visitor that generates it can now be removed.
If that's true, @ascott18 can you do that in this PR? If you prefer not to, let me know and I'll do it.
There was a problem hiding this comment.
Done!
I did also noticed that in the docs, this warning won't be applicable anymore. Not sure how warnings like that are usually handled when they only apply to some versions.
ascott18
force-pushed
the
ascott/split-query-consistent-order-9
branch
from
c3d754d
to
34a956b
Compare
|
@roji Friendly ping - is this still on track for EF 9? |
|
@ascott18 unfortunately not - EF 9 is already locked down and nothing but critical bug fixes is being merged in... My apologies that this took so long to process, the recent couple of months have simply been very busy. I'll make sure this gets merged in for 10. |
…and its occurrences in subqueries for collection includes. Fixes dotnet#26808
Co-authored-by: Shay Rojansky <roji@roji.org>
roji
force-pushed
the
ascott/split-query-consistent-order-9
branch
from
890a30c
to
0c318ac
Compare
|
@ascott18 finally merged this for 10 - thanks again for contributing this! I've also submitted dotnet/EntityFramework.Docs#4831 to amend the doc comment you referred to, thanks for that too. |
Make sort order consistent in split queries between the parent query and its occurrences in subqueries for collection includes.
Include_collection_skip_take_no_order_by,Include_collection_skip_no_order_by) due to this missing sort order in skip/take queries have been made non-throwing since the same ordering from the parent query is now applied to the subquery that lacked sorting.All the changes in the tests that add or change an
ORDER BYare such that the change makes the subquery exactly match the parent query.Since the same deterministic sorting was already being appended to the parent query, I didn't go down the paths discussed in #26808 of checking whether existing sorts on the query are already perfectly deterministic.
Fixes #26808
(Apologies for not asking for permission to work on this first - this started out as an exploration of whether I could even figure out how to approach fixing an issue like this).