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 explain output for dfs query #19972

Merged
merged 1 commit into from Aug 12, 2016

Conversation

Projects
None yet
3 participants
@jimczi
Copy link
Member

commented Aug 12, 2016

ContextIndexSearcher#explain ignores the dfs data to create the normalized weight.
This change fixes this discrepancy by using the dfs data to create the normalized weight when needed.

Fixes #15369

@javanna

This comment has been minimized.

Copy link
Member

commented Aug 12, 2016

I gues this fix is correct, but can you please write a test for it? ;)

@jimczi

This comment has been minimized.

Copy link
Member Author

commented Aug 12, 2016

Thanks @javanna.
I pushed a small test to validate that the fix is correct, could you please review it ?

@javanna

View changes

core/src/test/java/org/elasticsearch/search/basic/TransportTwoNodesSearchIT.java Outdated
assertThat(hit.explanation().getDetails()[0].getDetails().length, equalTo(2));
assertThat(hit.explanation().getDetails()[0].getDetails()[0].getDescription(),
endsWith("idf(docFreq=100, docCount=100)"));
assertThat(hit.explanation(), notNullValue());

This comment has been minimized.

Copy link
@javanna

javanna Aug 12, 2016

Member

this last line is redundant I think, we already check the same above

@javanna

View changes

core/src/test/java/org/elasticsearch/search/basic/TransportTwoNodesSearchIT.java Outdated
assertThat(hit.explanation().getDetails().length, equalTo(1));
assertThat(hit.explanation().getDetails()[0].getDetails().length, equalTo(2));
assertThat(hit.explanation().getDetails()[0].getDetails()[0].getDescription(),
endsWith("idf(docFreq=100, docCount=100)"));
assertThat(hit.explanation(), notNullValue());

This comment has been minimized.

Copy link
@javanna

javanna Aug 12, 2016

Member

should this be moved up?

@javanna

View changes

core/src/test/java/org/elasticsearch/search/basic/TransportTwoNodesSearchIT.java Outdated
assertThat(hit.explanation().getDetails().length, equalTo(1));
assertThat(hit.explanation().getDetails()[0].getDetails().length, equalTo(2));
assertThat(hit.explanation().getDetails()[0].getDetails()[0].getDescription(),
endsWith("idf(docFreq=100, docCount=100)"));
assertThat(hit.explanation(), notNullValue());

This comment has been minimized.

Copy link
@javanna

javanna Aug 12, 2016

Member

should this be moved up?

@javanna

View changes

core/src/test/java/org/elasticsearch/search/basic/TransportTwoNodesSearchIT.java Outdated
assertThat(hit.explanation().getDetails()[0].getDetails().length, equalTo(2));
assertThat(hit.explanation().getDetails()[0].getDetails()[0].getDescription(),
endsWith("idf(docFreq=100, docCount=100)"));
assertThat(hit.explanation(), notNullValue());
// assertThat("id[" + hit.id() + "]", hit.id(), equalTo(Integer.toString(100 - i - 1)));
assertThat("make sure we don't have duplicates", expectedIds.remove(hit.id()), notNullValue());
}

This comment has been minimized.

Copy link
@javanna

javanna Aug 12, 2016

Member

should we add the assertions to other methods in this class that use dfs?

This comment has been minimized.

Copy link
@javanna

javanna Aug 12, 2016

Member

ignore this, sorry for the noise :)

This comment has been minimized.

Copy link
@jimczi

jimczi Aug 12, 2016

Author Member

;)

@javanna

This comment has been minimized.

Copy link
Member

commented Aug 12, 2016

LGTM, but I am not too familiar with dfs so I would prefer @jpountz to have a quick second look.

@jpountz

This comment has been minimized.

Copy link
Contributor

commented Aug 12, 2016

LGTM!

Fix explain output for dfs query
ContextIndexSearcher#explain ignores the dfs data to create the normalized weight.
This change fixes this discrepancy by using the dfs data to create the normalized weight when needed.

@jimczi jimczi merged commit c9722c4 into elastic:master Aug 12, 2016

1 of 2 checks passed

elasticsearch-ci Build started sha1 is merged.
Details
CLA Commit author is a member of Elasticsearch
Details

@jimczi jimczi deleted the jimczi:dfs_explain branch Aug 12, 2016

@jimczi

This comment has been minimized.

Copy link
Member Author

commented Aug 12, 2016

Thansk @javanna and @jpountz !

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.