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: Join after GroupByAggregate throws when join key uses grouping key or aggregate function #10012

Closed
smitpatel opened this issue Oct 8, 2017 · 22 comments · Fixed by #21589
Assignees
Labels
area-query closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. punted-for-3.0 type-enhancement
Milestone

Comments

@smitpatel
Copy link
Member

something like

var query = context.Orders.GroupBy(o => o.CustomerID).Select(g => new { g.Key, Count = g.Count() })
.Join(context.Customers, o => o.Key, c => c.CustomerID, (o, c) => new {C = c, o.Count});
@smitpatel smitpatel self-assigned this Oct 8, 2017
@smitpatel smitpatel added this to the 2.1.0 milestone Oct 8, 2017
@ajcvickers ajcvickers modified the milestones: 2.1.0-preview1, 2.1.0 Jan 17, 2018
@smitpatel smitpatel changed the title Test: Query: Add tests for composition after GroupBy aggregate Query: Join after GroupByAggregate throws when join key uses grouping key or aggregate function Mar 13, 2018
@smitpatel
Copy link
Member Author

[ConditionalFact]
        public virtual void GroupBy_Aggregate_Join()
        {
            AssertQuery<Order, Customer>(
                (os, cs) =>
                    from a in os.GroupBy(o => o.CustomerID)
                                .Where(g => g.Count() > 5)
                                .Select(g => new { CustomerID = g.Key, LastOrderID = g.Max(o => o.OrderID) })
                    join c in cs on a.CustomerID equals c.CustomerID
                    join o in os on a.LastOrderID equals o.OrderID
                    select new { c, o });
        }
Compiling query model: 
  'from IGrouping<string, Order> g in 
      (from Order o in DbSet<Order>
      select [o]).GroupBy([o].CustomerID, [o])
  where 
      (from Order <generated>_1 in [g]
      select [<generated>_1]).Count() > 5
  join Customer c in DbSet<Customer>
  on [g].Key equals [c].CustomerID
  join Order o in DbSet<Order>
  on 
      (from Order o in [g]
      select [o].OrderID).Max() equals [o].OrderID
  select new <>f__AnonymousType14<Customer, Order>(
      [c], 
      [o]
  )'

Few issues here:

  • Since Key comes from anonymous binding from SelectExpression. It is alias expression which may need to unwrapped.
  • When the join key is aggregate function, the initial client eval does not use SqlTranslatingExpressionVisitor, causing issue with grouping parameter being non-sequence type.
  • Even after above is fixed, the QueryModelVisitor generated for subquery lacks parent QMV hence we are unable to find SelectExpression and fail to translate the key properly.

@smitpatel smitpatel removed this from the 2.1.0 milestone Mar 13, 2018
smitpatel added a commit that referenced this issue Mar 13, 2018
- Add support for translating OrderBy after GroupBy operator
- Add support for `HAVING` clause in SQL which would be generated when translating predicate after GroupByAggregate Resolves #10870
- Make sure client eval warning is not issued when translating GroupByAggregate Resolves #11157
- GroupBy Aggregate works when key/element/result selector is DTO instead of anonymous type Resolves #11176
- Make sure that SQL added to GROUP BY clause is not aliased Resolves #11218

Part of #10012
smitpatel added a commit that referenced this issue Mar 14, 2018
- Add support for translating OrderBy after GroupBy operator
- Add support for `HAVING` clause in SQL which would be generated when translating predicate after GroupByAggregate Resolves #10870
- Make sure client eval warning is not issued when translating GroupByAggregate Resolves #11157
- GroupBy Aggregate works when key/element/result selector is DTO instead of anonymous type Resolves #11176
- Make sure that SQL added to GROUP BY clause is not aliased Resolves #11218
- Translate GroupBy Constant/Parameter with aggregates Resolves #9969

Part of #10012
smitpatel added a commit that referenced this issue Mar 14, 2018
- Add support for translating OrderBy after GroupBy operator
- Add support for `HAVING` clause in SQL which would be generated when translating predicate after GroupByAggregate Resolves #10870
- Make sure client eval warning is not issued when translating GroupByAggregate Resolves #11157
- GroupBy Aggregate works when element/result selector is DTO instead of anonymous type Resolves #11176 (KeySelector has to be client evaluated)
- Make sure that SQL added to GROUP BY clause is not aliased Resolves #11218
- Translate GroupBy Constant/Parameter with aggregates Resolves #9969

