Refactored inner hits parsing and intoduced InnerHitBuilder by martijnvg · Pull Request #17291 · elastic/elasticsearch

martijnvg · 2016-03-23T16:48:47Z

Both top level and inline inner hits are now covered by InnerHitBuilder.
Although there are differences between top level and inline inner hits,
they now make use of the same builder logic.

The parsing of top level inner hits slightly changed to be more readable.
Before the nested path or parent/child type had to be specified as encapsuting
json object, now these settings are simple fields. Before this was required
to allow streaming parsing of inner hits without missing contextual information.

Once some issues are fixed with inline inner hits (around multi level hierachy of inner hits),
top level inner hits will be deprecated and removed in the next major version.

Also I think some SearchParseElement implementations can be removed, but I haven't done that yet.

cbuescher · 2016-03-24T13:51:32Z

core/src/main/java/org/elasticsearch/index/query/support/InnerHitsBuilder.java

Maybe this can be private.

Could you also check for null here?

cbuescher · 2016-03-24T16:51:24Z

@martijnvg this looks great, even though I didn't compare it completely to the way it was before the refactoring, I have the feeling this change simplifies a lot. Left a few minor comments, only thing that was causing some trouble with the tests on my side was the size shadowing issue in SearchSourceBuilder#readFrom(), but will run the whole tests again now with a local fix. Other than that, maybe it would be good to also have a basic test for InnerHitsBuilder, although that one doesn't have much logic of its own.
Its also a quiet a big PR, maybe somebody else involved in the search refactoring should take a second look after this round. I'll ask around.

martijnvg · 2016-03-24T23:08:38Z

@cbuescher Thanks for looking at this. I updated the PR based on your comment and fixed the size shadowing issue.

dimfeld · 2016-03-25T01:59:22Z

With top-level inner_hits going away, is there a recommended way to replace the functionality? My usual case here involves using a has_child in the query to look for parents with a particular value in a child, and then a top-level inner_hits to return the top few child objects of the same type, ranked by some value using a function_score.

I'm thinking that something like this will work; is there a better way?

{  query:  {  bool: {
   must: // main query with "real" has_child filter and whatever else goes here
   should: [
      { has_child: { type: childType, query: {function_score query}, inner_hits: {} } 
    ]
} } }

martijnvg · 2016-03-25T08:24:05Z

@dimfeld Yes, that should work too. Also I'm thinking of also allowing the query option inside inline inner hits (which would then overwrite the actual inner query), so that you can do something like this:

{  query:  {  bool: {
   must: [
      { has_child: { type: childType, query: {real query}, inner_hits: {query: {function_score query}} } 
    ]
} } }

This would be better, because it would be more efficient as only one has_child query would need to be specified.

The goal with replacing inner hits is that same things can be done with inline inner hits as top level inner hits.

dimfeld · 2016-03-25T14:22:35Z

Sounds good. Thanks @martijnvg!

cbuescher · 2016-03-29T12:39:09Z

@martijnvg thanks, changes look great and all works for me now, but maybe @colings86, @javanna or @s1monw want to take a second look at this before it gets in.

colings86 · 2016-03-30T10:00:51Z

core/src/main/java/org/elasticsearch/index/query/support/InnerHitBuilder.java

I wonder if we should add validation check to these setter methods like we do in the query builders? So for this one check that the value is >= 0 and for setName() make sure the name passed in is not null.

colings86 · 2016-03-30T10:07:33Z

@martijnvg I left a few minor comments but otherwise it LGTM

cbuescher · 2016-03-30T10:27:11Z

core/src/main/java/org/elasticsearch/search/SearchService.java

I'm curious about why this reset (and the one in the finally block) are done. I saw this pattern in other places, just trying to figure out why this is done. In case its important, maybe worth a comment?

This is the same logic in QueryShardContext#toQuery() where we reset also twice. Personally I think a reset at the end is sufficient, but I kept the logic consistent with this method.

The reason we need to invoke reset at all here is because inner hits modifies the QueryShardContext#nestedScope, so we need to reset at the end.

Thanks for the explanation, I've been looking at resets in shard contexts recently and this helps my understanding.

cbuescher · 2016-03-30T10:43:37Z

@martijnvg I did another quick round, left a few minor suggestions and one question for my own understanding.

martijnvg added review v5.0.0-alpha1 >breaking labels Mar 23, 2016

cbuescher reviewed Mar 24, 2016
View reviewed changes

martijnvg force-pushed the inner_hits_search_refactoring branch from d9081b5 to 1b36a9e Compare March 24, 2016 23:07

martijnvg force-pushed the inner_hits_search_refactoring branch from 1b36a9e to d453f70 Compare March 25, 2016 09:01

colings86 reviewed Mar 30, 2016
View reviewed changes

cbuescher reviewed Mar 30, 2016
View reviewed changes

Conversation

martijnvg commented Mar 23, 2016

Uh oh!

cbuescher Mar 24, 2016

Choose a reason for hiding this comment

Uh oh!

cbuescher Mar 30, 2016

Choose a reason for hiding this comment

Uh oh!

cbuescher commented Mar 24, 2016

Uh oh!

martijnvg commented Mar 24, 2016

Uh oh!

dimfeld commented Mar 25, 2016

Uh oh!

martijnvg commented Mar 25, 2016

Uh oh!

dimfeld commented Mar 25, 2016

Uh oh!

cbuescher commented Mar 29, 2016

Uh oh!

colings86 Mar 30, 2016

Choose a reason for hiding this comment

Uh oh!

colings86 commented Mar 30, 2016

Uh oh!

cbuescher Mar 30, 2016

Choose a reason for hiding this comment

Uh oh!

martijnvg Mar 30, 2016

Choose a reason for hiding this comment

Uh oh!

cbuescher Mar 30, 2016

Choose a reason for hiding this comment

Uh oh!

cbuescher commented Mar 30, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants