Skip to content

Fix SQL Server translation of IndexOf#33876

Merged
maumar merged 4 commits intodotnet:mainfrom
ranma42:indexof-18773
Jun 7, 2024
Merged

Fix SQL Server translation of IndexOf#33876
maumar merged 4 commits intodotnet:mainfrom
ranma42:indexof-18773

Conversation

@ranma42
Copy link
Contributor

@ranma42 ranma42 commented Jun 2, 2024

This updates the translation of IndexOf on SQL Server so that it propagates nullability in the standard way.
This fixes the mismatch reported in #18773 (comment) and effectively aligns it with the current behavior of the SQLite translation.

Fixes #18773

Comment on lines -1390 to +1392
SELECT 0
SELECT CASE
WHEN [c].[ContactName] IS NOT NULL THEN 0
END
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the fact that this test was passing is related to the fact that the dataset is initialized with a specific set of values; in particular, even though the ContactName column is nullable, none of the records in the dataset have a null ContactName

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could switch to using Region instead of ContactName:

    [ConditionalTheory]
    [MemberData(nameof(IsAsyncData))]
    public virtual Task Select_with_complex_expression_that_can_be_funcletized(bool async)
        => AssertQueryScalar(
            async,
            ss => ss.Set<Customer>().Where(c => c.CustomerID == "ALFKI").Select(c => (int?)c.Region.IndexOf("")),
            ss => ss.Set<Customer>().Where(c => c.CustomerID == "ALFKI").Select(c => c.Region == null ? (int?)null : 0),
            assertOrder: true);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I started using Region in two tests that were explicitly testing the behavior of IndexOf("") and, as expected, they started failing on SQL Server unless the patch is applied. Thank you for your suggestion! @maumar

Unfortunately this change also introduces a failure on Cosmos... I guess it's time for me to try and install the cosmos emulator

@cincuranet cincuranet self-requested a review June 3, 2024 20:12
@ranma42 ranma42 marked this pull request as draft June 4, 2024 22:23
@ranma42 ranma42 marked this pull request as ready for review June 5, 2024 06:50
@ranma42
Copy link
Contributor Author

ranma42 commented Jun 5, 2024

I updated the Cosmos baselines (this is the delta), but apparently several tests are currently failing on the main branch (hence also on the merged PR+main).

@maumar
Copy link
Contributor

maumar commented Jun 5, 2024

I updated the Cosmos baselines (this is the delta), but apparently several tests are currently failing on the main branch (hence also on the merged PR+main).

@roji, looks like the PG JSON work port (665549a) is the culprit

@roji
Copy link
Member

roji commented Jun 5, 2024

@maumar yeah, just submitted #33908 to revert the offending part.

ranma42 added 4 commits June 6, 2024 15:13
This change causes additional failures in SQL Server because its translation
does not propagates nulls.
so that it propagates nullability in the standard way.
@maumar maumar merged commit 61f1902 into dotnet:main Jun 7, 2024
@maumar
Copy link
Contributor

maumar commented Jun 7, 2024

thanks @ranma42 !

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Query: simplify IndexOf translation

5 participants