Query: Add support for SQL GROUP BY #16381
Conversation
smitpatel
force-pushed
the
smit/aggregates
branch
from
1ec33ad
to
9ac81cf
Compare
|
Re-targeting to preview7. This is pre-requisite for SQL GroupBy work. |
This allows us to use those methods to translate aggregates after GroupBy Resolves #15718
smitpatel
force-pushed
the
smit/aggregates
branch
from
9ac81cf
to
fd9ac43
Compare
smitpatel
changed the title
| @@ -5197,7 +5197,7 @@ public virtual Task Select_orderBy_take_count(bool isAsync) | |||
| cs => cs.OrderBy(c => c.Country).Take(7)); | |||
| } | |||
|
|
|||
| [ConditionalTheory(Skip = "Issue#15718")] | |||
| [ConditionalTheory] | |||
| [MemberData(nameof(IsAsyncData))] | |||
| public virtual Task Select_take_long_count(bool isAsync) | |||
There was a problem hiding this comment.
I don't see any coverage asserting the SQL for LongCount outside of SQL Server (where the default behavior is overridden). Can we assert the SQL in a SQLite test for coverage?
src/EFCore.Relational/Query/Pipeline/RelationalSqlTranslatingExpressionVisitor.cs
Show resolved
Hide resolved
| @@ -336,29 +474,25 @@ protected override Expression VisitParameter(ParameterExpression parameterExpres | |||
|
|
|||
| protected override Expression VisitExtension(Expression extensionExpression) | |||
| { | |||
| if (extensionExpression is EntityShaperExpression) | |||
| switch (extensionExpression) | |||
There was a problem hiding this comment.
Nobody likes switch expression in this project :(
There was a problem hiding this comment.
I converted to switch, no?
It wasn't switch earlier because it started with just 1 if and expanded later.
There was a problem hiding this comment.
I meant the new C# 8 expression, not switch statement - submitted #16421 which I hope we like.
There was a problem hiding this comment.
I think it reduces readability especially when doing function call rather than just returning some value.
There was a problem hiding this comment.
lol, I'm seeing a lot of people in the community who don't like the new switch expression. It'll be interesting to see where it does and doesn't get used. It feels a lot like LINQ comprehension syntax where it improves readability for some things, but makes it worse for others.
| { | ||
| Predicate = expression; | ||
| if (HavingExpression == null) |
| expression, | ||
| typeof(bool), | ||
| expression.TypeMapping); | ||
| if (Predicate == null) |
ghost
deleted the
This allows us to use those methods to translate aggregates after GroupBy
Resolves #15718
Query: Translate to SQL GROUP BY when aggregate operator is applied after GroupBy
Resolves #12826
Resolves #6658
Part of #15711
Resolves #15853
Resolves #12799
Resolves #12476
Resolves #11976