Bugfix: Cater to optional segmenter matches in filterByLookupOrder #35
Conversation
There was a problem hiding this comment.
Thanks for the changes, @krithika369! LGTM, left some comments/questions for clarification 🙏🏻
| @@ -128,38 +128,37 @@ func (es *experimentService) filterByLookupOrder( | |||
| filtered := matches | |||
There was a problem hiding this comment.
I'm wondering if the above // Stop search when we have at least 1 match is still relevant here. Feels like it should be shifted down 🤔
There was a problem hiding this comment.
I missed this comment - not sure it had much significance previously either. Removed it. Thanks for pointing out.
| currentFilteredList := []*models.ExperimentMatch{} | ||
| segmenterType := segmenterTypes[segmenter] | ||
| for _, transformedValue := range orderedValues { | ||
| if len(currentFilteredList) == 0 { | ||
| for _, segmenterMatch := range filtered { | ||
| segmenterMatchedValue := segmenterMatch.SegmenterMatches[segmenter] | ||
| for _, experiment := range filtered { |
There was a problem hiding this comment.
Thanks for renaming this 👍🏻 , segmenterMatch really didn't make any sense as I was reviewing these changes.
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| filtered = currentFilteredList | ||
| // currentFilteredList could be 0 in case of weak matches | ||
| if len(currentFilteredList) > 0 { |
There was a problem hiding this comment.
Just to affirm my understanding, this is because orderedValues could potentially be empty right?
There was a problem hiding this comment.
Correct. Added more info in the comment now.
What this PR does / why we need it:
When multiple experiments are matched by the Fetch Treatment API, the hierarchical experiment filters ensure that ultimately, only one experiment will be selected. However, in a case where there are multiple experiments after
filterByMatchStrengthand there are missing segmenter values to the Fetch Treatment request, thefilterByLookupOrderfunction would simply eliminate the experiment(s) from the matches. But, in fact, the overall matches should only be updated if there are non-zero matches for a given segmenter value (which will automatically handlenilsegmenter values).Which issue(s) this PR fixes:
This PR addresses the issue above by only updating the filtered experiments if the matches in the current iteration is non-zero, much like the other filters for hierarchical experiments.