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: Implement TPT for relational layer #21474

Merged
1 commit merged into from
Jul 3, 2020
Merged

Query: Implement TPT for relational layer #21474

1 commit merged into from
Jul 3, 2020

Conversation

smitpatel
Copy link
Member

Resolves #2266

@AndriySvyryd
Copy link
Member

Looking good so far

@smitpatel
Copy link
Member Author

Updated to expand owned navigations on TPT.

@smitpatel smitpatel force-pushed the smit/TPT branch 2 times, most recently from 0327d4d to 169e353 Compare July 2, 2020 19:15
@smitpatel smitpatel marked this pull request as ready for review July 2, 2020 19:16
@smitpatel
Copy link
Member Author

Updated to handle table splitting & TPT.
Pending items in original work, most likely only test change. Will submit it in separate PR.

  • Convert GearsOfWar to TPT
  • Convert M2M tests to TPT
  • Add split query tests in M2M/GearsOfWar/TPTRelationships
  • Convert FilterInheritance to TPT

@smitpatel smitpatel requested review from AndriySvyryd, roji, maumar and a team July 2, 2020 19:19
@AndriySvyryd
Copy link
Member

Why not separate commits? 😭

@smitpatel smitpatel force-pushed the smit/TPT branch 2 times, most recently from 592f9c5 to 3a3221b Compare July 3, 2020 17:51
@AndriySvyryd
Copy link
Member

Did you make a mistake with the commits again?

@smitpatel
Copy link
Member Author

Did you make a mistake with the commits again?

I had to rebase to pick up test infra changes from @maumar - There is another set of test changes coming for TPTRelationships tests.

@AndriySvyryd
Copy link
Member

I had to rebase to pick up test infra changes from @maumar - There is another set of test changes coming for TPTRelationships tests.

You can rebase without squashing

AndriySvyryd
AndriySvyryd reviewed Jul 3, 2020
View reviewed changes
src/EFCore.Relational/Query/SqlExpressions/ColumnExpression.cs
property.GetTableColumnMappings().Cast<IColumnMappingBase>().Concat(property.GetViewColumnMappings())
.FirstOrDefault()?.Column.Name // TODO: this should take table into account
?? property.GetColumnName(),
column?.Name ?? property.GetColumnName(),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you are going to keep GetColumnName() you might need to add code for views

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What should be used here?
There should always be column passed if there is mapping. GetColumnName is only used in scenario when there is no mapping.
Related: What should be the mappings used for FromSql & TVF methods (not the query root mapped to the function)?

Copy link
Member

@AndriySvyryd AndriySvyryd Jul 3, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FromSql should use the same mapping as a normal query. Method TVF mappings are also returned by GetFunctionMappings, but with IsDefaultFunctionMapping set to false.
Would there be any other situations where column is null?
Btw, you don't need to fix this in this PR

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Filed #21511

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You you should do it in #21507 😄

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would but I would like to understand more about which column mappings to use when no table(base)mappings are used from entity type. It also has overlap with #21508

@ghost
Copy link

ghost commented Jul 3, 2020

Hello @smitpatel!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@ghost ghost merged commit 20d587c into master Jul 3, 2020
@ghost ghost deleted the smit/TPT branch July 3, 2020 20:55
@ajcvickers ajcvickers mentioned this pull request Jul 9, 2020
Closed
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AndriySvyryd AndriySvyryd AndriySvyryd approved these changes

@roji roji Awaiting requested review from roji

@maumar maumar Awaiting requested review from maumar

Assignees
No one assigned
Labels
auto-merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Relational: TPT inheritance mapping pattern
2 participants
@smitpatel @AndriySvyryd