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

Query: null semantics not applied on subquery.Contains(null) #19499

Closed
maumar opened this issue Jan 6, 2020 · 3 comments · Fixed by #19744
Closed

Query: null semantics not applied on subquery.Contains(null) #19499

maumar opened this issue Jan 6, 2020 · 3 comments · Fixed by #19744
Assignees
Labels
area-query closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. type-bug
Milestone

Comments

@maumar
Copy link
Contributor

maumar commented Jan 6, 2020

Query:

context.LevelOne.Select(t => t.OneToOne_Optional_FK1).Contains(null);

this should return true, however we generate the following sql:

SELECT CASE
    WHEN NULL IN (
        SELECT [l0].[Id]
        FROM [LevelOne] AS [l]
        LEFT JOIN [LevelTwo] AS [l0] ON [l].[Id] = [l0].[Level1_Optional_Id]
    ) THEN CAST(1 AS bit)
    ELSE CAST(0 AS bit)
END

which evaluates to false

We should perhaps convert to

context.LevelOne.Select(t => t.OneToOne_Optional_FK1).Any(x => x == null);

which works correctly

@maumar maumar self-assigned this Jan 6, 2020
@ajcvickers ajcvickers added this to the 5.0.0 milestone Jan 10, 2020
@maumar maumar changed the title Query: optimize NULL IN (subquery) Query: null semantics not applied on subquery.Contains(null) Jan 25, 2020
@maumar maumar removed this from the 5.0.0 milestone Jan 25, 2020
@maumar
Copy link
Contributor Author

maumar commented Jan 28, 2020

We can do the transformation as part of pre-processing (AllAnyToContainsRewritingExpressionVisitor). However the problem is that we ONLY need to do this if the item argument of the Contains is nullable. We can do it always, but that results in uglier SQL, e.g.

old:

@__id_0='1'

SELECT [e].[Id], [e].[Name]
FROM [Entities] AS [e]
WHERE [e].[Id] IN (
    SELECT [e0].[Id]
    FROM [Entities] AS [e0]
    WHERE [e0].[Id] = @__id_0
)

new:

@__id_0='1'

SELECT [e].[Id], [e].[Name]
FROM [Entities] AS [e]
WHERE EXISTS (
    SELECT 1
    FROM [Entities] AS [e0]
    WHERE ([e0].[Id] = @__id_0) AND ([e0].[Id] = [e].[Id]))

Alternatively we could recognize the pattern during null semantics rewrite, when we know the nullability of the item. However that would require to synthesize a new SelectExpression, so not sure if its a good idea. @smitpatel thoughts?

maumar added a commit that referenced this issue Jan 30, 2020
…s(null)

We used to translate this into `NULL IN subquery` pattern, but this doesn't work because it doesn't match if the subquery also contains null.
Fix is to convert this into subquery.Any(e => e == NULL), which translates to EXISTS with predicate and we can correctly apply null semantics.
Also it allows us to translate contains on entities with composite keys.

Also made several small fixes:
- marked EXISTS and IN expressions as never nullable for purpose of null semantics,
- optimized EXISITS (subquery) with predicate that resolves to false, directly into false, since empty subquery never exisits,
- improves expression printer output for IN expression in the subquery scenario.
@maumar maumar added this to the 5.0.0 milestone Jan 30, 2020
maumar added a commit that referenced this issue Jan 30, 2020
…s(null)

We used to translate this into `NULL IN subquery` pattern, but this doesn't work because it doesn't match if the subquery also contains null.
Fix is to convert this into subquery.Any(e => e == NULL), which translates to EXISTS with predicate and we can correctly apply null semantics.
Also it allows us to translate contains on entities with composite keys.

Also made several small fixes:
- marked EXISTS and IN expressions as never nullable for purpose of null semantics,
- optimized EXISITS (subquery) with predicate that resolves to false, directly into false, since empty subquery never exisits,
- improves expression printer output for IN expression in the subquery scenario.
maumar added a commit that referenced this issue Jan 30, 2020
…s(null)

We used to translate this into `NULL IN subquery` pattern, but this doesn't work because it doesn't match if the subquery also contains null.
Fix is to convert this into subquery.Any(e => e == NULL), which translates to EXISTS with predicate and we can correctly apply null semantics.
Also it allows us to translate contains on entities with composite keys.

Also made several small fixes:
- marked EXISTS and IN expressions as never nullable for purpose of null semantics,
- optimized EXISITS (subquery) with predicate that resolves to false, directly into false, since empty subquery never exisits,
- improves expression printer output for IN expression in the subquery scenario.
@smitpatel
Copy link
Member

Run query perf numbers on both the generated SQL, then we can see which SQL is more beneficial and thing about it.

maumar added a commit that referenced this issue Feb 22, 2020
…s(null)

We used to translate this into `NULL IN subquery` pattern, but this doesn't work because it doesn't match if the subquery also contains null.
Fix is to convert this into subquery.Any(e => e == NULL), which translates to EXISTS with predicate and we can correctly apply null semantics.
Also it allows us to translate contains on entities with composite keys.

Also made several small fixes:
- marked EXISTS and IN expressions as never nullable for purpose of null semantics,
- optimized EXISITS (subquery) with predicate that resolves to false, directly into false, since empty subquery never exisits,
- improves expression printer output for IN expression in the subquery scenario.
maumar added a commit that referenced this issue Feb 28, 2020
…s(null)

We used to translate this into `NULL IN subquery` pattern, but this doesn't work because it doesn't match if the subquery also contains null.
Fix is to convert this into subquery.Any(e => e == NULL), which translates to EXISTS with predicate and we can correctly apply null semantics.
Also it allows us to translate contains on entities with composite keys.

Also made several small fixes:
- marked EXISTS as never nullable for purpose of null semantics,
- marked IN as never nullable for purpose of null semantics when both the subquery projection element and the item expression are not nullable,
- optimized EXISITS (subquery) and IN (subquery) with predicate that resolves to false, directly into false, since empty subquery never exisits and doesn't contain any results,
- improves expression printer output for IN expression in the subquery scenario.
maumar added a commit that referenced this issue Mar 4, 2020
…s(null)

We used to translate this into `NULL IN subquery` pattern, but this doesn't work because it doesn't match if the subquery also contains null.
Fix is to convert this into subquery.Any(e => e == NULL), which translates to EXISTS with predicate and we can correctly apply null semantics.
Also it allows us to translate contains on entities with composite keys.

Also made several small fixes:
- marked EXISTS as never nullable for purpose of null semantics,
- marked IN as never nullable for purpose of null semantics when both the subquery projection element and the item expression are not nullable,
- optimized EXISITS (subquery) and IN (subquery) with predicate that resolves to false, directly into false, since empty subquery never exisits and doesn't contain any results,
- improves expression printer output for IN expression in the subquery scenario.
@maumar maumar added the closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. label Mar 4, 2020
@maumar
Copy link
Contributor Author

maumar commented Mar 4, 2020

fixed in b84eec1

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

Successfully merging a pull request may close this issue.

3 participants