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

[API Proposal]: Make Lookup<TKey,TElement> publicly constructable #62949

Closed
ajcvickers opened this issue Dec 17, 2021 · 7 comments
Closed

[API Proposal]: Make Lookup<TKey,TElement> publicly constructable #62949

ajcvickers opened this issue Dec 17, 2021 · 7 comments
Labels
api-needs-work API needs work before it is approved, it is NOT ready for implementation area-System.Linq needs-author-action An issue or pull request that requires more info or actions from the author.

Comments

@ajcvickers
Copy link
Member

Background and motivation

This would allow EF Core to implement ToLookUpAsync in the same way we implement ToDictionaryAsync without needing to go through an intermediate data structure.

See dotnet/efcore#26867

API Proposal

Make current internal Create methods public.

    public static Lookup<TKey, TElement> Create<TSource>(
      IEnumerable<TSource> source,
      Func<TSource, TKey> keySelector,
      Func<TSource, TElement> elementSelector,
      IEqualityComparer<TKey> comparer)
    {
    }

    public static Lookup<TKey, TElement> Create(
      IEnumerable<TElement> source,
      Func<TElement, TKey> keySelector,
      IEqualityComparer<TKey> comparer)
    {
    }

API Usage

var childLookup = await context.ChildEntities
    .Where(c => Enumerable.Contains(parentIds, c.ParentId))
    .ToLookupAsync(c => c.ParentId);

Alternative Designs

No response

Risks

No response

@ajcvickers ajcvickers added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Dec 17, 2021
@dotnet-issue-labeler dotnet-issue-labeler bot added area-System.Linq untriaged New issue has not been triaged by the area owner labels Dec 17, 2021
@ghost
Copy link

ghost commented Dec 17, 2021

Tagging subscribers to this area: @dotnet/area-system-linq
See info in area-owners.md if you want to be subscribed.

Issue Details

Background and motivation

This would allow EF Core to implement ToLookUpAsync in the same way we implement ToDictionaryAsync without needing to go through an intermediate data structure.

See dotnet/efcore#26867

API Proposal

Make current internal Create methods public.

    public static Lookup<TKey, TElement> Create<TSource>(
      IEnumerable<TSource> source,
      Func<TSource, TKey> keySelector,
      Func<TSource, TElement> elementSelector,
      IEqualityComparer<TKey> comparer)
    {
    }

    public static Lookup<TKey, TElement> Create(
      IEnumerable<TElement> source,
      Func<TElement, TKey> keySelector,
      IEqualityComparer<TKey> comparer)
    {
    }

API Usage

var childLookup = await context.ChildEntities
    .Where(c => Enumerable.Contains(parentIds, c.ParentId))
    .ToLookupAsync(c => c.ParentId);

Alternative Designs

No response

Risks

No response

Author: ajcvickers
Assignees: -
Labels:

api-suggestion, area-System.Linq, untriaged

Milestone: -

@ajcvickers
Copy link
Member Author

/cc @smitpatel

@krwq krwq added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed untriaged New issue has not been triaged by the area owner labels Dec 21, 2021
@terrajobst
Copy link
Member

terrajobst commented Jan 4, 2022

Video

  • Looks good as proposed
    • We want to avoid adding conflicts with RX/IX so we don't to create extension methods like ToLookupAsync()
    • At the same time we don't want to make Lookup<TKey, TElement> mutable
    • This means we'd need a single entry point that takes an IAsyncEnumerable<TSource> and produces a Lookup<TKey, TElement>
    • Easiest seems to be factory method directly on Lookup<TKey, TElement>
  • Alternatively, EF could return a custom implementation of ILookup<TKey, TElement>, similar to PLinq
    • Sounds like we want to explore this first
namespace System.Linq
{
    public partial class Lookup<TKey, TElement>
    {
        public static Task<Lookup<TKey, TElement>> CreateAsync<TSource>(
            IAsyncEnumerable<TSource> source,
            Func<TSource, TKey> keySelector,
            Func<TSource, TElement> elementSelector,
            IEqualityComparer<TKey> comparer);
    }
}

@terrajobst terrajobst added api-needs-work API needs work before it is approved, it is NOT ready for implementation and removed api-ready-for-review API is ready for review, it is NOT ready for implementation api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Jan 4, 2022
@eiriktsarpalis
Copy link
Member

Looks good as proposed

I missed the review, but this seems inconsistent with the points made below.

It seems to me that we can close this until dotnet/efcore#26867 can explore alternative avenues, what say you @ajcvickers?

@ghost
Copy link

ghost commented Jan 10, 2022

This issue has been marked needs more info since it may be missing important information. Please refer to our contribution guidelines for tips on how to report issues effectively.

@terrajobst
Copy link
Member

Yes, the conclusion was that @smitpatel will try the custom implementation of ILookup<,> first.

@eiriktsarpalis
Copy link
Member

Closing in that case, can revisit if the need arises.

@eiriktsarpalis eiriktsarpalis added needs-author-action An issue or pull request that requires more info or actions from the author. and removed needs more info labels Jan 19, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Feb 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-needs-work API needs work before it is approved, it is NOT ready for implementation area-System.Linq needs-author-action An issue or pull request that requires more info or actions from the author.
Projects
None yet
Development

No branches or pull requests

4 participants