-
Notifications
You must be signed in to change notification settings - Fork 24.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Completion Suggester: Add doc-values based payload support #13659
Completion Suggester: Add doc-values based payload support #13659
Conversation
@@ -158,6 +160,14 @@ public CompletionSuggestParser(CompletionSuggester completionSuggester) { | |||
} else { | |||
throw new IllegalArgumentException("suggester [completion] doesn't support field [" + fieldName + "]"); | |||
} | |||
} else if (token == XContentParser.Token.START_ARRAY) { | |||
if ("payload".equals(fieldName)) { | |||
while (parser.nextToken() != XContentParser.Token.END_ARRAY) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe a bit better error checking here? E.g. if you see a START_OBJECT token, that's not allowed right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the pointer, I have made it stricter and also added tests to ensure it errors out on malformed input
This looks great, thanks @areek, I left a few small comments. Net/net this is more functionality vs. current suggesters, since you can retrieve more than one field as the payload (vs only one payload per suggestion today), and they can be typed fields (numbers, dates, etc.) too. The code is also quite simpler than supporting the 2nd fetch phase ... |
Wait we are using docvalues as stored fields now? I do not think this is something we should do. |
I think for the suggest usecase that's a good alternative to putting it into the FST. We will deprecate this option as well once we have the second roundtrip query_then_fetch for this too but that will go into 3.x |
Thanks @mikemccand for the review, addressed all your feedback. I think the docs can be improved to encourage users to use doc values enabled fields for payload? Maybe @clintongormley has some thoughts? @rmuir this is added to make the new implementation have feature parity with the current suggester and as @s1monw mentioned will go away in 3.x, where documents will be fetched in the 2nd phase. |
@areek I like it |
Thanks @areek, LGTM. |
d8918c4
to
d48a1d6
Compare
Note: the PR is against
feature/completion_suggester_v2
branchFollowup of #13576 (comment). The PR enables returning any doc values enabled fields as suggestion payload.
One can specify fields to be returned as part of the suggestion payload at query time, using the
payload
option:The specified payload field values will be returned as part of the suggestion
payload
:Now suggestion
payload
are not part of the completion (FST) index, as was the case before. Payload fields are fetched for the top N completions per shard hence specifying payload fields will incur additional search performance hit.