Completion Suggester: Support returning documents with completions

[areek]

Note: the PR is against feature/completion_suggester_v2 branch

Currently, the new completion suggester (#11740) can associate a document with a completion. This PR adds support to return documents enriched with their completions.

Example request:

POST music/_suggest 
{
  "song-suggest" : {
    "prefix" : "nev",
    "completion" : {
      "field" : "song_suggest"
    }
  }
}

Example response:

{
  "song-suggest": [
    {
      "text": "nev",
      "offset": 0,
      "length": 4,
      "options": [ {
          "text": "Nevermind",
          "_index": "music",
          "_type": "song",
          "_id": "52",
          "_score": 52,
          "_source": {
            ....
          }
        }, ...
      ]
    }
  ]
}

Source filtering options are supported.

Internally a suggestion request is represented as a search request implementing a two-phased suggest then fetch transport action for multiple shards and a single-phased suggest and fetch phase for single shard.
NOTE: the two-phased suggest transport action has yet to be integrated with the _search API

[s1monw]

I looked at this and I like the code but that PR is very delicate and big. I wonder if it would make more sense if we land the branch without this feature on 2.x and move this to 3.0. I think we can generally refactor how we fetch documents and reuse much code if we do it right. I don't know how others feel @mikemccand @clintongormley

[mikemccand]

The change is indeed biggish ...

If we land completion suggester for 2.1 without fetching docs (this PR), we've lost some functionality (payloads) vs the current suggester impls, right? Is that OK, or are payloads frequently used?

If we do delay this PR, why would we need to delay fetching docs until 3.0? Couldn't we target e.g. 2.2 or something?

[s1monw]

If we do delay this PR, why would we need to delay fetching docs until 3.0? Couldn't we target e.g. 2.2 or something?

the problem here is I guess that we can't really use this feature until we know all nodes support it otherwise you'd get partial results? The other option would be to return docs eagerly and don't do a second roundtrip which would be ok if fetching stuff from DocValues etc?

[mikemccand]

the problem here is I guess that we can't really use this feature until we know all nodes support it otherwise you'd get partial results?

Oh, I see, because a mixed minor version cluster is allowed... hrm.

The other option would be to return docs eagerly and don't do a second roundtrip which would be ok if fetching stuff from DocValues etc?

This would simplify things greatly (not needing 2nd fetch phase), but then there's some perf hit since we're pulling docs/docvalues for many hits that we then throw away when we merge suggestions across all shards ... and I wonder how much of a perf hit we are talking about even w/ the 2nd fetch phase since the current suggesters pull payloads directly from the FST.

[s1monw]

This would simplify things greatly (not needing 2nd fetch phase), but then there's some perf hit since we're pulling docs/docvalues for many hits that we then throw away when we merge suggestions across all shards ... and I wonder how much of a perf hit we are talking about even w/ the 2nd fetch phase since the current suggesters pull payloads directly from the FST.

I think what we should do is this:

• for 2.1 we only implement a payloads feature to fetch stuff from docvalues eagerly to have the same functionality as the previous one.
• for master we remove the SuggestReqeust/SuggestResponse/TransportSuggestAction completely and convert _suggest to be sugar for _search without a query and only suggest.
• once that's in we can implement all the second roundtrip as part of the search and things become much simpler.

WDYT? I discussed this briefly with @areek yesterday night already

[mikemccand]

+1 to that plan.

[areek]

I think what we should do is this:

• for 2.1 we only implement a payloads feature to fetch stuff from docvalues eagerly to have the same functionality as the previous one.
• for master we remove the SuggestReqeust/SuggestResponse/TransportSuggestAction completely and convert _suggest to be sugar for _search without a query and only suggest.
• once that's in we can implement all the second roundtrip as part of the search and things become much simpler.

This makes sense. I am working on a PR to support fetching docvalues eagerly for 2.1. I will open up followup issues to do the cleanup/enhancement in master.

[areek]

I opened #13659 which implements docvalues based payload support for 2.1, would appreciate a review :)

but then there's some perf hit since we're pulling docs/docvalues for many hits that we then throw away when we merge suggestions across all shards ... and I wonder how much of a perf hit we are talking about

@mikemccand I plan to benchmark #13659 against the current suggester payload support to try to shed some light on this

[clintongormley]

@areek @s1monw @mikemccand wondering if we should get this in for 5.0?

[areek]

@clintongormley @s1monw @mikemccand I opened #17198, this will simplify adding document fetching to completion

[areek]

closed as functionality has been implemented in #19536