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 Perf: Selecting entire table for navigation properties in projections #5738

Closed
ruchanb opened this issue Jun 13, 2016 · 19 comments
Closed
Assignees
Labels
area-perf closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. type-bug
Milestone

Comments

@ruchanb
Copy link

ruchanb commented Jun 13, 2016

Steps to reproduce

public class Country
{        
    public string CountryCode { get; set; }
    public string CountryName { get; set; }
    public virtual ICollection<State> States { get; set; }
}

public class State 
{
    public string StateCode { get; set; }
    public string StateName { get; set; }
    public string CountryCode { get; set; }       
    public Country Country { get; set; }
}

Mapping on Country

builder.ToTable("Country");
builder.HasKey(pr => pr.CountryCode);
builder.HasMany(m => m.States).WithOne(i => i.Country).HasForeignKey(m => m.CountryCode);

Mapping on State

builder.ToTable("State");
builder.HasKey(pr => pr.StateCode);
builder.HasOne(m => m.Country).WithMany(m => m.States).HasForeignKey(m => m.CountryCode);

Now the Query

var query = _context.Countries
                    .Where(i => i.CountryCode == "USA")
                    .Select(m => new
                                  {
                                       m.CountryName,
                                       m.CountryCode,
                                       States = m.States.Select(x => new
                                                         {
                                                            x.StateCode,
                                                            x.StateName,
                                                            x.CountryCode
                                                         })
                                  }).AsQueryable();
  return query.ToList();

The issue

When I run SQL Profiler on this query I get following

SELECT [i].[CountryName], [i].[CountryCode]
FROM [Country] AS [i]
WHERE [i].[CountryCode] = N'USA'

and

SELECT [x].[CountryCode], [x].[StateCode], [x].[StateName]
FROM [State] AS [x]

I understand the two queries are not combined as in previous versions of EntityFramework. However, here the query on State is just select all without any where condition from the Country it is looking for. It does gives me States from 'USA' but it is doing so in the memory, not the SQL server.

Now make the query to include about 5 countries, it gives me 5 of these queries without any condition for Country

SELECT [x].[CountryCode], [x].[StateCode], [x].[StateName]
FROM [State] AS [x]

Further technical details

EF Core version: 1.0.0-rc2-final
Operating system: Windows 10
SQL Server 2008
Visual Studio version: VS 2015 update 2
Framework Target: net461

@rowanmiller
Copy link
Contributor

@anpete @maumar is this already tracked by another issue?

@maumar
Copy link
Contributor

maumar commented Jun 13, 2016

Related to #4007

@ruchanb
Copy link
Author

ruchanb commented Jun 14, 2016

Is this a bug or work in progress?

@rowanmiller
Copy link
Contributor

Closing as a dupe

@rowanmiller rowanmiller changed the title Linq to SQL on relation Query: Generating unfiltered navigation property projections Jun 21, 2016
@rowanmiller
Copy link
Contributor

Reopening as we are going to track the lack of filtering issue separately from the n+1 optimization.

@rowanmiller rowanmiller reopened this Jun 21, 2016
@anpete
Copy link
Contributor

anpete commented Jun 21, 2016

cc @maumar

@rowanmiller rowanmiller changed the title Query: Generating unfiltered navigation property projections Query: Selecting entire table for navigation properties in projections Jun 21, 2016
@rowanmiller rowanmiller added this to the 1.0.1 milestone Jun 21, 2016
@gdoron
Copy link

gdoron commented Jun 21, 2016

Don't get me wrong, I really appreciate the team's hard work and all.
But EF might get the RTM title in couple of days, but it's not there yet.
A trivial query resulting in a full table retrieval, that's not RTM.

Many things have changed to the best in Microsoft in the last couple of years, including the open source and multi platform change, but "version 1 isn't reliable - wait for SP1" hasn't.
😢

@rowanmiller
Copy link
Contributor

@gdoron your feedback is fair, LINQ providers take time and EF Core is no exception. There are definitely some significant limitations in the initial RTM, but we are working to address these and I think it will mature quickly from this point on. Most of what we are doing is bug fixing now, though there are some notable things still to be implemented (such as GroupBy transalation). We've tried to be very transparent about what to expect when using EF Core https://docs.efproject.net/en/latest/efcore-vs-ef6/index.html.

I do think that v1 software in general tend to be less mature - not just from Microsoft. Historically this was amplified by our slow release cadence, think back to .NET being >1 year between releases. I'm hopeful that folks that are adopting new technology will be able to keep up-to-date as we get some releases out quickly that address the top issues folks are hitting.

@gdoron
Copy link

gdoron commented Jun 27, 2016

@rowanmiller I have no complains about EF core not being as good as EF 6, it's obvious that a rewritten software, specially in EF's size will have many bugs in its early development, it's a temporary condition and I believe everyone is aware of this and accept it.

I was arguing that EF core was rushed and released too early, before it's ready.
People will use an RTM product in production, and they will surely get hit by the many bugs that EF core still has, their entire website can easily crash because of this bug for example.

