Skip to content

Regression in PostgreSQL Contains on array #20369

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Closed
roji opened this issue Mar 21, 2020 · 5 comments · Fixed by #20489
Closed

Regression in PostgreSQL Contains on array #20369

roji opened this issue Mar 21, 2020 · 5 comments · Fixed by #20489
Assignees
Milestone

Comments

@roji
Copy link
Member

roji commented Mar 21, 2020

Since syncing EFCore.PG from commit 725adf7 to ac46335, Contains on arrays is broken. AllAnyContainsRewritingExpressionVisitor seems rewrites the Contains to Any, which then produces a translation failure. This rewriting seems problematic to me (shouldn't we leave alone calls on arrays or other stuff?), but let me know if this is intentional and I should change the provider to recognize Any on arrays instead of Contains.

@smitpatel
Copy link
Contributor

Suspected PR which adding offending code #19744

Related issue to improve #20155 (not sure if that would fix this too. Blocked on #20164

Can you also post expression tree which fails?

@roji
Copy link
Member Author

roji commented Mar 22, 2020

Sure, here you go:

[Microsoft.EntityFrameworkCore.Query.QueryRootExpression].Single(e => e.SomeArray.Contains(3))

After NormalizeQueryableMethodCall:

[Microsoft.EntityFrameworkCore.Query.QueryRootExpression].Single(e => e.SomeArray.AsQueryable().Contains(3))

After AllAnyContainsRewritingExpressionVisitor:

[Microsoft.EntityFrameworkCore.Query.QueryRootExpression].Single(e => e.SomeArray.AsQueryable().Any(p => (p == 3)))

@ajcvickers
Copy link
Contributor

I'm assuming, because we have tests for it, that Contains on byte arrays is still working. Do we special case byte arrays somewhere?

@ajcvickers ajcvickers added this to the 5.0.0 milestone Mar 23, 2020
@roji
Copy link
Member Author

roji commented Mar 25, 2020

I thought about this some more... The Npgsql provider should actually be able to translate a simple Any on arrays - as long as the tree above doesn't produce any exceptions in preprocessing and makes it to translation, I should just handle this structure. So this is probably not an issue (though let me confirm).

smitpatel added a commit that referenced this issue Apr 1, 2020
Part of #18923

Resolves #20155
Resolves #20369
We convert Queryable.Contains to Queryable.Any after navigation expansion has run so only true queraybles would have Queryable.Contains. Array properties would have Enumerable.Contains hence does not get rewritten.

Resolves #19433
smitpatel added a commit that referenced this issue Apr 1, 2020
Part of #18923

Resolves #20155
Resolves #20369
We convert Queryable.Contains to Queryable.Any after navigation expansion has run so only true queraybles would have Queryable.Contains. Array properties would have Enumerable.Contains hence does not get rewritten.

Resolves #19433
smitpatel added a commit that referenced this issue Apr 1, 2020
Part of #18923

Resolves #20155
Resolves #20369
We convert Queryable.Contains to Queryable.Any after navigation expansion has run so only true queraybles would have Queryable.Contains. Array properties would have Enumerable.Contains hence does not get rewritten.

Resolves #19433
@roji
Copy link
Member Author

roji commented Apr 15, 2020

FYI just unskipped the tests and they're green, thanks!

@ajcvickers ajcvickers modified the milestones: 5.0.0, 5.0.0-preview4 Apr 20, 2020
@ajcvickers ajcvickers modified the milestones: 5.0.0-preview4, 5.0.0 Nov 7, 2020
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants