Skip to content
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

Entity equality: Contains and OrderBy #16134

Merged
merged 2 commits into from
Jun 18, 2019
Merged

Entity equality: Contains and OrderBy #16134

merged 2 commits into from
Jun 18, 2019

Conversation

roji
Copy link
Member

@roji roji commented Jun 18, 2019

Closes #15939

For Contains, I realized half-way through that the test (Contains_over_entityType_should_rewrite_to_identity_equality) depends on #15855, since the second query sends the ID selected in the first query. Am still submitting the implementation - the expression tree generated internally looks good. I can also hold off until #15855 is done.

Also, I tested the case Orders.Contains(null) and was surprised to see the null is parameterized. Is this intended?

Finally, unrelated to EE: I understand the need to split into two queries for Single, since the 1-element verification needs to happen client-side. However, for First, FOD, Last, LOD we can translate as a single query:

SELECT (
    SELECT TOP(1) [o].[OrderID]
    FROM [Orders] AS [o]
    WHERE [o].[OrderID] = 10248)
IN (
    SELECT [o].[OrderID]
    FROM [Orders] AS [o]
    WHERE [o].[CustomerID] = N'VINET'
)");

(this is PostgreSQL, SQLServer requires the surrounding CASE for boolean). Is this optimization already tracked somewhere, should I open an issue?

For OrderBy, note that I've implemented support for composite keys, where we split the comparison into multiple OrderBy().ThenBy() operators.

@roji roji merged commit 6b60d6a into master Jun 18, 2019
@ghost ghost deleted the EELeftovers branch June 18, 2019 16:28
@smitpatel
Copy link
Member

This build was failing with error

Microsoft.EntityFrameworkCore.ApiConsistencyTest.Public_api_bool_parameters_should_not_be_prefixed
\r\n-- Prefixed bool parameters --\r\nMicrosoft.EntityFrameworkCore.Query.Pipeline.EntityEqualityRewritingExpressionVisitor.<VisitOrderingMethodCall>g__GetOrderingMethodInfo|18_0[isFirstOrdering]\r\nMicrosoft.EntityFrameworkCore.Query.Pipeline.EntityEqualityRewritingExpressionVisitor.<VisitOrderingMethodCall>g__GetOrderingMethodInfo|18_0[isAscending]\r\nExpected: False\r\nActual:   True
   at Microsoft.EntityFrameworkCore.ApiConsistencyTestBase.Public_api_bool_parameters_should_not_be_prefixed() in D:\code\EntityFrameworkCore\test\EFCore.Tests\ApiConsistencyTestBase.cs:line 248

Build break fixed in aafc355

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Entity Equality: rewrite Contains, OrderBy
2 participants