-
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
Improvement to Find/FindAsync with Partition Key #20461
Improvement to Find/FindAsync with Partition Key #20461
Conversation
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.
Comment and questions to changes.
src/EFCore.Cosmos/Query/Internal/CosmosParameterExtractingExpressionVisitor.cs
Outdated
Show resolved
Hide resolved
src/EFCore.Cosmos/Query/Internal/CosmosParameterExtractingExpressionVisitor.cs
Outdated
Show resolved
Hide resolved
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.
The fix should not change ParameterExtractingExpressionVisitor
in anyway. That is not what it meant for. Wait for @AndriySvyryd to provide his thoughts on how to fix.
Thank you @smitpatel. Changing the approach shouldn't be a big issue. The hard part was not writing the code, rather it was figuring out how it all works together in the Cosmos provider and then pick the best approach for implementing the suggested improvements, in a manner that best aligns with the existing pattern etc., and with minimal changes. Regarding Technically I haven't changed Anyhow, I'm sure there are other ways to collect those needed parameters. Would be great if your and/or @AndriySvyryd could point me in the direction of the approach you'd want me to take. |
What are the parameters you need to collect? |
Believe me, this is how I feel whenever I look at the query code! 😸 Thank you for your efforts here! |
@smitpatel what I need to extract from the query is:
It's all collected by Seems easy enough, right 😊 I also look to see if the query is attempting to fetch only one item - i.e if the It all comes together here in these few lines of code, where it is decided if the Cosmos query request can be fulfilled using the new |
So assumptions about first EntityType, setting EntityType/PK is all incorrect thing to put on QueryContext. If you see in translation phase, you will have a query root expression which will tell you which IEntityType it is. From IEntityType you can get IProperties which makes PK. From IEntityType you can also fetch IProperty for "id" and it's value generator. So from query perspective, whole processing should be around compilation time, no changes should be made to ParameterExtractingEV. |
What about |
It's a big no-no. |
I.e. using the |
This query passed to the |
Yes, in that method you can inspect the expression tree. (Use |
Here's the print:
Here's the debugview of the query:
How do I extract the |
|
In first pass, let's just optimize queries coming from Find where PK properties contain id & partition key both.
If all above matches, then you have a Find query. We can relax the pattern bit more in future so user written query which are similar to that can also be optimized, for example doing First/Single/Last or default with key values all could be optimized. Now if the pattern is identified then you create a new ShapedQueryExpression like below return new ShapedQueryExpression(
selectExpression,
new EntityShaperExpression(
entityType,
new ProjectionBindingExpression(
selectExpression,
new ProjectionMember(),
typeof(ValueBuffer)),
false)); But instead of using
Step 2: Taking care of materialization
Step 3 execute the query.
We can expand a lot more in future. This is minimum amount of basic stuff we need to add in first pass. |
[Update] public virtual Expression CreateShapedReadItemQueryExpression(Expression expression)
{
if (methodCallExpression.Method.IsGenericMethod
&& methodCallExpression.Method.Name == nameof(Queryable.FirstOrDefault))
{
if (methodCallExpression.Arguments[0] is MethodCallExpression queryRootMethodCallExpression
&& methodCallExpression.Method.IsGenericMethod
&& queryRootMethodCallExpression.Method.Name == nameof(Queryable.Where))
{
if (queryRootMethodCallExpression.Arguments[0] is QueryRootExpression queryRootExpression)
{
// Store EntityType in ReadItemExpression;
}
if (queryRootMethodCallExpression.Arguments[1] is UnaryExpression unaryExpression
&& unaryExpression.Operand is LambdaExpression lambdaExpression
&& lambdaExpression.Body is BinaryExpression binaryExpression)
{
VisitBinary(binaryExpression);
}
}
}
} Overriding |
You would need recursive method to identify structure of predicate since, it would have more nesting as number of PK properties increases. We can defer it for later. |
Also don't set |
Thank you. The recursive part makes better sense to me. I'd misunderstood the guide on this part and I had a misunderstanding on how the ExpressionVisitor works. I thought that it would traverse the tree by itself. It makes much more sense with the recursive call to I've updated the code in the comment above to reflect this. |
It would be actually separate method. Rather than base call. We cannot use ExpressionVisitor for that. efcore/src/EFCore.InMemory/Query/Internal/InMemoryQueryableMethodTranslatingExpressionVisitor.cs Lines 449 to 490 in bb88898
It collects Expressions from left & right. You will get EF.Property calls on left & ParameterExpression on right. |
@smitpatel Can you elaboate here. By this do you mean that the property names should be matched by index, rather than merely their names? Or? |
If predicate is |
@smitpatel I'm a bit stuck. Any hints on the following? When looking at the return value from [
...I'm getting this exception, but only in the ReadItemExpression code path:
The new protected override Expression VisitShapedQueryExpression(ShapedQueryExpression shapedQueryExpression)
{
Check.NotNull(shapedQueryExpression, nameof(shapedQueryExpression));
var jObjectParameter = Expression.Parameter(typeof(JObject), "jObject");
var shaperBody = shapedQueryExpression.ShaperExpression;
shaperBody = new JObjectInjectingExpressionVisitor().Visit(shaperBody);
shaperBody = InjectEntityMaterializers(shaperBody);
switch (shapedQueryExpression.QueryExpression)
{
case SelectExpression selectExpression:
selectExpression.ApplyProjection();
shaperBody = new CosmosProjectionBindingRemovingExpressionVisitor(selectExpression, jObjectParameter, IsTracking)
.Visit(shaperBody);
var shaperLambda = Expression.Lambda(
shaperBody,
QueryCompilationContext.QueryContextParameter,
jObjectParameter);
return Expression.New(
typeof(QueryingEnumerable<>).MakeGenericType(shaperLambda.ReturnType).GetConstructors()[0],
Expression.Convert(
QueryCompilationContext.QueryContextParameter,
typeof(QueryContext)),
Expression.Constant(_sqlExpressionFactory),
Expression.Constant(_querySqlGeneratorFactory),
Expression.Constant(selectExpression),
Expression.Constant(shaperLambda.Compile()),
Expression.Constant(_contextType),
Expression.Constant(_partitionKeyFromExtension, typeof(string)),
Expression.Constant(_logger));
case ReadItemExpression readItemExpression:
shaperBody =
new CosmosProjectionBindingRemovingReadItemExpressionVisitor(readItemExpression, jObjectParameter, IsTracking)
.Visit(shaperBody);
var shaperReadItemLambda = Expression.Lambda(
shaperBody,
QueryCompilationContext.QueryContextParameter,
jObjectParameter);
return Expression.New(
typeof(ReadItemQueryingEnumerable<>).MakeGenericType(shaperReadItemLambda.ReturnType).GetConstructors()[0],
Expression.Convert(
QueryCompilationContext.QueryContextParameter,
typeof(QueryContext)),
Expression.Constant(readItemExpression),
Expression.Constant(shaperReadItemLambda.Compile()),
Expression.Constant(_contextType),
Expression.Constant(_logger));
default:
return null;
}
} The query expression that goes into .Lambda #Lambda1<System.Func`3[Microsoft.EntityFrameworkCore.Query.QueryContext,Newtonsoft.Json.Linq.JObject,Microsoft.EntityFrameworkCore.Cosmos.EndToEndCosmosTest+Customer_WithResourceId]>(
Microsoft.EntityFrameworkCore.Query.QueryContext $queryContext,
Newtonsoft.Json.Linq.JObject $jObject) {
.Block(Newtonsoft.Json.Linq.JObject $jObject1) {
$jObject1 = .Call Microsoft.EntityFrameworkCore.Cosmos.Query.Internal.CosmosShapedQueryCompilingExpressionVisitor+CosmosProjectionBindingRemovingReadItemExpressionVisitor.SafeToObject(.Call $jObject.get_Item("c")
);
.If ($jObject1 == null) {
null
} .Else {
.Block(
Microsoft.EntityFrameworkCore.Storage.MaterializationContext $materializationContext1,
Microsoft.EntityFrameworkCore.Metadata.IEntityType $entityType1,
Microsoft.EntityFrameworkCore.Cosmos.EndToEndCosmosTest+Customer_WithResourceId $instance1,
Microsoft.EntityFrameworkCore.ChangeTracking.Internal.InternalEntityEntry $entry1,
System.Boolean $hasNullKey1) {
$materializationContext1 = .New Microsoft.EntityFrameworkCore.Storage.MaterializationContext(
.Constant<Microsoft.EntityFrameworkCore.Storage.ValueBuffer>(Microsoft.EntityFrameworkCore.Storage.ValueBuffer),
$queryContext.Context);
$instance1 = null;
$entry1 = .Call ($queryContext.StateManager).TryGetEntry(
.Constant<Microsoft.EntityFrameworkCore.Metadata.Internal.Key>(Key: Customer_WithResourceId.PartitionKey, Customer_WithResourceId.id, Customer_WithResourceId.Id PK),
.NewArray System.Object[] {
.Invoke (.Lambda #Lambda2<System.Func`2[Newtonsoft.Json.Linq.JToken,System.Object]>)(.Call $jObject1.get_Item("PartitionKey")
),
(System.Object).Call Microsoft.EntityFrameworkCore.Cosmos.Query.Internal.CosmosShapedQueryCompilingExpressionVisitor+CosmosProjectionBindingRemovingReadItemExpressionVisitor.SafeToObject(.Call $jObject1.get_Item("id")
),
(System.Object).Call Microsoft.EntityFrameworkCore.Cosmos.Query.Internal.CosmosShapedQueryCompilingExpressionVisitor+CosmosProjectionBindingRemovingReadItemExpressionVisitor.SafeToObject(.Call $jObject1.get_Item("Id")
)
},
True,
$hasNullKey1);
.If (
!$hasNullKey1
) {
.If ($entry1 != null) {
.Block() {
$entityType1 = $entry1.EntityType;
$instance1 = (Microsoft.EntityFrameworkCore.Cosmos.EndToEndCosmosTest+Customer_WithResourceId)$entry1.Entity
}
} .Else {
.Block(Microsoft.EntityFrameworkCore.Storage.ValueBuffer $shadowValueBuffer1) {
$shadowValueBuffer1 = .Constant<Microsoft.EntityFrameworkCore.Storage.ValueBuffer>(Microsoft.EntityFrameworkCore.Storage.ValueBuffer);
$entityType1 = .Block(System.String $discriminator) {
$discriminator = .Call Microsoft.EntityFrameworkCore.Cosmos.Query.Internal.CosmosShapedQueryCompilingExpressionVisitor+CosmosProjectionBindingRemovingReadItemExpressionVisitor.SafeToObject(.Call $jObject1.get_Item("Discriminator")
);
.Switch ($discriminator) {
.Case ("Customer_WithResourceId"):
.Constant<Microsoft.EntityFrameworkCore.Metadata.IEntityType>(EntityType: Customer_WithResourceId)
.Default:
.Block() {
.Throw .Call Microsoft.EntityFrameworkCore.Query.EntityShaperExpression.CreateUnableToDiscriminateException(
.Constant<Microsoft.EntityFrameworkCore.Metadata.Internal.EntityType>(EntityType: Customer_WithResourceId),
(System.Object)$discriminator);
null
}
}
};
$instance1 = .Switch ($entityType1) {
.Case (.Constant<Microsoft.EntityFrameworkCore.Metadata.IEntityType>(EntityType: Customer_WithResourceId)):
.Block() {
$shadowValueBuffer1 = .New Microsoft.EntityFrameworkCore.Storage.ValueBuffer(.NewArray System.Object[] {
(System.Object).Call Microsoft.EntityFrameworkCore.Cosmos.Query.Internal.CosmosShapedQueryCompilingExpressionVisitor+CosmosProjectionBindingRemovingReadItemExpressionVisitor.SafeToObject(.Call $jObject1.get_Item("Discriminator")
),
$jObject1
});
.Block(Microsoft.EntityFrameworkCore.Cosmos.EndToEndCosmosTest+Customer_WithResourceId $instance) {
$instance = .New Microsoft.EntityFrameworkCore.Cosmos.EndToEndCosmosTest+Customer_WithResourceId();
$instance.<PartitionKey>k__BackingField = .Invoke (.Lambda #Lambda3<System.Func`2[Newtonsoft.Json.Linq.JToken,System.Int32]>)(.Call $jObject1.get_Item("PartitionKey")
);
$instance.<id>k__BackingField = .Call Microsoft.EntityFrameworkCore.Cosmos.Query.Internal.CosmosShapedQueryCompilingExpressionVisitor+CosmosProjectionBindingRemovingReadItemExpressionVisitor.SafeToObject(.Call $jObject1.get_Item("id")
);
$instance.<Id>k__BackingField = (System.Int32).Call Microsoft.EntityFrameworkCore.Cosmos.Query.Internal.CosmosShapedQueryCompilingExpressionVisitor+CosmosProjectionBindingRemovingReadItemExpressionVisitor.SafeToObject(.Call $jObject1.get_Item("Id")
);
$instance.<Name>k__BackingField = .Call Microsoft.EntityFrameworkCore.Cosmos.Query.Internal.CosmosShapedQueryCompilingExpressionVisitor+CosmosProjectionBindingRemovingReadItemExpressionVisitor.SafeToObject(.Call $jObject1.get_Item("Name")
);
$instance
}
}
.Default:
null
};
$entry1 = .Call $queryContext.StartTracking(
$entityType1,
$instance1,
$shadowValueBuffer1);
$instance1
}
}
} .Else {
.Default(System.Void)
};
$instance1
}
}
}
}
.Lambda #Lambda2<System.Func`2[Newtonsoft.Json.Linq.JToken,System.Object]>(Newtonsoft.Json.Linq.JToken $var1) {
.If (
$var1 == .Default(Newtonsoft.Json.Linq.JToken) || $var1.Type == .Constant<Newtonsoft.Json.Linq.JTokenType>(Null)
) {
.Default(System.Object)
} .Else {
(System.Object).Block(System.Int32 $parsed) {
.If (
.Call System.Int32.TryParse(
.Call $var1.ToObject(),
.Constant<System.Globalization.NumberStyles>(Any),
.Constant<System.IFormatProvider>(),
$parsed)
) {
$parsed
} .Else {
0
}
}
}
}
.Lambda #Lambda3<System.Func`2[Newtonsoft.Json.Linq.JToken,System.Int32]>(Newtonsoft.Json.Linq.JToken $var2) {
.If (
$var2 == .Default(Newtonsoft.Json.Linq.JToken) || $var2.Type == .Constant<Newtonsoft.Json.Linq.JTokenType>(Null)
) {
.Default(System.Int32)
} .Else {
.Block(System.Int32 $parsed) {
.If (
.Call System.Int32.TryParse(
.Call $var2.ToObject(),
.Constant<System.Globalization.NumberStyles>(Any),
.Constant<System.IFormatProvider>(),
$parsed)
) {
$parsed
} .Else {
0
}
}
}
} |
Make sure that you override |
...mosShapedQueryCompilingExpressionVisitor.CosmosProjectionBindingRemovingExpressionVisitor.cs
Show resolved
Hide resolved
...mos/Query/Internal/CosmosShapedQueryCompilingExpressionVisitor.ReadItemQueryingEnumerable.cs
Show resolved
Hide resolved
src/EFCore.Cosmos/Query/Internal/CosmosQueryableMethodTranslatingExpressionVisitor.cs
Outdated
Show resolved
Hide resolved
...mos/Query/Internal/CosmosShapedQueryCompilingExpressionVisitor.ReadItemQueryingEnumerable.cs
Outdated
Show resolved
Hide resolved
...mos/Query/Internal/CosmosShapedQueryCompilingExpressionVisitor.ReadItemQueryingEnumerable.cs
Outdated
Show resolved
Hide resolved
...mos/Query/Internal/CosmosShapedQueryCompilingExpressionVisitor.ReadItemQueryingEnumerable.cs
Outdated
Show resolved
Hide resolved
...mos/Query/Internal/CosmosShapedQueryCompilingExpressionVisitor.ReadItemQueryingEnumerable.cs
Outdated
Show resolved
Hide resolved
...mos/Query/Internal/CosmosShapedQueryCompilingExpressionVisitor.ReadItemQueryingEnumerable.cs
Outdated
Show resolved
Hide resolved
src/EFCore.Cosmos/Query/Internal/CosmosQueryableMethodTranslatingExpressionVisitor.cs
Outdated
Show resolved
Hide resolved
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.
Looks good. Please squash and rebase on latest master
When rebasing to latest master, I can no longer open the solution. Not sure what's going on. All projects end up unloaded. |
Run |
Ahhh, the framework got updated. |
I see you've added netcoreapp5.0 tests now too. I assume this means that the tests I've written and/or changed for 3.1 need to be updated for 5.0 too? I can open the solution with
I will continue looking at it tomorrow. |
Suppress that warning in the source code |
Done.
|
Regarding squashing the commits, I cannot get rid of the 3 rebase to master I did along the way, so unable to squash below 3 commits. Probably due to my git skills coming to a short here. |
author Jasper Hedegaard Bojsen <[email protected]> 1584698113 +0100 committer Jasper Hedegaard Bojsen <[email protected]> 1586933746 +0200 parent 2972c62 author Jasper Hedegaard Bojsen <[email protected]> 1584698113 +0100 committer Jasper Hedegaard Bojsen <[email protected]> 1586933705 +0200 Find() with Cosmos DB
Good enough. Thanks for your contribution! |
Suggested improvements for
Find
/FindAsync
for Cosmos, allowing for ReadItem optimization, when the Partition Key is known and Cosmos Resource Id is provided or Cosmos Resource Id can be generated based on the default generator or a custom generator.Please see #17310