Improve SQL Server IndexOf translation and change tests to use Where (#…

…28031)
dotnet · May 23, 2022 · d6aba24 · d6aba24
1 parent a8bf609
commit d6aba24
Show file tree

Hide file tree

Showing 8 changed files with 191 additions and 153 deletions.
diff --git a/src/EFCore.SqlServer/Query/Internal/SqlServerStringMethodTranslator.cs b/src/EFCore.SqlServer/Query/Internal/SqlServerStringMethodTranslator.cs
@@ -443,7 +443,10 @@ private SqlExpression TranslateIndexOf(
 
         if (startIndex is not null)
         {
-            charIndexArguments.Add(_sqlExpressionFactory.Add(startIndex, _sqlExpressionFactory.Constant(1)));
+            charIndexArguments.Add(
+                startIndex is SqlConstantExpression { Value : int constantStartIndex }
+                    ? _sqlExpressionFactory.Constant(constantStartIndex + 1, typeof(int))
+                    : _sqlExpressionFactory.Add(startIndex, _sqlExpressionFactory.Constant(1)));
         }
 
         var argumentsPropagateNullability = Enumerable.Repeat(true, charIndexArguments.Count);
@@ -474,6 +477,15 @@ private SqlExpression TranslateIndexOf(
 
         charIndexExpression = _sqlExpressionFactory.Subtract(charIndexExpression, _sqlExpressionFactory.Constant(1));
 
+        // If the pattern is an empty string, we need to special case to always return 0 (since CHARINDEX return 0, which we'd subtract to
+        // -1). Handle separately for constant and non-constant patterns.
+        if (searchExpression is SqlConstantExpression { Value : string constantSearchPattern })
+        {
+            return constantSearchPattern == string.Empty
+                ? _sqlExpressionFactory.Constant(0, typeof(int))
+                : charIndexExpression;
+        }
+
         return _sqlExpressionFactory.Case(
             new[]
             {

diff --git a/test/EFCore.Cosmos.FunctionalTests/Query/NorthwindFunctionsQueryCosmosTest.cs b/test/EFCore.Cosmos.FunctionalTests/Query/NorthwindFunctionsQueryCosmosTest.cs
@@ -819,39 +819,73 @@ public override async Task Indexof_with_emptystring(bool async)
         await base.Indexof_with_emptystring(async);
 
         AssertSql(
-            @"SELECT INDEX_OF(c[""ContactName""], """") AS c
+            @"SELECT c
 FROM root c
-WHERE ((c[""Discriminator""] = ""Customer"") AND (c[""CustomerID""] = ""ALFKI""))");
+WHERE ((c[""Discriminator""] = ""Customer"") AND (INDEX_OF(c[""ContactName""], """") = 0))");
     }
 
-    public override async Task Indexof_with_starting_position(bool async)
+    public override async Task Indexof_with_one_constant_arg(bool async)
     {
-        await base.Indexof_with_starting_position(async);
+        await base.Indexof_with_one_constant_arg(async);
 
         AssertSql(
-            @"SELECT INDEX_OF(c[""ContactName""], ""a"", 3) AS c
+            @"SELECT c
 FROM root c
-WHERE ((c[""Discriminator""] = ""Customer"") AND (c[""CustomerID""] = ""ALFKI""))");
+WHERE ((c[""Discriminator""] = ""Customer"") AND (INDEX_OF(c[""ContactName""], ""a"") = 1))");
+    }
+
+    public override async Task Indexof_with_one_parameter_arg(bool async)
+    {
+        await base.Indexof_with_one_parameter_arg(async);
+
+        AssertSql(
+            @"@__pattern_0='a'
+
+SELECT c
+FROM root c
+WHERE ((c[""Discriminator""] = ""Customer"") AND (INDEX_OF(c[""ContactName""], @__pattern_0) = 1))");
+    }
+
+    public override async Task Indexof_with_constant_starting_position(bool async)
+    {
+        await base.Indexof_with_constant_starting_position(async);
+
+        AssertSql(
+            @"SELECT c
+FROM root c
+WHERE ((c[""Discriminator""] = ""Customer"") AND (INDEX_OF(c[""ContactName""], ""a"", 2) = 4))");
+    }
+
+    public override async Task Indexof_with_parameter_starting_position(bool async)
+    {
+        await base.Indexof_with_parameter_starting_position(async);
+
+        AssertSql(
+            @"@__start_0='2'
+
+SELECT c
+FROM root c
+WHERE ((c[""Discriminator""] = ""Customer"") AND (INDEX_OF(c[""ContactName""], ""a"", @__start_0) = 4))");
     }
 
     public override async Task Replace_with_emptystring(bool async)
     {
         await base.Replace_with_emptystring(async);
 
         AssertSql(
-            @"SELECT REPLACE(c[""ContactName""], ""ari"", """") AS c
+            @"SELECT c
 FROM root c
-WHERE ((c[""Discriminator""] = ""Customer"") AND (c[""CustomerID""] = ""ALFKI""))");
+WHERE ((c[""Discriminator""] = ""Customer"") AND (REPLACE(c[""ContactName""], ""ia"", """") = ""Mar Anders""))");
     }
 
     public override async Task Replace_using_property_arguments(bool async)
     {
         await base.Replace_using_property_arguments(async);
 
         AssertSql(
-            @"SELECT REPLACE(c[""ContactName""], c[""ContactName""], c[""CustomerID""]) AS c
+            @"SELECT c
 FROM root c
-WHERE ((c[""Discriminator""] = ""Customer"") AND (c[""CustomerID""] = ""ALFKI""))");
+WHERE ((c[""Discriminator""] = ""Customer"") AND (REPLACE(c[""ContactName""], c[""ContactName""], c[""CustomerID""]) = c[""CustomerID""]))");
     }
 
     public override async Task Substring_with_one_arg_with_zero_startindex(bool async)
@@ -1304,16 +1338,6 @@ FROM root c
 WHERE ((c[""Discriminator""] = ""OrderDetail"") AND (c[""Quantity""] < 5))");
     }
 
-    public override async Task Indexof_with_one_arg(bool async)
-    {
-        await base.Indexof_with_one_arg(async);
-
-        AssertSql(
-            @"SELECT INDEX_OF(c[""ContactName""], ""a"") AS c
-FROM root c
-WHERE ((c[""Discriminator""] = ""Customer"") AND (c[""CustomerID""] = ""ALFKI""))");
-    }
-
     private void AssertSql(params string[] expected)
         => Fixture.TestSqlLoggerFactory.AssertBaseline(expected);
 

diff --git a/test/EFCore.Specification.Tests/Query/NorthwindFunctionsQueryTestBase.cs b/test/EFCore.Specification.Tests/Query/NorthwindFunctionsQueryTestBase.cs
@@ -1482,38 +1482,66 @@ await AssertQuery(
     [ConditionalTheory]
     [MemberData(nameof(IsAsyncData))]
     public virtual Task Indexof_with_emptystring(bool async)
-        => AssertQueryScalar(
+        => AssertQuery(
             async,
-            ss => ss.Set<Customer>().Where(c => c.CustomerID == "ALFKI").Select(c => c.ContactName.IndexOf(string.Empty)));
+            ss => ss.Set<Customer>().Where(c => c.ContactName.IndexOf(string.Empty) == 0),
+            entryCount: 91);
 
     [ConditionalTheory]
     [MemberData(nameof(IsAsyncData))]
-    public virtual Task Indexof_with_one_arg(bool async)
-        => AssertQueryScalar(
+    public virtual Task Indexof_with_one_constant_arg(bool async)
+        => AssertQuery(
             async,
-            ss => ss.Set<Customer>().Where(c => c.CustomerID == "ALFKI").Select(c => c.ContactName.IndexOf("a")));
+            ss => ss.Set<Customer>().Where(c => c.ContactName.IndexOf("a") == 1),
+            entryCount: 32);
 
     [ConditionalTheory]
     [MemberData(nameof(IsAsyncData))]
-    public virtual Task Indexof_with_starting_position(bool async)
-        => AssertQueryScalar(
+    public virtual Task Indexof_with_one_parameter_arg(bool async)
+    {
+        var pattern = "a";
+
+        return AssertQuery(
             async,
-            ss => ss.Set<Customer>().Where(c => c.CustomerID == "ALFKI").Select(c => c.ContactName.IndexOf("a", 3)));
+            ss => ss.Set<Customer>().Where(c => c.ContactName.IndexOf(pattern) == 1),
+            entryCount: 32);
+    }
+
+    [ConditionalTheory]
+    [MemberData(nameof(IsAsyncData))]
+    public virtual Task Indexof_with_constant_starting_position(bool async)
+        => AssertQuery(
+            async,
+            ss => ss.Set<Customer>().Where(c => c.ContactName.IndexOf("a", 2) == 4),
+            entryCount: 15);
+
+    [ConditionalTheory]
+    [MemberData(nameof(IsAsyncData))]
+    public virtual Task Indexof_with_parameter_starting_position(bool async)
+    {
+        var start = 2;
+
+        return AssertQuery(
+            async,
+            ss => ss.Set<Customer>().Where(c => c.ContactName.IndexOf("a", start) == 4),
+            entryCount: 15);
+    }
 
     [ConditionalTheory]
     [MemberData(nameof(IsAsyncData))]
     public virtual Task Replace_with_emptystring(bool async)
         => AssertQuery(
             async,
-            ss => ss.Set<Customer>().Where(c => c.CustomerID == "ALFKI").Select(c => c.ContactName.Replace("ari", string.Empty)));
+            ss => ss.Set<Customer>().Where(c => c.ContactName.Replace("ia", string.Empty) == "Mar Anders"),
+            entryCount: 1);
 
     [ConditionalTheory]
     [MemberData(nameof(IsAsyncData))]
     public virtual Task Replace_using_property_arguments(bool async)
         => AssertQuery(
             async,
-            ss => ss.Set<Customer>().Where(c => c.CustomerID == "ALFKI")
-                .Select(c => c.ContactName.Replace(c.ContactName, c.CustomerID)));
+            ss => ss.Set<Customer>().Where(c => c.ContactName.Replace(c.ContactName, c.CustomerID) == c.CustomerID),
+            entryCount: 91);
 
     [ConditionalTheory]
     [MemberData(nameof(IsAsyncData))]

diff --git a/test/EFCore.SqlServer.FunctionalTests/BuiltInDataTypesSqlServerTest.cs b/test/EFCore.SqlServer.FunctionalTests/BuiltInDataTypesSqlServerTest.cs
@@ -83,10 +83,7 @@ public void String_indexOf_over_varchar_max()
 
             Assert.Equal(-1, Assert.Single(results));
             AssertSql(
-                @"SELECT CASE
-    WHEN 'a' = '' THEN 0
-    ELSE CAST(CHARINDEX('a', [m].[StringAsVarcharMax]) AS int) - 1
-END
+                @"SELECT CAST(CHARINDEX('a', [m].[StringAsVarcharMax]) AS int) - 1
 FROM [MappedNullableDataTypes] AS [m]
 WHERE [m].[Int] = 81");
         }

diff --git a/test/EFCore.SqlServer.FunctionalTests/Query/NorthwindFunctionsQuerySqlServerTest.cs b/test/EFCore.SqlServer.FunctionalTests/Query/NorthwindFunctionsQuerySqlServerTest.cs
@@ -1623,55 +1623,75 @@ public override async Task Indexof_with_emptystring(bool async)
         await base.Indexof_with_emptystring(async);
 
         AssertSql(
-            @"SELECT 0
+            @"SELECT [c].[CustomerID], [c].[Address], [c].[City], [c].[CompanyName], [c].[ContactName], [c].[ContactTitle], [c].[Country], [c].[Fax], [c].[Phone], [c].[PostalCode], [c].[Region]
+FROM [Customers] AS [c]");
+    }
+
+    public override async Task Indexof_with_one_constant_arg(bool async)
+    {
+        await base.Indexof_with_one_constant_arg(async);
+
+        AssertSql(
+            @"SELECT [c].[CustomerID], [c].[Address], [c].[City], [c].[CompanyName], [c].[ContactName], [c].[ContactTitle], [c].[Country], [c].[Fax], [c].[Phone], [c].[PostalCode], [c].[Region]
 FROM [Customers] AS [c]
-WHERE [c].[CustomerID] = N'ALFKI'");
+WHERE (CAST(CHARINDEX(N'a', [c].[ContactName]) AS int) - 1) = 1");
     }
 
-    public override async Task Indexof_with_one_arg(bool async)
+    public override async Task Indexof_with_one_parameter_arg(bool async)
     {
-        await base.Indexof_with_one_arg(async);
+        await base.Indexof_with_one_parameter_arg(async);
 
         AssertSql(
-            @"SELECT CASE
-    WHEN N'a' = N'' THEN 0
-    ELSE CAST(CHARINDEX(N'a', [c].[ContactName]) AS int) - 1
-END
+            @"@__pattern_0='a' (Size = 4000)
+
+SELECT [c].[CustomerID], [c].[Address], [c].[City], [c].[CompanyName], [c].[ContactName], [c].[ContactTitle], [c].[Country], [c].[Fax], [c].[Phone], [c].[PostalCode], [c].[Region]
 FROM [Customers] AS [c]
-WHERE [c].[CustomerID] = N'ALFKI'");
+WHERE CASE
+    WHEN @__pattern_0 = N'' THEN 0
+    ELSE CAST(CHARINDEX(@__pattern_0, [c].[ContactName]) AS int) - 1
+END = 1");
+    }
+
+    public override async Task Indexof_with_constant_starting_position(bool async)
+    {
+        await base.Indexof_with_constant_starting_position(async);
+
+        AssertSql(
+            @"SELECT [c].[CustomerID], [c].[Address], [c].[City], [c].[CompanyName], [c].[ContactName], [c].[ContactTitle], [c].[Country], [c].[Fax], [c].[Phone], [c].[PostalCode], [c].[Region]
+FROM [Customers] AS [c]
+WHERE (CAST(CHARINDEX(N'a', [c].[ContactName], 3) AS int) - 1) = 4");
     }
 
-    public override async Task Indexof_with_starting_position(bool async)
+    public override async Task Indexof_with_parameter_starting_position(bool async)
     {
-        await base.Indexof_with_starting_position(async);
+        await base.Indexof_with_parameter_starting_position(async);
 
         AssertSql(
-            @"SELECT CASE
-    WHEN N'a' = N'' THEN 0
-    ELSE CAST(CHARINDEX(N'a', [c].[ContactName], 3 + 1) AS int) - 1
-END
+            @"@__start_0='2'
+
+SELECT [c].[CustomerID], [c].[Address], [c].[City], [c].[CompanyName], [c].[ContactName], [c].[ContactTitle], [c].[Country], [c].[Fax], [c].[Phone], [c].[PostalCode], [c].[Region]
 FROM [Customers] AS [c]
-WHERE [c].[CustomerID] = N'ALFKI'");
+WHERE (CAST(CHARINDEX(N'a', [c].[ContactName], @__start_0 + 1) AS int) - 1) = 4");
     }
 
     public override async Task Replace_with_emptystring(bool async)
     {
         await base.Replace_with_emptystring(async);
 
         AssertSql(
-            @"SELECT REPLACE([c].[ContactName], N'ari', N'')
+            @"SELECT [c].[CustomerID], [c].[Address], [c].[City], [c].[CompanyName], [c].[ContactName], [c].[ContactTitle], [c].[Country], [c].[Fax], [c].[Phone], [c].[PostalCode], [c].[Region]
 FROM [Customers] AS [c]
-WHERE [c].[CustomerID] = N'ALFKI'");
+WHERE REPLACE([c].[ContactName], N'ia', N'') = N'Mar Anders'");
     }
 
     public override async Task Replace_using_property_arguments(bool async)
     {
         await base.Replace_using_property_arguments(async);
 
         AssertSql(
-            @"SELECT REPLACE([c].[ContactName], [c].[ContactName], [c].[CustomerID])
+            @"SELECT [c].[CustomerID], [c].[Address], [c].[City], [c].[CompanyName], [c].[ContactName], [c].[ContactTitle], [c].[Country], [c].[Fax], [c].[Phone], [c].[PostalCode], [c].[Region]
 FROM [Customers] AS [c]
-WHERE [c].[CustomerID] = N'ALFKI'");
+WHERE REPLACE([c].[ContactName], [c].[ContactName], [c].[CustomerID]) = [c].[CustomerID]");
     }
 
     public override async Task Substring_with_one_arg_with_zero_startindex(bool async)
@@ -1753,10 +1773,7 @@ public override async Task Substring_with_two_args_with_Index_of(bool async)
         await base.Substring_with_two_args_with_Index_of(async);
 
         AssertSql(
-            @"SELECT SUBSTRING([c].[ContactName], CASE
-    WHEN N'a' = N'' THEN 0
-    ELSE CAST(CHARINDEX(N'a', [c].[ContactName]) AS int) - 1
-END + 1, 3)
+            @"SELECT SUBSTRING([c].[ContactName], (CAST(CHARINDEX(N'a', [c].[ContactName]) AS int) - 1) + 1, 3)
 FROM [Customers] AS [c]
 WHERE [c].[CustomerID] = N'ALFKI'");
     }

diff --git a/test/EFCore.SqlServer.FunctionalTests/Query/NorthwindWhereQuerySqlServerTest.cs b/test/EFCore.SqlServer.FunctionalTests/Query/NorthwindWhereQuerySqlServerTest.cs
@@ -674,13 +674,7 @@ public override async Task Where_string_indexof(bool async)
         AssertSql(
             @"SELECT [c].[CustomerID], [c].[Address], [c].[City], [c].[CompanyName], [c].[ContactName], [c].[ContactTitle], [c].[Country], [c].[Fax], [c].[Phone], [c].[PostalCode], [c].[Region]
 FROM [Customers] AS [c]
-WHERE CASE
-    WHEN N'Sea' = N'' THEN 0
-    ELSE CAST(CHARINDEX(N'Sea', [c].[City]) AS int) - 1
-END <> -1 OR CASE
-    WHEN N'Sea' = N'' THEN 0
-    ELSE CAST(CHARINDEX(N'Sea', [c].[City]) AS int) - 1
-END IS NULL");
+WHERE (CAST(CHARINDEX(N'Sea', [c].[City]) AS int) - 1) <> -1 OR [c].[City] IS NULL");
     }
 
     public override async Task Where_string_replace(bool async)