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

Infer type mapping suitable for concatenating two results #32510

Merged
merged 3 commits into from
Dec 4, 2023
Merged

Infer type mapping suitable for concatenating two results #32510

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b2763b5
Work on type mapping inference for string concatenation
roji Dec 1, 2023
f8b2117
Tweaks, update baselines add more tests and cleanup
ajcvickers Dec 4, 2023
e3d5640
Update baselines, add more tests, some cleanup.
ajcvickers Dec 4, 2023
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
47 changes: 47 additions & 0 deletions src/EFCore.Relational/Query/SqlExpressionFactory.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,6 +202,53 @@ private SqlExpression ApplyTypeMappingOnSqlBinary(
}

case ExpressionType.Add:
inferredTypeMapping = typeMapping;

if (inferredTypeMapping is null)
{
var leftTypeMapping = left.TypeMapping;
var rightTypeMapping = right.TypeMapping;
if (leftTypeMapping != null || rightTypeMapping != null)
{
// Infer null size (nvarchar(max)) if either side has no size.
// Note that for constants, we could instead look at the value length; but that requires we know the type mappings
// which can have a size (string/byte[], maybe something else?).
var inferredSize = leftTypeMapping?.Size is { } leftSize && rightTypeMapping?.Size is { } rightSize
? leftSize + rightSize
: (int?)null;

// Unless both sides are fixed length, the result isn't fixed length.
var inferredFixedLength = leftTypeMapping?.IsFixedLength is true && rightTypeMapping?.IsFixedLength is true;

// Default to Unicode unless both sides are non-unicode.
var inferredUnicode = !(leftTypeMapping?.IsUnicode is false && rightTypeMapping?.IsUnicode is false);
var baseTypeMapping = leftTypeMapping ?? rightTypeMapping!;

inferredTypeMapping = leftTypeMapping?.Size == inferredSize
&& leftTypeMapping?.IsFixedLength == inferredFixedLength
&& leftTypeMapping?.IsUnicode == inferredUnicode
? leftTypeMapping
: rightTypeMapping?.Size == inferredSize
&& rightTypeMapping?.IsFixedLength == inferredFixedLength
&& rightTypeMapping?.IsUnicode == inferredUnicode
? rightTypeMapping
: _typeMappingSource.FindMapping(
baseTypeMapping.ClrType,
storeTypeName: null,
keyOrIndex: false,
inferredUnicode,
inferredSize,
rowVersion: false,
inferredFixedLength,
baseTypeMapping.Precision,
baseTypeMapping.Scale);
}
}

resultType = inferredTypeMapping?.ClrType ?? left.Type;
resultTypeMapping = inferredTypeMapping;
break;

case ExpressionType.Subtract:
case ExpressionType.Multiply:
case ExpressionType.Divide:
Expand Down
60 changes: 60 additions & 0 deletions test/EFCore.Cosmos.FunctionalTests/Query/NorthwindMiscellaneousQueryCosmosTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4622,6 +4622,66 @@ await AssertTranslationFailed(
AssertSql();
}

public override async Task Contains_over_concatenated_columns_with_different_sizes(bool async)
{
await base.Contains_over_concatenated_columns_with_different_sizes(async);

AssertSql(
"""
SELECT c
FROM root c
WHERE ((c["Discriminator"] = "Customer") AND (c["CustomerID"] || c["CompanyName"]) IN ("ALFKIAlfreds Futterkiste", "ANATRAna Trujillo Emparedados y helados"))
""");
}

public override async Task Contains_over_concatenated_column_and_constant(bool async)
{
await base.Contains_over_concatenated_column_and_constant(async);

AssertSql(
"""
SELECT c
FROM root c
WHERE ((c["Discriminator"] = "Customer") AND (c["CustomerID"] || "SomeConstant") IN ("ALFKISomeConstant", "ANATRSomeConstant", "ALFKIX"))
""");
}

public override async Task Contains_over_concatenated_columns_both_fixed_length(bool async)
{
await AssertTranslationFailed(
() => base.Contains_over_concatenated_columns_both_fixed_length(async));

AssertSql();
}

public override async Task Contains_over_concatenated_column_and_parameter(bool async)
{
await base.Contains_over_concatenated_column_and_parameter(async);

AssertSql(
"""
@__someVariable_1='SomeVariable'

SELECT c
FROM root c
WHERE ((c["Discriminator"] = "Customer") AND (c["CustomerID"] || @__someVariable_1) IN ("ALFKISomeVariable", "ANATRSomeVariable", "ALFKIX"))
""");
}

public override async Task Contains_over_concatenated_parameter_and_constant(bool async)
{
await base.Contains_over_concatenated_parameter_and_constant(async);

AssertSql(
"""
@__Contains_0='true'

SELECT c
FROM root c
WHERE ((c["Discriminator"] = "Customer") AND @__Contains_0)
""");
}

private void AssertSql(params string[] expected)
=> Fixture.TestSqlLoggerFactory.AssertBaseline(expected);

Expand Down
2 changes: 1 addition & 1 deletion test/EFCore.Relational.Specification.Tests/TestUtilities/TestSqlLoggerFactory.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -254,7 +254,7 @@ void RewriteSourceWithNewBaseline(string fileName, int lineNumber)
indentBuilder.Append(" ");
var indent = indentBuilder.ToString();
var newBaseLine = $@"Assert{(forUpdate ? "ExecuteUpdate" : "")}Sql(
{string.Join("," + Environment.NewLine + indent + "//" + Environment.NewLine, SqlStatements.Skip(offset).Take(count).Select(sql => "\"\"\"" + Environment.NewLine + sql + Environment.NewLine + "\"\"\""))})";
{string.Join("," + Environment.NewLine + indent + "//" + Environment.NewLine, SqlStatements.Skip(offset).Take(count).Select(sql => indent + "\"\"\"" + Environment.NewLine + sql + Environment.NewLine + "\"\"\""))})";
var numNewlinesInRewritten = newBaseLine.Count(c => c is '\n' or '\r');

writer.Write(newBaseLine);
Expand Down
4 changes: 2 additions & 2 deletions test/EFCore.Specification.Tests/Query/GearsOfWarQueryTestBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1603,9 +1603,9 @@ public virtual Task Non_unicode_string_literals_is_used_for_non_unicode_column_w
where c.Location.Contains("Jacinto")
select c);

[ConditionalTheory]
[ConditionalTheory] // Issue #32325
[MemberData(nameof(IsAsyncData))]
public virtual Task Non_unicode_string_literals_is_used_for_non_unicode_column_with_concat(bool async)
public virtual Task Unicode_string_literals_is_used_for_non_unicode_column_with_concat(bool async)
=> AssertQuery(
async,
ss => from c in ss.Set<City>()
Expand Down
57 changes: 57 additions & 0 deletions test/EFCore.Specification.Tests/Query/NorthwindMiscellaneousQueryTestBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5674,5 +5674,62 @@ await AssertQuery(
.Select(g => new { g.Key, MaxTimestamp = g.Select(e => e.Order.OrderDate).Max() })
.OrderBy(x => x.MaxTimestamp)
.Select(x => x));
}

[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual Task Contains_over_concatenated_columns_with_different_sizes(bool async)
{
var data = new[] { "ALFKI" + "Alfreds Futterkiste", "ANATR" + "Ana Trujillo Emparedados y helados" };

return AssertQuery(
async,
ss => ss.Set<Customer>().Where(c => data.Contains(c.CustomerID + c.CompanyName)));
}

[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual Task Contains_over_concatenated_column_and_constant(bool async)
Copy link
Member

Choose a reason for hiding this comment

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

For completeness, would be good to also have tests for column+parameter, and also parameter+constant (these are evaluated as a single parameter in regular queries, but the tree is preserved for compiled queries).

{
var data = new[] { "ALFKI" + "SomeConstant", "ANATR" + "SomeConstant", "ALFKI" + "X"};

return AssertQuery(
async,
ss => ss.Set<Customer>().Where(c => data.Contains(c.CustomerID + "SomeConstant")));
}

[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual Task Contains_over_concatenated_column_and_parameter(bool async)
{
var data = new[] { "ALFKI" + "SomeVariable", "ANATR" + "SomeVariable", "ALFKI" + "X" };
var someVariable = "SomeVariable";

return AssertQuery(
async,
ss => ss.Set<Customer>().Where(c => data.Contains(c.CustomerID + someVariable)));
}

[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual Task Contains_over_concatenated_parameter_and_constant(bool async)
{
var data = new[] { "ALFKI" + "SomeConstant", "ANATR" + "SomeConstant" };
var someVariable = "ALFKI";

return AssertQuery(
async,
ss => ss.Set<Customer>().Where(c => data.Contains(someVariable + "SomeConstant")));
}

[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual Task Contains_over_concatenated_columns_both_fixed_length(bool async)
{
var data = new[] { "ALFKIALFKI", "ALFKI", "ANATR" + "Ana Trujillo Emparedados y helados", "ANATR" + "ANATR"};

return AssertQuery(
async,
ss => ss.Set<Order>().Where(o => data.Contains(o.CustomerID + o.Customer.CustomerID)));
}
}
4 changes: 2 additions & 2 deletions test/EFCore.SqlServer.FunctionalTests/BulkUpdates/NorthwindBulkUpdatesSqlServerTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1038,8 +1038,8 @@ public override async Task Update_Where_set_property_plus_parameter(bool async)
await base.Update_Where_set_property_plus_parameter(async);

AssertExecuteUpdateSql(
"""
@__value_0='Abc' (Size = 30)
"""
@__value_0='Abc' (Size = 4000)

UPDATE [c]
SET [c].[ContactName] = COALESCE([c].[ContactName], N'') + @__value_0
Expand Down
Loading
Loading