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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 2 commits into from
May 23, 2022
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
57f7b5c
Change IndexOf/Replace tests to use Where instead of projection
roji May 16, 2022
c6a17e8
Improve SQL Server translation of string.IndexOf with constants
roji May 16, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 13 additions & 1 deletion src/EFCore.SqlServer/Query/Internal/SqlServerStringMethodTranslator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 }
Copy link
Member

Choose a reason for hiding this comment

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

nice, didn't know about this nested declaration.

Copy link
Member

Choose a reason for hiding this comment

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

To avoid this kind of missing things, probably we should do this as optimization in post-translation phase (arithmetic between 2 constant operations), can you file an issue?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah pattern matching has made some nice progress :)

#28036

? _sqlExpressionFactory.Constant(constantStartIndex + 1, typeof(int))
: _sqlExpressionFactory.Add(startIndex, _sqlExpressionFactory.Constant(1)));
}

var argumentsPropagateNullability = Enumerable.Repeat(true, charIndexArguments.Count);
Expand Down Expand Up @@ -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[]
{
Expand Down
64 changes: 44 additions & 20 deletions test/EFCore.Cosmos.FunctionalTests/Query/NorthwindFunctionsQueryCosmosTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -807,39 +807,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)
Expand Down Expand Up @@ -1292,16 +1326,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);

Expand Down
50 changes: 39 additions & 11 deletions test/EFCore.Specification.Tests/Query/NorthwindFunctionsQueryTestBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1406,38 +1406,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))]
Expand Down
5 changes: 1 addition & 4 deletions test/EFCore.SqlServer.FunctionalTests/BuiltInDataTypesSqlServerTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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");
}
Expand Down
65 changes: 41 additions & 24 deletions test/EFCore.SqlServer.FunctionalTests/Query/NorthwindFunctionsQuerySqlServerTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1571,55 +1571,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)
Expand Down Expand Up @@ -1701,10 +1721,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'");
}
Expand Down
8 changes: 1 addition & 7 deletions test/EFCore.SqlServer.FunctionalTests/Query/NorthwindWhereQuerySqlServerTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
Loading