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.

Sign up for GitHub

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

Jump to bottom

Query: Rownumber conversion needs better logic to figure out partitions #26756

Closed
smitpatel opened this issue Nov 18, 2021 · 0 comments
Closed

Query: Rownumber conversion needs better logic to figure out partitions #26756

smitpatel opened this issue Nov 18, 2021 · 0 comments
Assignees
@smitpatel
Labels
area-query closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. customer-reported Servicing-approved type-bug
Milestone
6.0.3

Comments

@smitpatel
Copy link
Member

Query like this

var query = db.Set<Parent>()
    .SelectMany(p => p.Children
        .Where(c => c.SomeNullableDateTime == null)
        .OrderBy(c => c.SomeInteger)
        .Take(1))
    .Where(c => c.SomeOtherNullableDateTime != null)
    .Select(c => c.SomeNullableDateTime);

When converting rownumber (Apply coming from SelectMany) has predicate on Select as p.Id != null && p.Id == c.ParentId && c.SomeNullableDateTime != null
We end up pulling the last term even though that is not part of join key and shouldn't be in partitions.
Faulty code which causes match

efcore/src/EFCore.Relational/Query/SqlExpressions/SelectExpression.cs

Lines 2340 to 2341 in e451959

if (IsContainedColumn(outer, sqlBinaryExpression.Right)
&& IsContainedColumn(inner, sqlBinaryExpression.Left))

@smitpatel smitpatel mentioned this issue Nov 18, 2021
Closed
@ajcvickers ajcvickers added this to the 6.0.x milestone Nov 19, 2021
smitpatel added a commit that referenced this issue Jan 26, 2022
@smitpatel
Query: Match correct predicate structure to convert apply to join
17b2756 
Resolves #26756

For property access on optional dependents sharing column with principal, we generate CaseExpression. In order to match them during join key search, we updated our recursive function to match shape of the test in CaseExpression which incorrectly matched similar structure outside of case block causing somewhat wrong join key to be extracted. While join key in itself could work, when generating partitions out of it due to paging operation, it gives incorrect results.
The fix is to match the special structure of CaseExpression.Test separately. Also made the key comparison match more robust by only allowing column or case block to appear in condition. Other structures don't represent part of join key in a comparison.
Null checks for join keys are handled separately so that we only remove null checks which are indeed used in comparison with other columns in other operations.
@smitpatel smitpatel added the closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. label Jan 26, 2022
@smitpatel smitpatel mentioned this issue Jan 26, 2022
Merged
@ajcvickers ajcvickers modified the milestones: 6.0.x, 6.0.3 Jan 27, 2022
smitpatel added a commit that referenced this issue Feb 2, 2022
@smitpatel
Query: Match correct predicate structure to convert apply to join
804137a 
Resolves #26756

For property access on optional dependents sharing column with principal, we generate CaseExpression. In order to match them during join key search, we updated our recursive function to match shape of the test in CaseExpression which incorrectly matched similar structure outside of case block causing somewhat wrong join key to be extracted. While join key in itself could work, when generating partitions out of it due to paging operation, it gives incorrect results.
The fix is to match the special structure of CaseExpression.Test separately. Also made the key comparison match more robust by only allowing column or case block to appear in condition. Other structures don't represent part of join key in a comparison.
Null checks for join keys are handled separately so that we only remove null checks which are indeed used in comparison with other columns in other operations.
@ajcvickers ajcvickers mentioned this issue Mar 17, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@smitpatel smitpatel

Labels
area-query closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. customer-reported Servicing-approved type-bug
Projects
None yet
Milestone
6.0.3
Development

No branches or pull requests
2 participants
@ajcvickers @smitpatel