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.

Sign up for GitHub

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

Jump to bottom

Implement GROUP BY #7697

Closed
wants to merge 1 commit into from
Closed

Implement GROUP BY #7697

wants to merge 1 commit into from

Conversation

tuespetre
Copy link
Contributor

@tuespetre tuespetre commented Feb 23, 2017 •
edited
Loading

I don't know when to quit.

Imgur

This PR (dependent on #7695 and #7696) implements GROUP BY, this time with only about 15 or 16 changed files less changed files, although still a good number due to test changes and a very focused approach to getting it done without a lot of byproduct.

@dnfclas
Copy link

dnfclas commented Feb 23, 2017

@tuespetre,
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by .NET Foundation. We will now review your pull request.
Thanks,
.NET Foundation Pull Request Bot

@tuespetre tuespetre mentioned this pull request Feb 23, 2017
Closed
@smitpatel
Copy link
Contributor

@tuespetre - Given the 2 dependent PRs are merged in. Can you update/rebase this PR?

@smitpatel smitpatel self-assigned this Mar 1, 2017
@tuespetre
Copy link
Contributor Author

@smitpatel sure can, but heads up, there are some test cases and scenarios I have thought of that still need to be addressed here. I wouldn't bother spending your time reviewing it or anything yet. Sorry ☹️

@smitpatel
Copy link
Contributor

Just change the prefix the title with [WIP] and update it once you are done working out. Or given there has not been any discussion here. You can close this and open a new PR when work is ready for review.

@tuespetre
Copy link
Contributor Author

OK. Thanks

@tuespetre tuespetre changed the title Implement GROUP BY [WIP] Implement GROUP BY Mar 1, 2017
@tuespetre tuespetre force-pushed the group-by branch 5 times, most recently from a714b1e to 7a6255c Compare March 7, 2017 15:39
@tuespetre
Copy link
Contributor Author

@smitpatel I am now going to state that this PR will depend on #7767 and #7771

@tuespetre tuespetre force-pushed the group-by branch 3 times, most recently from 9407cc4 to d14681e Compare March 8, 2017 22:01
@tuespetre tuespetre changed the title [WIP] Implement GROUP BY Implement GROUP BY Mar 8, 2017
@tuespetre
Copy link
Contributor Author

🆙📅 finished work on tests: split out separate AsyncGroupByQuery____ and GroupByQuery___ test classes, and added several new and complex test cases for grouping, plus made them actually work :trollface:

@smitpatel
Copy link
Contributor

@tuespetre - The query you posted in PR #7771 , certainly it is a complex query for group join.
What is expected Sql for that?
In general what are the blockers for group by translation in current stack. (not just PR #7771 but other issues you faced which prohibits group by translations)

@tuespetre
Copy link
Contributor Author

@smitpatel here is the query I was basing that on:

https://github.com/tuespetre/EntityFramework/blob/d14681e89b2b2cb95840abc460186167ddcb9a85/test/EFCore.SqlServer.FunctionalTests/GroupByQuerySqlServerTest.cs#L469

I don't see anything else blocking generally useful GROUP BY translations for now. One scenario I have not covered in this PR is aggregate subqueries that include WhereClauses (see the GroupBy_Sum_Where test in that same file for an example.) I viewed that as an additional enhancement beyond the basic ability to group and aggregate.

@tuespetre tuespetre force-pushed the group-by branch 2 times, most recently from c35a631 to 4309788 Compare March 15, 2017 19:10
@tuespetre
Copy link
Contributor Author

🆙📅 ➕ ♻️🅱️

@tuespetre tuespetre mentioned this pull request Mar 16, 2017
Closed
@tuespetre
Copy link
Contributor Author

🆙📅 ➕ ♻️🅱️ again

smitpatel
smitpatel reviewed Apr 11, 2017
View reviewed changes
src/EFCore.Relational/Query/ExpressionVisitors/RelationalProjectionExpressionVisitor.cs
var sqlExpression = TranslateExpression(memberExpression);

var handledExpression
= TryHandleGroupingKeyExpression(
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove these (and also methodcall) methods, and call appropriate code from main Visit.
It is good to have expression type specific visitors but if they are doing the same task as the main Visit method then it does not serve much value.

@smitpatel
Copy link
Contributor

@tuespetre - Please remove any trailing code changes from any other PRs which are not merged. Also for reviewing purpose create 2 different commits. 2nd one containing only test changes (there are 2200 lines of addition just from tests 😄 nice.) And let us know when it is ready for review.

@smitpatel
Copy link
Contributor

We are closing this pull request due to lack of response. If you decide to work on the issue again then feel free to reopen the PR and push more commits to it. We would be happy to review it.
Thank you helping us out in improving EF Core.

@smitpatel smitpatel closed this May 15, 2017
@smitpatel
Copy link
Contributor

@tuespetre - I have taken tests from this PR. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@smitpatel smitpatel smitpatel left review comments

Assignees

@smitpatel smitpatel

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@tuespetre @dnfclas @smitpatel