jpountz commented on code in PR #12072:
URL: https://github.com/apache/lucene/pull/12072#discussion_r1067223779
##########
lucene/core/src/java/org/apache/lucene/search/BooleanQuery.java:
##########
@@ -203,9 +203,17 @@ BooleanQuery rewriteNoScoring(IndexSearcher indexSearcher)
throws IOException {
for (BooleanClause clause : clauses) {
Query query = clause.getQuery();
- Query rewritten = new ConstantScoreQuery(query).rewrite(indexSearcher);
- if (rewritten instanceof ConstantScoreQuery) {
- rewritten = ((ConstantScoreQuery) rewritten).getQuery();
+ Query rewritten;
+ // If the clause query is a boolean query, we shouldn't call
ConstantScoreQuery#rewrite as
+ // this causes
+ // exponential growth of runtime.
+ if (query instanceof BooleanQuery booleanQuery) {
+ rewritten = booleanQuery.rewriteNoScoring(indexSearcher);
+ } else {
+ rewritten = new ConstantScoreQuery(query).rewrite(indexSearcher);
+ if (rewritten instanceof ConstantScoreQuery constantScoreQuery) {
+ rewritten = constantScoreQuery.getQuery();
+ }
Review Comment:
I'm contemplating replacing your fix with this one which is similar:
```patch
diff --git a/lucene/core/src/java/org/apache/lucene/search/BooleanQuery.java
b/lucene/core/src/java/org/apache/lucene/search/BooleanQuery.java
index 0354280eb01..be323bf0e4c 100644
--- a/lucene/core/src/java/org/apache/lucene/search/BooleanQuery.java
+++ b/lucene/core/src/java/org/apache/lucene/search/BooleanQuery.java
@@ -203,7 +203,13 @@ public class BooleanQuery extends Query implements
Iterable<BooleanClause> {
for (BooleanClause clause : clauses) {
Query query = clause.getQuery();
- Query rewritten = new
ConstantScoreQuery(query).rewrite(indexSearcher);
+ // NOTE: rewritingNoScoring() should not call rewrite(), otherwise
this
+ // method could run in exponential time with the depth of the query as
+ // every new level would rewrite 2x more than its parent level.
+ Query rewritten = query;
+ if (rewritten instanceof BoostQuery) {
+ rewritten = ((BoostQuery) query).getQuery();
+ }
if (rewritten instanceof ConstantScoreQuery) {
rewritten = ((ConstantScoreQuery) rewritten).getQuery();
}
```
I think it would be a bit more robust, as it would keep working if there are
`ConstantScoreQuery` wrappers between the inner levels of `BooleanQuery`s.
##########
lucene/core/src/test/org/apache/lucene/search/TestBooleanRewrites.java:
##########
@@ -322,6 +323,45 @@ public void testMatchAllMustNot() throws IOException {
assertEquals(new MatchNoDocsQuery(), searcher.rewrite(bq2));
}
+ public void testDeeplyNestedBooleanRewrite() throws IOException {
+ IndexSearcher searcher = newSearcher(new MultiReader());
+
+ Directory dir = newDirectory();
+ (new RandomIndexWriter(random(), dir)).close();
+ IndexReader r = DirectoryReader.open(dir);
+ Function<Integer, TermQuery> termQueryFunction =
+ (i) -> new TermQuery(new Term("layer[" + i + "]", "foo"));
+ int depth = TestUtil.nextInt(random(), 10, 30);
+ TermQuery tq = termQueryFunction.apply(depth);
+ Query expectedQuery = new BooleanQuery.Builder().add(tq,
Occur.FILTER).build();
+ Query deepBuilder = new BooleanQuery.Builder().add(tq, Occur.MUST).build();
+ for (int i = depth; i > 0; i--) {
+ tq = termQueryFunction.apply(i);
+ // Do this to accurately set setMinimumNumberShouldMatch to the number
of should clauses.
+ // This makes setting expectation for rewrite much easier.
+ boolean useShoulds = random().nextBoolean();
+ BooleanQuery.Builder bq =
+ new BooleanQuery.Builder()
+ .setMinimumNumberShouldMatch(useShoulds ? 2 : 0)
+ .add(tq, useShoulds ? Occur.SHOULD : Occur.MUST)
+ .add(deepBuilder, useShoulds ? Occur.SHOULD : Occur.MUST);
+ deepBuilder = bq.build();
+ BooleanQuery.Builder expectedBq = new BooleanQuery.Builder().add(tq,
Occur.FILTER);
+ if (i == depth - 1) {
+ expectedBq.add(termQueryFunction.apply(depth), Occur.FILTER);
+ } else {
+ expectedBq.add(expectedQuery, Occur.FILTER);
+ }
+ expectedQuery = expectedBq.build();
+ }
+ BooleanQuery bq = new BooleanQuery.Builder().add(deepBuilder,
Occur.FILTER).build();
+ expectedQuery = new BoostQuery(new ConstantScoreQuery(expectedQuery),
0.0f);
+ Query rewritten = searcher.rewrite(bq);
+ r.close();
+ dir.close();
+ assertEquals(expectedQuery, rewritten);
Review Comment:
I wonder if we could improve the test by e.g. having a special query
implementation that would count the number of times that rewrite() gets called
on the innermost query of this tree of queries, and assert that rewrite() has
only been called once.
--
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]
