Skip to content

Conversation

@martijnvg
Copy link
Member

  • Inner hits are now only provided and prepared in the constructors of the nested, has_child and has_parent queries. This will make fixing has_child and inner_hits for grandchild hit doesn't work #11118 easier and then allow us to drop the top level inner hit syntax.
  • Also made score_mode a required constructor parameter. (fromXContent(...) method maintain their defaults)
  • Moved has_child's min_child/max_children validation from doToQuery(...) to a setter. (so we can fail sooner on the coordinating node)

@martijnvg martijnvg force-pushed the cleanup_query_builder_for_inner_hits branch 2 times, most recently from f88e18b to 698ef63 Compare April 14, 2016 07:24
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

QueryBuilders should always set the defaults since they can be constructed without ever passing through the parser (Java API)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • I think score_mode should be a constructor parameter, as it is important to always think about that.
  • I'll move inner hits to a setter.
  • I'll bring back the defaults for min and max kids.

@colings86
Copy link
Contributor

Apart from my comments about query builder default values this looks pretty good. Am I right in saying that these QueryBuilders are effectively immutable now? (i.e. they have no setters?). If so, I think #17748 will change that as ignore_unmapped is not mandatory. I am also wondering why you needed to move score and InnerHitsBuilder to the constructor?Is there a reason they cannot be set after construction? I only ask as it is nice if only the mandatory parameters are specified in the constructor so the user doesn't have to worry about what the sensible defaults are.

@martijnvg
Copy link
Member Author

@colings86 Only score_mode will additionally be made a mandatory parameter. The other params remain optional.

@colings86
Copy link
Contributor

LGTM

@martijnvg martijnvg force-pushed the cleanup_query_builder_for_inner_hits branch 4 times, most recently from ce4696b to 8bf7117 Compare April 14, 2016 12:42
* Inner hits can now only be provided and prepared via setter in the nested, has_child and has_parent query.
* Also made `score_mode` a required constructor parameter.
* Moved has_child's min_child/max_children validation from doToQuery(...) to a setter.
@martijnvg martijnvg force-pushed the cleanup_query_builder_for_inner_hits branch from 8bf7117 to 2928fd6 Compare April 14, 2016 12:43
@martijnvg martijnvg merged commit 2928fd6 into elastic:master Apr 14, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants