Fix exponential runtime for Boolean#rewrite #12072
Merged
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
benwtrent
commented
Jan 10, 2023
lucene/core/src/java/org/apache/lucene/search/BooleanQuery.java
Outdated
Show resolved
Hide resolved
|
@jpountz You probably want to review this one as it relates to your original optimizations. |
javanna
reviewed
Jan 11, 2023
There was a problem hiding this comment.
thanks @benwtrent the fix looks good to me, I left a couple of comments.
lucene/core/src/java/org/apache/lucene/search/BooleanQuery.java
Outdated
Show resolved
Hide resolved
| // this causes | ||
| // exponential growth of runtime. | ||
| if (query instanceof BooleanQuery booleanQuery) { | ||
| rewritten = booleanQuery.rewriteNoScoring(indexSearcher); |
There was a problem hiding this comment.
I would make the same change to the main rewrite method. That will reduce further the amount of time we spend on needlessly rewriting boolean queries.
lucene/core/src/test/org/apache/lucene/search/TestBooleanRewrites.java
Outdated
Show resolved
Hide resolved
lucene/core/src/test/org/apache/lucene/search/TestBooleanRewrites.java
Outdated
Show resolved
Hide resolved
|
I did some additional testing to understand the impact of the regression and in light of that I view this as a bug rather than a performance regression, because we end up performing many needless rewrite steps that waste resources and end up making rewrite take tens of seconds depending on the depth of a boolean query. I added some logging to the rewrite methods involved to more clearly show the issue. The following is the output from a boolean query with depth 3. Output before the fix (87 lines): Output after the fix (52 lines): Output after shortcutting the main rewrite method to rewriteNoScoring instead of wrapping a boolean query with constant score query (39 lines): Obviously the effect of this grows further as the depth of the query increases. |
jpountz
reviewed
Jan 11, 2023
| rewritten = new ConstantScoreQuery(query).rewrite(indexSearcher); | ||
| if (rewritten instanceof ConstantScoreQuery constantScoreQuery) { | ||
| rewritten = constantScoreQuery.getQuery(); | ||
| } |
There was a problem hiding this comment.
I'm contemplating replacing your fix with this one which is similar:
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 BooleanQuerys.
benwtrent
requested review from
jpountz and
javanna
and removed request for
jpountz
javanna
reviewed
Jan 11, 2023
lucene/core/src/java/org/apache/lucene/search/BooleanQuery.java
Outdated
Show resolved
Hide resolved
benwtrent
requested review from
javanna and
jpountz
and removed request for
javanna
jpountz
approved these changes
Jan 12, 2023
|
@benwtrent could you add a changelog entry too? |
|
Thanks @benwtrent ! |
javanna
pushed a commit
that referenced
this pull request
Jan 12, 2023
When #672 was introduced, it added many nice rewrite optimizations. However, in the case when there are many multiple nested Boolean queries under a top level Boolean#filter clause, its runtime grows exponentially. The key issue was how the BooleanQuery#rewriteNoScoring redirected yet again to the ConstantScoreQuery#rewrite. This causes BooleanQuery#rewrite to be called again recursively , even though it was previously called in ConstantScoreQuery#rewrite, and THEN BooleanQuery#rewriteNoScoring is called again, recursively. This causes exponential growth in rewrite time based on query depth. The change here hopes to short-circuit that and only grow (near) linearly by calling BooleanQuery#rewriteNoScoring directly, instead if attempting to redirect through ConstantScoreQuery#rewrite. closes: #12069
3 tasks
8 tasks
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 #672 was introduced, it added many nice rewrite optimizations. However, in the case when there are many multiple nested
Booleanqueries under a top levelBoolean#filterclause, its runtime grows exponentially.The key issue was how the
BooleanQuery#rewriteNoScoringredirected yet again to theConstantScoreQuery#rewrite. This causesBooleanQuery#rewriteto be called again recursively , even though it was previously called inConstantScoreQuery#rewrite, and THENBooleanQuery#rewriteNoScoringis called again, recursively.This causes exponential growth in rewrite time based on query depth. The change here hopes to short-circuit that and only grow (near) linearly by calling
BooleanQuery#rewriteNoScoringdirectly, instead if attempting to redirect throughConstantScoreQuery#rewrite.The absolute worst case I was able to test is many nested
SHOULDclauses with a depth of 22. This ran for over 7 seconds without my change. With my change it took less than 70ms.I had to cancel the test (without my change) when the depth was 30. It was simply taking too long. With my change, it was around 100ms.
closes: #12069