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 18, 2023
1 parent c218c1a commit 0b378ce
Show file tree
Hide file tree
Showing 8 changed files with 798 additions and 84 deletions.
38 changes: 32 additions & 6 deletions src/EFCore.Relational/Query/SqlExpressions/SelectExpression.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -109,12 +109,18 @@ internal SelectExpression(SqlExpression? projection)
/// <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>
/// <param name="identifierColumnName">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>
public SelectExpression(
TableExpressionBase tableExpression,
string columnName,
Type columnType,
RelationalTypeMapping? columnTypeMapping,
bool? isColumnNullable = null)
bool? isColumnNullable = null,
string? identifierColumnName = null,
Type? identifierColumnType = null,
RelationalTypeMapping? identifierColumnTypeMapping = null)
: base(null)
{
var tableReferenceExpression = new TableReferenceExpression(this, tableExpression.Alias!);
Expand All @@ -128,6 +134,18 @@ public SelectExpression(
isColumnNullable ?? columnType.IsNullableType());

_projectionMapping[new ProjectionMember()] = columnExpression;

if (identifierColumnName != null && identifierColumnType != null && identifierColumnTypeMapping != null)
{
var identifierColumn = new ConcreteColumnExpression(
identifierColumnName,
tableReferenceExpression,
identifierColumnType.UnwrapNullableType(),
identifierColumnTypeMapping,
identifierColumnType.IsNullableType());

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

internal SelectExpression(IEntityType entityType, ISqlExpressionFactory sqlExpressionFactory)
Expand Down Expand Up @@ -2156,7 +2174,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 +2226,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 Expression, ValueComparer Comparer)>();

// 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 +2329,12 @@ private void ApplySetOperation(SetOperationType setOperationType, SelectExpressi
}
}

otherExpressions.Add(outerProjection);
// we need comparer (that we get from 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.KeyComparer));
}
}

Expand Down Expand Up @@ -2349,10 +2375,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.Expression is ColumnExpression))
{
_identifier.AddRange(entityProjectionIdentifiers.Zip(entityProjectionValueComparers));
_identifier.AddRange(otherExpressions.Select(e => ((ColumnExpression)e, e.TypeMapping!.KeyComparer)));
_identifier.AddRange(otherExpressions.Select(e => ((ColumnExpression)e.Expression, e.Comparer)));
}
}
}
Expand Down
149 changes: 100 additions & 49 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;
using Microsoft.EntityFrameworkCore.SqlServer.Storage.Internal;
Expand Down Expand Up @@ -112,75 +113,125 @@ 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)>();
var tablesChanged = false;

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);
tablesChanged = tablesChanged || (!table.Equals(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");

// Binary data (varbinary) is stored in JSON as base64, which OPENJSON knows how to decode as long the type is
// specified in the WITH clause. We're now removing the WITH and applying a relational CAST, but that doesn't work
// for base64 data.
if (typeMapping is SqlServerByteArrayTypeMapping)
{
throw new InvalidOperationException(SqlServerStrings.QueryingOrderedBinaryJsonCollectionsNotSupported);
}

_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;

var newSelectExpression = selectExpression.Update(
selectExpression.Projection,
newTables,
selectExpression.Predicate,
selectExpression.GroupBy,
selectExpression.Having,
selectExpression.Orderings,
selectExpression.Limit,
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.
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");

// Binary data (varbinary) is stored in JSON as base64, which OPENJSON knows how to decode as long the type is
// specified in the WITH clause. We're now removing the WITH and applying a relational CAST, but that doesn't work
// for base64 data.
if (typeMapping is SqlServerByteArrayTypeMapping)
else
{
throw new InvalidOperationException(SqlServerStrings.QueryingOrderedBinaryJsonCollectionsNotSupported);
newTables.Add(table);
}

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

// SelectExpression.Update always creates a new instance - we should avoid it when tables haven't changed
// see #31276
var newSelectExpression = tablesChanged
? selectExpression.Update(
selectExpression.Projection,
newTables,
selectExpression.Predicate,
selectExpression.GroupBy,
selectExpression.Having,
selectExpression.Orderings,
selectExpression.Limit,
selectExpression.Offset)
: selectExpression;

// 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.
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,
columnName: "value",
columnType: elementClrType,
columnTypeMapping: elementTypeMapping,
isColumnNullable,
identifierColumnName: "key",
identifierColumnType: typeof(string),
identifierColumnTypeMapping: _typeMappingSource.FindMapping("nvarchar(4000)"));

// 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,
columnName: "value",
columnType: elementClrType,
columnTypeMapping: elementTypeMapping,
isColumnNullable,
identifierColumnName: "key",
identifierColumnType: typeof(int),
identifierColumnTypeMapping: _typeMappingSource.FindMapping(typeof(int)));

// If we have a collection column, we know the type mapping at this point (as opposed to parameters, whose type mapping will get
// inferred later based on usage in SqliteInferredTypeMappingApplier); we should be able to apply any SQL logic needed to convert
Expand Down
Loading

0 comments on commit 0b378ce

Please sign in to comment.