-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
Introduce liftable constants to shaper to prepare for precompilation #32721
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
// Licensed to the .NET Foundation under one or more agreements. | ||
// The .NET Foundation licenses this file to you under the MIT license. | ||
|
||
namespace Microsoft.EntityFrameworkCore.Query; | ||
|
||
public interface IRelationalLiftableConstantFactory : ILiftableConstantFactory | ||
{ | ||
LiftableConstantExpression CreateLiftableConstant( | ||
Expression<Func<RelationalMaterializerLiftableConstantContext, object>> resolverExpression, | ||
string variableName, | ||
Type type); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
// Licensed to the .NET Foundation under one or more agreements. | ||
// The .NET Foundation licenses this file to you under the MIT license. | ||
|
||
namespace Microsoft.EntityFrameworkCore.Query; | ||
|
||
public class RelationalLiftableConstantFactory : LiftableConstantFactory, IRelationalLiftableConstantFactory | ||
{ | ||
public virtual LiftableConstantExpression CreateLiftableConstant( | ||
Expression<Func<RelationalMaterializerLiftableConstantContext, object>> resolverExpression, | ||
string variableName, | ||
Type type) | ||
=> new(resolverExpression, variableName, type); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,32 @@ | ||
// Licensed to the .NET Foundation under one or more agreements. | ||
// The .NET Foundation licenses this file to you under the MIT license. | ||
|
||
using Microsoft.EntityFrameworkCore.Query.Internal; | ||
|
||
namespace Microsoft.EntityFrameworkCore.Query; | ||
|
||
#pragma warning disable EF1001 // LiftableConstantProcessor is internal | ||
|
||
public class RelationalLiftableConstantProcessor : LiftableConstantProcessor | ||
{ | ||
private RelationalMaterializerLiftableConstantContext _relationalMaterializerLiftableConstantContext; | ||
|
||
public RelationalLiftableConstantProcessor( | ||
ShapedQueryCompilingExpressionVisitorDependencies dependencies, | ||
RelationalShapedQueryCompilingExpressionVisitorDependencies relationalDependencies) | ||
: base(dependencies) | ||
=> _relationalMaterializerLiftableConstantContext = new(dependencies, relationalDependencies); | ||
|
||
protected override ConstantExpression InlineConstant(LiftableConstantExpression liftableConstant) | ||
{ | ||
if (liftableConstant.ResolverExpression is Expression<Func<RelationalMaterializerLiftableConstantContext, object>> | ||
resolverExpression) | ||
{ | ||
var resolver = resolverExpression.Compile(preferInterpretation: true); | ||
var value = resolver(_relationalMaterializerLiftableConstantContext); | ||
return Expression.Constant(value, liftableConstant.Type); | ||
} | ||
|
||
return base.InlineConstant(liftableConstant); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
// Licensed to the .NET Foundation under one or more agreements. | ||
// The .NET Foundation licenses this file to you under the MIT license. | ||
|
||
namespace Microsoft.EntityFrameworkCore.Query; | ||
|
||
public record RelationalMaterializerLiftableConstantContext( | ||
ShapedQueryCompilingExpressionVisitorDependencies Dependencies, | ||
RelationalShapedQueryCompilingExpressionVisitorDependencies RelationalDependencies) | ||
: MaterializerLiftableConstantContext(Dependencies); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,7 +11,14 @@ namespace Microsoft.EntityFrameworkCore.Query; | |
|
||
public partial class RelationalShapedQueryCompilingExpressionVisitor | ||
{ | ||
private sealed partial class ShaperProcessingExpressionVisitor : ExpressionVisitor | ||
/// <summary> | ||
/// This is an internal API that supports the Entity Framework Core infrastructure and not subject to | ||
/// the same compatibility standards as public APIs. It may be changed or removed without notice in | ||
/// any release. You should only use it directly in your code with extreme caution and knowing that | ||
/// doing so can result in application failures when updating to a new Entity Framework Core release. | ||
/// </summary> | ||
[EntityFrameworkInternal] | ||
public sealed partial class ShaperProcessingExpressionVisitor : ExpressionVisitor | ||
{ | ||
private static readonly MethodInfo ThrowReadValueExceptionMethod = | ||
typeof(ShaperProcessingExpressionVisitor).GetTypeInfo().GetDeclaredMethod(nameof(ThrowReadValueException))!; | ||
|
@@ -160,7 +167,14 @@ private static void IncludeReference<TEntity, TIncludingEntity, TIncludedEntity> | |
} | ||
} | ||
|
||
private static void InitializeIncludeCollection<TParent, TNavigationEntity>( | ||
/// <summary> | ||
/// This is an internal API that supports the Entity Framework Core infrastructure and not subject to | ||
/// the same compatibility standards as public APIs. It may be changed or removed without notice in | ||
/// any release. You should only use it directly in your code with extreme caution and knowing that | ||
/// doing so can result in application failures when updating to a new Entity Framework Core release. | ||
/// </summary> | ||
[EntityFrameworkInternal] | ||
public static void InitializeIncludeCollection<TParent, TNavigationEntity>( | ||
int collectionId, | ||
QueryContext queryContext, | ||
DbDataReader dbDataReader, | ||
|
@@ -201,17 +215,27 @@ private static void InitializeIncludeCollection<TParent, TNavigationEntity>( | |
resultCoordinator.SetSingleQueryCollectionContext(collectionId, collectionMaterializationContext); | ||
} | ||
|
||
private static void PopulateIncludeCollection<TIncludingEntity, TIncludedEntity>( | ||
/// <summary> | ||
/// This is an internal API that supports the Entity Framework Core infrastructure and not subject to | ||
/// the same compatibility standards as public APIs. It may be changed or removed without notice in | ||
/// any release. You should only use it directly in your code with extreme caution and knowing that | ||
/// doing so can result in application failures when updating to a new Entity Framework Core release. | ||
/// </summary> | ||
[EntityFrameworkInternal] | ||
public static void PopulateIncludeCollection<TIncludingEntity, TIncludedEntity>( | ||
int collectionId, | ||
QueryContext queryContext, | ||
DbDataReader dbDataReader, | ||
SingleQueryResultCoordinator resultCoordinator, | ||
Func<QueryContext, DbDataReader, object[]> parentIdentifier, | ||
Func<QueryContext, DbDataReader, object[]> outerIdentifier, | ||
Func<QueryContext, DbDataReader, object[]> selfIdentifier, | ||
IReadOnlyList<ValueComparer> parentIdentifierValueComparers, | ||
IReadOnlyList<ValueComparer> outerIdentifierValueComparers, | ||
IReadOnlyList<ValueComparer> selfIdentifierValueComparers, | ||
IReadOnlyList<Func<object, object, bool>> parentIdentifierValueComparers, | ||
IReadOnlyList<Func<object, object, bool>> outerIdentifierValueComparers, | ||
IReadOnlyList<Func<object, object, bool>> selfIdentifierValueComparers, | ||
// IReadOnlyList<ValueComparer> parentIdentifierValueComparers, | ||
// IReadOnlyList<ValueComparer> outerIdentifierValueComparers, | ||
// IReadOnlyList<ValueComparer> selfIdentifierValueComparers, | ||
Comment on lines
+236
to
+238
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👀 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @AndriySvyryd yeah, this code is very much what I hacked together to make basic things work during the prototype. This absolutely needs to be cleaned up (and support extended for all cases). |
||
Func<QueryContext, DbDataReader, ResultContext, SingleQueryResultCoordinator, TIncludedEntity> innerShaper, | ||
INavigationBase? inverseNavigation, | ||
Action<TIncludingEntity, TIncludedEntity> fixup, | ||
|
@@ -229,14 +253,14 @@ private static void PopulateIncludeCollection<TIncludingEntity, TIncludedEntity> | |
return; | ||
} | ||
|
||
if (!CompareIdentifiers( | ||
if (!CompareIdentifiers2( | ||
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 (!CompareIdentifiers( | ||
if (!CompareIdentifiers2( | ||
parentIdentifierValueComparers, | ||
parentIdentifier(queryContext, dbDataReader), collectionMaterializationContext.ParentIdentifier)) | ||
{ | ||
|
@@ -255,7 +279,7 @@ private static void PopulateIncludeCollection<TIncludingEntity, TIncludedEntity> | |
|
||
if (collectionMaterializationContext.SelfIdentifier != null) | ||
{ | ||
if (CompareIdentifiers(selfIdentifierValueComparers, innerKey, collectionMaterializationContext.SelfIdentifier)) | ||
if (CompareIdentifiers2(selfIdentifierValueComparers, innerKey, collectionMaterializationContext.SelfIdentifier)) | ||
{ | ||
// repeated row for current element | ||
// If it is pending materialization then it may have nested elements | ||
|
@@ -319,7 +343,14 @@ void GenerateCurrentElementIfPending() | |
} | ||
} | ||
|
||
private static void InitializeSplitIncludeCollection<TParent, TNavigationEntity>( | ||
/// <summary> | ||
/// This is an internal API that supports the Entity Framework Core infrastructure and not subject to | ||
/// the same compatibility standards as public APIs. It may be changed or removed without notice in | ||
/// any release. You should only use it directly in your code with extreme caution and knowing that | ||
/// doing so can result in application failures when updating to a new Entity Framework Core release. | ||
/// </summary> | ||
[EntityFrameworkInternal] | ||
public static void InitializeSplitIncludeCollection<TParent, TNavigationEntity>( | ||
int collectionId, | ||
QueryContext queryContext, | ||
DbDataReader parentDataReader, | ||
|
@@ -358,7 +389,14 @@ private static void InitializeSplitIncludeCollection<TParent, TNavigationEntity> | |
resultCoordinator.SetSplitQueryCollectionContext(collectionId, splitQueryCollectionContext); | ||
} | ||
|
||
private static void PopulateSplitIncludeCollection<TIncludingEntity, TIncludedEntity>( | ||
/// <summary> | ||
/// This is an internal API that supports the Entity Framework Core infrastructure and not subject to | ||
/// the same compatibility standards as public APIs. It may be changed or removed without notice in | ||
/// any release. You should only use it directly in your code with extreme caution and knowing that | ||
/// doing so can result in application failures when updating to a new Entity Framework Core release. | ||
/// </summary> | ||
[EntityFrameworkInternal] | ||
public static void PopulateSplitIncludeCollection<TIncludingEntity, TIncludedEntity>( | ||
int collectionId, | ||
RelationalQueryContext queryContext, | ||
IExecutionStrategy executionStrategy, | ||
|
@@ -367,7 +405,8 @@ private static void PopulateSplitIncludeCollection<TIncludingEntity, TIncludedEn | |
bool detailedErrorsEnabled, | ||
SplitQueryResultCoordinator resultCoordinator, | ||
Func<QueryContext, DbDataReader, object[]> childIdentifier, | ||
IReadOnlyList<ValueComparer> identifierValueComparers, | ||
IReadOnlyList<Func<object, object, bool>> identifierValueComparers, | ||
// IReadOnlyList<ValueComparer> identifierValueComparers, | ||
Func<QueryContext, DbDataReader, ResultContext, SplitQueryResultCoordinator, TIncludedEntity> innerShaper, | ||
Action<QueryContext, IExecutionStrategy, SplitQueryResultCoordinator>? relatedDataLoaders, | ||
INavigationBase? inverseNavigation, | ||
|
@@ -414,7 +453,7 @@ static RelationalDataReader InitializeReader( | |
{ | ||
while (dataReaderContext.HasNext ?? dbDataReader.Read()) | ||
{ | ||
if (!CompareIdentifiers( | ||
if (!CompareIdentifiers2( | ||
identifierValueComparers, | ||
splitQueryCollectionContext.ParentIdentifier, childIdentifier(queryContext, dbDataReader))) | ||
{ | ||
|
@@ -451,7 +490,8 @@ private static async Task PopulateSplitIncludeCollectionAsync<TIncludingEntity, | |
bool detailedErrorsEnabled, | ||
SplitQueryResultCoordinator resultCoordinator, | ||
Func<QueryContext, DbDataReader, object[]> childIdentifier, | ||
IReadOnlyList<ValueComparer> identifierValueComparers, | ||
IReadOnlyList<Func<object, object, bool>> identifierValueComparers, | ||
// IReadOnlyList<ValueComparer> identifierValueComparers, | ||
Func<QueryContext, DbDataReader, ResultContext, SplitQueryResultCoordinator, TIncludedEntity> innerShaper, | ||
Func<QueryContext, IExecutionStrategy, SplitQueryResultCoordinator, Task>? relatedDataLoaders, | ||
INavigationBase? inverseNavigation, | ||
|
@@ -506,7 +546,7 @@ static async Task<RelationalDataReader> InitializeReaderAsync( | |
{ | ||
while (dataReaderContext.HasNext ?? await dbDataReader.ReadAsync(queryContext.CancellationToken).ConfigureAwait(false)) | ||
{ | ||
if (!CompareIdentifiers( | ||
if (!CompareIdentifiers2( | ||
identifierValueComparers, | ||
splitQueryCollectionContext.ParentIdentifier, childIdentifier(queryContext, dbDataReader))) | ||
{ | ||
|
@@ -538,7 +578,8 @@ static async Task<RelationalDataReader> InitializeReaderAsync( | |
} | ||
} | ||
|
||
private static TCollection InitializeCollection<TElement, TCollection>( | ||
[EntityFrameworkInternal] | ||
public static TCollection InitializeCollection<TElement, TCollection>( | ||
int collectionId, | ||
QueryContext queryContext, | ||
DbDataReader dbDataReader, | ||
|
@@ -560,7 +601,8 @@ private static TCollection InitializeCollection<TElement, TCollection>( | |
return (TCollection)collection; | ||
} | ||
|
||
private static void PopulateCollection<TCollection, TElement, TRelatedEntity>( | ||
[EntityFrameworkInternal] | ||
public static void PopulateCollection<TCollection, TElement, TRelatedEntity>( | ||
int collectionId, | ||
QueryContext queryContext, | ||
DbDataReader dbDataReader, | ||
|
@@ -1066,6 +1108,20 @@ private static async Task TaskAwaiter(Func<Task>[] taskFactories) | |
} | ||
} | ||
|
||
private static bool CompareIdentifiers2(IReadOnlyList<Func<object, object, bool>> valueComparers, object[] left, object[] right) | ||
{ | ||
// 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](left[i], right[i])) | ||
{ | ||
return false; | ||
} | ||
} | ||
|
||
return true; | ||
} | ||
|
||
private static bool CompareIdentifiers(IReadOnlyList<ValueComparer> valueComparers, object[] left, object[] right) | ||
{ | ||
// Ignoring size check on all for perf as they should be same unless bug in code. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be better to also store the constant value in
LiftableConstant
and only useCompile
to assert it's the same value in DebugThat gives the C# generator a chance to determine whether it can just generate the constant instead of the resolver without having to compile
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that's something I debated with myself when writing this. I think you're right, and I like the DEBUG-only check (though I sometimes wish we ran tests in CI in DEBUG as well...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@maumar if you're interested in doing this, it would be an opportunity to put hands inside the infra as well - let me know.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW there could be some issues with asserting "same value" here - I'm not sure whether in all cases reference equality works (i.e. if the resolver instantiates a new instance that's equivalent but not reference-equal, this starts to possibly get complicated). Anyway, we can think about this together once the other work in the PR is done.