Copilot commented on code in PR #4475:
URL: https://github.com/apache/solr/pull/4475#discussion_r3455530007
##########
solr/core/src/java/org/apache/solr/search/BoolQParserPlugin.java:
##########
@@ -73,11 +72,10 @@ protected Query unwrapQuery(Query query,
BooleanClause.Occur occur) {
@Override
protected Map<QParser, BooleanClause.Occur> clauses() throws SyntaxError
{
IdentityHashMap<QParser, BooleanClause.Occur> clauses = new
IdentityHashMap<>();
- SolrParams solrParams = SolrParams.wrapDefaults(localParams, params);
- addQueries(clauses, solrParams.getParams("must"),
BooleanClause.Occur.MUST);
- addQueries(clauses, solrParams.getParams("must_not"),
BooleanClause.Occur.MUST_NOT);
- addQueries(clauses, solrParams.getParams("filter"),
BooleanClause.Occur.FILTER);
- addQueries(clauses, solrParams.getParams("should"),
BooleanClause.Occur.SHOULD);
+ addQueries(clauses, localParams.getParams("must"),
BooleanClause.Occur.MUST);
+ addQueries(clauses, localParams.getParams("must_not"),
BooleanClause.Occur.MUST_NOT);
+ addQueries(clauses, localParams.getParams("filter"),
BooleanClause.Occur.FILTER);
+ addQueries(clauses, localParams.getParams("should"),
BooleanClause.Occur.SHOULD);
return clauses;
Review Comment:
`clauses()` now pulls `must`/`filter`/`should` exclusively from
`localParams`, which can be null and also prevents callers from specifying
these as normal request params when using `defType=bool` or `{!bool}` with
defaults. Restore the wrapped-default params lookup so local params override
request params but still allow request-level configuration.
##########
solr/core/src/test/org/apache/solr/search/TestMmBoolQParserPlugin.java:
##########
@@ -109,6 +110,40 @@ public void testMinShouldMatchThresholdsLower() throws
Exception {
assertEquals(expected, actual);
}
+ @Test
+ public void testEmptyStopWordClauseShouldCountForMMWithAutoRelaxFalse()
throws Exception {
+
+ Query actual =
+ parseQuery(
+ req(
+ "q",
+ "{!bool should=name:foo should=name:bar should=teststop:to
mm=-1 mm.autoRelax=false}"));
+
+ BooleanQuery expected =
+ shouldBuilder("foo", "bar")
+ .setMinimumNumberShouldMatch(2)
+ .add(new MatchNoDocsQuery(""), BooleanClause.Occur.SHOULD)
Review Comment:
The parser now creates `new MatchNoDocsQuery("empty query")` for
stopword-only clauses (see `SolrQueryParserBase#createFieldQuery`), so this
expected query should use the same reason string (or the no-arg ctor) to keep
`Query.equals` stable.
##########
solr/core/src/test/org/apache/solr/search/TestMmBoolQParserPlugin.java:
##########
@@ -109,6 +110,40 @@ public void testMinShouldMatchThresholdsLower() throws
Exception {
assertEquals(expected, actual);
}
+ @Test
+ public void testEmptyStopWordClauseShouldCountForMMWithAutoRelaxFalse()
throws Exception {
+
+ Query actual =
+ parseQuery(
+ req(
+ "q",
+ "{!bool should=name:foo should=name:bar should=teststop:to
mm=-1 mm.autoRelax=false}"));
+
+ BooleanQuery expected =
+ shouldBuilder("foo", "bar")
+ .setMinimumNumberShouldMatch(2)
+ .add(new MatchNoDocsQuery(""), BooleanClause.Occur.SHOULD)
+ .build();
+ assertEquals(expected, actual);
+ }
+
+ @Test
+ public void testEmptyStopWordClauseShouldNotCountForMMWithAutoRelaxTrue()
throws Exception {
+
+ Query actual =
+ parseQuery(
+ req(
+ "q",
+ "{!bool should=name:foo should=name:bar should=teststop:to
mm=-1 mm.autoRelax=true}"));
+
+ BooleanQuery expected =
+ shouldBuilder("foo", "bar")
+ .setMinimumNumberShouldMatch(1)
+ .add(new MatchNoDocsQuery(""), BooleanClause.Occur.SHOULD)
Review Comment:
Same issue as above: `MatchNoDocsQuery` equality includes the reason string,
so the expected query should match the production reason (or use the no-arg
ctor consistently).
##########
solr/core/src/java/org/apache/solr/search/BoolQParserPlugin.java:
##########
@@ -50,9 +50,8 @@ public Query parse() throws SyntaxError {
@Override
protected BooleanQuery parseImpl() throws SyntaxError {
BooleanQuery query = super.parseImpl();
- SolrParams solrParams = SolrParams.wrapDefaults(localParams, params);
- String minShouldMatch =
SolrPluginUtils.parseMinShouldMatch(req.getSchema(), solrParams);
- boolean mmAutoRelax = params.getBool(DisMaxParams.MM_AUTORELAX, false);
+ String minShouldMatch =
SolrPluginUtils.parseMinShouldMatch(req.getSchema(), localParams);
+ boolean mmAutoRelax = localParams.getBool(DisMaxParams.MM_AUTORELAX,
false);
return SolrPluginUtils.setMinShouldMatch(query, minShouldMatch,
mmAutoRelax);
Review Comment:
`BoolQParserPlugin` now reads `mm` / `mm.autoRelax` only from `localParams`.
This breaks existing behavior where these can be supplied as request params
(e.g., `q.op=AND` affecting the default `mm`), and it can also throw an NPE if
the parser is used without local params (e.g., `defType=bool` where
`localParams` may be null). Use `SolrParams.wrapDefaults(localParams, params)`
like before so local params override request params but still fall back
correctly.
##########
solr/core/src/test/org/apache/solr/util/SolrPluginUtilsTest.java:
##########
@@ -124,119 +124,149 @@ public void testParseFieldBoosts() {
assertEquals("spacey e2", e2, SolrPluginUtils.parseFieldBoosts(" \t
"));
}
- @Test
- public void testDisjunctionMaxQueryParser() throws Exception {
-
- Query out;
- String t;
-
- SolrQueryRequest req = req("df", "text");
- QParser qparser = QParser.getParser("hi", "dismax", req);
-
- DisjunctionMaxQueryParser qp =
- new SolrPluginUtils.DisjunctionMaxQueryParser(qparser,
req.getParams().get("df"));
-
- qp.addAlias(
- "hoss",
- 0.01f,
- SolrPluginUtils.parseFieldBoosts("title^2.0 title_stemmed name^1.2
subject^0.5"));
- qp.addAlias("test", 0.01f, SolrPluginUtils.parseFieldBoosts("text^2.0"));
- qp.addAlias("unused", 1.0f, SolrPluginUtils.parseFieldBoosts("subject^0.5
sind^1.5"));
-
- /* first some sanity tests that don't use aliasing at all */
- t = "XXXXXXXX";
- out = qp.parse(t);
+ /* sanity: bare term with no field and no alias → TermQuery on default field
*/
+ @Test
+ public void testDismaxParser_bareTermUsesDefaultField() throws Exception {
+ DisjunctionMaxQueryParser qp = newDismaxParserWithAliases();
+ String t = "XXXXXXXX";
+ Query out = qp.parse(t);
assertNotNull(t + " sanity test gave back null", out);
assertTrue(t + " sanity test isn't TermQuery: " + out.getClass(), out
instanceof TermQuery);
assertEquals(
t + " sanity test is wrong field",
qp.getDefaultField(),
((TermQuery) out).getTerm().field());
+ }
- t = "subject:XXXXXXXX";
- out = qp.parse(t);
+ /* sanity: explicit non-aliased field → TermQuery on that field */
+ @Test
+ public void testDismaxParser_explicitFieldNoAlias() throws Exception {
+ DisjunctionMaxQueryParser qp = newDismaxParserWithAliases();
+ String t = "subject:XXXXXXXX";
+ Query out = qp.parse(t);
assertNotNull(t + " sanity test gave back null", out);
assertTrue(t + " sanity test isn't TermQuery: " + out.getClass(), out
instanceof TermQuery);
assertEquals(t + " sanity test is wrong field", "subject", ((TermQuery)
out).getTerm().field());
+ }
- /* field has untokenized type, so this should be a term anyway */
- t = "sind:\"simple phrase\"";
- out = qp.parse(t);
+ /* sanity: untokenized field type → still a TermQuery even with quoted
phrase */
+ @Test
+ public void testDismaxParser_quotedPhraseOnUntokenizedFieldIsTermQuery()
throws Exception {
+ DisjunctionMaxQueryParser qp = newDismaxParserWithAliases();
+ String t = "sind:\"simple phrase\"";
+ Query out = qp.parse(t);
assertNotNull(t + " sanity test gave back null", out);
assertTrue(t + " sanity test isn't TermQuery: " + out.getClass(), out
instanceof TermQuery);
assertEquals(t + " sanity test is wrong field", "sind", ((TermQuery)
out).getTerm().field());
+ }
- t = "subject:\"simple phrase\"";
- out = qp.parse(t);
+ /* sanity: tokenized field with quoted phrase → PhraseQuery */
+ @Test
+ public void testDismaxParser_quotedPhraseOnTokenizedFieldIsPhraseQuery()
throws Exception {
+ DisjunctionMaxQueryParser qp = newDismaxParserWithAliases();
+ String t = "subject:\"simple phrase\"";
+ Query out = qp.parse(t);
assertNotNull(t + " sanity test gave back null", out);
assertTrue(t + " sanity test isn't PhraseQuery: " + out.getClass(), out
instanceof PhraseQuery);
assertEquals(
t + " sanity test is wrong field", "subject", ((PhraseQuery)
out).getTerms()[0].field());
+ }
- /* now some tests that use aliasing */
-
- /* basic usage of single "term" */
- t = "hoss:XXXXXXXX";
- out = qp.parse(t);
+ /* basic alias use: single term on an alias mapping to 4 fields → DMQ with 4
clauses */
+ @Test
+ public void testDismaxParser_aliasedTermProducesDMQClausePerField() throws
Exception {
+ DisjunctionMaxQueryParser qp = newDismaxParserWithAliases();
+ String t = "hoss:XXXXXXXX";
+ Query out = qp.parse(t);
assertNotNull(t + " was null", out);
assertTrue(t + " wasn't a DMQ:" + out.getClass(), out instanceof
DisjunctionMaxQuery);
assertEquals(
t + " wrong number of clauses", 4, countItems(((DisjunctionMaxQuery)
out).iterator()));
+ }
- /* odd case, but should still work, DMQ of one clause */
- t = "test:YYYYY";
- out = qp.parse(t);
+ /* edge case: alias points to a single field → DMQ with 1 clause */
+ @Test
+ public void testDismaxParser_singleFieldAliasIsDMQWithOneClause() throws
Exception {
+ DisjunctionMaxQueryParser qp = newDismaxParserWithAliases();
+ String t = "test:YYYYY";
+ Query out = qp.parse(t);
assertNotNull(t + " was null", out);
assertTrue(t + " wasn't a DMQ:" + out.getClass(), out instanceof
DisjunctionMaxQuery);
assertEquals(
t + " wrong number of clauses", 1, countItems(((DisjunctionMaxQuery)
out).iterator()));
+ }
- /* basic usage of multiple "terms" */
- t = "hoss:XXXXXXXX test:YYYYY";
- out = qp.parse(t);
+ /* multiple aliased terms → BooleanQuery whose clauses are DMQs */
+ @Test
+ public void testDismaxParser_multipleAliasedTermsProduceBooleanOfDMQs()
throws Exception {
+ DisjunctionMaxQueryParser qp = newDismaxParserWithAliases();
+ String t = "hoss:XXXXXXXX test:YYYYY";
+ Query out = qp.parse(t);
assertNotNull(t + " was null", out);
assertTrue(t + " wasn't a boolean:" + out.getClass(), out instanceof
BooleanQuery);
- {
- BooleanQuery bq = (BooleanQuery) out;
- List<BooleanClause> clauses = new ArrayList<>(bq.clauses());
- assertEquals(t + " wrong number of clauses", 2, clauses.size());
- Query sub = clauses.get(0).query();
- assertTrue(t + " first wasn't a DMQ:" + sub.getClass(), sub instanceof
DisjunctionMaxQuery);
- assertEquals(
- t + " first had wrong number of clauses",
- 4,
- countItems(((DisjunctionMaxQuery) sub).iterator()));
- sub = clauses.get(1).query();
- assertTrue(t + " second wasn't a DMQ:" + sub.getClass(), sub instanceof
DisjunctionMaxQuery);
- assertEquals(
- t + " second had wrong number of clauses",
- 1,
- countItems(((DisjunctionMaxQuery) sub).iterator()));
- }
- /* a phrase and a term that is a stop word for some fields */
- t = "hoss:\"XXXXXX YYYYY\" hoss:the";
- out = qp.parse(t);
+ BooleanQuery bq = (BooleanQuery) out;
+ List<BooleanClause> clauses = new ArrayList<>(bq.clauses());
+ assertEquals(t + " wrong number of clauses", 2, clauses.size());
+
+ Query first = clauses.get(0).query();
+ assertTrue(t + " first wasn't a DMQ:" + first.getClass(), first instanceof
DisjunctionMaxQuery);
+ assertEquals(
+ t + " first had wrong number of clauses",
+ 4,
+ countItems(((DisjunctionMaxQuery) first).iterator()));
+
+ Query second = clauses.get(1).query();
+ assertTrue(
+ t + " second wasn't a DMQ:" + second.getClass(), second instanceof
DisjunctionMaxQuery);
+ assertEquals(
+ t + " second had wrong number of clauses",
+ 1,
+ countItems(((DisjunctionMaxQuery) second).iterator()));
+ }
+
+ /* phrase + a term that is a stop word in some of the aliased fields */
+ @Test
+ public void testDismaxParser_stopWordReducesDMQClauseCount() throws
Exception {
+ DisjunctionMaxQueryParser qp = newDismaxParserWithAliases();
+ String t = "hoss:\"XXXXXX YYYYY\" hoss:the";
+ Query out = qp.parse(t);
assertNotNull(t + " was null", out);
assertTrue(t + " wasn't a boolean:" + out.getClass(), out instanceof
BooleanQuery);
- {
- BooleanQuery bq = (BooleanQuery) out;
- List<BooleanClause> clauses = new ArrayList<>(bq.clauses());
- assertEquals(t + " wrong number of clauses", 2, clauses.size());
- Query sub = clauses.get(0).query();
- assertTrue(t + " first wasn't a DMQ:" + sub.getClass(), sub instanceof
DisjunctionMaxQuery);
- assertEquals(
- t + " first had wrong number of clauses",
- 4,
- countItems(((DisjunctionMaxQuery) sub).iterator()));
- sub = clauses.get(1).query();
- assertTrue(t + " second wasn't a DMQ:" + sub.getClass(), sub instanceof
DisjunctionMaxQuery);
- assertEquals(
- t + " second had wrong number of clauses (stop words)",
- 2,
- countItems(((DisjunctionMaxQuery) sub).iterator()));
- }
+
+ BooleanQuery bq = (BooleanQuery) out;
+ List<BooleanClause> clauses = new ArrayList<>(bq.clauses());
+ assertEquals(t + " wrong number of clauses", 2, clauses.size());
+
+ Query first = clauses.get(0).query();
+ assertTrue(t + " first wasn't a DMQ:" + first.getClass(), first instanceof
DisjunctionMaxQuery);
+ assertEquals(
+ t + " first had wrong number of clauses",
+ 4,
+ countItems(((DisjunctionMaxQuery) first).iterator()));
+
+ Query second = clauses.get(1).query();
+ assertTrue(
+ t + " second wasn't a DMQ:" + second.getClass(), second instanceof
DisjunctionMaxQuery);
+ assertEquals(
+ t + " second had wrong number of clauses (stop words)",
+ 2,
+ countItems(((DisjunctionMaxQuery) second).iterator()));
Review Comment:
This assertion assumes stopwords cause aliased fields to be dropped
(disjunct count shrinks). With `SolrQueryParserBase#createFieldQuery` now
returning `MatchNoDocsQuery` (instead of null) for stopword-only terms,
`DisjunctionMaxQueryParser` will likely keep those disjuncts (possibly
boosted), so the DMQ clause count won’t be reduced anymore and this test will
fail. Either update the DisjunctionMaxQueryParser alias logic to skip
`MatchNoDocsQuery` disjuncts (preserving previous behavior), or update this
expectation to match the new semantics.
##########
solr/core/src/java/org/apache/solr/parser/SolrQueryParserBase.java:
##########
@@ -538,6 +538,24 @@ protected Query newFieldQuery(
return query;
}
+ /**
+ * Delegates to {@link
QueryBuilder#createFieldQuery(org.apache.lucene.analysis.Analyzer,
+ * org.apache.lucene.search.BooleanClause.Occur, java.lang.String,
java.lang.String, boolean,
+ * int)} but returns MatchNoDocsQuery rather than null
+ */
+ protected Query createFieldQuery(
+ Analyzer analyzer,
+ BooleanClause.Occur operator,
+ String field,
+ String queryText,
+ boolean quoted,
+ int phraseSlop) {
+
Review Comment:
Add `@Override` to `createFieldQuery(...)` to ensure it actually overrides
Lucene's `QueryBuilder#createFieldQuery` (and to make future signature drift a
compile-time error).
##########
solr/core/src/java/org/apache/solr/util/SolrPluginUtils.java:
##########
@@ -595,7 +596,9 @@ public static void setMinShouldMatch(BooleanQuery.Builder
q, String spec, boolea
optionalDismaxClauses++;
}
} else {
- optionalClauses++;
+ if (!(mmAutoRelax && (c.query() instanceof MatchNoDocsQuery))) {
+ optionalClauses++;
+ }
}
Review Comment:
The new mm.autoRelax logic only skips counting empty clauses when the SHOULD
query is a direct `MatchNoDocsQuery`. In practice, empty queries are often
wrapped (e.g., `BoostQuery(MatchNoDocsQuery)` via boosts, and
`MatchNoDocsQuery` disjuncts inside `DisjunctionMaxQuery`), so they’ll still be
counted and auto-relaxation won’t kick in. Consider unwrapping common wrappers
and (for `DisjunctionMaxQuery`) counting only non-empty disjuncts when
computing the disjunct size / optional clause counts.
--
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]
