OlivierJaquemet opened a new issue, #16441:
URL: https://github.com/apache/lucene/issues/16441

   ### Description
   
   ## Disclaimer
   
   I found this bug in our codebase and manually wrote the unit tests covering 
the various cases. I then used Claude AI  to help identify the root cause and 
draft this report. Finally, I verified both the reported behavior and the 
proposed patch against Lucene's main branch (running all tests) before 
submitting this report and corresponding merge request.
   
   I could not find any policy in the [Contributing to Lucene 
Guide](https://github.com/apache/lucene/blob/main/CONTRIBUTING.md) preventing 
me from going this way. 
   My apologies if this is not authorized.
   
   ## Summary
   
   When `MultiFieldQueryParser` is configured with per-field boosts and the 
default operator is set to `AND`, a single-term query whose text happens to be 
sent through the "multi-term" (whitespace-splitting) code path produces a query 
where **every field clause is forced to `MUST`**, even though the parser is 
expected to keep the per-field alternatives as `SHOULD` inside an optional 
group. Without boosts, the same input produces the expected (unforced) result.
   
   **Correct behavior with "simple" term query (no boost):**
   ```
   MultiFieldQueryParser parser = new MultiFieldQueryParser(new String[] 
{"field1", "field2"}, new StandardAnalyzer());
   parser.setDefaultOperator(QueryParser.Operator.AND);
   assertEquals("field1:hello field2:hello", 
                parser.parse(QueryParser.escape("hello !")).toString()); 
   ```
       
   **Incorrect behavior with boosted query:**
   ```
   MultiFieldQueryParser parser2 = new MultiFieldQueryParser(new String[] 
{"field1", "field2"}, new StandardAnalyzer(), Map.of("field1", 2.0f));
   parser2.setDefaultOperator(QueryParser.Operator.AND);
   assertEquals("field1:hello field2:hello", 
                parser2.parse(QueryParser.escape("hello !")).toString()); // KO 
: +(field1:hello)^2.0 +field2:hello
   ```
   
   # Root cause
   
   `QueryParser.escape("hello !")` escapes the `!`, so the query grammar sees 
two
   adjacent `TERM` tokens (`hello`, `\!`) rather than a `NOT` operator. With
   `splitOnWhitespace=false` (the default), this hits the `MultiTerm` 
production in
   `QueryParser.jj`, which concatenates the tokens into a single string and 
calls:
   
   ```java
   firstQuery = getFieldQuery(field, discardEscapeChar(text.image), false);
   addMultiTermClauses(clauses, firstQuery);
   ```
   
   `StandardAnalyzer` reduces `"hello !"` to the single token `hello` (the `!` 
is
   punctuation and is dropped), so `MultiFieldQueryParser.getFieldQuery(null, 
"hello !", false)`
   returns a flat `BooleanQuery` combining one `SHOULD` clause per field:
   
   - **Without boost:** `BooleanQuery(SHOULD(TermQuery field1:hello), 
SHOULD(TermQuery field2:hello))`
   - **With boost:** `BooleanQuery(SHOULD(BoostQuery(TermQuery field1:hello, 
2.0)), SHOULD(TermQuery field2:hello))`
   
   The bug is in `QueryParserBase#addMultiTermClauses`:
   
   ```java
   protected void addMultiTermClauses(List<BooleanClause> clauses, Query q) {
     if (q == null) return;
     boolean allNestedTermQueries = false;
     if (q instanceof BooleanQuery) {
       allNestedTermQueries = true;
       for (BooleanClause clause : ((BooleanQuery) q).clauses()) {
         if (!(clause.getQuery() instanceof TermQuery)) {
           allNestedTermQueries = false;
           break;
         }
       }
     }
     if (allNestedTermQueries) {
       clauses.addAll(((BooleanQuery) q).clauses());          // keeps original 
Occur
     } else {
       BooleanClause.Occur occur = operator == OR_OPERATOR ? SHOULD : MUST;
       if (q instanceof BooleanQuery) {
         for (BooleanClause clause : ((BooleanQuery) q).clauses()) {
           clauses.add(newBooleanClause(clause.getQuery(), occur));  // 
overwrites Occur
         }
       } else {
         clauses.add(newBooleanClause(q, occur));
       }
     }
   }
   ```
   
   The `allNestedTermQueries` heuristic is meant to detect "this `BooleanQuery` 
is just
   the result of the analyzer splitting one field's text into several terms", 
in which
   case the original per-clause `Occur` is preserved as-is. It uses a strict
   `clause.getQuery() instanceof TermQuery` check.
   
   - Without a boost, every clause is a bare `TermQuery` → the check passes → 
the
     original `SHOULD` occurs are copied through untouched (this happens to 
look correct
     here, though it also means the default operator is silently ignored for 
this path
     in general — a separate, pre-existing quirk).
   - With a boost, the `field1` clause becomes a `BoostQuery` wrapping a 
`TermQuery`, so
     `instanceof TermQuery` is `false` → the check fails → the code falls into 
the `else`
     branch. Because `q` is still a `BooleanQuery`, it is unwrapped and **every 
nested
     clause is re-added with a single, forced `Occur`** (`MUST` here, since the 
default
     operator is `AND`), discarding the `SHOULD` semantics that expressed "any 
of these
     fields matching is enough". The result: two independently required clauses 
instead
     of an optional group with boosts.
   
   In short: `addMultiTermClauses` cannot distinguish "a nested `BooleanQuery` 
that
   happens to contain something more than a bare `TermQuery` (e.g. a boosted 
term)" from
   "a nested `BooleanQuery` that legitimately needs its per-clause structure 
discarded
   and replaced by a single shared `Occur`" (e.g. synonym expansion). The 
multi-field
   disjunction built by `MultiFieldQueryParser` falls victim to the latter path 
purely
   because one of its clauses is boosted.
   
   ## Impact
   
   Any use of `MultiFieldQueryParser` with per-field boosts, `AND` as the 
default operator, and query text that is analyzed down to a single term while 
going through the whitespace-splitting (`MultiTerm`) grammar path will silently 
produce an over-restrictive query (`MUST` on every field) instead of the 
intended optional/boosted disjunction. This can happen with escaped punctuation 
(as above), but also with any input where the analyzer collapses multiple raw 
tokens into one term (stop-word removal, synonym filtering that drops a token, 
etc.), so it is not limited to this specific escaping scenario.
   
   ### Version and environment details
   
   Affected version: main and 10.4.0
   Component: lucene-queryparser (classic package)
   Affected classes: 
   - org.apache.lucene.queryparser.classic.MultiFieldQueryParser, 
   - org.apache.lucene.queryparser.classic.QueryParserBase 
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to