It seems that the only reason EF core is being released now is to follow the same schedule as the rest of .NET Core and ASP.NET Core.

Each brick should be independent, just like WebSockets and SignalR that were left behind for future releases.
This is what I meant in this coment.

As a side note, as a consumer of the library, it's by far more important for us that EF Core's core features will work as they should, with good performance and reliability than having another nice to have feature (like scaffolding for instance).

With great appreciation to you and your team,
Doron

@rowanmiller rowanmiller changed the title Query: Selecting entire table for navigation properties in projections Query Perf: Selecting entire table for navigation properties in projections Jun 29, 2016
maumar added a commit that referenced this issue Jul 1, 2016
maumar added a commit that referenced this issue Jul 2, 2016
maumar added a commit that referenced this issue Jul 6, 2016
…erties in projections

Fix is to allow more queries to be translated to SQL, specifically subqueries that are correlated with outer query, e.g.

customers.Select(c => c.Orders.FirstOrDefault())

would get translated to:

customers.Select(c => orders.Where(o => o.CustomerId == c.Id).FirstOrDefault())

This would produce outer query to fetch all the customers and for each customer another query, fetching ALL the orders each time filtering out the orders for each customer and returning only the first one on the client (because we didn't know how to handle o.CustomerId in this case)

With this change we will convert o.CustomerId into a SQL parameter, whose value is set for each iteration of the outer query - this can be easily translated to SQL and produce much more efficient queries:

SELECT [t].[CustomerID]
FROM [Customers] AS [c]

@_outer_CustomerID: ALFKI (Size = 450)

SELECT TOP(1) [o].[OrderID], [o].[CustomerID], [o].[EmployeeID], [o].[OrderDate]
FROM [Orders] AS [o]
WHERE @_outer_CustomerID = [o].[CustomerID]

We achieve this by introducing a new query method that executes a delegate before enumerating it's source, and then executing another delegate after all elements have been enumerated or the enumerator has been disposed.
Those functions set and remove the necessary parameters just before/after the inner query is accessed.
@rowanmiller rowanmiller removed the pri0 label Jul 6, 2016
@jnm2
Copy link

jnm2 commented Jul 8, 2016

@gdoron I have to fundamentally disagree with you. Rowan is on point. Yes, it's ideal if enough people use your betas that v1.0.0 is flawless, but that may not be realistic. It's far worse if you get paralyzed into needing it to be perfect before you ship.

Version 1 Sucks, But Ship It Anyway is a great read by the co-founder of Stack Exchange.

Also, If you aren't embarrassed by v1.0 you didn't release it early enough.

@gdoron
Copy link

gdoron commented Jul 8, 2016

@jnm2 I read Jeff's blog post briefly but I believe I got the general idea.
I agree with concept, but EF doesn't suck, it's broken.

There's no reason to ship version 1 when you are already fully aware of the huge issues your software has.

If your version 1 will take servers and applications down because of N+1 queries and full tables(with a capital S) retrieval in a single query (as I painfully experienced), and you're aware of it, DO NOT SHIP IT.

There's a huge difference between being perfect and being reliable.
If your product is a critical one, you can ship with subset of features, but they do need to be reliable.

I guess you wouldn't want your bank's R&D department to read that blog and say, awesome idea! so lets go live with "version 1 sucks" and see what the feedback will be.

I am very disappointed with EF Core and will move our company to use Dapper or EF 6.1 if things won't get fixed very soon.

I think EF somewhere on the road made a huge mistake at prioritizing the features (maybe because of someone else making a terrible decision and changing the schedule and roadmap midway, as I believe happened).
You can have

  • Shadow properties (that I have no idea what are they good for).
  • Reverse engineer an existing DB etc'
  • Etc.

but off top of my head, issues that we suffer from:

  • Async operations are tremendously slower than the sync API - it was improved by 70% in matter of minutes by @anpete, kudos for that! but it should have happened much earlier, I'm sure EF team have benchamrks so they knew about it (if they don't, that is...).
  • Simple queries return the entire table and evaluate on the client.
  • Simple queries are translated to N+1 queries.
  • Join queries are translated to multiple queries.
  • Views are not supported.
  • SP are not supported.
  • Raw SQL that doesn't return a single track entity isn't supported.
  • Many limitations with Many to Many (not sure if they are known or not).

So there are many issues, but there isn't even a way of overcome them with RawSql.

The team, as you can see in the issues and the pull requests, are working hard and being transparent with the community. But they must prioritize the basic features of ORM first.

@jnm2
Copy link

jnm2 commented Jul 8, 2016

But they must prioritize the basic features of ORM first.

But that's your list. I'd never use views and SPs directly with EF. I think many to many without a join entity is a bad idea personally. Shadow properties are just as much a basic feature of an ORM as view and SP support, if not more fundamentally so. I can hardly believe we're finally getting them! The only bugs that would affect my company are the perf ones which is why I'm holding out for 1.0.1.

