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 EntityFrameworkQueryableExtensions.ToLookupAsync #26867

Open
jcracknell opened this issue Dec 1, 2021 · 8 comments
Open

Add EntityFrameworkQueryableExtensions.ToLookupAsync #26867

jcracknell opened this issue Dec 1, 2021 · 8 comments

Comments

@jcracknell
Copy link

We already have ToDictionaryAsync, however when loading associations often what you want is a lookup which you would otherwise have to construct using two separate statements:

var childLookup = await context.ChildEntities
.Where(c => Enumerable.Contains(parentIds, c.ParentId))
.ToLookupAsync(c => c.ParentId);
@ajcvickers
Copy link
Member

@jcracknell The blocker here is that the LookUp class is not publicly constructable.

@jcracknell
Copy link
Author

jcracknell commented Dec 6, 2021

I did see that when I did some preliminary investigation, however in the interim I wouldn't worry about the list allocation, as an implementation of the general form:

public static async Task<ILookup<K, A>> ToLookupAsync<A, K>(this IQueryable<A> query, Func<A, K> keySelector) {
  var list = await query.ToListAsync();
  return list.ToLookup(keySelector);
}

would at least save you having to write exactly those two statements and coming up with a name for the intermediary list. It also allows users to express intent, so you can then backtrack and fix it later.

@ajcvickers
Copy link
Member

Filed dotnet/runtime#62949 to make LookUp publicly constructable.

@ajcvickers ajcvickers added this to the Backlog milestone Dec 17, 2021
@jcracknell
Copy link
Author

I'm going to note that the issue in relation to making the Lookup constructor public is now closed on the assumption that the EF team will investigate a custom Lookup implementation.

In the interim can we consider adding my suggested list-materializing implementation and pursue optimizations later, as this is what users currently have to do anyways?

@ajcvickers
Copy link
Member

ajcvickers commented Feb 18, 2022

@jcracknell There is a reasonable expectation that an XxxAsync method behave in an async manner. An implementation that does not breaks this expectation. We believe it is preferable that people use their own code where this is apparent, rather than be caught thinking a method behaves in an async manner when it does not.

@jcracknell
Copy link
Author

Not trying to be antagonistic, but when people call an async method the expectation is that they are not spin-waiting for a database query, which is not what happens with an implementation based on ToListAsync.

In fact the only reason not to implement this per the above is if you desperately want to avoid instantiating the intermediary list. I don't care about the allocation - it's totally insignificant in comparison to the cost of running the database query - but that's me.

@ajcvickers ajcvickers removed this from the Backlog milestone Feb 18, 2022
@ajcvickers
Copy link
Member

@jcracknell You are correct. I'll bring it up again with the team.

@ajcvickers
Copy link
Member

Discussed with the team and we still feel this is better done without the additional copy.

@ajcvickers ajcvickers added this to the Backlog milestone Mar 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants