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

LINQ "Contains" generates SQL with wrong char length #32325

Closed
CardenInsurance opened this issue Nov 16, 2023 · 4 comments · Fixed by #32510
Closed

LINQ "Contains" generates SQL with wrong char length #32325

CardenInsurance opened this issue Nov 16, 2023 · 4 comments · Fixed by #32510
Assignees
Labels
area-query closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. customer-reported regression Servicing-approved type-bug
Milestone

Comments

@CardenInsurance
Copy link

CardenInsurance commented Nov 16, 2023

File a bug

My program worked correctly with EF7, but when I upgraded to EF8,I encountered this problem. When I use Contains(x => x.Property1 + ";" + x.Property2) in a LINQ query, the generated SQL uses a variable length limited to the length of only Property1, so the Contains() always returns zero results.

This also happens with varchar columns, not just char columns.

Include your code

Full demo attached.
snippet from my attached demo:

List<string> queryData = ["AAA;BBBBB", "AAA;CCCCC"];
List<Person> results = dbContext.Persons.Where(x => queryData.Contains(x.ThreeCharacterProperty + ";" + x.FiveCharacterProperty)).ToList();

Generated SQL:

exec sp_executesql N'SELECT [p].[Id], [p].[FiveCharacterProperty], [p].[ThreeCharacterProperty]
FROM [Persons] AS [p]
WHERE [p].[ThreeCharacterProperty] + '';'' + [p].[FiveCharacterProperty] IN (
    SELECT [q].[value]
    FROM OPENJSON(@__queryData_0) WITH ([value] char(3) ''$'') AS [q]
)',N'@__queryData_0 nvarchar(4000)',@__queryData_0=N'["AAA;BBBBB","AAA;CCCCC"]'

// the problem can be fixed by changing char(3) to char(9) because 9 = 3 + 1 + 5

BugRepro1.zip

Include provider and version information

EF Core version: 8.0.0
Database provider: Microsoft.EntityFrameworkCore.SqlServer
Target framework:.NET 8.0
Operating system: Windows 10 Pro 22H2
IDE: Visual Studio 2022 17.8.0

@roji
Copy link
Member

roji commented Nov 17, 2023

Minimal repro:

await using var context = new BlogContext();  
await context.Database.EnsureDeletedAsync();  
await context.Database.EnsureCreatedAsync();  
  
context.Persons.Add(new() { ThreeCharacterProperty = "AAA", FiveCharacterProperty = "BBBBB" });  
context.SaveChanges();  
  
var queryData = new[] { "AAA;BBBBB", "AAA;CCCCC" };  
Console.WriteLine(context.Persons.Count(x => queryData.Contains(x.ThreeCharacterProperty + ";" + x.FiveCharacterProperty)));  
  
public class BlogContext : DbContext  
{  
    public DbSet<Person> Persons { get; set; }  
  
    protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)  
        => optionsBuilder  
            .UseSqlServer(@"Server=localhost;Database=test;User=SA;Password=Abcd5678;Connect Timeout=60;ConnectRetryCount=0;Encrypt=false")  
            .LogTo(Console.WriteLine, LogLevel.Information)  
            .EnableSensitiveDataLogging();  
}  
  
public class Person  
{  
    public int Id { get; set; }  
    [Column(TypeName = "char(3)")]  
    public string ThreeCharacterProperty { get; set; }  
    [Column(TypeName = "char(5)")]  
    public string FiveCharacterProperty { get; set; }  
}

What's happening here is that our type inference logic for concatenation (and other binary types) simply selects the left type mapping when there are mappings on both sides. This bubbles up the tree, and then gets inferred and applied to the columns coming out of the OPENJSON expression. I'm pretty sure we already have other bugs because of this.

The correct fix here would be to improve our type inference logic, so that instead of just taking the left mapping we'd combine the two (opened #32333). We may also be able to do something more narrowly-targeted that would make sense in a patch.

@CardenInsurance
Copy link
Author

A temporary workaround that helped me with this issue was to disable EF8's JSON features by adding this to my dbcontext.

protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
{
    optionsBuilder.UseSqlServer(connectionString, x => x.UseCompatibilityLevel(120));
}

@ajcvickers ajcvickers added this to the 8.0.x milestone Nov 29, 2023
@MasaMatsu
Copy link

MasaMatsu commented Nov 30, 2023

I have same problem.
I'll try "Convert.ToString()" tomorrow on my office.

It will be nvarchar (max)

@MasaMatsu
Copy link

MasaMatsu commented Nov 30, 2023

I have same problem.
I'll try "Convert.ToString()" tomorrow on my office.

It will be nvarchar (max)

It was a little different from the result I imagined, but it works.

Console.WriteLine(
    context.Persons.Count(x =>
        queryData.Contains(
            Convert.ToString(x.ThreeCharacterProperty + ";" + x.FiveCharacterProperty)
        )
    )
);

it is translated to

SELECT COUNT(*)
FROM [Persons] AS [p]
WHERE EXISTS (
    SELECT 1
    FROM OPENJSON(@__queryData_0) WITH ([value] nvarchar(max) '$') AS [q]
    WHERE [q].[value] = CONVERT(nvarchar(max), [p].[ThreeCharacterProperty] + ';' + [p].[FiveCharacterProperty]))

This idea may be effective if you need to take temporary action until the modified version comes out.

roji added a commit to roji/efcore that referenced this issue Dec 1, 2023
roji added a commit to roji/efcore that referenced this issue Dec 2, 2023
ajcvickers pushed a commit that referenced this issue Dec 4, 2023
ajcvickers added a commit that referenced this issue Dec 4, 2023
* Work on type mapping inference for string concatenation

Fixes #32325, see also #32333

* Tweaks, update baselines add more tests and cleanup

* Update baselines, add more tests, some cleanup.

---------

Co-authored-by: Shay Rojansky <roji@roji.org>
@ajcvickers ajcvickers reopened this Dec 4, 2023
@roji roji assigned ajcvickers and unassigned roji Dec 4, 2023
ajcvickers added a commit that referenced this issue Dec 5, 2023
* Work on type mapping inference for string concatenation

Fixes #32325, see also #32333

* Tweaks, update baselines add more tests and cleanup

* Update baselines, add more tests, some cleanup.

---------

Co-authored-by: Shay Rojansky <roji@roji.org>
@ajcvickers ajcvickers modified the milestones: 8.0.x, 8.0.2 Dec 5, 2023
@ajcvickers ajcvickers added the closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. label Dec 5, 2023
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. customer-reported regression Servicing-approved type-bug
Projects
None yet
4 participants