BooleanQuery.rewrite does not work properly for minNumberShouldMatch

• Key: LUCENE-1134
• Type: Bug
• Status: Resolved
• Resolution: Fixed
• Priority: Minor
• Assignee: Unassigned
• Reporter: Shai Erera
• Component/s: Search
• Affects Version/s: 2.4

Description

BooleanQuery.rewrite does not respect minNumberShouldMatch if the number of clauses is 1. This causes inconsistencies for the queries "+def" and "+abc +def", while setting the minNumShouldMatch to '1' for both.
For the first query, results are returned although there are no SHOULD clauses in the query.
For the second query no results are returned.
The reason lies in the optimization BooleanQuery.rewrite has for one clauses queries.
Patch included - optimize the query for a single clause only if the minNumShouldMatch <= 0.

Comments

[Shai Erera]

The fix is very trivial. Which tests should I run to ensure this doesn't break anything?

[Paul Elschot]

I'd start with these tests:

ant -Dtestcase='Test*Bool*' test-core

You might also consider adding a test case in one of those tests that shows the bug without the patch applied.

[Hoss Man]

At first glance, the patch seems right .. but an optimization can probably be made ... if there is only once clause, and minNrShouldMatch is greater then 1, the query can rewrite itself to a "no-op" type query that doesn't match any docs.

Actually: BooleanQuery.rewrite can do something like that anytime minNrShouldMath is greater then the number of optional clauses.

[Paul Elschot]

There is no real need for this optimization because BooleanScorer2 already uses a NonMatchingScorer for this case.

[Hoss Man]

Ah... good call.

(Hmmm.... it stil might be worth doing just to make it clear to clients earlier rather then later that they won't match anything. ie:If you inspect a rewritten FuzzyQuery you get to see right away how many clauses it has ... why not let people inspecting a rewritten BooleanQuery in cases like this see right away that their query is a No-Op ?)

[Paul Elschot]

A FuzzyQuery rewrites to a BooleanQuery with SHOULD clauses only, so a check and cast to BooleanQuery and then a clauses().size() == 0 can check for a No-Op after a rewrite().

[Hoss Man]

I think you're misunderstanding me Paul: i was just trying to make an analogy – even if the Scorer optimizes away a BooleanQuery that can never match anything into a NonMatchingScorer, there are other advantages to having rewrite detect these situations (when it can) ... inspection of the rewritten queries being an example.

[Shai Erera]

Hi

W.r.t. testing, I found TestBooleanMinShouldMatch. It had testNoOptionalButMin which added two MUST clauses. I added testNoOptionalButMin2 that adds only a single clause. It fails without the patch and passes with the patch. Other than that the tests in this class pass.
I updated the patch to include the additional test.

W.r.t. the optimization that was discussed: I don't understand how it can be done. BooleanQuery has a clauses list. It is not aware of any SHOULD, MUST or MUST_NOT clauses in its rewrite() method. So how exactly this optimization can be done? The only class that seems to be aware of the type of clauses is BooleanScorer2 in its add() method.

Fix + Test

[Michael Busch]

Committed. Thanks, Shai!