-
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
Query: Introduce custom query roots #20083
Conversation
Mandatory review from @AndriySvyryd if you see any shortcoming or issues in taking this direction. |
@@ -421,8 +421,7 @@ private static bool CompareConstant(ConstantExpression a, ConstantExpression b) | |||
=> a.Value == b.Value | |||
|| (a.Value != null | |||
&& b.Value != null | |||
&& (a.IsEntityQueryable() && b.IsEntityQueryable() && a.Value.GetType() == b.Value.GetType() | |||
|| Equals(a.Value, b.Value))); | |||
&& (Equals(a.Value, b.Value))); |
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.
Bugfix: Queryroots of share type entity types are identified as same causing incorrect query cache hit.
24312e4
to
9a3d2e8
Compare
This change introduces derived types of EntityQueryable<T> to be processed as query root in core pipeline. Currently FromSql and Queryable functions generate a method call in query pipeline which would be mapped to a query root. This means that any visitor which need to detect query root, they have to have special code to handle this. With this change, any provider bringing custom query roots can inject custom query root for relevant subtree in the query and it would be processed transparently through the stack. Only during translation, once the custom query root needs to be intercepted to generate correct SQL. Converted FromSql in this PR. Will submit another PR to convert queryable functions. Custom query roots can be generated during query construction itself (before it reaches EF), such query root requires - Overriden Equals/GetHashCode methods so we differentiate them during query caching. - Optional ToString method for debugging print out. - Conversion to such custom roots in QueryFilter if needed. - Components of custom root cannot be parameterized in ParameterExtractingExpressionVisitor Part of #18923
9a3d2e8
to
c041da9
Compare
This is the approach I took for queryable functions back when it was built on Re-Linq. I debating doing it this way for 5.0 but I didn't want to start ripping apart your beautiful new query pipeline too much :) |
Function queryables have arguments which are required to be visited in visitors. Hence custom query root in this way will not work. I will look into having it slightly differently. Putting this PR on hold till then. |
I just took a closer look at the code and saw that the EntityQueryable was stored in a ConstantExpression. Back in the ReLinq days I had created a QueryableFunctionExpression and during the initial Linq parsing replaced the queryable method calls with that expression. I was then able to deal with the expression during VisitExtension for any ExpressionVisitor's that needed to deal with it. Could you do something similar here instead of using ConstantExpression? |
That's the plan. |
This change introduces derived types of EntityQueryable to be processed as query root in core pipeline.
Currently FromSql and Queryable functions generate a method call in query pipeline which would be mapped to a query root. This means that any visitor which need to detect query root, they have to have special code to handle this.
With this change, any provider bringing custom query roots can inject custom query root for relevant subtree in the query and it would be processed transparently through the stack. Only during translation, once the custom query root needs to be intercepted to generate correct SQL.
Converted FromSql in this PR.
Will submit another PR to convert queryable functions.
Custom query roots can be generated during query construction itself (before it reaches EF), such query root requires
Part of #18923