Query: Support for GroupBy entity type #29019
Conversation
| .GetProjection(projectionBindingExpression); | ||
| foreach (var property in GetAllPropertiesInHierarchy(entityProjectionExpression.EntityType)) | ||
| { | ||
| PopulateGroupByTerms(entityProjectionExpression.BindProperty(property), groupByTerms, groupByAliases, name: null); |
There was a problem hiding this comment.
So if I understand correctly, we're opting for grouping by all the entity type's properties, as opposed to grouping by its primary key properties (I can see some discussion in #17653).
Wouldn't grouping by the primary key be potentially much more efficient (I can do some quick benchmarking)? On the hand, I understand that in some databases we'd need to use a trick such as MAX() in order to project out the other properties...
BTW what happens with keyless entities, where two rows may have the same property values and therefore get grouped together? Seems like we should block that no?
There was a problem hiding this comment.
EF6 did group by all properties so this implementation just aligns the behavior with that. The complexity of utilizing key in query after grouping is quite high, which questions why user is not writing group by PK in first place. Can improve it later.
For keyless entity, fine either way. 2 rows can get grouped together but change tracker has no behavior that they are same entities or different entities so neither behavior is wrong.
There was a problem hiding this comment.
Re grouping by all properties, yeah, we can see about improving in the future. It's true the user can explicitly group by the primary key, but that can be said about the feature as a whole.. Specifically, if grouping by all properties is inefficient compared to grouping by primary key (will try to benchmark this), we may be doing users a disservice by providing a slower translation (and they wouldn't know about it) - a translation failure at least forces them to explicitly specify what they want to group by.
Re keyless, if our translation groups two rows together because all their properties are the same, isn't that different from the LINQ to Objects behavior which groups by the reference, and so never returns a group of two?
There was a problem hiding this comment.
Comment by @smitpatel: we already compare all properties for the Distinct translation, so GroupBy is consistent with that here.
| @@ -109,11 +109,18 @@ public virtual Task GroupBy_is_optimized_when_grouping_by_row_and_projecting_col | |||
| [ConditionalTheory] | |||
| [MemberData(nameof(IsAsyncData))] | |||
| public virtual Task Grouping_by_all_columns_doesnt_produce_a_groupby_statement(bool async) | |||
There was a problem hiding this comment.
nit:
| public virtual Task Grouping_by_all_columns_doesnt_produce_a_groupby_statement(bool async) | |
| public virtual Task Grouping_by_all_columns_does_not_produce_a_groupby_statement(bool async) |
|
|
||
| [ConditionalTheory] | ||
| [ConditionalTheory(Skip = "Issue#29014")] |
There was a problem hiding this comment.
This should work some day.
|
Is it possible this to be merged into version 6 as well? |
|
@theCuriousOne please see our release planning process, and specifically what qualified for patch releases. This isn't a bug fix, but rather an enhancement. |
|
I understand. From my perspective this classifies as "It is a regression from a previous release" since it worked like that in EF6 (Framework). While doing an upgrade from net4.8 to .net 6, I encountered this issue, but more likely you mean regression from previous .net core version. Thank you all. |
|
Old EF6 is a different product, and some code that worked there won't work on EF Core; we don't consider that a regression. Though we indeed do our best to enable as many coding EF6 patterns as possible on EF Core (in fact, that's one of the main reasons this issue got implemented). |
Resolves #17653