-
Notifications
You must be signed in to change notification settings - Fork 225
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
Wrong SQL parentheses #2601
Comments
@petergummer thanks for filing this. Can you please provide a minimal code sample, including the LINQ query and model which trigger this query? I suspect this may be a general EF issue rather than an EFCore.PG one. |
@petergummer ping, if there's a bug here (and it seems that there is) it's quite important that we find the root cause and fix it ASAP. |
@roji, I've finally managed to reproduce the problem with an isolated program, using version 7.0.1. The short story is that to replicate it, the expression tree has to use Or rather than OrElse. It seems quite similar to dotnet/efcore#29222 which was closed a couple of months ago. This program uses | in the second Where clause.
An interesting variation of this problem occurs if I delete the first Where clause.
using Microsoft.EntityFrameworkCore;
var ctx = new Ctx();
var query = ctx.Bunches
.Where(b => b.Label == "coconuts") // To see an interesting variation, delete this Where clause.
.Where(b =>
b.Things.Any(t => t.Name == "1st") |
b.Things.Any(t => t.Name == "2nd") |
b.Things.Any(t => t.Name == "3rd"))
.Include(b => b.Things);
Console.WriteLine(query.ToQueryString());
public class Thing
{
public int BunchId { get; set; }
public virtual Bunch Bunch { get; set; } = null!;
public string Name { get; set; } = null!;
}
public class Bunch
{
public int Id { get; set; }
public string Label { get; set; } = null!;
public virtual ISet<Thing> Things { get; private set; } = new HashSet<Thing>();
}
public class Ctx : DbContext
{
public DbSet<Bunch> Bunches { get; set; } = null!;
protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder) => optionsBuilder.UseNpgsql();
protected override void OnModelCreating(ModelBuilder builder)
{
builder.Entity<Bunch>().ToTable(nameof(Bunch));
builder.Entity<Thing>().HasKey(t => new { t.BunchId, t.Name });
builder.Entity<Thing>().HasOne(t => t.Bunch)
.WithMany(g => g.Things).HasForeignKey(t => t.BunchId).IsRequired();
}
} This program outputs the following SQL: SELECT b."Id", b."Label", t2."BunchId", t2."Name"
FROM "Bunch" AS b
LEFT JOIN "Thing" AS t2 ON b."Id" = t2."BunchId"
WHERE b."Label" = 'coconuts' AND (EXISTS (
SELECT 1
FROM "Thing" AS t
WHERE b."Id" = t."BunchId" AND t."Name" = '1st') OR EXISTS (
SELECT 1
FROM "Thing" AS t0
WHERE b."Id" = t0."BunchId" AND t0."Name" = '2nd')) OR EXISTS (
SELECT 1
FROM "Thing" AS t1
WHERE b."Id" = t1."BunchId" AND t1."Name" = '3rd')
ORDER BY b."Id", t2."BunchId" You may be wondering why my original code was using Or. This part of the expression tree is generated like so: Expression.Lambda<Func<T, bool>>(expressions.Skip(1).Aggregate(
expressions[0],
(accumulator, next) => Expression.Or(accumulator, next)),
anEntity) I've confirmed that the correct SQL is generated if I change this to OrElse. |
@roji, has this problem been looked at? |
@petergummer sorry, your comment slipped under my radar... This seems to be a duplicate of dotnet/efcore#30181, which is being fixed for 8.0 via new parentheses logic in dotnet/efcore#27177. So I'm going to go ahead close this issue as a duplicate of that; but if there's something else at play feel free to post back and I'll reopen as needed. |
Duplicate of dotnet/efcore#30181 |
I'm sorry to report that the problem is not fixed yet in the latest prerelease 8.0.0-preview.1, @roji. My test program above outputs exactly the same as 7.0.1 did. The extra closing parenthesis is still there: WHERE b."Id" = t0."BunchId" AND t0."Name" = '2nd')) OR EXISTS ( |
@petergummer I never wrote the bug was fixed for 8.0.0-preview.1... This is flagged as a duplicate of dotnet/efcore#30181, which, as its milestone indicates, was fixed for 8.0.0-preview.3 (out in around a month). |
Thanks @roji, good to know. I was just making sure that it didn't get lost again. |
Good news, @roji, with prerelease 8.0.0-preview.3 my test program above outputs this: SELECT b."Id", b."Label", t2."BunchId", t2."Name"
FROM "Bunch" AS b
LEFT JOIN "Thing" AS t2 ON b."Id" = t2."BunchId"
WHERE b."Label" = 'coconuts' AND (EXISTS (
SELECT 1
FROM "Thing" AS t
WHERE b."Id" = t."BunchId" AND t."Name" = '1st') OR EXISTS (
SELECT 1
FROM "Thing" AS t0
WHERE b."Id" = t0."BunchId" AND t0."Name" = '2nd') OR EXISTS (
SELECT 1
FROM "Thing" AS t1
WHERE b."Id" = t1."BunchId" AND t1."Name" = '3rd'))
ORDER BY b."Id", t2."BunchId" The parentheses are positioned correctly. Thanks! |
Thanks for the confirmation @petergummer, good to hear! |
7.0.0 is not generating correct parentheses with some queries. It generates the following SQL, which selects far too many rows. The problem appears to be that the third OR EXISTS is not correctly nested, causing the earlier AND conditions to be ignored when it's evaluated:
7.0.1-ci.20221201T124005+sha.09bc34fd0 still generates the same incorrect SQL.
6.07 generates this, which is exactly the same SQL except that it has more parentheses that select the correct rows:
The text was updated successfully, but these errors were encountered: