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

SqlNullabilityProcessor ignores Contains item visitation #32208

Closed
shvmgpt116 opened this issue Nov 2, 2023 · 6 comments · Fixed by #32214
Closed

SqlNullabilityProcessor ignores Contains item visitation #32208

shvmgpt116 opened this issue Nov 2, 2023 · 6 comments · Fixed by #32214
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 Servicing-approved type-bug
Milestone

Comments

@shvmgpt116
Copy link

I am running GIT test QueryBugsTest.Nested_contains_with_enum. The test is executing following LINQ-

var query = context.Todos
    .Where(x => keys.Contains(todoTypes.Contains(x.Type) ? key : key))
    .ToList();

The test was working fine with Oracle EFCore 7 provider. However it seems there is some regression issue introduced from the relational layer in EFCore 8 daily label, due to which the test is failing.

Here is my observation.

SqlServer EFCore 7 provider used to generate following query for it-

SELECT [t].[Id], [t].[Type]
FROM [Todos] AS [t]
WHERE CASE
    WHEN [t].[Type] = 0 THEN @__key_2
    ELSE @__key_2
END IN ('0a47bcb7-a1cb-4345-8944-c58f82d6aac7', '5f221fb9-66f4-442a-92c9-d97ed5989cc7')

However in EFCore 8 provider, the query has been changed to-

SELECT [t].[Id], [t].[Type]
FROM [Todos] AS [t]
WHERE CASE
    WHEN [t].[Type] = 0 THEN @__key_2
    ELSE @__key_2
END IN (
    SELECT [k].[value]
    FROM OPENJSON(@__keys_0) WITH ([value] uniqueidentifier '$') AS [k]
)

For the database providers which don't support OPENJSON or similar functionality, they may want to keep on generating the query which was getting generated with EFCore 7 provider.

For Ex:- Oracle EFCore 7 provider generates following query for the test-

SELECT "t"."Id", "t"."Type"
FROM "Todos" "t"
WHERE CASE
    WHEN "t"."Type" = 0 THEN :key_2
    ELSE :key_2
END IN ('B7BC470ACBA145438944C58F82D6AAC7', 'B91F225FF4662A4492C9D97ED5989CC7')

However in EFCore 8 the query has been changed to-

SELECT "t"."Id", "t"."Type"
FROM "Todos" "t"
WHERE CASE
WHEN "t"."Type" IN ( ) THEN :key_2
ELSE :key_2
END IN ('B7BC470ACBA145438944C58F82D6AAC7', 'B91F225FF4662A4492C9D97ED5989CC7')

While debugging the relational layer code (Method SqlNullabilityProcessor.VisitIn) . It seems EFCore 7 used to convert certain InExpression to SqlBinaryExpression.
For Ex:- There will be two InExpression for the above LINQ but the inner one will be converted to SqlBinaryExpression and the provider will generate [t].[Type] = 0 corresponding to the SqlBinaryExpression.
However in EFCore 8 provider, the converted SqlBinaryExpression isn't used to generate the SQL if InExpression.SubQuery is null.

Is this expected? If yes, then InExpression.Value field shouldn't be null (which is the case as of now). The InExpression which is used to generate SQL "t"."Type" IN ( ) has null value.

This seems to be an issue from the relational layer. Please could someone clarify.

@shvmgpt116
Copy link
Author

@ajcvickers
@roji

@roji
Copy link
Member

roji commented Nov 2, 2023

@shvmgpt116 thanks, I can indeed see this bug occuring also with SQL Server with QueryBugsTest.Nested_contains_with_enum, once the compatibility level is set to under 130; older versions of SQL Server don't yet support OPENJSON, so in those cases the provider is in the same state as Oracle. The It also trips the debug assertion in QuerySqlGenerator.GenerateIn. Note that this test represents a pretty rare edge case which shouldn't affect too many actual queries (but of course I'll work on a fix).

Generated SQL on SQL Server with low compatibility level
SELECT [t].[Id], [t].[Type]
FROM [Todos] AS [t]
WHERE CASE
    WHEN [t].[Type] IN (

    ) THEN @__key_2
    ELSE @__key_2
END IN ('0a47bcb7-a1cb-4345-8944-c58f82d6aac7', '5f221fb9-66f4-442a-92c9-d97ed5989cc7')

On a related note, are you planning on implementing the new JSON support for the Oracle provider? For example, the analog of SQL Server's OPENJSON seems to be the JSON_TABLE function on Oracle. I also know that Oracle has a special JSON type for storing JSON data in an optimized way; I'm currently working on making the PostgreSQL provider support that, you could do similar work on the Oracle side.

Anyway, let me know if you're interested in this direction and need any assistance!

@roji roji added the type-bug label Nov 2, 2023
@roji roji self-assigned this Nov 2, 2023
@roji roji added the area-query label Nov 2, 2023
@roji roji changed the title EFCore 8 daily label - LINQ with nested 'Contains' is generating incorrect sql SqlNullabilityProcessor ignores Contains item visitation Nov 2, 2023
@roji roji added the closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. label Nov 2, 2023
@roji
Copy link
Member

roji commented Nov 2, 2023

This turned out to be a symptom of a larger bug where we were ignoring the results of visiting item in SqlNullabilityProcessor.VisitIn; this affects more scenarios beyond this specific query type.

@shvmgpt116
Copy link
Author

@roji Thanks for confirming the issue.
I will surely ask for your guidance when working on supporting JSOM_TABLE functionality for Oracle.

@roji
Copy link
Member

roji commented Nov 3, 2023

Reopening to discuss patching 8.0.

@roji roji reopened this Nov 3, 2023
@ajcvickers ajcvickers added this to the 8.0.x milestone Nov 8, 2023
roji added a commit to roji/efcore that referenced this issue Nov 9, 2023
roji added a commit that referenced this issue Nov 13, 2023
@roji
Copy link
Member

roji commented Nov 13, 2023

Merged fix to 8.0.1 via #32265.

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 Servicing-approved type-bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants