fix: Avoid duplicated requestable fields #1621
Conversation
islamaliev
added
bug
So far I didn't see any public impact. I'm not sure why. The collected fields are used internally and I assume that so far the code didn't rely on uniqueness of the fields in the list. |
Then why make the change? |
I could have added this code to the part related to secondary indexes, but I though that it would be beneficial for us to have a generic solution. Or you are asking why a separate PR? I though it was quite related to my stuff so that the score is limited. I can include this fix in secondary indexes task. Not a problem. |
There was a problem hiding this comment.
I could have added this code to the part related to secondary indexes, but I though that it would be beneficial for us to have a generic solution. Or you are asking why a separate PR? I though it was quite related to my stuff so that the score is limited. I can include this fix in secondary indexes task. Not a problem.
I was asking why you are making the change at all, what it does, and what you have done to check that it does what you want it to do :) In the absence of tests it is quite hard to tell.
Approving with a minor suggestion, and assuming that you have checked it has the result you think it has (going by the linked ticket). I would prefer that there were tests for this, but going by your previous answers it looks like secondary indexes is not working because of this, and integration tests for secondary indexes will flag any regressions here?
planner/mapper/mapper.go
Outdated
| isDuplicate := false | ||
| for _, field := range existingFields { | ||
| if field.GetIndex() == keyIndex { | ||
| isDuplicate = true |
There was a problem hiding this comment.
suggestion: You can drop the isDuplicate var by naming the outer loop and continuing here:
sourceLoop:
for key := range source {
...
if field.GetIndex() == keyIndex {
continue sourceLoop
}
There was a problem hiding this comment.
yeah, I though about it but the label would be way up there... Anyway I will switch to use the label
There was a problem hiding this comment.
Is quite a distance for sure :) No pressure if you prefer the bool
| @@ -844,6 +845,11 @@ func resolveInnerFilterDependencies( | |||
| // If the key index is outside the bounds of the child mapping array, then | |||
| // this is not a relation/join and we can add it to the fields and | |||
| // continue (no child props to process) | |||
| for _, field := range existingFields { | |||
There was a problem hiding this comment.
question: You are removing duplicate fields that didn't impact query results at the expense of an extra for loop for every field. Could you explain why you had to do this? Or guide me towards the explanation if it exists already.
There was a problem hiding this comment.
The linked issue does cover this (#1615), but it does require some imagination in order to connect the dots. Was why I was quizing him quite hard RE testing.
There was a problem hiding this comment.
It doesn't. It shows state of variables. I understand that there was duplicates and doing this removes the duplicates. I'd like to know if it's just for cleanliness or if it has a direct impact on something else which could help support the extra for loop.
There was a problem hiding this comment.
I already replied to Andy on this matter. Here is my quote:
So far I didn't see any public impact. I'm not sure why. The collected fields are used internally and I assume that so far the code didn't rely on uniqueness of the fields in the list.
I noticed this by observing weird behaviour while working on indexes.
islamaliev
force-pushed
the
fix/islam/avoid-duplicated-requestable-fields
branch
from
510eb37 to
779e6a1
Compare
Codecov ReportPatch coverage:
@@ Coverage Diff @@
## develop #1621 +/- ##
===========================================
+ Coverage 76.03% 76.13% +0.10%
===========================================
Files 192 192
Lines 19852 19856 +4
===========================================
+ Hits 15094 15116 +22
+ Misses 3723 3711 -12
+ Partials 1035 1029 -6
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 6 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
## Relevant issue(s) Resolves sourcenetwork#1615 ## Description Makes sure the requestable fields aren't duplicated
Relevant issue(s)
Resolves #1615
Description
Makes sure the requestable fields aren't duplicated
Tasks
How has this been tested?
Intergration tests
Specify the platform(s) on which this was tested: