fix: Queries with filter on 2 rel fields of composite index #3035
Conversation
islamaliev
added
bug
islamaliev
changed the title
internal/planner/multi.go
Outdated
| if doc.Fields[i] == nil { | ||
| doc.Fields[i] = newFields[i] | ||
| } else if newSubDoc, ok := newFields[i].(core.Doc); ok { | ||
| doc.Fields[i] = p.mergeDoc(doc.Fields[i].(core.Doc), newSubDoc.Fields) |
There was a problem hiding this comment.
todo: Please document why this is needed, it is not terribly clear why the node should be mutating child relations.
There was a problem hiding this comment.
well, it now justifies the name of method nextMerge.
I'll add a comment.
There was a problem hiding this comment.
I'm not sure the new comment answers what I'm most uncertain about - why does this function recursively mutate child documents, instead of just the top doc? Why and under which circumstances is the mutation of the child documents required? Do please message/call me on discord if you unsure as to what I'm asking.
There was a problem hiding this comment.
there is no real reason for recursion here. I just though it would cover some future perverted edge-cases.
I made it merge just top fields to keep it simple.
There was a problem hiding this comment.
Ah got it, thanks Islam!
| @@ -70,3 +70,106 @@ func TestQueryWithUniqueCompositeIndex_WithFilterOnIndexedRelation_ShouldFilter( | |||
|
|
|||
| testUtils.ExecuteTestCase(t, test) | |||
| } | |||
|
|
|||
| func TestQueryWithUniqueCompositeIndex_WithIndexComprising2RelationsAndFilterOnIt_ShouldFilter(t *testing.T) { | |||
There was a problem hiding this comment.
suggestion: It might be highlighting with a comment why this test is important to have, for example compared to just for composite scalar tests.
Please also document the significance of including two id fields, and one object field in the index. Perhaps highlighting why a test with two object fields in the index (e.g. includes: [{name: "owner"}, {name: "manufacturer"} is not required.
There was a problem hiding this comment.
The test case is actually a bit simpler. I wanted to update it, but forgot. Thanks for reminding.
I will add some comments.
There was a problem hiding this comment.
Thanks Islam, looks good!
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3035 +/- ##
===========================================
- Coverage 79.35% 79.35% -0.00%
===========================================
Files 333 333
Lines 25837 25841 +4
===========================================
+ Hits 20501 20504 +3
- Misses 3866 3869 +3
+ Partials 1470 1468 -2
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.
|
islamaliev
force-pushed
the
fix/unique-index-return-all-children
branch
from
4d75ba0 to
9519e28
Compare
Relevant issue(s)
Resolves #3032 #2928
Description
Make parallel node perform a real merge instead of copying fields of a fetched doc, which resulted every next doc overwrite previous fields.