Expose duplicate removal in the completion suggester #26496
Conversation
This change exposes the duplicate removal option added in Lucene for the completion suggester with a new option called `skip_duplicates` (defaults to false). This commit also adapts the custom suggest collector to handle deduplication when multiple contexts match the input. Closes elastic#23364
jimczi
added
:Search Relevance/Suggesters
There was a problem hiding this comment.
@jimczi I took a look out of interest how this feature is implemented in Lucene. My knowledge of how the documents collector works is limited, but the rest looks good to me. Left a few nits and questions, those mostly because I'm not too familiar with the underlying feature.
| public CompletionSuggestion() { | ||
| } | ||
|
|
||
| public CompletionSuggestion(String name, int size) { | ||
| public CompletionSuggestion(String name, int size, boolean skipDuplicates) { |
There was a problem hiding this comment.
I always get confused, is this a user-facing API? In that case, should we keep the original constructor with "false" as the default? If not all good.
| return skipDuplicates; | ||
| } | ||
|
|
||
| public CompletionSuggestionBuilder skipDuplicates(boolean skipDuplicates) { |
There was a problem hiding this comment.
nit: maybe add some javadoc since this is what most users see that use the java api
| @@ -210,6 +223,15 @@ private CompletionSuggestionBuilder contexts(XContentBuilder contextBuilder) { | |||
| return this; | |||
| } | |||
|
|
|||
| public boolean skipDuplicates() { | |||
There was a problem hiding this comment.
nit: maybe add some javadoc since this is what most users see that use the java api
| * This collector is also able to filter duplicate suggestion coming from different documents. | ||
| * When different contexts match the same suggestion form only the best one (sorted by weight) is kept. | ||
| * In order to keep this feature fast, the de-duplication of suggestions with different contexts is done | ||
| * only on the top N*num_contexts suggestions per segment. This means that skip_duplicates will visit at most N*num_contexts suggestions |
There was a problem hiding this comment.
question for my own understanding: what is N here?
There was a problem hiding this comment.
The completion suggester is document based so N here is the number of documents to return.
| @@ -150,93 +164,57 @@ void add(CharSequence key, CharSequence context, float score) { | |||
| } | |||
| } | |||
|
|
|||
| private static final class SuggestDocPriorityQueue extends PriorityQueue<SuggestDoc> { | |||
There was a problem hiding this comment.
For my own understanding, can you briefly explain what the queue did here before and why this can now be done in TopSuggestDocsCollector (at least thats what it looks to me)
There was a problem hiding this comment.
Prior to this change the custom collector was used to group the suggestions coming from different contexts per document.
The TopSuggestDocsCollector is not document based and can return suggestions coming from different documents so this custom collector was needed. What changed is that we record the document in a map during the collection and if a document was already visited we just hide it from the parent collector. When collection is finished we get the top N suggestions from the parent collector and re-group them per document. Because we ensured that a document can only be visited once, the N suggestions returned by the parent collector are all coming from different docs and we just need to link the skipped suggestions to the final top N documents.
Early termination is done the same except that the decision is taken by the parent collector.
…hes the input with different suggestions within the same context
|
Thanks @cbuescher |
|
Thanks @cbuescher ! |
… tests. The completion suggester has a `shard_size` option that sets the size of the suggestions to retrieve per shard but it is ignored by the builder. This commit restores the handling of this option and fixes a test that can randomly fail without it.
…GeoBoosting This test should not rely on strict ordering for same score suggestions. The Lucene completion suggester uses the doc id in case of a tie and documents are indexed randomly.
This change exposes the duplicate removal option added in Lucene for the completion suggester with a new option called `skip_duplicates` (defaults to false). This commit also adapts the custom suggest collector to handle deduplication when multiple contexts match the input. Closes #23364
…rflow * origin/master: (59 commits) Fix Lucene version of 5.6.1. Remove azure deprecated settings (elastic#26099) Handle the 5.6.0 release Allow plugins to validate cluster-state on join (elastic#26595) Remove index mapper dynamic settings (elastic#25734) update AWS SDK for ECS Task IAM support in discovery-ec2 (elastic#26479) Azure repository: Accelerate the listing of files (used in delete snapshot) (elastic#25710) Build: Remove norelease from forbidden patterns (elastic#26592) Fix reference to painless inside expression engine (elastic#26528) Build: Move javadoc linking to root build.gradle (elastic#26529) Test: Remove leftover static bwc test case (elastic#26584) Docs: Remove remaining references to file and native scripts (elastic#26580) Snapshot fallback should consider build.snapshot elastic#26496: Set the correct bwc version after backport to 6.x Fix the MapperFieldType.rangeQuery API. (elastic#26552) Deduplicate `_field_names`. (elastic#26550) [Docs] Update method setSource(byte[] source) (elastic#26561) [Docs] Fix typo in javadocs (elastic#26556) Allow multiple digits in Vagrant 2.x minor versions Support Vagrant 2.x ...
This change exposes the duplicate removal option added in Lucene for the completion suggester
with a new option called
skip_duplicates(defaults to false).This commit also adapts the custom suggest collector to handle deduplication when multiple contexts match the input.
Closes #23364