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

`require_field_match` now defaults to `true` #11067

Conversation

@javanna
Copy link
Member

commented May 8, 2015

The default false for require_field_match is a bit odd and confusing for users, given that field names get ignored by default and every field gets highlighted if it contains terms extracted out of the query, regardless of which fields were queried. Changed the default to true, it can always be customized per request.

Closes #10627

The default `false` for `require_field_match` is a bit odd and confusing for users, given that field names get ignored by default and every field gets highlighted if it contains terms extracted out of the query, regardless of which fields were queries. Changed the default to `true`, it can always be changed per request.

Closes #10627
@nik9000

This comment has been minimized.

Copy link
Contributor

commented May 8, 2015

+1

I'm so glad this is built in and doesn't have to happen 4 times....

@johtani

This comment has been minimized.

Copy link
Member

commented May 9, 2015

@javanna I left some comment.

@@ -1914,8 +1879,8 @@ public void testPostingsHighlighter() throws Exception {

logger.info("--> searching on _all, highlighting on field1");
source = searchSource()
.query(termQuery("_all", "test"))
.highlight(highlight().field("field1").preTags("<xxx>").postTags("</xxx>"));
.query(termQuery("field1", "test"))

This comment has been minimized.

Copy link
@johtani

johtani May 9, 2015

Member

Why change the field name to "field1"?

This comment has been minimized.

Copy link
@javanna

javanna May 9, 2015

Author Member

because if we keep _all and highlight field1 there we get no output back. I didn't rely on requireFieldMatch for this one as the postings highlighter is soon not going to support it anymore, see #11077

This comment has been minimized.

Copy link
@johtani

johtani May 9, 2015

Member

I see. Then, I think we should change the log message from searching on _all to searching on field1

This comment has been minimized.

Copy link
@javanna

javanna May 9, 2015

Author Member

good point, will do!

This comment has been minimized.

Copy link
@javanna

javanna May 11, 2015

Author Member

thanks for the review Jun I changed this test to leave querying for _all and set require_field_match to false explicitly. Will change again when #11077, doesn't make much sense to take it into account here.

@@ -1923,8 +1888,8 @@ public void testPostingsHighlighter() throws Exception {

logger.info("--> searching on _all, highlighting on field2");
source = searchSource()
.query(termQuery("_all", "quick"))
.highlight(highlight().field("field2").order("score").preTags("<xxx>").postTags("</xxx>"));
.query(termQuery("field2", "quick"))

This comment has been minimized.

Copy link
@johtani

johtani May 9, 2015

Member

Why change the field name to "field2"?

This comment has been minimized.

Copy link
@javanna

javanna May 9, 2015

Author Member

same as above

@@ -1932,8 +1897,8 @@ public void testPostingsHighlighter() throws Exception {

logger.info("--> searching on _all, highlighting on field2");
source = searchSource()
.query(matchPhraseQuery("_all", "quick brown"))
.highlight(highlight().field("field2").preTags("<xxx>").postTags("</xxx>"));
.query(matchPhraseQuery("field2", "quick brown"))

This comment has been minimized.

Copy link
@johtani

johtani May 9, 2015

Member

Why change the field name to "field2"?

This comment has been minimized.

Copy link
@javanna

javanna May 9, 2015

Author Member

same as above

@@ -1943,7 +1908,7 @@ public void testPostingsHighlighter() throws Exception {
//lets fall back to the standard highlighter then, what people would do to highlight query matches
logger.info("--> searching on _all, highlighting on field2, falling back to the plain highlighter");
source = searchSource()
.query(matchPhraseQuery("_all", "quick brown"))
.query(matchPhraseQuery("field2", "quick brown"))

This comment has been minimized.

Copy link
@johtani

johtani May 9, 2015

Member

Why change the field name to "field2"?

This comment has been minimized.

Copy link
@javanna

javanna May 9, 2015

Author Member

same as above

@@ -1943,7 +1908,7 @@ public void testPostingsHighlighter() throws Exception {
//lets fall back to the standard highlighter then, what people would do to highlight query matches
logger.info("--> searching on _all, highlighting on field2, falling back to the plain highlighter");
source = searchSource()
.query(matchPhraseQuery("_all", "quick brown"))
.query(matchPhraseQuery("field2", "quick brown"))
.highlight(highlight().field("field2").preTags("<xxx>").postTags("</xxx>").highlighterType("highlighter"));

This comment has been minimized.

Copy link
@johtani

johtani May 9, 2015

Member

Should add .requireFieldMatch(false)

This comment has been minimized.

Copy link
@javanna

javanna May 9, 2015

Author Member

same explanation as above :)

@javanna

This comment has been minimized.

Copy link
Member Author

commented May 12, 2015

@clintongormley you ok with this change?

@rmuir

This comment has been minimized.

Copy link
Contributor

commented May 15, 2015

looks good

@clintongormley

This comment has been minimized.

Copy link
Member

commented May 15, 2015

@javanna javanna removed the review label May 15, 2015
@javanna javanna closed this in a843008 May 15, 2015
@clintongormley clintongormley changed the title Highlighting: require_field_match set to true by default `require_field_match` now defaults to `true` Jun 6, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.