Part of #10012
Part of #2341
smitpatel added a commit that referenced this issue Mar 14, 2018
- Add support for translating OrderBy after GroupBy operator
- Add support for `HAVING` clause in SQL which would be generated when translating predicate after GroupByAggregate Resolves #10870
- Make sure client eval warning is not issued when translating GroupByAggregate Resolves #11157
- GroupBy Aggregate works when element/result selector is DTO instead of anonymous type Resolves #11176 (KeySelector has to be client evaluated)
- Make sure that SQL added to GROUP BY clause is not aliased Resolves #11218
- Translate GroupBy Constant/Parameter with aggregates Resolves #9969

Part of #10012
Part of #2341
smitpatel added a commit that referenced this issue Mar 14, 2018
- Add support for translating OrderBy after GroupBy operator
- Add support for `HAVING` clause in SQL which would be generated when translating predicate after GroupByAggregate Resolves #10870
- Make sure client eval warning is not issued when translating GroupByAggregate Resolves #11157
- GroupBy Aggregate works when element/result selector is DTO instead of anonymous type Resolves #11176 (KeySelector has to be client evaluated)
- Make sure that SQL added to GROUP BY clause is not aliased Resolves #11218
- Translate GroupBy Constant/Parameter with aggregates Resolves #9969

Part of #10012
Part of #2341
@ajcvickers ajcvickers added this to the Backlog milestone Mar 14, 2018
@ajcvickers ajcvickers modified the milestones: Backlog, 2.1.0 Mar 14, 2018
@smitpatel smitpatel modified the milestones: 2.1.0, Backlog Mar 14, 2018
@mattheiler
Copy link

mattheiler commented Nov 26, 2019

    public class Foo
    {
        public int Id { get; set; }

        public ICollection<FooBar> Bars { get; } = new List<FooBar>();
    }

    public class FooBar
    {
        public virtual Foo Foo { get; set; }

        public virtual int FooId { get; set; }

        public virtual Bar Bar { get; set; }

        public virtual int BarId { get; set; }
    }

    public class Bar
    {
        public virtual int Id { get; set; }

        public virtual ICollection<FooBar> Foos { get; } = new List<FooBar>();

        public virtual ICollection<BarBaz> Bazs { get; } = new List<BarBaz>();
    }

    public class BarBaz
    {
        public virtual Bar Bar { get; set; }

        public virtual int BarId { get; set; }

        public virtual Baz Baz { get; set; }

        public virtual int BazId { get; set; }
    }

    public class Baz
    {
        public virtual int Id { get; set; }

        public virtual ICollection<BarBaz> Bars { get; } = new List<BarBaz>();
    }

    public class BazResult
    {
        public Baz Baz { get; set; }

        public int BazBarsCount { get; set; }
    }

    public class EfTestDbContext : DbContext
    {
        public virtual DbSet<Foo> Foos { get; private set; }

        public virtual DbSet<Bar> Bars { get; private set; }

        public virtual DbSet<Baz> Bazs { get; private set; }

        protected override void OnConfiguring(DbContextOptionsBuilder options)
        {
            options.UseSqlServer("Data Source=localhost; Initial Catalog=EfTest; Integrated Security=true; MultipleActiveResultSets=True;");
        }

        protected override void OnModelCreating(ModelBuilder model)
        {
            model.Entity<FooBar>().HasKey(e => new {e.FooId, e.BarId});
            model.Entity<BarBaz>().HasKey(e => new {e.BarId, e.BazId});
        }
    }

    public class Program
    {
        public static async Task Main(string[] args)
        {
            await using var context = new EfTestDbContext();

            const int fooId = 0;
                
            var barBazsByFoo =
                from foo in context.Foos
                where foo.Id == fooId
                from fooBar in foo.Bars
                let bar = fooBar.Bar
                from barBaz in bar.Bazs
                select barBaz;

            var barBazs =
                from barBaz in barBazsByFoo
                group barBaz by barBaz.BazId
                into barBazGroups
                join barBaz in barBazsByFoo on barBazGroups.Key equals barBaz.BazId
                select new BazResult
                {
                    Baz = barBaz.Baz,
                    BazBarsCount = barBazGroups.Count()
                };

            await barBazs.ToListAsync();
        }
    }

