-
Notifications
You must be signed in to change notification settings - Fork 24.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changed inner_hits to work with the new join field type and #25074
Conversation
357d670
to
2198da9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for rewriting all the tests.
I left one comment. I think we need to be explicit when we build the inner hit query.
continue; | ||
} | ||
|
||
String parentIdFieldName = ParentJoinFieldMapper.getParentIdFieldName(joinFieldMapper.name(), typeName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can remove the leniency and make sure that we build parent
or children
inner hits depending on the context. hasParent
should only build inner hits of parent
documents and hasChild for children
documents.
With this information in JoinFieldInnerHitSubContext
you can just call:
joinFieldMapper.getJoinFieldMapper(typeName, isParentInnerHits);
and use JoinFieldMapper#hasParent
and JoinFieldMapper#hasChild
to build the inner hits query ?
QueryShardContext qsc = context.getQueryShardContext(); | ||
|
||
Query q; | ||
if (parentIdField == null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this work if the document is a child and a parent ? I think it's simpler if we use the context directly.
hasParent
expects a child
and builds parent
inner hits. hasChildren
expects a parent
and builds children
inner hits ?
2198da9
to
0cf2790
Compare
@jimczi I made the join field inner hits logic more explicit. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
at the same time maintaining support for the `_parent` meta field type/ Relates to elastic#20257
8057af0
to
db8aa8e
Compare
My mapping My query to sort by view count throws |
at the same time maintaining support for the
_parent
meta field type.Relates to #20257