Skip to content

Commit

Permalink
Fix to #30922 - Most cases of projecting a primitive collection don't…
Browse files Browse the repository at this point in the history 
… work

Problem was that for cases that compose on the collection of primitives and then project it, we need identifier to properly bucket the results. On SqlServer we can use hidden key column that is a result of OPENJSON - we need something that is unique so we can't use the value itself.
Fix is to add identifier to the SelectExpression based on OPENJSON and also modify the logic that figures out if we need key (and thus need to revert to OPENJSON without WITH clause). Also some minor fixes around nullability - when projecting element of a collection of primitives using OUTER APPLY/JOIN we must set the projection binding to nullable, as the value can always be null, in case of empty collection.

Fixes #30922
  • Loading branch information
@maumar
maumar committed Jul 15, 2023
1 parent c50e2f6 commit d3d784b
Show file tree
Hide file tree
Showing 8 changed files with 480 additions and 67 deletions.
62 changes: 57 additions & 5 deletions src/EFCore.Relational/Query/SqlExpressions/SelectExpression.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,51 @@ public SelectExpression(
_projectionMapping[new ProjectionMember()] = columnExpression;
}

/// <summary>
/// Creates a new instance of the <see cref="SelectExpression" /> class given a <see cref="TableExpressionBase" />, with a single
/// column projection.
/// </summary>
/// <param name="identidierColumnName">The name of the column to use as identifier.</param>
/// <param name="identifierColumnType">The type of the column to use as identifier.</param>
/// <param name="identifierColumnTypeMapping">The type mapping of the column to use as identifier.</param>
/// <param name="tableExpression">The table expression.</param>
/// <param name="columnName">The name of the column to add as the projection.</param>
/// <param name="columnType">The type of the column to add as the projection.</param>
/// <param name="columnTypeMapping">The type mapping of the column to add as the projection.</param>
/// <param name="isColumnNullable">Whether the column projected out is nullable.</param>
public SelectExpression(
TableExpressionBase tableExpression,
string identidierColumnName,
Type identifierColumnType,
RelationalTypeMapping? identifierColumnTypeMapping,
string columnName,
Type columnType,
RelationalTypeMapping? columnTypeMapping,
bool? isColumnNullable = null)
: base(null)
{
var tableReferenceExpression = new TableReferenceExpression(this, tableExpression.Alias!);
AddTable(tableExpression, tableReferenceExpression);

var identifierColumn = new ConcreteColumnExpression(
identidierColumnName,
tableReferenceExpression,
identifierColumnType.UnwrapNullableType(),
identifierColumnTypeMapping,
identifierColumnType.IsNullableType());

var columnExpression = new ConcreteColumnExpression(
columnName,
tableReferenceExpression,
columnType.UnwrapNullableType(),
columnTypeMapping,
isColumnNullable ?? columnType.IsNullableType());

_projectionMapping[new ProjectionMember()] = columnExpression;

_identifier.Add((identifierColumn, identifierColumnTypeMapping!.Comparer));
}

internal SelectExpression(IEntityType entityType, ISqlExpressionFactory sqlExpressionFactory)
: base(null)
{
Expand Down Expand Up @@ -2156,7 +2201,10 @@ public void ApplyIntersect(SelectExpression source2, bool distinct)
public void ApplyUnion(SelectExpression source2, bool distinct)
=> ApplySetOperation(SetOperationType.Union, source2, distinct);

private void ApplySetOperation(SetOperationType setOperationType, SelectExpression select2, bool distinct)
private void ApplySetOperation(
SetOperationType setOperationType,
SelectExpression select2,
bool distinct)
{
// TODO: Introduce clone method? See issue#24460
var select1 = new SelectExpression(
Expand Down Expand Up @@ -2205,7 +2253,7 @@ private void ApplySetOperation(SetOperationType setOperationType, SelectExpressi
: Array.Empty<ColumnExpression?>();
var entityProjectionIdentifiers = new List<ColumnExpression>();
var entityProjectionValueComparers = new List<ValueComparer>();
var otherExpressions = new List<SqlExpression>();
var otherExpressions = new List<(SqlExpression, RelationalTypeMapping)>();

// Push down into a subquery if limit/offset are defined. If not, any orderings can be discarded as set operations don't preserve
// them.
Expand Down Expand Up @@ -2308,7 +2356,11 @@ private void ApplySetOperation(SetOperationType setOperationType, SelectExpressi
}
}

otherExpressions.Add(outerProjection);
// we need type mapping for identifiers - it may happen that one side of the set operation comes from collection parameter
// and therefore doesn't have type mapping (yet - we infer those after the translation is complete)
// but for set operation at least one side should have type mapping, otherwise whole thing would have been parameterized out
var outerTypeMapping = innerProjection1.Expression.TypeMapping ?? innerProjection2.Expression.TypeMapping!;
otherExpressions.Add((outerProjection, outerTypeMapping));
}
}

Expand Down Expand Up @@ -2349,10 +2401,10 @@ private void ApplySetOperation(SetOperationType setOperationType, SelectExpressi
// If there are no other expressions then we can use all entityProjectionIdentifiers
_identifier.AddRange(entityProjectionIdentifiers.Zip(entityProjectionValueComparers));
}
else if (otherExpressions.All(e => e is ColumnExpression))
else if (otherExpressions.All(e => e.Item1 is ColumnExpression))
{
_identifier.AddRange(entityProjectionIdentifiers.Zip(entityProjectionValueComparers));
_identifier.AddRange(otherExpressions.Select(e => ((ColumnExpression)e, e.TypeMapping!.KeyComparer)));
_identifier.AddRange(otherExpressions.Select(e => ((ColumnExpression)e.Item1, e.Item2.KeyComparer)));
}
}
}
Expand Down
111 changes: 78 additions & 33 deletions src/EFCore.SqlServer/Query/Internal/SqlServerQueryTranslationPostprocessor.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// The .NET Foundation licenses this file to you under the MIT license.