Processing of the LINQ expression 'GroupByShaperExpression:
KeySelector: b.BazId,
ElementSelector:EntityShaperExpression:
EntityType: BarBaz
ValueBufferExpression:
ProjectionBindingExpression: Outer
IsNullable: False
' by 'RelationalProjectionBindingExpressionVisitor' failed. This may indicate either a bug or a limitation in EF Core. See https://go.microsoft.com/fwlink/?linkid=2101433 for more detailed information.

This is with EF Core SqlServer 3.0.0.

@mattheiler
Copy link

I upgraded to 3.1.0-preview3.19554.8 and the issue persists.

@smitpatel
Copy link
Member Author

@mattheiler - It happens since you are selecting navigation off the entity you are joining. Hence it won't work for now.

@mattheiler
Copy link

mattheiler commented Nov 26, 2019

Hence it won't work for now.

So this scenario is a known issue? Alrighty.

@hidegh
Copy link

hidegh commented Dec 31, 2019

Same with EF 3.1 on a fairly simple query: hidegh/Meal-tracker@a85ac8e

It's just EF core issue, same approach works within EF6.x and well, I'd bet it works on NHibernate (even older versions) too.

@Mhirji
Copy link

Mhirji commented Jan 7, 2020

Just wondering if this issue is going to be addressed and when. At the moment, it is blocking us from upgrading from dotnet core 2.2 to 3.1 and I looking at the thread, I suspect others are in the same boat.

@MolallaComm
Copy link

MolallaComm commented Jan 30, 2020

Yes, regarding odata - it works fine with efcore 2.2, but breaks with efcore 3+. I'm not sure if it is the same bug as discussed in this issue since this issue was originally entered before 2.2 even came out and odata works fine with 2.2 - just not 3+. It would be nice if someone from the efcore team could take a look and at least classify and/or comment on the problem with odata. For an official (and useful) microsoft project, odata doesn't get much thought from the other teams - for months aspnetcore changes were preventing people using odata migrating to .net core 3+ - they finally fix those - only to find out efcore changes are breaking it now at least for people who need group by.

@ajcvickers
Copy link
Member

@MolallaComm We plan to sync with the OData team to get a better understanding of some of the queries that are generated.

@smitpatel smitpatel removed this from the Backlog milestone Mar 15, 2020
@smitpatel smitpatel self-assigned this Mar 15, 2020
@ajcvickers ajcvickers added this to the 5.0.0 milestone Mar 16, 2020
@efcoyote
Copy link

Imho this issue should be handled with pressure and not being open for several years. Let's have an example.
You have 1.000.000 customers. Every customer has some orders with a date. And now you need the 50 customers (paging) with the newest orders. In SQL thats something like (without TOP 50):

SELECT C.*, O.DATE
FROM CUSTOMER C
JOIN (
  SELECT CUSTOMER_ID, MAX(DATE) DATE
  FROM ORDERS
  GROUP BY CUSTOMER_ID) O ON C.CUSTOMER_ID = O.CUSTOMER_ID
ORDER BY O.DATE DESC

This I currently impossible with EF Core. Reading all 1.000.000 customers and select 50 of them is not a solution. The only solution I found is to write raw SQL.

@smitpatel smitpatel added the closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. label Jul 10, 2020
smitpatel added a commit that referenced this issue Jul 10, 2020
smitpatel added a commit that referenced this issue Jul 10, 2020
@ghost ghost closed this as completed in #21589 Jul 11, 2020
ghost pushed a commit that referenced this issue Jul 11, 2020
@smitpatel
Copy link
Member Author

All the queries described in this issue were already working. They may have started working in 3.1 or in earlier previews of 5.0.

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-query closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. punted-for-3.0 type-enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.