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
[feature/netcore] $select executes on client side #1387
Comments
@vsopko - What backend DB are you using? |
@robward-ms Microsoft SQL Server Express (64-bit) 14.0.1000.169 on Windows 10 Pro |
@vsopko - What is ".Select(x => x);" doing for you? I think this might be forcing the evaluation of _ctx.Users before the application of the query. |
@robward-ms It's just a mark of projection experiments. Without it (var users = _ctx.Users;) SQL Server Profiler show the same query, with all columns. Anyway, i think evaluation of IQueriable occurs in Ok result, after ApplyTo method. |
@robward-ms - Have you managed to reproduce this issue? It's a large perfomance problem for our projects. |
@vsopko I took a look. From the Linq, we have the following highlight projection: I have a separate test, it seem if i remove the "Instance", it will only return the given property. |
I built a nightly with some codes changed. Would you please try it and let us know the performance? Also, I created a sample project based on the above nightly, it show me the different query. Feel free to clone and try. Please let me know your found. Thanks. |
My tests with 7.0.0-Nightly201806082048 and above configuration: Url Test 1.
Sql result: Partially correct (extra Id column) Test 2.
Sql result: Fetch all columns Then i've changed model to:
Test 3.
Sql result: Partially correct, and the same as in Test 1, but without FROM SELECT Test 4.
Sql result: Completely bypass odata Model builder was not changed
All test prodeces the same ouptput for client
|
same problem here requesting the following url: https://localhost:44356/odata/Groups(1)?$expand=Teams($select=Name) results with all entities of Team
exec sp_executesql N'SELECT [t0].[Id], [t0].[GroupId], [t0].[Name] AS [Value], [t0].[Rank] |
I have a test project named "BasicEFCoreTest" that you can find https://github.com/xuzhg/WebApiSample/tree/master/AspNetCore When i issue a request like: I can get the following logging: It seems only retrive the Would you please test on my project? or would you please share me your repro project? |
@xuzhg With your samples everything is okay. The point is that you are not using any projections in your test project. In real app we want to get
we have all columns fetched, where it looks like OData understand projected IQueryable but process only $top and $orderby clauses |
@xuzhg I put a sample project in my github please run and see the sql generated is
for
the sql is
so all properties are queried... |
@xuzhg After upgrade to 7.0.0-Nightly201806181242, SelectAllAndExpand not work.
If add $select clause work fine. |
@genusP Would you please try the latest nightly version: 7.0.0-Nightly201806192313 |
@eyalkapah I created a PR for your project. eyalkapah/OdataSample#1 For the detail information about my changes and testing, I added a |
@xuzhg 7.0.0-Nightly201806192313 work fine. SelectAll work and retrieve only need fields. |
@xuzhg with 7.0.0-Nightly201806192313, your BasicEFCoreTest app and dto simulation as desribed in my previous comment, i have this results:
Looks like now it generates incorrect SQL (with all columns) only for paging scenarios. |
@vsopko |
@xuzhg incorrect is only #_2 with $top param |
@xuzhg this build not from PR? |
@genusP this build is from the PR. But the PR is not merged, there's some test cases failed need to fix. |
@vsopko Interesting. I am testing your project, you're using my project. With your project, when i issue For my project, would you please create a PR with your changes to me to dig more? |
@xuzhg I have tested with the latest nightly and now seems like it's working good. |
@vsopko Sorry, I can't understand why you did a mapping as below: ` I have a test without using OData, just using EFCore. var aa = db.Customers.Select(e => new Customer
{
FirstName = e.FirstName,
LastName = e.LastName,
UserName = e.UserName
})
.Take(1).Select(e => new CusomterDto { FullName = e.FirstName + " " + e.LastName }); if I test as: var aa = db.Customers.Take(1).Select(e => new CusomterDto { FullName = e.FirstName + " " + e.LastName }); So, i think: Owing that you have a mapping before take, it will retrieve all the properties given in the mapping". |
@xuzhg this query without using OData (IQueryable projection, then another projection and then take first)
executes with this sql: So I guess OData in your app with my query should work this way, or am i missing something? Why I consider such tests: In our application we have a lot of dtos, that used as business objects, projected from ef core FromSql raw sql queries, OData model builder contains information about those dtos, instead of ef core context models. So we want to query those dtos with OData and evaluate produced IQueryable on sql server side. |
Why do i get the different log: var aa = db.Customers.Select(e => new Customer
{
FirstName = e.FirstName,
LastName = e.LastName,
UserName = e.UserName
})
.Select(e => new CusomterDto { FullName = e.FirstName + " " + e.LastName }).Take(1); |
@xuzhg Interesting, with last query from your new project
I have this in SQL Profiler: exec sp_executesql N'SELECT TOP(@__p_0) ([e].[FirstName] + N'' '') + [e].[LastName] AS [FullName] and this in project logs:
|
@vsopko The version referencing to EFCore is different. EFCore 2.0 vs EFCore 2.1. |
@xuzhg i have the same query result with EFCore 2.0, 2.1 and 2.1.1 |
@vsopko Sorry, please allow me to summary it:
we get different SQL as: SELECT TOP(@__p_0) ([e].[FirstName] + N' ') + [e].[LastName] AS [FullName]
FROM [Customers] AS [e] So, what i am thinking is that:
Thoughts? |
@xuzhg for OData apply Take after all Selects would resolve this issue for me. I think OData url means this: firstly select, expand, filter and then count and take top what was selected. But i'm not shure that its logically right for everyone. May be the order of $top and $select params in the url must be significant, but if i remember there was and old issue where people asks to make params order irrelevant. |
@vsopko I create an issue at EFCore side. See: dotnet/efcore#12453 Would you please help us file a new issue at: webapi/issues for OData part. |
@xuzhg what do you mean under "file a new issue"? My thoughts on the order of the $top and $select url params, or to move linq Take() at the end? For the order of params i don't know what the people need and I guess it is about OData url conventions and specifications, i believe this is up to you. For making Take() on final query i think this issue enough, isn't it? Without digging in EFCore linq composition it was simple for me - i have |
@vsopko Sorry to confusing you. What I mean a new issue is related to adjust the "Take()" because I think a new issue is easy for us to track the "Take()" order problem. However, if you think this issue is enough, I am ok. By the way, I got the following respond from EFCore team in dotnet/efcore#12453 This is by design, Take operator forces us to produce a subquery, which complicates the overall query. You should still get correct results for both queries, difference is that in first case the second projection is performed on the client. |
Great, waiting for fix, thanks a lot. Recommendations of the EFCore team are completely in the mainstream of our conversation: "In general you will get better queries if operators like Skip/Take/Distinct are used as late as possible" |
@xuzhg, nothing changes in 7.0.0-Nightly201806261242, projected query with $select and $top fetch all columns. |
@vsopko However, if you have a mapping in the controller, it will retrieve all the given properties. |
@xuzhg of course i have mapping in controller, your test app was not changed and i have mapping as explained in this comment (also was proposed as PR for your test app). Moreover, we are working here with a simplest case of organizing a sequence of projections. In our real app we've generate complex projections in external dll and everything works fine except $select $top bunch of url params. Under your words
i expected that PR, wich closes this issue, would contain a solution with above discussion. |
@vsopko I closed it accidently. |
Let’s look at the Linq expression from Web API OData, the Red number in the picture is same as the items in the “Investigation”. The final SQL execution is as follows (it retrieves “Name” along with “Age” ) Investigation:
Experiment:Only I do the following:
I can get the following Linq expression (left) and the final SQL execution (Right, see only “Age” is retrieved): Then i can get the following result: However, if I remove the “keys”, “$select=Default.MyFunction” can’t work, if I add “keys”, Property “Name” will be retrieved. |
@xuzhg those currently OData have two mutually exclusive situations to fix this issue:
What do you think to overcome this problem (if I understand it correctly) in the current state of affairs? May be to check OData url for compliance with functions defined in edm model and if there is no match move Take() to end of query? Or why not just move Take() after all selects? |
@xuzhg can be closed, thanks. |
After upgrading from Microsoft.AspNetCore.OData Version=7.0.0-beta1 to Microsoft.AspNetCore.OData Version=7.0.0-beta2 $select gets all entity properties from database and produces correct result on client side. This also occurs in nightly builds.
Assemblies affected
Microsoft.AspNetCore.OData Version=7.0.0-beta2
Microsoft.AspNetCore.OData Version=7.0.0-Nightly*
Reproduce steps
Url:
http://localhost:61734/api/values/?$select=Age&$top=1
Controller:
Context model:
Expected result
Return value:
[{"Age": 50}]
Sql profiler:
exec sp_executesql N'SELECT TOP(@__TypedProperty_0)
[x].[Age] AS [Value]
FROM [User] AS [x] N'@__TypedProperty_0 int',@__TypedProperty_0=1
Actual result
Return value:
[{"Age": 50}]
Sql profiler:
exec sp_executesql N'SELECT TOP(@__TypedProperty_0)
[x].[Id],
[x].[Age],
[x].[FirstName],
[x].[LastName],
[x].[UserName]
FROM [User] AS [x] N'@__TypedProperty_0 int',@__TypedProperty_0=1
The text was updated successfully, but these errors were encountered: