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: Prevent client evaluation when subquery returning entity is compared to null #7915

Closed
joshmouch opened this issue Mar 16, 2017 · 9 comments
Assignees
Labels
closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. type-enhancement
Milestone

Comments

@joshmouch
Copy link

joshmouch commented Mar 16, 2017

I'm using version 1.1.0, so let me know if this is fixed in 1.1.1. I can't upgrade to 1.1.1 because it gives me different problems.

I have a simple query:


// clientId passed as parameter to method
var results = (from e1 in this.Repository.DataContext.EntityOne
	let e2 = e1.EntityTwos.FirstOrDefault(a => a.DeleteFlag == false && a.ClientId == clientId)
	select new
	{
		e1.PropertyOne,
		PropertyTwo = (e2 != null) ? e2.PropertyTwo : e1.PropertyTwo,
	}
).ToList();

And this query causes N+1 hits to the database where N in the number of rows in EntityOne. So if there are 10 rows in EntityOne, then there will be 11 database hits.
This used to be supported in EF.

@joshmouch joshmouch changed the title Lots of extra queries being called for simple LINQ statement Lots of extra database hits for simple LINQ statement Mar 16, 2017
@joshmouch
Copy link
Author

joshmouch commented Mar 16, 2017

If I upgrade to 1.1.1, I get the error talked about in this ticket: #7714.

If I then refactor (incorrectly) to this:


// clientId passed as parameter to method
var results = (from e1 in this.Repository.DataContext.EntityOne
	select new
	{
		e1.PropertyOne,
		PropertyTwo = e1.EntityTwos.FirstOrDefault(a => a.DeleteFlag == false && a.ClientId == clientId),
	}
).ToList();

Then I still get the extra database hits.

@maumar
Copy link
Contributor

maumar commented Mar 17, 2017

@joshmouch you mean this used to work (i.e. produce single query) in previous versions of EF Core, or do you mean EF6? Also, could you post the model you are using? (entities and contents of OnModelCreating method on the DbContext)

@joshmouch
Copy link
Author

Yes, I meant it used to produce a single query in EF6.

The entities look something like this:


    public partial class EntityOne : ICreateableEntity, IUpdateableEntity, IDeletableEntity, IVersionableEntity
    {
        public EntityOne()
        {
            EntityTwos = new HashSet<EntityTwo>();
        }

		public Guid EntityOneId {get;set;}
		...
		
        public virtual ICollection<EntityTwo> EntityTwos { get; set; }
    }
	
    public partial class EntityTwo : ICreateableEntity, IUpdateableEntity, IDeletableEntity, IVersionableEntity
    {
		public Guid EntityTwoId {get;set;}
		public Guid? EntityOneId {get;set;}
        ...
		
        public virtual EntityOne EntityOne { get; set; }
    }	

The OnModelCreating is 12k+ lines long, so I can't give it to you exactly, but here's a simplified version:



	entity.HasOne(d => d.EntityOne)
		.WithMany(p => p.EntityTwos)
		.HasForeignKey(d => d.EntityOneId)
		.OnDelete(DeleteBehavior.Restrict)
		.HasConstraintName("FK_xxxx");	
		

@maumar
Copy link
Contributor

maumar commented Mar 17, 2017

@joshmouch try the following:

from e1 in ctx.EntityOne
let e2 = e1.EntityTwos.FirstOrDefault(a => a.DeleteFlag == false && a.ClientId == clientId)
select new
{
   e1.PropertyOne,
   PropertyTwo = e2.PropertyTwo ?? e1.PropertyTwo
};

this should translate to:

SELECT [e1].[PropertyOne], COALESCE((
                SELECT TOP(1) [a0].[PropertyTwo]
                FROM [EntityTwo] AS [a0]
                WHERE (([a0].[DeleteFlag] = 0) AND ([a0].[ClientId] = @__clientId_0)) AND ([e1].[EntityOneId] = [a0].[EntityOneId])
            ), [e1].[PropertyTwo]) AS [Coalesce]
            FROM [EntityOne] AS [e1]

in both 1.1.0 and 1.1.1

comparing queries to null is in general, problematic and if the query is fully translated to SQL you don't actually need the null protection logic (since its built-in SQL already)

@joshmouch
Copy link
Author

joshmouch commented Mar 18, 2017

@maumar that does work for nullable types (e.g. String or Nullable).
However, it doesn't work for a non-nullable type (i.e. Decimal).

In other words if e2 PropertyTwo were a string, null propagation works, but if it's a decimal, it doesn't.

I've tried your suggestion before when experimenting with different options and I had the idea to use a null propogation operator to change the type to nullable. For example:

PropertyTwo = e2?.PropertyTwo ?? e1.PropertyTwo

However, then I get an build error that "An expression tree lambda may not contain a null propagating operator."

@joshmouch
Copy link
Author

joshmouch commented Mar 18, 2017

Actually, I just tried simply casting the type to a nullable (even though it's not), and now the simplified query works as expected.


from e1 in ctx.EntityOne
let e2 = e1.EntityTwos.FirstOrDefault(a => a.DeleteFlag == false && a.ClientId == clientId)
select new
{
   e1.PropertyOne,
   PropertyTwo = ((Decimal?)e2.PropertyTwo) ?? e1.PropertyTwo
};

However, there are other cases where we compare a query to null. You say that this is problematic. Does that mean it won't be supported in 2.0, either?

For example:


from e1 in ctx.EntityOne
let e2 = e1.EntityTwos.FirstOrDefault(a => a.DeleteFlag == false && a.ClientId == clientId)
let e3 = e1.EntityThrees.Where(a => a.DeleteFlag == false && a.ClientId == clientId)
where (e2 != null || e3.Any())
select new
{
   e1.PropertyOne,
};

So far I'm able to rewrite every one of these to use an Any instead of a null comparison, so it may not be a big deal....

@maumar
Copy link
Contributor

maumar commented Mar 18, 2017

@joshmouch We have made a lot of improvements in 2.0.0 (and many more are still pending), so a lot of queries should be supported or more efficient. I will look into this example on Monday. In the meantime, you can try a following trick for those cases:

from e1 in ctx.EntityOne
let e2 = e1.EntityTwos.Select(a => a.EntityOneId).FirstOrDefault(a => a.DeleteFlag == false && a.ClientId == clientId)
let e3 = e1.EntityThrees.Where(a => a.DeleteFlag == false && a.ClientId == clientId)
where (e2 != default(Guid) || e3.Any())
select new
{
   e1.PropertyOne,
};

Basically, rather than comparing entire entity to null, compare it's key to the default value. Assuming that your database doesn't have default key values, it should work.

@maumar
Copy link
Contributor

maumar commented Mar 28, 2017

@joshmouch Here are the result of my investigation on the current dev:

from e1 in ctx.EntityOne
let e2 = e1.EntityTwos.FirstOrDefault(a => a.DeleteFlag == false && a.ClientId == clientId)
let e3 = e1.EntityThrees.Where(a => a.DeleteFlag == false && a.ClientId == clientId)
where (e2 != null || e3.Any())
select new
{
   e1.PropertyOne,
};

produces N+1:

SELECT [e1].[EntityOneId], [e1].[PropertyOne]
FROM [EntityOnes] AS [e1]

SELECT TOP(1) [a0].[EntityTwoId], [a0].[ClientId], [a0].[DeleteFlag], [a0].[EntityOneId], [a0].[PropertyTwo]
FROM [EntityTwos] AS [a0]
WHERE (([a0].[DeleteFlag] = 0) AND ([a0].[ClientId] = @__clientId_0)) AND (@_outer_EntityOneId = [a0].[EntityOneId])

SELECT CASE
    WHEN EXISTS (
        SELECT 1
        FROM [EntityThrees] AS [a1]
        WHERE (([a1].[DeleteFlag] = 0) AND ([a1].[ClientId] = @__clientId_1)) AND (@_outer_EntityOneId1 = [a1].[EntityOneId]))
    THEN CAST(1 AS BIT) ELSE CAST(0 AS BIT)
END

(filter is performed on the client)

However, if the query is slightly modified to:

from e1 in ctx.EntityOnes
let e2 = e1.EntityTwos.Where(a => a.DeleteFlag == false && a.ClientId == clientId)
let e3 = e1.EntityThrees.Where(a => a.DeleteFlag == false && a.ClientId == clientId)
where (e2.Any() || e3.Any())
select new
{
    e1.PropertyOne,
};

we get:

SELECT [e1].[PropertyOne]
        FROM [EntityOnes] AS [e1]
        WHERE EXISTS (
            SELECT 1
            FROM [EntityTwos] AS [a]
            WHERE (([a].[DeleteFlag] = 0) AND ([a].[ClientId] = @__clientId_0)) AND ([e1].[EntityOneId] = [a].[EntityOneId])) OR EXISTS (
            SELECT 1
            FROM [EntityThrees] AS [a0]
            WHERE (([a0].[DeleteFlag] = 0) AND ([a0].[ClientId] = @__clientId_1)) AND ([e1].[EntityOneId] = [a0].[EntityOneId]))

@maumar maumar removed this from the 1.1.2 milestone Mar 28, 2017
@maumar maumar changed the title Lots of extra database hits for simple LINQ statement Query: Comparing subquery returning entity to null leads to client evaluation Mar 28, 2017
@ajcvickers ajcvickers added this to the Backlog milestone Mar 31, 2017
@maumar
Copy link
Contributor

maumar commented Mar 31, 2017

In current dev bits we generate the following query:

SELECT [e1].[PropertyOne], [e1].[Id], (
            SELECT TOP(1) [a2].[PropertyTwo]
            FROM [EntityTwos] AS [a2]
            WHERE (([a2].[DeleteFlag] = 0) AND ([a2].[ClientId] = @__clientId_0)) AND ([e1].[Id] = [a2].[EntityOneId])
        ), [e1].[PropertyTwo]
        FROM [EntityOnes] AS [e1]

the ? : is still performed on the client, but we produce only one query

@maumar maumar closed this as completed Mar 31, 2017
@maumar maumar modified the milestones: 2.0.0, Backlog Mar 31, 2017
@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 Mar 31, 2017
@divega divega changed the title Query: Comparing subquery returning entity to null leads to client evaluation Query: Prevent client evaluation when subquery returning entity is compared to null May 9, 2017
@ajcvickers ajcvickers modified the milestones: 2.0.0-preview1, 2.0.0 Oct 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. type-enhancement
Projects
None yet
Development

No branches or pull requests

3 participants