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

Add minimum_should_match section to the Query String Query documentation (#34142) #36109

Merged
merged 9 commits into from
Nov 30, 2018
Merged

Add minimum_should_match section to the Query String Query documentation (#34142) #36109

merged 9 commits into from
Nov 30, 2018

Conversation

cbismuth
Copy link
Contributor

This PR adds a section to the Query String Query documentation to explain why the minimum_should_match parameter isn't applied when the final boolean query is a disjunction max over multiple fields.

Copy link
Contributor

@jimczi jimczi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @cbismuth , I left some comments

analyzed entirely. One query is created for each field and finally the field
queries are merged into a single query. When the final query is a boolean query
with a single clause which is the disjunction max over the fields, the
`minimum_should_match` parameter can't be applied.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can replace this by:

The `query_string` splits the query around each operator to create a boolean query for the entire input. You can use `minimum_should_match` to controll how many "should" clauses in the resulting query should match. 

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's easier to read, fixed in d2a3ec6.


that matches documents with at least two of the terms `this`, `that` or `thus`
in the single field `title`.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can maybe start a sub-section for the multi-fields example ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Totally, fixed in 877d7a2.

`((content:this content:that content:thus) | (title:this title:that title:thus))`

that matches documents with the disjunction max over the fields `title` and
`content`. Here the `minimum_should_match` parameter can't be applied.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can mention here that adding explicit operators forces each term to be considered as a separate clause.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, done in 8cacbe4.

`((content:this | title:this) (content:that | title:that) (content:thus | title:thus))~2`

that matches documents with at least two of the three `SHOULD` clauses, each of
them made of the disjunction max over the fields for each term.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can also mention the "type": "cross_fields" option for fields that share the same analyzer ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, added in dedicated sub-section in 1f19505.

And minor formatting inconsistencies also fixed in 85e7ecf.

@jasontedor
Copy link
Member

@elasticmachine test this please

@jasontedor
Copy link
Member

@elasticmachine test this please

3 similar comments
@jasontedor
Copy link
Member

@elasticmachine test this please

@jasontedor
Copy link
Member

@elasticmachine test this please

@jasontedor
Copy link
Member

@elasticmachine test this please

@cbismuth
Copy link
Contributor Author

cbismuth commented Nov 30, 2018

Hi, it seems we have a CI-related issue:

13:58:35 Please install <xsltproc> at /var/lib/jenkins/workspace/elastic+elasticsearch+pull-request+docbldesx/git_home/docs/build_docs.pl line 483.

@jasontedor
Copy link
Member

We're in the process of investigating on our side, thanks.

@cbismuth
Copy link
Contributor Author

Great, thank you @jasontedor 👍

@jasontedor
Copy link
Member

This issue should be addressed now.

@elasticsearch test this please

Copy link
Contributor

@jimczi jimczi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks @cbismuth

@jimczi jimczi merged commit acdf966 into elastic:master Nov 30, 2018
@jimczi jimczi added >docs General docs changes :Search/Search Search-related issues that do not fall into other categories labels Nov 30, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search

@cbismuth
Copy link
Contributor Author

That is great, thanks @jimczi and @jasontedor 👍

@cbismuth cbismuth deleted the 34142_minimum_should_match branch November 30, 2018 19:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>docs General docs changes :Search/Search Search-related issues that do not fall into other categories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants