Skip to content

Commit

Permalink
Remove special merging behavior for line matches (#888)
Browse files Browse the repository at this point in the history
Usually, if there are candidate matches with overlapping ranges, then we just remove matches that overlap. However, when `opts.ChunkMatches = false`, we had special logic to merge overlapping matches.

This PR removes the overlapping logic to simplify the behavior. I couldn't see a good reason to keep this special handling. Plus, we are moving towards making `ChunkMatches` the default. 

Another benefit of this change is that it makes the BM25 behavior easier to understand. If we merged together ranges, then we would be calculating term frequencies for spurious terms (like `new`, `queue`, `newqueue`, `queuenew`, etc.) Note: we currently only use BM25 with `ChunkMatches = true`, so there's not an active bug here.
  • Loading branch information
jtibshirani authored Jan 15, 2025
1 parent 4c8bb19 commit d301e83
Showing 1 changed file with 9 additions and 36 deletions.
45 changes: 9 additions & 36 deletions eval.go
Original file line number Diff line number Diff line change
Expand Up @@ -323,8 +323,7 @@ nextFileMatch:
// Important invariant for performance: finalCands is sorted by offset and
// non-overlapping. gatherMatches respects this invariant and all later
// transformations respect this.
shouldMergeMatches := !opts.ChunkMatches
finalCands := d.gatherMatches(nextDoc, mt, known, shouldMergeMatches)
finalCands := d.gatherMatches(nextDoc, mt, known)

if opts.ChunkMatches {
fileMatch.ChunkMatches = cp.fillChunkMatches(finalCands, opts.NumContextLines, fileMatch.Language, opts)
Expand Down Expand Up @@ -417,7 +416,7 @@ func addRepo(res *SearchResult, repo *Repository) {
// If `merge` is set, overlapping and adjacent matches will be merged
// into a single match. Otherwise, overlapping matches will be removed,
// but adjacent matches will remain.
func (d *indexData) gatherMatches(nextDoc uint32, mt matchTree, known map[matchTree]bool, merge bool) []*candidateMatch {
func (d *indexData) gatherMatches(nextDoc uint32, mt matchTree, known map[matchTree]bool) []*candidateMatch {
var cands []*candidateMatch
visitMatches(mt, known, 1, func(mt matchTree, scoreWeight float64) {
if smt, ok := mt.(*substrMatchTree); ok {
Expand Down Expand Up @@ -449,9 +448,10 @@ func (d *indexData) gatherMatches(nextDoc uint32, mt matchTree, known map[matchT
}}
}

// Remove overlapping candidates. This guarantees that the matches
// are non-overlapping, but also preserves expected match counts.
sort.Sort((sortByOffsetSlice)(cands))
res := cands[:0]
mergeRun := 1
for i, c := range cands {
if i == 0 {
res = append(res, c)
Expand All @@ -466,39 +466,12 @@ func (d *indexData) gatherMatches(nextDoc uint32, mt matchTree, known map[matchT
continue
}

if merge {
// Merge adjacent candidates. This guarantees that the matches
// are non-overlapping.
lastEnd := last.byteOffset + last.byteMatchSz
end := c.byteOffset + c.byteMatchSz
if lastEnd >= c.byteOffset {
mergeRun++
// Average out the score across the merged candidates. Only do it if
// we are boosting to avoid floating point funkiness in the normal
// case.
if !(epsilonEqualsOne(last.scoreWeight) && epsilonEqualsOne(c.scoreWeight)) {
last.scoreWeight = ((last.scoreWeight * float64(mergeRun-1)) + c.scoreWeight) / float64(mergeRun)
}

// latest candidate goes further, update our end
if end > lastEnd {
last.byteMatchSz = end - last.byteOffset
}

continue
} else {
mergeRun = 1
}
} else {
// Remove overlapping candidates. This guarantees that the matches
// are non-overlapping, but also preserves expected match counts.
lastEnd := last.byteOffset + last.byteMatchSz
if lastEnd > c.byteOffset {
continue
}
// Only add the match if its range doesn't overlap
lastEnd := last.byteOffset + last.byteMatchSz
if lastEnd <= c.byteOffset {
res = append(res, c)
continue
}

res = append(res, c)
}
return res
}
Expand Down

0 comments on commit d301e83

Please sign in to comment.