Upstream issue #11961: alternative de-duplication approach #1
Conversation
We currently list the same page multiple times if, for example, both the title and content search match it. Since the preview is always the same, this is not very helpful.
… the de-duplication string
jayaddison
changed the title
| @@ -345,7 +350,8 @@ const Search = { | |||
| // note the reversing of results, so that in the case of duplicates, the highest-scoring entry is kept | |||
| let seen = new Set(); | |||
| results = results.reverse().reduce((acc, result) => { | |||
| let resultStr = result.slice(0, 4).concat([result[5]]).map(v => String(v)).join(','); | |||
| const [docName, _title, anchor, descr, _score, filename] = result; | |||
| const resultStr = `${docName},${anchor},${descr},${filename}`; | |||
There was a problem hiding this comment.
Hmm, I don't see how this would pass the test below. 🤔 The body search and title search will have different resultStr's (since they have different anchors), so you'd see them both in the results.
There was a problem hiding this comment.
The test does succeed with that - could that imply a bug in the test?
There was a problem hiding this comment.
There's only one search result because my other fixes haven't been applied (I left them out for ease of review). If you rebase your branch on top of these pull requests the test should start failing:
There was a problem hiding this comment.
Hm. Are you sure? I can still get the test to pass with a docid of 1 instead of zero, and the partial-matching only affects the scoring, if I understand correctly?
There was a problem hiding this comment.
Took a look, I think it's actually sphinx-doc#11959 (the multiple matches) that allows multiple results to be generated as you'd expect. If you apply this squashed patch your tests should start failing: https://gist.github.com/wlach/74124608c2113d8eb9737cdced4762db
There was a problem hiding this comment.
Bug raised as sphinx-doc#11965.
There was a problem hiding this comment.
A further change may be needed to duplicate between those client-side, yes, but I think it could be simplified (more like e2d07a1) if the duplicate titles index entries are removed.
Hmm, I'm confused-- that looks similar to my original solution which you rejected here:
There was a problem hiding this comment.
It's not ideal, but it's smaller -- there's no isDocumentTitle, and no additional boolean passed down to the de-duplication phase.
There was a problem hiding this comment.
(and in fact, it might not even be required. if the index-construction issue is valid and fixed, then we could try removing it and relying purely on an updated de-duplication key)
There was a problem hiding this comment.
It's not ideal, but it's smaller -- there's no isDocumentTitle, and no additional boolean passed down to the de-duplication phase.
Ok, I don't have a super strong opinion: I thought your original critique had a point but I don't think it matters all that much in the end.
(and in fact, it might not even be required. if the index-construction issue is valid and fixed, then we could try removing it and relying purely on an updated de-duplication key)
I am pretty sure we still have the problem of the title search / term search returning more-or-less the same result. The unit test uses a hand-constructed index without this problem and still reproduces the bug.
…nges; currently requires enabling mutability of _displayNextItem - not great.
|
I'm going to close this as it looks like the discussion has ended and I'm satisfied with my approach in sphinx-doc#11942 |
Feature or Bugfix
Purpose
Detail
titlefield from the search result de-duplication key (resultStr).Relates