-
Notifications
You must be signed in to change notification settings - Fork 51
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
feat: Add support for one sided relations #3021
feat: Add support for one sided relations #3021
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3021 +/- ##
===========================================
+ Coverage 79.39% 79.44% +0.05%
===========================================
Files 331 331
Lines 25680 25686 +6
===========================================
+ Hits 20387 20404 +17
+ Misses 3834 3827 -7
+ Partials 1459 1455 -4
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 12 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
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.
LGTM. Thanks for fixing this.
// The field definition of the relation-object field on this side of the relation. | ||
// | ||
// This will always have a value on the primary side, but it may not have a value on | ||
// the secondary side, as the secondary half of the relation is optional. |
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.
thought: I look forward to the secondary side being handle automatically with the proposed handling of relationship from the primary side.
@@ -286,6 +286,11 @@ func findFilteredByRelationFields( | |||
} | |||
|
|||
func (p *Planner) tryOptimizeJoinDirection(node *invertibleTypeJoin, parentPlan *selectTopNode) error { | |||
if !node.childSide.relFieldDef.HasValue() { | |||
// If the relation is one sided we cannot invert the join, so return early | |||
return nil |
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.
suggestion: If not too much trouble would be nice to hit this case as it shows not covered right now.
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.
It might be dead code, but I'm not sure - the invertable join stuff is quite complex and if it isn't hit by the new test I don't know what will hit it. And I don't want to spend much time on this right now, especially given the pending secondary relation rework Fred mentioned.
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.
LGTM, just one suggestion
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.
LGTM!
c949d8e
to
df324b0
Compare
Relevant issue(s)
Resolves #2830
Description
Adds support for one sided relations.
I thought we had this a while back, but David discovered otherwise.