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

EF Core 8 Query Translation Bug: Array operations with union #33258

Open
elad-legit opened this issue Mar 6, 2024 · 11 comments
Open

EF Core 8 Query Translation Bug: Array operations with union #33258

elad-legit opened this issue Mar 6, 2024 · 11 comments

Comments

@elad-legit
Copy link

Description

After upgrading to EF8 from EF6, one of our complex queries including array operations stopped working ( everything worked fine with EF6 ).
After debugging, we narrowed the problem to array operation after using union or concat. ( writing the same query without using concat worked fine )

Entity Framework Query

public enum SomeEnum
{
    E1,
    E2,
}

public class Obj1
{
    public string Id { get; set; }
    public SomeEnum[] Types { get; set; }
}

public class Obj2
{
    public string Id { get; set; }
    public SomeEnum[] Types { get; set; }
}

var queryable = context.objs1.Select(_ => new { _.Id, _.Types })
     .Concat(context.objs2.Select(_ => new { _.Id, _.Types }))
    .Where(_ => _.Types.Contains(SomeEnum.E1))
    .ToList();

Stack traces

.Contains(__p_2))' could not be translated. Additional information: Translation of method 'System.Linq.Enumerable.Contains' failed. If this method can be mapped to your custom function, see https://go.microsoft.com/fwlink/?linkid=2132413 for more information. Either rewrite the query in a form that can be translated, or switch to client evaluation explicitly by inserting a call to 'AsEnumerable', 'AsAsyncEnumerable', 'ToList', or 'ToListAsync'. See https://go.microsoft.com/fwlink/?linkid=2101038 for more information.

EF Core version: 8.0.2
Database provider: Npgsql 8.0.2
Target framework: .NET 8.0

@yair-legit
Copy link

yair-legit commented Mar 6, 2024

This also happens if we try using Union instead of Concat

@roji
Copy link
Member

roji commented Mar 6, 2024

tl;dr confirmed, regression on EFCore.PG (the scenario wasn't working on other providers, since primitive collections weren't supported before 8.0). As a workaround, move the Where into the two set operation legs - while it doesn't look as good, it produces the same results and should have the same performance:

_ = context.objs1.Where(o => o.Ints.Contains(8)).Select(o => o.Ints)
    .Concat(context.objs2.Where(o => o.Ints.Contains(8)).Select(o => o.Ints))
    .ToList();

Bug analysis

For the query:

_ = context.objs1.Select(o => o.Ints)
    .Concat(context.objs2.Select(o => o.Ints))
    .Where(o => o.Contains(8))
    .ToList();

If we remove the Concat, the query works; the tree that comes out of preprocessing is:

[Microsoft.EntityFrameworkCore.Query.EntityQueryRootExpression].Where(o => Property(o, "Ints").AsQueryable().Contains(8)).Select(o => Property(o, "Ints"))

If we put the Concat, we get the following tree:

[Microsoft.EntityFrameworkCore.Query.EntityQueryRootExpression].Select(o => Property(o, "Ints")).Concat([Microsoft.EntityFrameworkCore.Query.EntityQueryRootExpression].Select(o0 => Property(o0, "Ints"))).Where(e => e.Contains(8))

In other words, if the Concat isn't there, the Select is moved forward (pending selector), and the Where is processed correctly - we end up with a queryable Contains. When the Concat is there, no such movement occurs, and the Contains remains an enumerable Contains (instead of queryable, this is why we fail the translation).

I'm not sure why we end up with an enumerable Contains when there's a set operation - @maumar any clue?

Minimal runnable repro
await using var context = new BlogContext();
await context.Database.EnsureDeletedAsync();
await context.Database.EnsureCreatedAsync();

var queryable = context.objs1.Select(o => o.Ints)
    .Concat(context.objs2.Select(o => o.Ints))
    .Where(o => o.Contains(8))
    .ToList();

public class BlogContext : DbContext
{
    public DbSet<Obj1> objs1 { get; set; }
    public DbSet<Obj2> objs2 { get; set; }

    protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
        => optionsBuilder
            .UseNpgsql("Host=localhost;Username=test;Password=test")
            .LogTo(Console.WriteLine, LogLevel.Information)
            .EnableSensitiveDataLogging();
}

public class Obj1
{
    public string Id { get; set; }
    public int[] Ints { get; set; }
}

public class Obj2
{
    public string Id { get; set; }
    public int[] Ints { get; set; }
}

@roji roji removed their assignment Mar 6, 2024
@roji
Copy link
Member

roji commented Mar 6, 2024

@elad-legit as a side comment - we use upvotes to gauge how many of our users actually run into an issue, and having lots of people around you voting for it creates a false image of that (it seems that all 20 votes above come from your company). Can you please refrain from doing that?

@elad-legit
Copy link
Author

@elad-legit as a side comment - we use upvotes to gauge how many of our users actually run into an issue, and having lots of people around you voting for it creates a false image of that (it seems that all 20 votes above come from your company). Can you please refrain from doing that?

Yes, sorry about that.

Unfortunately, our actual use case is much more complicated, and we can't easily move the where before the Concat.
Thanks for the very quick response!

@maumar maumar self-assigned this Mar 9, 2024
@yair-legit
Copy link

@maumar Is there any timeline we should expect regarding this issue?

@maumar
Copy link
Contributor

maumar commented Apr 7, 2024

@elad-legit @yair-legit Sorry for late replay. We are absolutely swamped with investigations regular dev work and other stuff, so response times are way longer than usual.
As a workaround you can apply Where operation inside Concat for both sources separately:

        var queryable = context.objs1.Select(_ => new { _.Id, _.Types }).Where(_ => _.Types.Contains(SomeEnum.E1))
            .Concat(context.objs2.Select(_ => new { _.Id, _.Types }).Where(_ => _.Types.Contains(SomeEnum.E1)))
            //.Where(_ => _.Types.Contains(SomeEnum.E1))
            .ToList();

this produces the following sql:

SELECT [o].[Id], [o].[Types]
FROM [objs1] AS [o]
WHERE 0 IN (
    SELECT [t].[value]
    FROM OPENJSON([o].[Types]) WITH ([value] int '$') AS [t]
)
UNION ALL
SELECT [o0].[Id], [o0].[Types]
FROM [objs2] AS [o0]
WHERE 0 IN (
    SELECT [t0].[value]
    FROM OPENJSON([o0].[Types]) WITH ([value] int '$') AS [t0]
)

which is not ideal, but should unblock the scenarios.

@maumar
Copy link
Contributor

maumar commented Apr 7, 2024

Problem is in nav expansion. For the case without Concat, when processing source of Where method, we break that part down to QueryRootExpression (objs) and pending selector of member access to Ints primitive collection. Then, during ExpandNavigationsForSource step, we process the pending selector, recognize that it's a primitive collection and produce PrimitiveCollectionReference. Then, during ExpandNavigationsForSource visitation we have code to process primitive collection, that appends AsQueryable call and encases everything in NavigationExpansionExpression

In the Concat case, nav expansion is not smart enough to extract pending selector (we couldn't do it even if we knew how - can't Concat two different sources, so the projections need to stay inside concat). We end up with source being QueryRoot.Select(x => x.Ints).Concat(...) and the pending selector is just identity projection. Then, ExpandingExpressionVisitor doesn't create PrimitiveCollectionReference, and the visitation thinks we it's dealing with regular NavigationTreeExpression. The AsQueryable() that we put there earlier, during NormalizeQueryableMethod gets gobbled up (as nav expansion thinks it's not needed) and we end up with the issue.

Ultimately it's a pending selector issue (#32957)

@maumar maumar added this to the 9.0.0 milestone Apr 9, 2024
@AlmightyLks

This comment was marked as duplicate.

@roji

This comment was marked as duplicate.

@AlmightyLks

This comment was marked as duplicate.

@roji

This comment was marked as duplicate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants