Conversation
benashford
left a comment
benashford
left a comment
There was a problem hiding this comment.
Thanks for this, tidies up a few rough-edges. I have a few questions in-line that would help me figure out where this fits into the big-picture.
| pub fields: Option<Value>, | ||
| pub highlight: Option<HighlightResult> | ||
| pub highlight: Option<HighlightResult>, | ||
| pub inner_hits: Option<HashMap<String, Value>> |
There was a problem hiding this comment.
Is the Value needed due to the potential of more-than-one different type coming back in the results? In which case its probably unavoidable.
There was a problem hiding this comment.
Indeed - You can have a look at this as reference: https://www.elastic.co/guide/en/elasticsearch/reference/current/search-request-inner-hits.html
| impl<T> SearchHitsHitsResult<T> | ||
| where T: DeserializeOwned { | ||
|
|
||
| pub fn inner_hit<S>(&self, key: &str) -> Option<SearchHitsResult<S>> where S: DeserializeOwned { |
There was a problem hiding this comment.
This should perhaps be a Result<Option<SearchHitsResult<S>>, EsError> so we can tell apart the difference between a missing value and a malformed value?
serde_json errors will auto-promote to EsErrors so replacing the .ok() on line 791 with a ? would do the trick with the larger return type.
There was a problem hiding this comment.
I couldn't use ? as I found the need to specify to rustc what kind of error are we dealing with - aabd707
| /// Has Child query | ||
| #[derive(Debug, Default, Serialize)] | ||
| pub struct HasChildQuery { | ||
| #[serde(rename="type")] |
There was a problem hiding this comment.
Thanks for this :) it must have been missing for quite a while
| max_children: Option<u64> | ||
| max_children: Option<u64>, | ||
| #[serde(skip_serializing_if="ShouldSkip::should_skip")] | ||
| inner_hits: Option<Value> |
There was a problem hiding this comment.
Preferably this would be a concrete type rather than Value, but the ElasticSearch documentation is... less than comprehensive on this matter. This seems to be the best reference: https://www.elastic.co/guide/en/elasticsearch/reference/1.5/search-request-inner-hits.html unless there's a better one somewhere?
There was a problem hiding this comment.
The link I sent you in the other comment is similar to this one - Although I agree that a concrete type would be way preferable, I find not using a Value quite a hard way here 😞
There was a problem hiding this comment.
Because of Inner Hits potentially returning more than one query? Yes, that's true.
We could have a hybrid option as there is some predictable structure. E.g. the "total" and "_id" fields, it's only really the "_source" field that needs to be a Value?
| max_children: Option<u64> | ||
| max_children: Option<u64>, | ||
| #[serde(skip_serializing_if="ShouldSkip::should_skip")] | ||
| inner_hits: Option<Value> |
There was a problem hiding this comment.
Could we also add the same to the HasParentQuery as the inner_hits field should in-theory work there too?
There was a problem hiding this comment.
Does it actually? https://www.elastic.co/guide/en/elasticsearch/reference/current/query-dsl-has-parent-query.html I can't find it here but in case sure, I can add inner_hits in there too. I will wait for a your response before to commit it just to double check I understood correctly what you mean :)
There was a problem hiding this comment.
I don't think I've ever needed it on a has_parent query, and it's not mentioned in the documentation for it either. But it is mentioned here: "Inner hits can be used by defining a inner_hits definition on a nested, has_child or has_parent query and filter" it might be something that's missing from the documentation elsewhere.
2 participants
Hello!
I made this changes quite a while ago but then I forgot to open a PR for them.
The changes consist in:
_parent)