ranking: allow partial overlap with symbol #742

stefanhengl · 2024-02-22T16:53:29Z

With this update, we score matches which overlap with a symbol in any way. Before, we only added a score if the match was fully contained in a symbol.

Test plan:

new unit tests
updated scoring tests

keegancsmith · 2024-02-22T18:36:36Z

internal/e2e/testdata/coverage_data_writer.txt

@@ -5,7 +5,7 @@ targetRank: 13
 github.com/golang/go/src/internal/coverage/stringtab/stringtab.go
 19:type Writer struct {
 27:func (stw *Writer) InitWriter() {
-9:	"internal/coverage/slicereader"
+70:func (stw *Writer) Write(w io.Writer) error {


minor: this looks like a much better line match than the older import line.

keegancsmith · 2024-02-22T18:37:41Z

internal/e2e/testdata/time_compare.txt

@@ -0,0 +1,38 @@
+queryString: time compare\(
+query: (and substr:"time" substr:"compare(")


neat that this works now :)

To check I understand -- will the Sourcegraph query parser convert user input like "func resolve(" into this form, where the parenthesis is escaped?

will the Sourcegraph query parser convert user input like "func resolve(" into this form, where the parenthesis is escaped?

Not exactly. In Sourcegraph, if the pattern compare( is labeled as "literal" (which it is if the query is patternType:keyword time compare(), we escape the "(" before parsing it as regexp. Then we convert it to a substring query. This means, what Zoekt gets is a substring query with pattern "compare(".

The test here is based on the behavior of Zoekt, which expects valid regex as input.

jtibshirani

This is looking good to me! Playing "Devil's Advocate", are there any classes of queries where you think this will make things worse?

jtibshirani · 2024-02-22T19:28:34Z

internal/e2e/testdata/time_compare.txt

@@ -0,0 +1,38 @@
+queryString: time compare\(
+query: (and substr:"time" substr:"compare(")


To check I understand -- will the Sourcegraph query parser convert user input like "func resolve(" into this form, where the parenthesis is escaped?

jtibshirani · 2024-02-22T19:30:44Z

contentprovider.go

@@ -637,7 +666,7 @@ func (p *contentProvider) candidateMatchScore(ms []*candidateMatch, language str
 			} else if startMatch || endMatch {
 				addScore("EdgeSymbol", (scoreSymbol+scorePartialSymbol)/2)
 			} else {
-				addScore("InnerSymbol", scorePartialSymbol)
+				addScore("OverlapSymbol", scorePartialSymbol)


I like how this change is so simple. Switching from Inner -> Overlap makes sense to me conceptually.

Some dark arts, but should we consider innersymbol worth more than overlapsymbol? Hard to imagine a query where you get both innersymbol and overlapsymbol so likely not worth it.

stefanhengl · 2024-02-23T14:21:56Z

Playing "Devil's Advocate", are there any classes of queries where you think this will make things worse?

Whenever you search something that didn't match a symbol and now it matches one partially, things might get worse. For example, you might be looking for all TODOs in a repo. Previously, you could have used TODO( to match the pattern "TODO()", but now this will rank functions TODO(....) as top hit.

.github/workflows/ci.yml

jtibshirani · 2024-02-23T17:43:31Z

Whenever you search something that didn't match a symbol and now it matches one partially, things might get worse

I see. This sounds like an unusual case, so not a big concern.

I guess another example would be... given a symbol WriteFiles, these two queries now produce same score:

WriteFile
FilesBuffer

That may be a bit surprising. But I like your simple approach, we can play around with it and adjust later if needed.

contentprovider_test.go

keegancsmith · 2024-02-26T10:48:15Z

I guess another example would be... given a symbol WriteFiles, these two queries now produce same score:

WriteFile

FilesBuffer

I think this isn't possible? For FilesBuffer query to get the overlap symbol you need both FilesBuffer to appear in the corpus and partially be symbol. So it won't match on WriteFiles unless your language somehow has

WriteFilesBuffer
^^^^^^^^^^        only this part is symbol
          ^^^^^^  this part is not a symbol

I'm having a hard time coming up with a query that looks reasonable where previously we had nicer behaviour.

keegancsmith

LGTM

keegancsmith · 2024-02-26T10:51:08Z

contentprovider.go

+	// Find the first section that overlaps.
+	j := sort.Search(len(secs), func(i int) bool { return secs[i].End > off })


do you have an example where it is important to find the max overlap? Can't we just score this based on the first overlap found. If I am not mistaken it doesn't affect the score since any overlap you get a boost.

I thought a bit more about this and went the following route: We now calculate the relative overlap and return the section with the largest relative overlap. This way we prefer complete over partial overlaps, even if the partial overlap is bigger. It's important which section we pick because the boost depends on the symbol type behind the section.

contentprovider.go

keegancsmith · 2024-03-01T03:27:15Z

contentprovider.go

+	// Find the first section that might overlap
+	j := sort.Search(len(secs), func(i int) bool { return secs[i].End > off })
+
+	if j == len(secs) || secs[j].Start >= off+sz {


minor: I don't know if this makes the code better, but I felt like I had to keep ensuring that .Start was with off and .End was off+sz. Maybe two local variables start := off; end := off+sz would make it more obvious at each site we do a comparison that we are doing the right one?

keegancsmith · 2024-03-01T03:38:05Z

contentprovider.go

+		return 0, false
+	}
+
+	relOverlap := func(j int) float64 {


I think we can be more direct than relative overlap. If I am not mistaken, we only care about partial vs full overlap? Could we not do something like:

// if the 2nd section has a full overlap always return it if j+1 < len(secs) && secs[j+1].Start >= off && secs[j+1].End <= off+sz { return j+1, true } // Otherwise the first section is atleast as good of an overlap return j, secs[j].Start < off+sz

If I am not mistaken, we only care about partial vs full overlap?

I think the amount of the overlap should count. For example, if your code looks like

aaaa bbbb

both aaaa and bbbb are symbols. If you search for a bbb it does make sense to prefer the second symbol over the first: the smaller the overlap is, the higher the chance that the match is low quality.

Sure, both are just partial overlaps and get the same boost for that. However, we don't just care about partial vs. complete overlap. The two symbols might be of a different kind and hence get a different score in scoreSymbolKind.

aah that is a good point. Almost makes me think we should be scoring every symbol match and taking the max score?

stefanhengl added 3 commits February 22, 2024 17:44

ranking: allow partial overlap with symbol

6d36bba

state assumption about non-overlapping sections

11029de

reverse change to bytes_buffer.txt

d9626cd

keegancsmith reviewed Feb 22, 2024

View reviewed changes

jtibshirani reviewed Feb 22, 2024

View reviewed changes

pin go version in tests to 1.21

eefa29c

stefanhengl commented Feb 23, 2024

View reviewed changes

.github/workflows/ci.yml Show resolved Hide resolved

stefanhengl marked this pull request as ready for review February 23, 2024 14:23

stefanhengl requested review from keegancsmith and jtibshirani February 23, 2024 14:24

jtibshirani approved these changes Feb 23, 2024

View reviewed changes

contentprovider_test.go Outdated Show resolved Hide resolved

keegancsmith approved these changes Feb 26, 2024

View reviewed changes

use relative overlap instead of absolute overlap

0a7b689

keegancsmith approved these changes Mar 1, 2024

View reviewed changes

introduce start and end

c412114

stefanhengl merged commit 74bbcc9 into main Mar 1, 2024
8 checks passed

stefanhengl deleted the sh/scoring/overlap branch March 1, 2024 09:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ranking: allow partial overlap with symbol #742

ranking: allow partial overlap with symbol #742

stefanhengl commented Feb 22, 2024 •

edited

Loading

keegancsmith Feb 22, 2024

keegancsmith Feb 22, 2024

jtibshirani Feb 22, 2024

stefanhengl Feb 23, 2024 •

edited

Loading

jtibshirani left a comment

jtibshirani Feb 22, 2024

jtibshirani Feb 22, 2024

keegancsmith Feb 26, 2024

stefanhengl commented Feb 23, 2024

jtibshirani commented Feb 23, 2024

keegancsmith commented Feb 26, 2024

keegancsmith left a comment

keegancsmith Feb 26, 2024

stefanhengl Feb 29, 2024

keegancsmith Mar 1, 2024

keegancsmith Mar 1, 2024 •

edited

Loading

stefanhengl Mar 1, 2024 •

edited

Loading

keegancsmith Mar 3, 2024

		@@ -0,0 +1,38 @@
		queryString: time compare\(
		query: (and substr:"time" substr:"compare(")

		// Find the first section that overlaps.
		j := sort.Search(len(secs), func(i int) bool { return secs[i].End > off })

ranking: allow partial overlap with symbol #742

ranking: allow partial overlap with symbol #742

Conversation

stefanhengl commented Feb 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stefanhengl Feb 23, 2024 • edited Loading

Choose a reason for hiding this comment

jtibshirani left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stefanhengl commented Feb 23, 2024

jtibshirani commented Feb 23, 2024

keegancsmith commented Feb 26, 2024

keegancsmith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keegancsmith Mar 1, 2024 • edited Loading

Choose a reason for hiding this comment

stefanhengl Mar 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stefanhengl commented Feb 22, 2024 •

edited

Loading

stefanhengl Feb 23, 2024 •

edited

Loading

keegancsmith Mar 1, 2024 •

edited

Loading

stefanhengl Mar 1, 2024 •

edited

Loading