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
Reset the ShardTargetType
after serializing inner hits.
#12261
Reset the ShardTargetType
after serializing inner hits.
#12261
Conversation
@@ -808,7 +810,9 @@ public void writeTo(StreamOutput out, InternalSearchHits.StreamContext context) | |||
out.writeVInt(innerHits.size()); | |||
for (Map.Entry<String, InternalSearchHits> entry : innerHits.entrySet()) { | |||
out.writeString(entry.getKey()); | |||
entry.getValue().writeTo(out, InternalSearchHits.streamContext().streamShardTarget(InternalSearchHits.StreamContext.ShardTargetType.NO_STREAM)); | |||
ShardTargetType previous = InternalSearchHits.streamContext().streamShardTarget(); |
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.
Is there a better name for this variable? I'm not really sure what previous is here? Could this not just be shardTarget
?
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.
it is the ShardTargetType
of the current hit and we remember that so that it doesn't get lost when the inner_hits are serialised. Because for the inner hits we don't need shard target, because that is always the same as the top level search hit (shard and index that is).
I'm ok with renaming it to shardTarget
.
Left a minor comment but otherwise it LGTM |
9847926
to
c955970
Compare
This fixes a bug where only the first top level search hit has a shard target and any subsequent search hits don't.
c955970
to
942d040
Compare
This fixes a bug where only the first top level search hit has a shard target and any subsequent search hits don't.
This bug was reported in the forum: https://discuss.elastic.co/t/elasticsearch-innerhits-java-api-causing-npe/25532