Fix query analyzer logic for mixed conjunctions of terms and ranges #49803
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
romseygeek
added
>bug
:Search Relevance/Percolator
|
Pinging @elastic/es-search (:Search/Percolator) |
jpountz
reviewed
Dec 3, 2019
| .build(); | ||
|
|
||
| Result r = analyze(disj, Version.CURRENT); | ||
| assertThat(r.minimumShouldMatch, equalTo(1)); |
There was a problem hiding this comment.
can you count the number of extractions and check verified for all queries?
| .build(); | ||
|
|
||
| result = analyze(q2, Version.CURRENT); | ||
| assertThat(result.minimumShouldMatch, equalTo(2)); |
There was a problem hiding this comment.
let's maybe also have a test for the case that there are multiple range queries on the same field, or multiple range queries on different fields?
|
Thanks for the review @jpountz . I added more tests, and discovered that the logic was still not quite correct - tricky, as you say... We now only check that range fields from a particular set of extractions have not been seen in other results. Multiple identical range fields within a single Result extraction set will have already been dealt with, so don't require any adjustment to the msm. |
jpountz
approved these changes
Dec 4, 2019
| @@ -286,6 +284,9 @@ private static Result handleConjunction(List<Result> conjunctionsWithUnknowns) { | |||
| verified = false; | |||
| } | |||
| } | |||
| // add range fields from this Result to the seenRangeFields set so that minimumShouldMatch is correctly | |||
| // calculated for subsequent Results | |||
| result.extractions.stream().filter(e -> e.range != null).forEach(e -> seenRangeFields.add(e.range.fieldName)); | |||
There was a problem hiding this comment.
nit: I generally have a preference for using method refs when possible and using new lines when chaining, e.g.
Suggested change
| result.extractions.stream().filter(e -> e.range != null).forEach(e -> seenRangeFields.add(e.range.fieldName)); | |
| result.extractions.stream() | |
| .filter(Objects::nonNull) | |
| .map(e -> e.range) | |
| .forEach(seenRangeFields::add); |
romseygeek
added a commit
that referenced
this pull request
Dec 10, 2019
…49803) When the query analyzer examines a conjunction containing both terms and ranges, it should only include ranges in the minimum_should_match calculation if there are no other range queries on that same field within the conjunction. This is because we cannot build a selection query over disjoint ranges on the same field, and it is not easy to check if two range queries have an overlap. The current logic to calculate this just sets minimum_should_match to 1 or 0, dependent on whether or not the current range is over a field that has already been seen. However, this can be incorrect in the case that there are terms in the same match group which adjust the minimum_should_match downwards. Instead, the logic should be changed to match the terms extraction, whereby we adjust minimum_should_match downwards if we have already seen a range field. Fixes #49684
SivagurunathanV
pushed a commit
to SivagurunathanV/elasticsearch
that referenced
this pull request
Jan 23, 2020
…lastic#49803) When the query analyzer examines a conjunction containing both terms and ranges, it should only include ranges in the minimum_should_match calculation if there are no other range queries on that same field within the conjunction. This is because we cannot build a selection query over disjoint ranges on the same field, and it is not easy to check if two range queries have an overlap. The current logic to calculate this just sets minimum_should_match to 1 or 0, dependent on whether or not the current range is over a field that has already been seen. However, this can be incorrect in the case that there are terms in the same match group which adjust the minimum_should_match downwards. Instead, the logic should be changed to match the terms extraction, whereby we adjust minimum_should_match downwards if we have already seen a range field. Fixes elastic#49684
This was referenced Feb 3, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When the query analyzer examines a conjunction containing both terms and ranges,
it should only include ranges in the
minimum_should_matchcalculation if there are noother range queries on that same field within the conjunction. This is because we cannot
build a selection query over disjoint ranges on the same field, and it is not easy to check
if two range queries have an overlap.
The current logic to calculate this just sets
minimum_should_matchto1or0, dependenton whether or not the current range is over a field that has already been seen. However, this
can be incorrect in the case that there are terms in the same match group which adjust the
minimum_should_matchdownwards. Instead, the logic should be changed to match theterms extraction, whereby we adjust
minimum_should_matchdownwards if we have alreadyseen a range field.
Fixes #49684