using System.Diagnostics.CodeAnalysis;
using Microsoft.EntityFrameworkCore.Metadata.Internal;
using Microsoft.EntityFrameworkCore.Query.SqlExpressions;
using Microsoft.EntityFrameworkCore.SqlServer.Internal;

Expand Down Expand Up @@ -111,30 +112,73 @@ public Expression Process(Expression expression)
case ShapedQueryExpression shapedQueryExpression:
return shapedQueryExpression.UpdateQueryExpression(Visit(shapedQueryExpression.QueryExpression));

case SelectExpression
case SelectExpression selectExpression:
{
Tables: [SqlServerOpenJsonExpression { ColumnInfos: not null } openJsonExpression, ..],
Orderings:
[
var newTables = new List<TableExpressionBase>();
var appliedCasts = new List<(SqlServerOpenJsonExpression, string)>();

foreach (var table in selectExpression.Tables)
{
if (table is SqlServerOpenJsonExpression { ColumnInfos: not null }
or JoinExpressionBase { Table: SqlServerOpenJsonExpression { ColumnInfos: not null } })
{
Expression: SqlUnaryExpression
var keyReferencedInOrderings = selectExpression.Orderings.Any(o => o.Expression is SqlUnaryExpression
{
OperatorType: ExpressionType.Convert,
Operand: ColumnExpression { Name: "key", Table: var keyColumnTable }
} && keyColumnTable == table);

var keyReferencedInProjection = selectExpression.Projection.Any(p => p.Expression is ColumnExpression
{ Name: "key", Table: var keyColumnTable } && keyColumnTable == table);

var keyWithConvertReferencedInProjection = selectExpression.Projection.Any(p => p.Expression is SqlUnaryExpression
{
OperatorType: ExpressionType.Convert,
Operand: ColumnExpression { Name: "key", Table: var keyColumnTable }
} && keyColumnTable == table);

if (keyReferencedInOrderings || keyReferencedInProjection || keyWithConvertReferencedInProjection)
{
// Remove the WITH clause from the OPENJSON expression
var openJsonExpression = (SqlServerOpenJsonExpression)((table as JoinExpressionBase)?.Table ?? table);
var newOpenJsonExpression = openJsonExpression.Update(
openJsonExpression.JsonExpression,
openJsonExpression.Path,
columnInfos: null);

TableExpressionBase newTable = table switch
{
InnerJoinExpression ij => ij.Update(newOpenJsonExpression, ij.JoinPredicate),
LeftJoinExpression lj => lj.Update(newOpenJsonExpression, lj.JoinPredicate),
CrossJoinExpression cj => cj.Update(newOpenJsonExpression),
CrossApplyExpression ca => ca.Update(newOpenJsonExpression),
OuterApplyExpression oa => oa.Update(newOpenJsonExpression),
_ => newOpenJsonExpression,
};

newTables.Add(newTable);

foreach (var column in openJsonExpression.ColumnInfos!)
{
var typeMapping = _typeMappingSource.FindMapping(column.StoreType);
Check.DebugAssert(
typeMapping is not null,
$"Could not find mapping for store type {column.StoreType} when converting OPENJSON/WITH");

_castsToApply.Add((newOpenJsonExpression, column.Name), typeMapping);
appliedCasts.Add((newOpenJsonExpression, column.Name));
}
}
else
{
newTables.Add(table);
}
}
]
} selectExpression
when keyColumnTable == openJsonExpression:
{
// Remove the WITH clause from the OPENJSON expression
var newOpenJsonExpression = openJsonExpression.Update(
openJsonExpression.JsonExpression,
openJsonExpression.Path,
columnInfos: null);

var newTables = selectExpression.Tables.ToArray();
newTables[0] = newOpenJsonExpression;
else
{
newTables.Add(table);
}
}

var newSelectExpression = selectExpression.Update(
selectExpression.Projection,
Expand All @@ -144,36 +188,37 @@ public Expression Process(Expression expression)
selectExpression.Having,
selectExpression.Orderings,
selectExpression.Limit,
selectExpression.Offset);
selectExpression.Offset);

// Record the OPENJSON expression and its projected column(s), along with the store type we just removed from the WITH
// clause. Then visit the select expression, adding a cast around the matching ColumnExpressions.
// TODO: Need to pass through the type mapping API for converting the JSON value (nvarchar) to the relational store type
// (e.g. datetime2), see #30677
foreach (var column in openJsonExpression.ColumnInfos)
{
var typeMapping = _typeMappingSource.FindMapping(column.StoreType);
Check.DebugAssert(
typeMapping is not null,
$"Could not find mapping for store type {column.StoreType} when converting OPENJSON/WITH");

_castsToApply.Add((newOpenJsonExpression, column.Name), typeMapping);
}

var result = base.Visit(newSelectExpression);

foreach (var column in openJsonExpression.ColumnInfos)
foreach (var appliedCast in appliedCasts)
{
_castsToApply.Remove((newOpenJsonExpression, column.Name));
_castsToApply.Remove(appliedCast);
}

return result;
}

case ColumnExpression { Table: SqlServerOpenJsonExpression openJsonTable, Name: var name } columnExpression
when _castsToApply.TryGetValue((openJsonTable, name), out var typeMapping):
case ColumnExpression columnExpression:
{
return _sqlExpressionFactory.Convert(columnExpression, columnExpression.Type, typeMapping);
if (columnExpression.Table is SqlServerOpenJsonExpression openJsonTable
&& _castsToApply.TryGetValue((openJsonTable, columnExpression.Name), out var typeMapping))
{
return _sqlExpressionFactory.Convert(columnExpression, columnExpression.Type, typeMapping);
}

if (columnExpression.Table is JoinExpressionBase { Table: SqlServerOpenJsonExpression innerOpenJsonTable }
&& _castsToApply.TryGetValue((innerOpenJsonTable, columnExpression.Name), out var innerTypeMapping))
{
return _sqlExpressionFactory.Convert(columnExpression, columnExpression.Type, innerTypeMapping);
}

return base.Visit(expression);
}

default:
Expand Down
19 changes: 17 additions & 2 deletions src/EFCore.SqlServer/Query/Internal/SqlServerQueryableMethodTranslatingExpressionVisitor.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,14 @@ protected override Expression VisitExtension(Expression extensionExpression)
var isColumnNullable = elementClrType.IsNullableType();

var selectExpression = new SelectExpression(
openJsonExpression, columnName: "value", columnType: elementClrType, columnTypeMapping: elementTypeMapping, isColumnNullable);
openJsonExpression,
identidierColumnName: "key",
identifierColumnType: typeof(string),
identifierColumnTypeMapping: _typeMappingSource.FindMapping("nvarchar(4000)"),
columnName: "value",
columnType: elementClrType,
columnTypeMapping: elementTypeMapping,
isColumnNullable);

// OPENJSON doesn't guarantee the ordering of the elements coming out; when using OPENJSON without WITH, a [key] column is returned
// with the JSON array's ordering, which we can ORDER BY; this option doesn't exist with OPENJSON with WITH, unfortunately.
Expand All @@ -180,7 +187,15 @@ protected override Expression VisitExtension(Expression extensionExpression)
_typeMappingSource.FindMapping(typeof(int))),
ascending: true));

var shaperExpression = new ProjectionBindingExpression(selectExpression, new ProjectionMember(), elementClrType);
var shaperExpression = (Expression)new ProjectionBindingExpression(selectExpression, new ProjectionMember(), elementClrType.MakeNullable());
if (shaperExpression.Type != elementClrType)
{
Check.DebugAssert(
elementClrType.MakeNullable() == shaperExpression.Type,
"expression.Type must be nullable of targetType");

shaperExpression = Expression.Convert(shaperExpression, elementClrType);
}

return new ShapedQueryExpression(selectExpression, shaperExpression);
}
Expand Down
9 changes: 8 additions & 1 deletion src/EFCore.Sqlite.Core/Query/Internal/SqliteQueryableMethodTranslatingExpressionVisitor.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -220,7 +220,14 @@ protected override QueryableMethodTranslatingExpressionVisitor CreateSubqueryVis
var isColumnNullable = elementClrType.IsNullableType();

var selectExpression = new SelectExpression(
jsonEachExpression, columnName: "value", columnType: elementClrType, columnTypeMapping: elementTypeMapping, isColumnNullable);
jsonEachExpression,
identidierColumnName: "key",
identifierColumnType: typeof(int),
identifierColumnTypeMapping: _typeMappingSource.FindMapping(typeof(int)),
columnName: "value",
columnType: elementClrType,
columnTypeMapping: elementTypeMapping,
isColumnNullable);

// TODO: SQLite does have REAL and BLOB types, which JSON does not. Need to possibly cast to that.
if (elementTypeMapping is not null)
Expand Down
Loading

0 comments on commit d3d784b

Please sign in to comment.