Skip to content
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

Suggestion. Use a stable sorting algorithm #25228

Closed
mikhail-khalizev opened this issue Jul 9, 2021 · 3 comments · Fixed by #25874
Closed

Suggestion. Use a stable sorting algorithm #25228

mikhail-khalizev opened this issue Jul 9, 2021 · 3 comments · Fixed by #25874
Labels
area-save-changes closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. customer-reported type-bug
Milestone

Comments

@mikhail-khalizev
Copy link

Suggestion. Use a stable sorting algorithm when forming the sequence of command for insert and updating entities.

https://github.com/dotnet/efcore/blob/main/src/EFCore.Relational/Update/Internal/CommandBatchPreparer.cs#L81

- independentCommandSet.Sort(_modificationCommandComparer);
-
- var batch = _modificationCommandBatchFactory.Create();
- foreach (var modificationCommand in independentCommandSet)
+ var independentCommandSetSorted = independentCommandSet.OrderBy(x => x, _modificationCommandComparer);
+
+ var batch = _modificationCommandBatchFactory.Create();
+ foreach (var modificationCommand in independentCommandSetSorted)
@mikhail-khalizev
Copy link
Author

This change allows to solve #11686

@AndriySvyryd
Copy link
Member

AndriySvyryd commented Jul 12, 2021

Did you perform any testing that would show that this change would make any difference?

However, even if it would, the order in which the modification commands are in after the topological sort doesn't necessarily reflect the order of the items in the collection navigations.

Instead https://github.com/dotnet/efcore/blob/ac2bb48b10ecf1289b568a94b7a35e8075c6d787/src/EFCore.Relational/Update/Internal/ModificationCommandComparer.cs should be changed to compare keys on inserted entities too. This will reflect the order in which they were added to the change tracker, which should be closer to what is expected in #11686

@mikhail-khalizev
Copy link
Author

Yes, I did some testing, this change works.

But your approach (change ModificationCommandComparer) will be better.

@ajcvickers ajcvickers added this to the 6.0.0 milestone Jul 13, 2021
@AndriySvyryd AndriySvyryd removed their assignment Sep 4, 2021
@AndriySvyryd AndriySvyryd added the closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. label Sep 4, 2021
AndriySvyryd added a commit that referenced this issue Sep 4, 2021
Always sort deletes before updates and updates before inserts for the same table.

Fixes #14371
Fixes #15180
Fixes #25228
AndriySvyryd added a commit that referenced this issue Sep 4, 2021
AndriySvyryd added a commit that referenced this issue Sep 4, 2021
AndriySvyryd added a commit that referenced this issue Sep 7, 2021
@ajcvickers ajcvickers modified the milestones: 6.0.0, 6.0.0-rc2 Sep 8, 2021
@ajcvickers ajcvickers modified the milestones: 6.0.0-rc2, 6.0.0 Nov 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-save-changes closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. customer-reported type-bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants