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: simplify IndexOf translation #18773

Closed
maumar opened this issue Nov 5, 2019 · 3 comments · Fixed by #33876
Closed

Query: simplify IndexOf translation #18773

maumar opened this issue Nov 5, 2019 · 3 comments · Fixed by #33876
Labels
area-query breaking-change closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. community-contribution type-enhancement
Milestone

Comments

@maumar
Copy link
Contributor

maumar commented Nov 5, 2019

Currently query like:

es.Where(e => e.NullableStringA.IndexOf("oo") == e.NullableIntA).Select(e => e.Id)

gets translated to:

SELECT [e].[Id]
FROM [Entities1] AS [e]
WHERE (CASE
    WHEN N'oo' = N'' THEN 0
    ELSE CHARINDEX(N'oo', [e].[NullableStringA]) - 1
END = [e].[NullableIntA]) OR (CASE
    WHEN N'oo' = N'' THEN 0
    ELSE CHARINDEX(N'oo', [e].[NullableStringA]) - 1
END IS NULL AND [e].[NullableIntA] IS NULL)

This seems too complicated. While it's true that if argument is and empty string we can short circuit and not compute the result of CHARINDEX function it doesn't seem too common (usually queries are on indexof different than emtpy).
Another reason might have been that for cases when target is null, and argument is empty string we return 0, rather than null (i.e. any string starts with empty string - which is consistent with out mental model of null meaning unknown value). However this scenario is invalid on c# already (null ref) and seems like sacrificing 99% scenario for the sake of 1%.

Instead, we could simply translate to:

SELECT [e].[Id]
FROM [Entities1] AS [e]
WHERE CHARINDEX(N'oo', [e].[NullableStringA]) - 1 = [e].[NullableIntA]
OR (CHARINDEX(N'oo', [e].[NullableStringA]) IS NULL AND [e].[NullableIntA] IS NULL)

which, once #18555 is done, can be further optimized to:

SELECT [e].[Id]
FROM [Entities1] AS [e]
WHERE CHARINDEX(N'oo', [e].[NullableStringA]) - 1 = [e].[NullableIntA]
OR ([e].[NullableStringA] IS NULL AND [e].[NullableIntA] IS NULL)
@maumar
Copy link
Contributor Author

maumar commented Nov 7, 2019

Also this scenario yields different results between SqlServer and Sqlite, when the argument to IndexOf is empty string, i.e.

es.Where(e => e.NullableStringA.IndexOf("") == e.NullableIntA)

Sqlite produces:

SELECT "e"."Id"
FROM "Entities1" AS "e"
WHERE ((instr("e"."NullableStringA", '') - 1) = "e"."NullableIntA") OR (instr("e"."NullableStringA", '') IS NULL AND "e"."NullableIntA" IS NULL)

@ajcvickers ajcvickers added this to the Backlog milestone Nov 8, 2019
@ajcvickers
Copy link
Contributor

Theoretically, could do fancy parameter snipping to special case empty string.

@ranma42
Copy link
Contributor

ranma42 commented Jun 2, 2024

Currently the translation for

ss.Set<NullSemanticsEntity1>().Where(e => e.NullableStringA.IndexOf("oo") == e.NullableIntA).Select(e => e.Id)

(see here) is

SELECT [e].[Id]
FROM [Entities1] AS [e]
WHERE CAST(CHARINDEX(N'oo', [e].[NullableStringA]) AS int) - 1 = [e].[NullableIntA]
OR ([e].[NullableStringA] IS NULL AND [e].[NullableIntA] IS NULL)

(see here).

Is there any further improvement that we should look for? (I see that there is a CAST in addition to the expected query)

Regarding the mismatch in the results from SQLite and SQL Server, it was apparently caused by the non-propagation of NULL when the pattern was the empty string; I tried to fix it in #33876.

@maumar maumar modified the milestones: Backlog, 9.0.0 Jun 7, 2024
@maumar maumar added closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. and removed needs-design labels Jun 7, 2024
maumar pushed a commit that referenced this issue Jun 7, 2024
This updates the translation of IndexOf on SQL Server so that it propagates nullability in the standard way  and effectively aligns it with the current behavior of the SQLite translation.

Fixes #18773
@ajcvickers ajcvickers modified the milestones: 9.0.0, 9.0.0-preview6 Jun 7, 2024
@roji roji modified the milestones: 9.0.0-preview6, 9.0.0 Oct 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-query breaking-change closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. community-contribution type-enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants