Skip to content

Commit

Permalink
Query: Identifiers should compare using KeyValueComparer
Browse files Browse the repository at this point in the history 
Resolves #19641

Also fixed some off-by-one errors
  • Loading branch information
@smitpatel
smitpatel committed Feb 8, 2020
1 parent 6b6e3ca commit c2c469d
Show file tree
Hide file tree
Showing 6 changed files with 190 additions and 66 deletions.
2 changes: 1 addition & 1 deletion src/EFCore.Relational/Query/Internal/CollectionJoinApplyingExpressionVisitor.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ protected override Expression VisitExtension(Expression extensionExpression)
if (selectExpression.IsDistinct
|| selectExpression.Limit != null
|| selectExpression.Offset != null
|| selectExpression.GroupBy.Count > 1)
|| selectExpression.GroupBy.Count > 0)
{
selectExpression.PushdownIntoSubquery();
}
Expand Down
36 changes: 33 additions & 3 deletions src/EFCore.Relational/Query/RelationalCollectionShaperExpression.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,30 +5,54 @@
using System.Collections.Generic;
using System.Linq.Expressions;
using JetBrains.Annotations;
using Microsoft.EntityFrameworkCore.ChangeTracking;
using Microsoft.EntityFrameworkCore.Metadata;
using Microsoft.EntityFrameworkCore.Utilities;

namespace Microsoft.EntityFrameworkCore.Query
{
public class RelationalCollectionShaperExpression : Expression, IPrintableExpression
{
[Obsolete("Use ctor which takes value comaprers.")]
public RelationalCollectionShaperExpression(
int collectionId,
[NotNull] Expression parentIdentifier,
[NotNull] Expression outerIdentifier,
[CanBeNull] Expression selfIdentifier,
[CanBeNull] Expression innerShaper,
[NotNull] Expression selfIdentifier,
[NotNull] Expression innerShaper,
[CanBeNull] INavigation navigation,
[NotNull] Type elementType)
: this(collectionId, parentIdentifier, outerIdentifier, selfIdentifier,
null, null, null, innerShaper, navigation, elementType)
{
}


public RelationalCollectionShaperExpression(
int collectionId,
[NotNull] Expression parentIdentifier,
[NotNull] Expression outerIdentifier,
[NotNull] Expression selfIdentifier,
[CanBeNull] IReadOnlyList<ValueComparer> parentIdentifierValueComparers,
[CanBeNull] IReadOnlyList<ValueComparer> outerIdentifierValueComparers,
[CanBeNull] IReadOnlyList<ValueComparer> selfIdentifierValueComparers,
[NotNull] Expression innerShaper,
[CanBeNull] INavigation navigation,
[NotNull] Type elementType)
{
Check.NotNull(parentIdentifier, nameof(parentIdentifier));
Check.NotNull(outerIdentifier, nameof(outerIdentifier));
Check.NotNull(selfIdentifier, nameof(selfIdentifier));
Check.NotNull(innerShaper, nameof(innerShaper));
Check.NotNull(elementType, nameof(elementType));

CollectionId = collectionId;
ParentIdentifier = parentIdentifier;
OuterIdentifier = outerIdentifier;
SelfIdentifier = selfIdentifier;
ParentIdentifierValueComparers = parentIdentifierValueComparers;
OuterIdentifierValueComparers = outerIdentifierValueComparers;
SelfIdentifierValueComparers = selfIdentifierValueComparers;
InnerShaper = innerShaper;
Navigation = navigation;
ElementType = elementType;
Expand All @@ -38,6 +62,10 @@ public RelationalCollectionShaperExpression(
public virtual Expression ParentIdentifier { get; }
public virtual Expression OuterIdentifier { get; }
public virtual Expression SelfIdentifier { get; }
public virtual IReadOnlyList<ValueComparer> ParentIdentifierValueComparers { get; }
public virtual IReadOnlyList<ValueComparer> OuterIdentifierValueComparers { get; }
public virtual IReadOnlyList<ValueComparer> SelfIdentifierValueComparers { get; }

public virtual Expression InnerShaper { get; }
public virtual INavigation Navigation { get; }
public virtual Type ElementType { get; }
Expand Down Expand Up @@ -73,7 +101,9 @@ public virtual RelationalCollectionShaperExpression Update(
|| selfIdentifier != SelfIdentifier
|| innerShaper != InnerShaper
? new RelationalCollectionShaperExpression(
CollectionId, parentIdentifier, outerIdentifier, selfIdentifier, innerShaper, Navigation, ElementType)
CollectionId, parentIdentifier, outerIdentifier, selfIdentifier,
ParentIdentifierValueComparers, OuterIdentifierValueComparers, SelfIdentifierValueComparers,
innerShaper, Navigation, ElementType)
: this;
}

Expand Down
47 changes: 40 additions & 7 deletions ...RelationalShapedQueryCompilingExpressionVisitor.CustomShaperCompilingExpressionVisitor.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
using System.Linq;
using System.Linq.Expressions;
using System.Reflection;
using Microsoft.EntityFrameworkCore.ChangeTracking;
using Microsoft.EntityFrameworkCore.Infrastructure;
using Microsoft.EntityFrameworkCore.Metadata;
using Microsoft.EntityFrameworkCore.Query.Internal;
Expand Down Expand Up @@ -86,6 +87,9 @@ private static void PopulateCollection<TCollection, TElement, TRelatedEntity>(
Func<QueryContext, DbDataReader, object[]> parentIdentifier,
Func<QueryContext, DbDataReader, object[]> outerIdentifier,
Func<QueryContext, DbDataReader, object[]> selfIdentifier,
IReadOnlyList<ValueComparer> parentIdentifierValueComparers,
IReadOnlyList<ValueComparer> outerIdentifierValueComparers,
IReadOnlyList<ValueComparer> selfIdentifierValueComparers,
Func<QueryContext, DbDataReader, ResultContext, ResultCoordinator, TRelatedEntity> innerShaper)
where TRelatedEntity : TElement
where TCollection : class, ICollection<TElement>
Expand All @@ -104,13 +108,13 @@ private static void PopulateCollection<TCollection, TElement, TRelatedEntity>(
return;
}

if (!StructuralComparisons.StructuralEqualityComparer.Equals(
if (!CompareIdentifiers(outerIdentifierValueComparers,
outerIdentifier(queryContext, dbDataReader), collectionMaterializationContext.OuterIdentifier))
{
// Outer changed so collection has ended. Materialize last element.
GenerateCurrentElementIfPending();
// If parent also changed then this row is now pointing to element of next collection
if (!StructuralComparisons.StructuralEqualityComparer.Equals(
if (!CompareIdentifiers(parentIdentifierValueComparers,
parentIdentifier(queryContext, dbDataReader), collectionMaterializationContext.ParentIdentifier))
{
resultCoordinator.HasNext = true;
Expand All @@ -128,7 +132,7 @@ private static void PopulateCollection<TCollection, TElement, TRelatedEntity>(

if (collectionMaterializationContext.SelfIdentifier != null)
{
if (StructuralComparisons.StructuralEqualityComparer.Equals(
if (CompareIdentifiers(selfIdentifierValueComparers,
innerKey, collectionMaterializationContext.SelfIdentifier))
{
// repeated row for current element
Expand Down Expand Up @@ -197,6 +201,9 @@ private static void PopulateIncludeCollection<TIncludingEntity, TIncludedEntity>
Func<QueryContext, DbDataReader, object[]> parentIdentifier,
Func<QueryContext, DbDataReader, object[]> outerIdentifier,
Func<QueryContext, DbDataReader, object[]> selfIdentifier,
IReadOnlyList<ValueComparer> parentIdentifierValueComparers,
IReadOnlyList<ValueComparer> outerIdentifierValueComparers,
IReadOnlyList<ValueComparer> selfIdentifierValueComparers,
Func<QueryContext, DbDataReader, ResultContext, ResultCoordinator, TIncludedEntity> innerShaper,
INavigation inverseNavigation,
Action<TIncludingEntity, TIncludedEntity> fixup,
Expand All @@ -212,13 +219,13 @@ private static void PopulateIncludeCollection<TIncludingEntity, TIncludedEntity>
return;
}

if (!StructuralComparisons.StructuralEqualityComparer.Equals(
if (!CompareIdentifiers(outerIdentifierValueComparers,
outerIdentifier(queryContext, dbDataReader), collectionMaterializationContext.OuterIdentifier))
{
// Outer changed so collection has ended. Materialize last element.
GenerateCurrentElementIfPending();
// If parent also changed then this row is now pointing to element of next collection
if (!StructuralComparisons.StructuralEqualityComparer.Equals(
if (!CompareIdentifiers(parentIdentifierValueComparers,
parentIdentifier(queryContext, dbDataReader), collectionMaterializationContext.ParentIdentifier))
{
resultCoordinator.HasNext = true;
Expand All @@ -236,8 +243,7 @@ private static void PopulateIncludeCollection<TIncludingEntity, TIncludedEntity>

if (collectionMaterializationContext.SelfIdentifier != null)
{
if (StructuralComparisons.StructuralEqualityComparer.Equals(
innerKey, collectionMaterializationContext.SelfIdentifier))
if (CompareIdentifiers(selfIdentifierValueComparers, innerKey, collectionMaterializationContext.SelfIdentifier))
{
// repeated row for current element
// If it is pending materialization then it may have nested elements
Expand Down Expand Up @@ -301,6 +307,27 @@ void GenerateCurrentElementIfPending()
}
}

private static bool CompareIdentifiers(IReadOnlyList<ValueComparer> valueComparers, object[] left, object[] right)
{
if (valueComparers != null)
{
// Ignoring size check on all for perf as they should be same unless bug in code.
for (var i = 0; i < left.Length; i++)
{
if (valueComparers[i] != null
? !valueComparers[i].Equals(left[i], right[i])
: !Equals(left[i], right[i]))
{
return false;
}
}

return true;
}

return StructuralComparisons.StructuralEqualityComparer.Equals(left, right);
}

private static readonly MethodInfo _initializeIncludeCollectionMethodInfo
= typeof(CustomShaperCompilingExpressionVisitor).GetTypeInfo()
.GetDeclaredMethod(nameof(InitializeIncludeCollection));
Expand Down Expand Up @@ -498,6 +525,9 @@ protected override Expression VisitExtension(Expression extensionExpression)
collectionShaper.SelfIdentifier,
QueryCompilationContext.QueryContextParameter,
_dbDataReaderParameter).Compile()),
Expression.Constant(collectionShaper.ParentIdentifierValueComparers, typeof(IReadOnlyList<ValueComparer>)),
Expression.Constant(collectionShaper.OuterIdentifierValueComparers, typeof(IReadOnlyList<ValueComparer>)),
Expression.Constant(collectionShaper.SelfIdentifierValueComparers, typeof(IReadOnlyList<ValueComparer>)),
Expression.Constant(((LambdaExpression)Visit(collectionShaper.InnerShaper)).Compile()),
Expression.Constant(inverseNavigation, typeof(INavigation)),
Expression.Constant(
Expand Down Expand Up @@ -530,6 +560,9 @@ protected override Expression VisitExtension(Expression extensionExpression)
collectionShaper.SelfIdentifier,
QueryCompilationContext.QueryContextParameter,
_dbDataReaderParameter).Compile()),
Expression.Constant(collectionShaper.ParentIdentifierValueComparers, typeof(IReadOnlyList<ValueComparer>)),
Expression.Constant(collectionShaper.OuterIdentifierValueComparers, typeof(IReadOnlyList<ValueComparer>)),
Expression.Constant(collectionShaper.SelfIdentifierValueComparers, typeof(IReadOnlyList<ValueComparer>)),
Expression.Constant(((LambdaExpression)Visit(collectionShaper.InnerShaper)).Compile()));
}

Expand Down
Loading

0 comments on commit c2c469d

Please sign in to comment.