EF Core is not broken. For some people, EF Core is working perfectly. The EF team made it clear that EF Core will not be at parity with EF 6 for a long time, so everything is up for grabs. I experimentally moved our stuff to EF Core so I could provide feedback, but ultimately the onus is on you to verify whether taking the dive in production is a good idea. Every RTM release will have bugs and if servers went down because you didn't spend enough time testing your use cases before going to production that is your responsibility. This is the case with every library.

If EF Core's feature priorities don't match your priorities and you need your EF6-ish features, that is why they clearly say that you should remain on EF6. All the information that you needed to make a good judgment call was available.

@rowanmiller rowanmiller modified the milestones: 1.1.0, 1.0.1 Jul 8, 2016
@rowanmiller
Copy link
Contributor

Realocating to 1.1.0 as the fix for this was pretty high risk.

@maumar
Copy link
Contributor

maumar commented Jul 8, 2016

fixed in #6022, merged to dev in 0669372

@maumar maumar closed this as completed Jul 8, 2016
@maumar maumar added the closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. label Jul 8, 2016
@gdoron
Copy link

gdoron commented Jul 9, 2016

@rowanmiller Thanks for the fix and the update!
Is there an estimation on when 1.1.0 will be released?
The roadmap hasn't been updated for a month.

@ruchanb
Copy link
Author

ruchanb commented Sep 12, 2016

I used 1.1.0-alpha1-22107 and see no change in the query.

@maumar
Copy link
Contributor

maumar commented Sep 20, 2016

@ruchanb I just run the following code:

    class Program
    {
        static void Main(string[] args)
        {
            using (var ctx = new MyContext())
            {
                ctx.Database.EnsureDeleted();
                ctx.Database.EnsureCreated();

                var s11 = new State { CountryCode = "USA", StateCode = "WA", StateName = "Washington" };
                var s12 = new State { CountryCode = "USA", StateCode = "CA", StateName = "California" };
                var s21 = new State { CountryCode = "PL", StateCode = "DS", StateName = "Dolnoslaskie" };

                var c1 = new Country { CountryCode = "USA", CountryName = "United States", States = new List<State> { s11, s12 } };
                var c2 = new Country { CountryCode = "PL", CountryName = "Poland", States = new List<State> { s21 } };

                ctx.States.AddRange(s11, s12, s21);
                ctx.Countries.AddRange(c1, c2);
                ctx.SaveChanges();
            }

            using (var ctx = new MyContext())
            {
                var query = ctx.Countries
                    .Where(i => i.CountryCode == "USA")
                    .Select(m => new
                    {
                        m.CountryName,
                        m.CountryCode,
                        States = m.States.Select(x => new
                        {
                            x.StateCode,
                            x.StateName,
                            x.CountryCode
                        })
                    }).AsQueryable();
                var result = query.ToList();
                foreach (var r in result)
                {
                    var foo = r.States.ToList();
                    Console.WriteLine(foo);
                }
            }
        }
    }

    public class Country
    {
        public string CountryCode { get; set; }
        public string CountryName { get; set; }
        public virtual ICollection<State> States { get; set; }
    }

    public class State
    {
        public string StateCode { get; set; }
        public string StateName { get; set; }
        public string CountryCode { get; set; }
        public Country Country { get; set; }
    }

    public class MyContext : DbContext
    {
        public DbSet<Country> Countries { get; set; }
        public DbSet<State> States { get; set; }

        protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
        {
            optionsBuilder.UseSqlServer("Server=.;Database=Repro5738;Trusted_Connection=True;MultipleActiveResultSets=true");
        }

        protected override void OnModelCreating(ModelBuilder modelBuilder)
        {
            modelBuilder.Entity<Country>(b =>
            {
                b.ToTable("Country");
                b.HasKey(pr => pr.CountryCode);
                b.HasMany(m => m.States).WithOne(i => i.Country).HasForeignKey(m => m.CountryCode);
            });

            modelBuilder.Entity<State>(b =>
            {
                b.ToTable("State");
                b.HasKey(pr => pr.StateCode);
                b.HasOne(m => m.Country).WithMany(m => m.States).HasForeignKey(m => m.CountryCode);
            });
        }
    }

on our nightly build and got this (expected) sql:

SELECT [i].[CountryName], [i].[CountryCode]
FROM [Country] AS [i]
WHERE [i].[CountryCode] = N'USA'

exec sp_executesql N'SELECT [x].[StateCode], [x].[StateName], [x].[CountryCode]
FROM [State] AS [x]
WHERE @_outer_CountryCode = [x].[CountryCode]',N'@_outer_CountryCode nvarchar(450)',@_outer_CountryCode=N'USA'

Is there any difference between the code you have and the one above?

@ruchanb
Copy link
Author

ruchanb commented Sep 21, 2016

Hmm, I tried your code and the sql seems to be as you said. What about the no of queries on State though? State is queried each time as the no of countries listed..
Thanks.

@maumar
Copy link
Contributor

maumar commented Sep 21, 2016

@ruchanb N+1 queries for top level collections is tracked here: #4007

this fix aimed to mitigate it somewhat by only pulling in relevant entities, rather than entire DbSets

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

No branches or pull requests

7 participants