GeezerVison

Ilya (@iSazonov) Why did you add the Breaking-Change label? Because the Top parameter was moved to a separate parameter set (which would invalidate code someone had written specifically against the Default parameter set if they expected Top to be in there)? Or is there a functional breaking change you see here that I don't?

Kirk Munro (@KirkMunro) Formally it is a breaking change because (1) a parameter set is changed, (2) sort order is changed and some scripts can fail. I believe we should inform users in change log about this. Also we need PowerShell Committee approvement.

Kirk Munro (@KirkMunro) Please update Markdig.Signed package version 0.15.2 -> 0.15.3 to pass CI tests.

Ilya (@iSazonov) I updated the 3 packages that had updates available.

On the breaking change comments, where you say "sort order is changed and some scripts may fail", the sort order has not changed with this PR. A new option has been added to allow users to choose a stable sort, where the order of duplicate items is preserved through the sort. That addition should not have any impact whatsoever over the sort order of existing sorts, because you only get that functionality by adding the new -Stable switch. For yourself and other readers, the reason this was done is because Top/Bottom sorts are implicitly stable, so there is no need to have a -Stable switch on those parameter sets. The behavior of the default parameter set has not changed either. The only "breaking" change is if you use -Top, you're invoking a different parameter set, that's all. The only people who might run into that as a breaking change are teams building tooling around PowerShell where they explicitly look for a Top parameter in the Default parameter set, if any such teams exist.

Anyhow, I agree with the committee review to decide if change log details are necessary for this.

Kirk Munro (@KirkMunro) Thanks for clarify. Please update the PR description about the "breaking" change.

Now we are waiting PowerShell Committee conclusion.

Steve Lee (@SteveL-MSFT) we need to make sure this breaking change gets reviewed by PS Committee...
Review - Committee label was added almost a month ago.

Andrew (@anmenaga) yes, this is in the queue for review but we have so many items now it's taking a bit longer

Kirk Munro (@KirkMunro) Please resolve merge conflicts.

Change looks good, awaiting review from committee.

Docs issue MicrosoftDocs/PowerShell-Docs#3021

Bruce Payette (@BrucePay) friendly ping. This PR needs your review.

Kirk Munro (@KirkMunro) Andrew (@anmenaga) My main question is this: do we really need a custom implementation of a heap sort? LINQ OrderBy and OrderByDescrnding are stable sorts. Why not just use them? It would be a lot less code to maintain.

There has been a significant performance increase in # 2518 + support Top/Bottom.
There is open issues in CoreFX for new API that can replace the code.

Bruce Payette (@BrucePay) That question has nothing to do with this PR. This PR is to correct issue #7846, while also allowing full stable sorts using the same algorithm. The sorting algorithm isn't new here. It was added for PowerShell 6.0 in PR #2518 to greatly improve Sort-Object performance issues while adding -Top and -Bottom to Sort-Object.

To get the answer to your question, I think it would be better to open another issue requesting a comparison between LINQ's OrderBy and OrderByDescending and the existing indexed min-/max-heap sort algorithm, which should include an in depth analysis of results to ensure sorting does not change as well as a thorough performance analysis. Either that or compare with the CoreFX issue that Ilya (@iSazonov) just mentioned (Ilya, can you provide a link to that please?).

Waiting PowerShell Committee review.

@PowerShell/powershell-committee approves of the changes

This PR has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed if no further activity occurs within 10 days.
Thank you for your contributions.
Community members are welcome to grab these works.

Steve Lee (@SteveL-MSFT) What's the status on this PR?

Kirk Munro (@KirkMunro) Can you resolve the merge conflict?

Sean Wheeler (@sdwheeler) Kirk Munro (@KirkMunro) I believe the changes are approved by committee and individuals, so once the merge conflict is resolved this can be merged

Kirk Munro (@KirkMunro) Fixed the merge conflict.

Kirk Munro (@KirkMunro) Thank you for your contribution!

Thanks Aditya Patwardhan (@adityapatwardhan), for fixing the merge conflict and merging the PR. I missed the earlier comments about the merge conflict.