Added searching for multiple similar documents #5857
Conversation
| } | ||
| if (exclude != true) { | ||
| builder.field("exclude", exclude); | ||
| } |
There was a problem hiding this comment.
Maybe this should use a Boolean instead (the object wrapper) and only write it if not null.
I know the other integers are not doing it, but I think this one is different since true is a valid value while -1 is an invalid value for the other fields.
| be specified, but not both. The texts are fetched from `fields` and `fields` | ||
| must be specified in this case. | ||
|
|
||
| |`_type` |When using `ids`, this disambiguates which type to fetch the |
There was a problem hiding this comment.
since what we do here is essentially a multi get why don't we just support the multi_get syntax and share the parsing code?
http://www.elasticsearch.org/guide/en/elasticsearch/reference/current/docs-multi-get.html
There was a problem hiding this comment.
It would be nice to support the full multi-get syntax, but note that it would allow users to fetch for documents not found in the index(s)/type(s) specified in the query dsl request. In this case what should be the mlt query? Would it make sense to fetch for apples but ask for bananas? One possibility is to allow these cases though, and clarify the exact usage of this syntax in the documentation.
There was a problem hiding this comment.
yes I absolutely think we should allow for this. Lets say you have a video search. You have two types adult_videos / non_adult_videos and you do a MLT you might not specify a type in the request but those are essentially the same. It's a bit like ducktyping in dynamic languages IMO
There was a problem hiding this comment.
we should document the multi get syntax here maybe just put a reference to mget and add an example?
|
I left some comments but I think it's close... I will remove the review tag, can you put it back once you have a new iteration? |
|
I like it - I think it's ready to go @jpountz do you wanna take another look? |
| if (item.type() != null) { | ||
| builder.field("_type", item.type()); | ||
| } | ||
| builder.endObject(); |
There was a problem hiding this comment.
I think you need to write the field names as well?
There was a problem hiding this comment.
Sure, and I suppose _routing as well, but not _source.
|
I left comments/questions but I agree this looks very nice! |
| } | ||
|
|
||
| @Override | ||
| public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { |
There was a problem hiding this comment.
so what about the other settings you can set on the Item like source filtering? I think we should expose all of them here though.
|
I left one comment other than that LGTM |
| } | ||
| if (this.routing() != null) { | ||
| builder.field("_routing", this.routing()); | ||
| } |
There was a problem hiding this comment.
I am still missing _version, _version_type, fields, and _parent here we should add them!
|
I did another round sorry for being picky but there is still stuff missing... |
| @Override | ||
| public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { | ||
| builder.startObject(); | ||
| builder.field("_index", this.index()); |
There was a problem hiding this comment.
index is required? ti can't be null? I saw taht you change this...?
|
one small comment other than that LGTM |
|
LGTM |
The syntax to specify one or more items is the same as for the Multi GET API. If only one document is specified, the results returned are the same as when using the More Like This API. Relates elastic#4075 Closes elastic#5857
clintongormley
changed the title
clintongormley
changed the title
clintongormley
added
:Search/Search
The documents are specified by a list of ids. The index and type where each document is
fetched from is assumed to be the first specified in the URL.
Relates #4075