javanna commented on code in PR #12072:
URL: https://github.com/apache/lucene/pull/12072#discussion_r1067144911
##########
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();
Review Comment:
Would it make sense to write a similar test for boolean queries that do need
scoring? I am worried that the wrapping we do in the main rewrite has some
impact too but we don't exercise it as much with this test.
##########
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);
Review 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:
##########
@@ -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();
Review Comment:
I wonder if we should have a separate test with should clauses, just to
simplify the test a bit.
##########
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:
Does this test actually verifies that we are not doing any needless
rewriting? I was under the impression that how long the test takes depending on
the depth of the boolean query is the only way to catch a regression. Would it
be possible to write specific tests that verify the intermediate steps by
calling the query rewrite method against some of the intermediate
representations of the query and checking what is returned? I have not given
this a lot of thought, it may be hard... but I am curious to hear what you
think.
##########
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 agree. I am tempted to go and simplify the logic further (do we need the
wrapping at all?) but I will leave that for a follow-up. I like how your fix is
contained.
--
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]
