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

Add LINQ Shuffle #112173

Merged
merged 7 commits into from
Feb 6, 2025
Merged

Add LINQ Shuffle #112173

merged 7 commits into from
Feb 6, 2025

Conversation

stephentoub
Copy link
Member

Fixes #111221

Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

1 similar comment
Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@stephentoub stephentoub requested a review from Copilot February 5, 2025 17:34

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 21 out of 21 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (1)

src/libraries/System.Linq/src/System/Linq/Shuffle.SpeedOpt.cs:36

  • [nitpick] TryGetLast returns a random element instead of the last one in the sequence, which can confuse callers expecting the final element. Consider adjusting the implementation or clarifying its intended behavior.
public override TSource? TryGetLast(out bool found) => TryGetElementAt(0, out found);

Tip: Copilot code review supports C#, Go, Java, JavaScript, Markdown, Python, Ruby and TypeScript, with more languages coming soon. Learn more

Copy link
Member

@bartonjs bartonjs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reservoir collection in the Shuffle-Take iterator took some thinking about, but it seems legit to me.

- Avoid ToArray in TryGetElement, instead using SampleToList
- Rename TakeShufferIterator to ShuffleTakeIterator
- Avoid full enumeration in ShuffleTakeIterator.GetCount()
- Add distribution test
@stephentoub stephentoub requested a review from Copilot February 6, 2025 04:03

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Overview

This pull request introduces a new Shuffle extension method for both synchronous and asynchronous collections, as well as IQueryable sources. It randomizes the order of items in the sequence using Random.Shared or a ThreadStatic Random on older TFMs. Additionally, it removes the old Shuffler test utility and updates all relevant test cases and references.

  • Adds Shuffle methods to System.Linq, System.Linq.AsyncEnumerable, and System.Linq.Queryable.
  • Removes the Shuffler helper and replaces its usage in tests with the new Shuffle method.
  • Includes expanded tests ensuring correct behavior, including coverage on edge cases and randomization validation.

Changes

File Description
src/libraries/System.Linq/src/System/Linq/Shuffle.cs Defines the new Shuffle iterator for sync LINQ and checks for empty arrays.
src/libraries/System.Linq.Queryable/tests/ShuffleTests.cs Adds test coverage for IQueryable.Shuffle() usage.
src/libraries/System.Linq/src/System/Linq/Shuffle.SpeedOpt.cs Adds optimized Shuffle iterator with reservoir sampling for partial sequences.
src/libraries/System.Linq.AsyncEnumerable/tests/ShuffleTests.cs Adds test coverage for IAsyncEnumerable.Shuffle() usage.
src/libraries/System.Linq.Queryable/src/System/Linq/Queryable.cs Wires up IQueryable.Shuffle() using expression tree calls.
src/libraries/System.Linq/tests/AverageTests.cs Replaces calls to Shuffler with new Shuffle extension in Average tests.
src/libraries/System.Net.Http/tests/UnitTests/Headers/HttpHeadersTest.cs Updates a test loop to Shuffle actions on HttpRequestHeaders.
src/libraries/System.Linq/tests/MaxTests.cs Replaces Shuffler calls with new Shuffle usage in Max tests.
src/libraries/System.Linq/ref/System.Linq.cs Updates reference assembly to include the new Shuffle extension.
src/libraries/System.Linq/tests/MinTests.cs Replaces Shuffler calls with new Shuffle usage in Min tests.
src/libraries/System.Linq.AsyncEnumerable/ref/System.Linq.AsyncEnumerable.cs Updates reference assembly to include the new async Shuffle extension.
src/libraries/System.Linq/tests/ShuffleTests.cs New tests verifying Shuffle correctness, randomization, and special edge cases.
src/libraries/System.Linq.Queryable/ref/System.Linq.Queryable.cs Updates reference assembly to include the new Shuffle extension for IQueryable.
src/libraries/System.Linq.AsyncEnumerable/src/System/Linq/Shuffle.cs Implements IAsyncEnumerable Shuffle method using an internal array-based shuffle strategy.
src/libraries/System.Linq/tests/Shuffler.cs Removes the old Shuffler test helper as now replaced by Shuffle extension methods.

Copilot reviewed 21 out of 21 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (1)

src/libraries/System.Linq/src/System/Linq/Shuffle.SpeedOpt.cs:36

  • TryGetLast returns a random element rather than the last one in the randomized sequence, which may be confusing or incorrect behavior if the consumer expects the last enumerated element.
public override TSource? TryGetLast(out bool found) => TryGetElementAt(0, out found);

Tip: If you use Visual Studio Code, you can request a review from Copilot before you push from the "Source Control" tab. Learn more

@stephentoub stephentoub merged commit 4008f82 into dotnet:main Feb 6, 2025
82 of 85 checks passed
@stephentoub stephentoub deleted the addshuffle branch February 6, 2025 10:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[API Proposal]: System.Linq.Shuffle()
7 participants