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

Query generation issues when using ODATA #2092

Closed
djkunkel opened this issue Feb 13, 2020 · 8 comments · Fixed by #2102
Closed

Query generation issues when using ODATA #2092

djkunkel opened this issue Feb 13, 2020 · 8 comments · Fixed by #2102
Assignees
Labels
area: linq status: has-pr There is active PR for issue type: bug
Milestone

Comments

@djkunkel
Copy link

djkunkel commented Feb 13, 2020

Worked through a few more of my ODATA examples, I've discovered a few further issues with query generation when using Microsoft ODATA WebApi (https://github.com/OData/WebApi) for .net core 3.1. I'm using a build from commit 8cd544b for linq2db in the master branch with the fix for #2070

Sample project here:
https://github.com/djkunkel/linq2db-aggregate-odata-sample

The following requests fails to generate SQL:

Without orderby:

/odata/People?$apply=groupby((Title),aggregate(YearsExperience with countdistinct as Test))

With orderby:

/odata/People?$apply=groupby((Title),aggregate(YearsExperience with sum as TotalExperience))&$orderby=TotalExperience
/odata/People?$apply=groupby((Title),aggregate(YearsExperience with min as Test))&$orderby=Test
/odata/People?$apply=groupby((Title),aggregate(YearsExperience with max as Test))&$orderby=Test
/odata/People?$apply=groupby((Title),aggregate(YearsExperience with average as Test))&$orderby=Test
/odata/People?$apply=groupby((Title),aggregate(YearsExperience with countdistinct as Test))&$orderby=Test

The following cases do work for reference:

/odata/People?$apply=groupby((Title),aggregate(YearsExperience with sum as Test))
/odata/People?$apply=groupby((Title),aggregate(YearsExperience with average as Test))
/odata/People?$apply=groupby((Title),aggregate(YearsExperience with min as Test))
/odata/People?$apply=groupby((Title),aggregate(YearsExperience with max as Test))
/odata/People?$apply=groupby((Title),aggregate($count as NumPeople))&$orderby=NumPeople
/odata/People?$apply=groupby((Title),aggregate($count as NumPeople))&$count=true
/odata/People?$apply=filter(Title eq 'Engineer' or Title eq 'QA')/groupby((Title),aggregate($count as NumPeople))&$count=true
/odata/People?$apply=groupby((Office/Name),aggregate($count as NumPeople))&$count=true
/odata/People?$apply=filter(Title eq 'QA')/groupby((Office/Id,Office/Name),aggregate($count as NumPeople))&$count=true&$orderby=NumPeople desc
/odata/people?$expand=Office

Thank you for this project again!

@sdanyliv
Copy link
Member

Thanks for the cases! It will be definitely easy to fix them all in one batch.

Off-top:
I'm pretty sure you have tested that with EF Core. What is the difference? Generated SQL, missing functionality? Other cases?
Just interesting :) Sometimes we need motivation and choose right direction.

@djkunkel
Copy link
Author

EF Core 3.1 fails to execute the aggregates at all. Using EF Core 2.2 it can't generate SQL but it silently loads the whole table and processes the aggregation in memory.

OData/WebApi#2019
OData/WebApi#1413

@sdanyliv
Copy link
Member

sdanyliv commented Feb 13, 2020

I see, one team creates something not compatible with others. But I saw in OData code hacks for each EF version. Workarounds can not help if there are no test cases on EF side.

Anyway OData team are also clever guys, they have defined dynamic aggregation without generating run-time classes on the fly. I was surprised with the idea to just inject into JSON serialization with specially prepared classes.

@djkunkel
Copy link
Author

Curious about that second part and dynamic aggregation. Can you point me to where you see that in the ODATA code if you have a chance?

@sdanyliv
Copy link
Member

Ok, sorry I have no time to explain in deep. There is expression that was generated when I've tried to reproduce previous issue from your sample:
https://github.com/linq2db/linq2db/pull/2074/files#diff-c3c89aa28fcb45014dd3a23b742c567dR139-R167

They manipulate with names and projection to get information for serialization later. It is not a real classes from OData library, but copied and simplified for easy reproducing. If you interested in, you can analyze their sources, names are exactly the same.

@sdanyliv sdanyliv self-assigned this Feb 14, 2020
@sdanyliv sdanyliv added this to the 2.9.7 milestone Feb 14, 2020
sdanyliv added a commit that referenced this issue Feb 22, 2020
sdanyliv added a commit that referenced this issue Feb 22, 2020
@sdanyliv sdanyliv added the status: has-pr There is active PR for issue label Feb 22, 2020
MaceWindu added a commit that referenced this issue Feb 29, 2020
Fix for #2092. Improved SelectContext to handle grouping and unexpected conversions.
MaceWindu pushed a commit that referenced this issue Feb 29, 2020
MaceWindu added a commit that referenced this issue Mar 2, 2020
#2108)

* Fix for #2092. Improved SelectContext to handle grouping and unexpected conversions.

(cherry picked from commit faac70b)

* Fixed tests regression.

Co-authored-by: Svyatoslav Danyliv <sdanyliv@gmail.com>
@sdanyliv
Copy link
Member

sdanyliv commented Mar 2, 2020

@djkunkel, version 2.9.7 will be released on Thursday. Dsitinct.Count queries will work but hey have non optimal SQL generation. If you expect COUNT (DISTICT x) - pity that it is not ready yet but i'm working on it.

@sdanyliv
Copy link
Member

sdanyliv commented Mar 5, 2020

Anyway, let me know how it works for you.

@djkunkel
Copy link
Author

Sorry for delay, but these improvements are working very well for me. Thanks to you and the team!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: linq status: has-pr There is active PR for issue type: bug
Development

Successfully merging a pull request may close this issue.

2 participants