Skip to content
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

Fix explanation streaming #7257

Closed
wants to merge 4 commits into from

Conversation

Projects
None yet
3 participants
@brwe
Copy link
Contributor

commented Aug 13, 2014

Complex explanations were always read as Explanations. Depending
on if the response was streamed or not the explanation was
therefore generated by a ComplexExplanation or by a regular
Explanation.

@rjernst

View changes

src/main/java/org/elasticsearch/common/lucene/Lucene.java Outdated
Explanation explanation = new Explanation(value, description);

Explanation explanation;
if (in.getVersion().onOrAfter(org.elasticsearch.Version.V_1_4_0)) {

This comment has been minimized.

Copy link
@rjernst

rjernst Aug 21, 2014

Member

I think you can join the next conditional up to here with && in.readBoolean(), then have just one else condition?

@rjernst

View changes

src/main/java/org/elasticsearch/common/lucene/Lucene.java Outdated
out.writeFloat(explanation.getValue());
out.writeString(explanation.getDescription());
if (out.getVersion().onOrAfter(org.elasticsearch.Version.V_1_4_0)) {
if (explanation instanceof ComplexExplanation) {
out.writeBoolean(true);

This comment has been minimized.

Copy link
@rjernst

rjernst Aug 21, 2014

Member

It's a little odd to see this boolean in the middle of the serialization. I would expect something like this which is a flag for the type to be at the beginning of the data.

e.g. Explanation:
false
value
description

And ComplexExplanation:
true
optional boolean
value
description

I think that could simplify the reading code a bit too.

@rjernst

This comment has been minimized.

Copy link
Member

commented Aug 21, 2014

I left a couple comments. Looks fine overall.

brwe added some commits Aug 13, 2014

explain score: fix explanation streaming
Complex explanations were always read as Explanations. Depending
on if the response was streamed or not the explanation was
therefore generated by a ComplexExplanation or by a regular
Explanation.

@brwe brwe force-pushed the brwe:fix-explanation-streaming branch to 5a82dd9 Aug 21, 2014

@brwe

This comment has been minimized.

Copy link
Contributor Author

commented Aug 21, 2014

Thanks for the comments! Added commits.

@@ -320,7 +320,7 @@ public static ValueAndBoost parseCreateFieldForString(ParseContext context, Stri
if (token == XContentParser.Token.FIELD_NAME) {
currentFieldName = parser.currentName();
} else {
if ("value".equals(currentFieldName) || "_value".equals(currentFieldName)) {
if ("value.j.java".equals(currentFieldName) || "_value".equals(currentFieldName)) {

This comment has been minimized.

Copy link
@rjernst

rjernst Aug 22, 2014

Member

Was this change intentional?

This comment has been minimized.

Copy link
@brwe

brwe Aug 26, 2014

Author Contributor

definitely not! glad you spotted it...

@rjernst

This comment has been minimized.

Copy link
Member

commented Aug 22, 2014

LGTM. One oddity in the diff I left a note about.

@brwe

This comment has been minimized.

Copy link
Contributor Author

commented Aug 26, 2014

Removed the oddity.

@rjernst

This comment has been minimized.

Copy link
Member

commented Aug 26, 2014

+1 to commit.

@brwe brwe added v1.3.3 and removed review labels Aug 27, 2014

brwe added a commit that referenced this pull request Aug 27, 2014

explain score: fix explanation streaming
Complex explanations were always read as Explanations. Depending
on if the response was streamed or not the explanation was
therefore generated by a ComplexExplanation or by a regular
Explanation.

closes #7257

@brwe brwe closed this in a92300c Aug 27, 2014

brwe added a commit that referenced this pull request Sep 8, 2014

explain score: fix explanation streaming
Complex explanations were always read as Explanations. Depending
on if the response was streamed or not the explanation was
therefore generated by a ComplexExplanation or by a regular
Explanation.

closes #7257

@clintongormley clintongormley changed the title explain score: fix explanation streaming Internal: Fix explanation streaming Sep 8, 2014

@clintongormley clintongormley changed the title Internal: Fix explanation streaming Fix explanation streaming Jun 7, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.