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

Make sort order consistent in split queries #34097

Merged
merged 3 commits into from
Oct 10, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
20 changes: 12 additions & 8 deletions src/EFCore.Relational/Query/SqlExpressions/SelectExpression.cs
Original file line number Diff line number Diff line change
Expand Up @@ -912,6 +912,18 @@ static Expression RemoveConvert(Expression expression)
{
var outerSelectExpression = (SelectExpression)cloningExpressionVisitor!.Visit(baseSelectExpression!);

// Inject deterministic orderings (the identifier columns) to both the main query and the split query.
// Note that just below we pushdown the split query if it has limit/offset/distinct/groupby; this ensures
// that the orderings are also propagated to that split subquery if it has limit/offset, which ensures that
// that subquery returns the same rows as the main query (#26808)
var actualParentIdentifier = _identifier.Take(outerSelectExpression._identifier.Count).ToList();
for (var j = 0; j < actualParentIdentifier.Count; j++)
{
AppendOrdering(new OrderingExpression(actualParentIdentifier[j].Column, ascending: true));
outerSelectExpression.AppendOrdering(
new OrderingExpression(outerSelectExpression._identifier[j].Column, ascending: true));
}

if (outerSelectExpression.Limit != null
|| outerSelectExpression.Offset != null
|| outerSelectExpression.IsDistinct
Expand All @@ -923,7 +935,6 @@ static Expression RemoveConvert(Expression expression)
innerSelectExpression = sqlRemappingVisitor.Remap(innerSelectExpression);
}

var actualParentIdentifier = _identifier.Take(outerSelectExpression._identifier.Count).ToList();
var containsOrdering = innerSelectExpression.Orderings.Count > 0;
List<OrderingExpression>? orderingsToBeErased = null;
if (containsOrdering
Expand All @@ -941,13 +952,6 @@ static Expression RemoveConvert(Expression expression)
outerSelectExpression._aliasForClientProjections.AddRange(innerSelectExpression._aliasForClientProjections);
innerSelectExpression = outerSelectExpression;

for (var j = 0; j < actualParentIdentifier.Count; j++)
{
AppendOrdering(new OrderingExpression(actualParentIdentifier[j].Column, ascending: true));
innerSelectExpression.AppendOrdering(
new OrderingExpression(innerSelectExpression._identifier[j].Column, ascending: true));
}

// Copy over any nested ordering if there were any
if (containsOrdering)
{
Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 0 additions & 3 deletions src/EFCore.SqlServer/Properties/SqlServerStrings.resx
Original file line number Diff line number Diff line change
Expand Up @@ -321,9 +321,6 @@
<data name="SequenceBadType" xml:space="preserve">
<value>SQL Server sequences cannot be used to generate values for the property '{property}' on entity type '{entityType}' because the property type is '{propertyType}'. Sequences can only be used with integer properties.</value>
</data>
<data name="SplitQueryOffsetWithoutOrderBy" xml:space="preserve">
<value>The query uses 'Skip' without specifying ordering and uses split query mode. This generates incorrect results. Either provide ordering or run query in single query mode using `AsSingleQuery()`. See https://go.microsoft.com/fwlink/?linkid=2196526 for more information.</value>
</data>
<data name="TemporalAllEntitiesMappedToSameTableMustBeTemporal" xml:space="preserve">
<value>Entity type '{entityType}' should be marked as temporal because it shares table mapping with another entity that has been marked as temporal. Alternatively, other entity types that share the same table must be non-temporal.</value>
</data>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ public class SqlServerQueryTranslationPostprocessor : RelationalQueryTranslation
{
private readonly SqlServerJsonPostprocessor _jsonPostprocessor;
private readonly SqlServerAggregateOverSubqueryPostprocessor _aggregatePostprocessor;
private readonly SkipWithoutOrderByInSplitQueryVerifier _skipWithoutOrderByInSplitQueryVerifier = new();
private readonly SqlServerSqlTreePruner _pruner = new();

/// <summary>
Expand Down Expand Up @@ -49,7 +48,6 @@ public override Expression Process(Expression query)

var query2 = _jsonPostprocessor.Process(query1);
var query3 = _aggregatePostprocessor.Visit(query2);
_skipWithoutOrderByInSplitQueryVerifier.Visit(query3);

return query3;
}
Expand All @@ -72,37 +70,4 @@ protected override Expression ProcessTypeMappings(Expression expression)
/// </summary>
protected override Expression Prune(Expression query)
=> _pruner.Prune(query);

private sealed class SkipWithoutOrderByInSplitQueryVerifier : ExpressionVisitor
{
[return: NotNullIfNotNull(nameof(expression))]
public override Expression? Visit(Expression? expression)
{
switch (expression)
{
case ShapedQueryExpression shapedQueryExpression:
Visit(shapedQueryExpression.ShaperExpression);
return shapedQueryExpression;

case RelationalSplitCollectionShaperExpression relationalSplitCollectionShaperExpression:
foreach (var table in relationalSplitCollectionShaperExpression.SelectExpression.Tables)
{
Visit(table);
}

Visit(relationalSplitCollectionShaperExpression.InnerShaper);

return relationalSplitCollectionShaperExpression;

case SelectExpression { Offset: not null, Orderings.Count: 0 }:
throw new InvalidOperationException(SqlServerStrings.SplitQueryOffsetWithoutOrderBy);

case UpdateExpression or DeleteExpression:
return expression;

default:
return base.Visit(expression);
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -277,6 +277,7 @@ ORDER BY [o].[Id]
SELECT TOP(1) [o].[Id]
FROM [Offers] AS [o]
WHERE [o].[Id] = 1
ORDER BY [o].[Id]
) AS [o0]
INNER JOIN (
SELECT [v].[Id], [v].[NestedId], [v].[OfferId], [v].[payment_brutto], [v].[payment_netto], [n].[Id] AS [Id0], [n].[payment_brutto] AS [payment_brutto0], [n].[payment_netto] AS [payment_netto0]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3437,6 +3437,7 @@ ORDER BY [l].[Id]
SELECT TOP(1) [l].[Id]
FROM [LevelOne] AS [l]
WHERE [l].[Id] = 1
ORDER BY [l].[Id]
) AS [l3]
INNER JOIN [LevelTwo] AS [l2] ON [l3].[Id] = [l2].[OneToMany_Optional_Inverse2Id]
ORDER BY [l3].[Id]
Expand Down Expand Up @@ -3965,7 +3966,7 @@ SELECT TOP(1) [l].[Id], [l0].[Id] AS [Id0], [l].[Name]
FROM [LevelOne] AS [l]
LEFT JOIN [LevelTwo] AS [l0] ON [l].[Id] = [l0].[Level1_Optional_Id]
WHERE [l].[Id] = 2
ORDER BY [l].[Name]
ORDER BY [l].[Name], [l].[Id], [l0].[Id]
) AS [s]
CROSS APPLY (
SELECT [l21].[Id], [l21].[Date], [l21].[Name], [l21].[OneToMany_Optional_Self_Inverse1Id], [l21].[OneToMany_Required_Self_Inverse1Id], [l21].[OneToOne_Optional_Self1Id], (
Expand Down
Loading