-
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
Support member access on parameter and constant introduced in entity equality #16469
Conversation
test/EFCore.Specification.Tests/Query/SimpleQueryTestBase.ResultOperators.cs
Show resolved
Hide resolved
@@ -1994,7 +2010,7 @@ where es.SingleOrDefault(e2 => e2.EmployeeID == 42) == new Employee() | |||
select e1); | |||
} | |||
|
|||
[ConditionalFact] | |||
[ConditionalFact(Skip = "Does not throw")] |
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.
Seems like we have a bug here with Single in subquery. Let me know if I should open a new issue or point me to an existing.
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.
We stopped throwing behavior for Single in subquery. See #15559
Though it should generate invalid SQL for now since single should generate Top (2) in subquery which is wrong.
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.
Yes, this is very similar to Where_query_composition_entity_equality_multiple_elements_SingleOrDefault from the comment above, except that this test doesn't generate any rows at all and SqlServer is OK with that... I will also reference #15559 in the skip message.
test/EFCore.Relational.Specification.Tests/Query/FromSqlQueryTestBase.cs
Outdated
Show resolved
Hide resolved
Need to update PR for Cosmos (always forget that), will do that tomorrow. |
src/EFCore.Relational/Query/Pipeline/RelationalSqlTranslatingExpressionVisitor.cs
Outdated
Show resolved
Hide resolved
src/EFCore.Relational/Query/Pipeline/RelationalSqlTranslatingExpressionVisitor.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 logic must be in core optimization phase. This expression is side-effect of optimization of tree in entity equality so it is not way related to translation pipeline or relational.
- Parameter created by front-end are prefixed by CompiledQueryCache.CompiledQueryParameterPrefix (Query filter has slightly different prefix). So it is easy to identify the case. Prefix can still have clash in edge case but it has worked in past and we live with it.
- QCC should not have direct access to QC. For that purpose, we use different prefix. QueryFilter got own prefix. Since the functionality of runtime parameters from query filter and EE is merging, we can determine a runtime parameter prefix and use it. The numbering will auto-adjust since QCC has size.
- Nested is out of scope since EE doesn't/can't generate that.
- Refactoring has low value. When it produces lambdas for query filter, it still evaluate partial expressions to generate constants. So evaluating subtree part is not going away regardless of it generating contextAccessor
19b0a36
to
75f5e86
Compare
Thanks for guidance @smitpatel, all makes sense - was mainly confused around identifying the parameter, and about the possible need to perform the logic generically for other possible visitors which may produce property access. The new version performs everything with EE.
Makes sense. I still propose to keep the EE and query filter parameter prefixes distinct, to help debugging where the parameters came from. We can keep "__" for regular parameters, and have "EE" and "QF" for entity equality and query filters, respectively. How does that sound?
My problem here is mainly with the fact that ParameterExtractingEV populates |
src/EFCore/Query/Pipeline/EntityEqualityRewritingExpressionVisitor.cs
Outdated
Show resolved
Hide resolved
Also fixes #14645 |
Also resolves #14644 since parameters are added to parameter values before we go through translation/SQL generation phase. |
75f5e86
to
ae4a9ed
Compare
src/EFCore/Query/Pipeline/EntityEqualityRewritingExpressionVisitor.cs
Outdated
Show resolved
Hide resolved
src/EFCore/Query/Pipeline/EntityEqualityRewritingExpressionVisitor.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.
some tiny bits to fix.
src/EFCore/Query/Pipeline/EntityEqualityRewritingExpressionVisitor.cs
Outdated
Show resolved
Hide resolved
test/EFCore.Cosmos.FunctionalTests/Query/SimpleQueryCosmosTest.cs
Outdated
Show resolved
Hide resolved
ae4a9ed
to
87777ba
Compare
Want to give this a last quick look @smitpatel? If not I'll merge. |
src/EFCore/Query/Pipeline/EntityEqualityRewritingExpressionVisitor.cs
Outdated
Show resolved
Hide resolved
src/EFCore/Query/Pipeline/EntityEqualityRewritingExpressionVisitor.cs
Outdated
Show resolved
Hide resolved
87777ba
to
7b2cc0d
Compare
Windows build failed with the following, merging:
|
Entity equality introduces member access expressions on what may be a
parameter or a constant. Identify these cases and transform them into
a new parameter (for access of a parameter) or evaluate the constant.
Fixes #15855
Fixes #14645
Fixes #14644
Notes (additional comments inline):
TypeAttributes.NestedPrivate
to ParameterExpression. We could go around this by having ParameterExtractingEV inject a special, custom ParameterExpression which we could easily identify later - not sure if that's a good idea. We can always revisit all this later._<member name>
, e.g.@__local_0_CustomerID
. We could go the extra mile to make this@__local_CustomerID_x
if it's important - ideally that would mean the QCC has access to the QueryContext (to see how many parameters are on it), or we can code-generate calculating it in runtime, which seems bad.Dictionary<string, object>
to QCC but it always contains lambda values. I'd like to take a look and see if it could be cleaner, i.e. two subclasses - one for the frontend and another for the filters.