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

Query: Support for GroupBy entity type #29019

Merged
merged 1 commit into from
Sep 13, 2022
Merged

Query: Support for GroupBy entity type #29019

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions src/EFCore.InMemory/Query/Internal/InMemoryQueryExpression.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -870,6 +870,22 @@ private static Expression GetGroupingKey(Expression key, List<Expression> groupi

return memberInitExpression.Update(updatedNewExpression, memberBindings);

case EntityShaperExpression entityShaperExpression
when entityShaperExpression.ValueBufferExpression is ProjectionBindingExpression projectionBindingExpression:
var entityProjectionExpression = (EntityProjectionExpression)((InMemoryQueryExpression)projectionBindingExpression.QueryExpression)
.GetProjection(projectionBindingExpression);
var readExpressions = new Dictionary<IProperty, MethodCallExpression>();
foreach (var property in GetAllPropertiesInHierarchy(entityProjectionExpression.EntityType))
{
readExpressions[property] = (MethodCallExpression)GetGroupingKey(
entityProjectionExpression.BindProperty(property),
groupingExpressions,
groupingKeyAccessExpression);
}

return entityShaperExpression.Update(
new EntityProjectionExpression(entityProjectionExpression.EntityType, readExpressions));

default:
var index = groupingExpressions.Count;
groupingExpressions.Add(key);
Expand Down
4 changes: 4 additions & 0 deletions src/EFCore.InMemory/Query/Internal/InMemoryQueryableMethodTranslatingExpressionVisitor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -455,6 +455,10 @@ private static ShapedQueryExpression CreateShapedQueryExpressionStatic(IEntityTy

return memberInitExpression.Update(updatedNewExpression, newBindings);

case EntityShaperExpression entityShaperExpression
when entityShaperExpression.ValueBufferExpression is ProjectionBindingExpression projectionBindingExpression:
return entityShaperExpression;

default:
var translation = TranslateExpression(expression);
if (translation == null)
Expand Down
11 changes: 10 additions & 1 deletion src/EFCore.Relational/Query/RelationalQueryableMethodTranslatingExpressionVisitor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -450,7 +450,16 @@ private static ShapedQueryExpression CreateShapedQueryExpression(IEntityType ent
var translatedKey = TranslateGroupingKey(remappedKeySelector);
if (translatedKey == null)
{
return null;
// This could be group by entity type
if (remappedKeySelector is not EntityShaperExpression
{ ValueBufferExpression : ProjectionBindingExpression })
{
// ValueBufferExpression can be JsonQuery, ProjectionBindingExpression, EntityProjection
// We only allow ProjectionBindingExpression which represents a regular entity
return null;
}

translatedKey = remappedKeySelector;
}

if (elementSelector != null)
Expand Down
17 changes: 17 additions & 0 deletions src/EFCore.Relational/Query/SqlExpressions/SelectExpression.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1857,6 +1857,23 @@ private static void PopulateGroupByTerms(
PopulateGroupByTerms(unaryExpression.Operand, groupByTerms, groupByAliases, name);
break;

case EntityShaperExpression entityShaperExpression
when entityShaperExpression.ValueBufferExpression is ProjectionBindingExpression projectionBindingExpression:
var entityProjectionExpression = (EntityProjectionExpression)((SelectExpression)projectionBindingExpression.QueryExpression)
.GetProjection(projectionBindingExpression);
foreach (var property in GetAllPropertiesInHierarchy(entityProjectionExpression.EntityType))
{
PopulateGroupByTerms(entityProjectionExpression.BindProperty(property), groupByTerms, groupByAliases, name: null);
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

Comment by @smitpatel: we already compare all properties for the Distinct translation, so GroupBy is consistent with that here.

}

if (entityProjectionExpression.DiscriminatorExpression != null)
{
PopulateGroupByTerms(
entityProjectionExpression.DiscriminatorExpression, groupByTerms, groupByAliases, name: DiscriminatorColumnAlias);
}

break;

default:
throw new InvalidOperationException(RelationalStrings.InvalidKeySelectorForGroupBy(keySelector, keySelector.GetType()));
}
Expand Down
159 changes: 73 additions & 86 deletions test/EFCore.Specification.Tests/Query/Ef6GroupByTestBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

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

nit:

Suggested change
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)

// GroupBy entityType. Issue #17653.
=> AssertTranslationFailed(
() => AssertQuery(
async,
ss => ss.Set<ArubaOwner>().GroupBy(o => o).Select(g => g.Key)));
=> AssertQuery(
async,
ss => ss.Set<ArubaOwner>().GroupBy(o => o).Select(g => g.Key),
elementSorter: e => e.Id,
elementAsserter: (e, a) =>
{
Assert.Equal(e.Id, a.Id);
Assert.Equal(e.Alias, a.Alias);
Assert.Equal(e.FirstName, a.FirstName);
Assert.Equal(e.LastName, a.LastName);
},
entryCount: 10);

[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
Expand All @@ -132,111 +139,93 @@ public virtual Task Grouping_by_all_columns_with_aggregate_function_works_1(bool
[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual Task Grouping_by_all_columns_with_aggregate_function_works_2(bool async)
// GroupBy entityType. Issue #17653.
=> AssertTranslationFailed(
() => AssertQueryScalar(
async,
ss => ss.Set<ArubaOwner>().GroupBy(o => o, c => new { c.LastName, c.FirstName }, (k, g) => g.Count())));
=> AssertQueryScalar(
async,
ss => ss.Set<ArubaOwner>().GroupBy(o => o, c => new { c.LastName, c.FirstName }, (k, g) => g.Count()));

[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual Task Grouping_by_all_columns_with_aggregate_function_works_3(bool async)
// GroupBy entityType. Issue #17653.
=> AssertTranslationFailed(
() => AssertQueryScalar(
async,
ss => ss.Set<ArubaOwner>().GroupBy(o => o, c => c, (k, g) => g.Count())));
=> AssertQueryScalar(
async,
ss => ss.Set<ArubaOwner>().GroupBy(o => o, c => c, (k, g) => g.Count()));

[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual Task Grouping_by_all_columns_with_aggregate_function_works_4(bool async)
// GroupBy entityType. Issue #17653.
=> AssertTranslationFailed(
() => AssertQuery(
async,
ss => ss.Set<ArubaOwner>().GroupBy(o => o, c => c, (k, g) => new { Count = g.Count() })));
=> AssertQuery(
async,
ss => ss.Set<ArubaOwner>().GroupBy(o => o, c => c, (k, g) => new { Count = g.Count() }));

[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual Task Grouping_by_all_columns_with_aggregate_function_works_5(bool async)
// GroupBy entityType. Issue #17653.
=> AssertTranslationFailed(
() => AssertQuery(
async,
ss => ss.Set<ArubaOwner>().GroupBy(o => o, c => c, (k, g) => new { k.Id, Count = g.Count() })));
=> AssertQuery(
async,
ss => ss.Set<ArubaOwner>().GroupBy(o => o, c => c, (k, g) => new { k.Id, Count = g.Count() }));

[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual Task Grouping_by_all_columns_with_aggregate_function_works_6(bool async)
// GroupBy entityType. Issue #17653.
=> AssertTranslationFailed(
() => AssertQuery(
async,
ss => ss.Set<ArubaOwner>().GroupBy(
o => o, c => c, (k, g) => new
{
k.Id,
k.Alias,
Count = g.Count()
})));
=> AssertQuery(
async,
ss => ss.Set<ArubaOwner>().GroupBy(
o => o, c => c, (k, g) => new
{
k.Id,
k.Alias,
Count = g.Count()
}));

[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual Task Grouping_by_all_columns_with_aggregate_function_works_7(bool async)
// GroupBy entityType. Issue #17653.
=> AssertTranslationFailed(
() => AssertQueryScalar(
async,
ss => from o in ss.Set<ArubaOwner>()
group o by o
into g
select g.Count()));
=> AssertQueryScalar(
async,
ss => from o in ss.Set<ArubaOwner>()
group o by o
into g
select g.Count());

[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual Task Grouping_by_all_columns_with_aggregate_function_works_8(bool async)
// GroupBy entityType. Issue #17653.
=> AssertTranslationFailed(
() => AssertQuery(
async,
ss => from o in ss.Set<ArubaOwner>()
group o by o
into g
select new { g.Key.Id, Count = g.Count() }));
=> AssertQuery(
async,
ss => from o in ss.Set<ArubaOwner>()
group o by o
into g
select new { g.Key.Id, Count = g.Count() });

[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual Task Grouping_by_all_columns_with_aggregate_function_works_9(bool async)
// GroupBy entityType. Issue #17653.
=> AssertTranslationFailed(
() => AssertQuery(
async,
ss => from o in ss.Set<ArubaOwner>()
group o by o
into g
select new
{
g.Key.Id,
g.Key.Alias,
Count = g.Count()
}));
=> AssertQuery(
async,
ss => from o in ss.Set<ArubaOwner>()
group o by o
into g
select new
{
g.Key.Id,
g.Key.Alias,
Count = g.Count()
});

[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual Task Grouping_by_all_columns_with_aggregate_function_works_10(bool async)
// GroupBy entityType. Issue #17653.
=> AssertTranslationFailed(
() => AssertQuery(
async,
ss => from o in ss.Set<ArubaOwner>()
group o by o
into g
select new
{
g.Key.Id,
Sum = g.Sum(x => x.Id),
Count = g.Count()
}));
=> AssertQuery(
async,
ss => from o in ss.Set<ArubaOwner>()
group o by o
into g
select new
{
g.Key.Id,
Sum = g.Sum(x => x.Id),
Count = g.Count()
});

[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
Expand Down Expand Up @@ -731,7 +720,6 @@ public virtual Task Whats_new_2021_sample_13(bool async)
[ConditionalTheory] // From #12088
[MemberData(nameof(IsAsyncData))]
public virtual Task Whats_new_2021_sample_14(bool async)
// GroupBy entityType. Issue #17653.
=> AssertTranslationFailed(
() => AssertQuery(
async,
Expand All @@ -742,13 +730,12 @@ public virtual Task Whats_new_2021_sample_14(bool async)
[ConditionalTheory] // From #12088
[MemberData(nameof(IsAsyncData))]
public virtual Task Whats_new_2021_sample_15(bool async)
// GroupBy entityType. Issue #17653.
=> AssertTranslationFailed(
() => AssertQuery(
async,
ss => ss.Set<Person>()
.GroupBy(bp => bp.Feet)
.Select(g => g.OrderByDescending(bp => bp.Id).FirstOrDefault())));
=> AssertQuery(
async,
ss => ss.Set<Person>()
.GroupBy(bp => bp.Feet)
.Select(g => g.OrderByDescending(bp => bp.Id).FirstOrDefault()),
entryCount: 12);

[ConditionalTheory] // From #12573
[MemberData(nameof(IsAsyncData))]
Expand Down
Loading