You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by "renatoh (via GitHub)" <gi...@apache.org> on 2026/05/29 15:20:20 UTC
[PR] skip empty BooleanQuery with Occur=SHOULD [solr]
renatoh opened a new pull request, #4475:
URL: https://github.com/apache/solr/pull/4475
@dsmiley
This is a follow up of https://github.com/apache/solr/pull/4406
I realized that the minimum match expression like mm=-1 does not work properly if one should-clause is entirely removed by the analyzer, e.g. it only contains stopwords.
The root cause is that org.apache.solr.search.join.FiltersQParser#parseImpl can return empty-queries.
Given you comment on that method added quite recently, I am not sure if this is the right approach, but this change would fix the root of the issue. Alternatively I could just filter them out in the BoolQParserPlugin.
Do you know if such the empty clauses should only be filtered out if mmAutoRelax==true? If that is the case I need to do it in BoolQParserPlugin anyway.
Thanks
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] skip empty BooleanQuery with Occur=SHOULD [solr]
Posted by "renatoh (via GitHub)" <gi...@apache.org>.
renatoh commented on code in PR #4475:
URL: https://github.com/apache/solr/pull/4475#discussion_r3330452595
##########
solr/core/src/java/org/apache/solr/search/join/FiltersQParser.java:
##########
@@ -59,7 +59,14 @@ protected BooleanQuery parseImpl() throws SyntaxError {
BooleanQuery.Builder builder = createBuilder();
for (Map.Entry<QParser, Occur> clause : clauses.entrySet()) {
- builder.add(unwrapQuery(clause.getKey().getQuery(), clause.getValue()), clause.getValue());
+
+ Query query = clause.getKey().getQuery();
+ Occur occur = clause.getValue();
+
+ if (occur == Occur.SHOULD && query instanceof BooleanQuery boolQ && boolQ.clauses().isEmpty()) {
Review Comment:
I don't think org.apache.solr.schema.TextField#getFieldQuery is called, the call stack to arrive at org.apache.lucene.util.QueryBuilder#createFieldQuery is:
```
at org.apache.lucene.util.QueryBuilder.createFieldQuery(QueryBuilder.java:257)
at org.apache.solr.parser.SolrQueryParserBase.newFieldQuery(SolrQueryParserBase.java:529)
at org.apache.solr.parser.QueryParser.newFieldQuery(QueryParser.java:68)
at org.apache.solr.parser.SolrQueryParserBase.getFieldQuery(SolrQueryParserBase.java:1122)
at org.apache.solr.parser.SolrQueryParserBase.handleBareTokenQuery(SolrQueryParserBase.java:861)
at org.apache.solr.parser.QueryParser.Term(QueryParser.java:454)
at org.apache.solr.parser.QueryParser.Clause(QueryParser.java:293)
at org.apache.solr.parser.QueryParser.Query(QueryParser.java:173)
at org.apache.solr.parser.QueryParser.TopLevelQuery(QueryParser.java:143)
at org.apache.solr.parser.SolrQueryParserBase.parse(SolrQueryParserBase.java:276)
at org.apache.solr.search.LuceneQParser.parse(LuceneQParser.java:51)
at org.apache.solr.search.QParser.getQuery(QParser.java:197)
at org.apache.solr.search.join.FiltersQParser.parseImpl(FiltersQParser.java:64)
at org.apache.solr.search.BoolQParserPlugin$1.parseImpl(BoolQParserPlugin.java:52)
at org.apache.solr.search.BoolQParserPlugin$1.parse(BoolQParserPlugin.java:47)
at org.apache.solr.search.QParser.getQuery(QParser.java:197)
at org.apache.solr.search.TestMmBoolQParserPlugin.parseQuery(TestMmBoolQParserPlugin.java:49)
at org.apache.solr.search.TestMmBoolQParserPlugin.testMinShouldMatchThresholdsLower(TestMmBoolQParserPlugin.java:103)
```
From what you are saying, it wouldn't be sufficient to just check if org.apache.lucene.util.QueryBuilder.createFieldQuery(QueryBuilder.java:257) returns null and then return MatchNoDocsQuery instead of passing null up the stack?
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] skip empty clause for mm count in BoolQParserPlugin [solr]
Posted by "renatoh (via GitHub)" <gi...@apache.org>.
renatoh commented on code in PR #4475:
URL: https://github.com/apache/solr/pull/4475#discussion_r3503792395
##########
solr/core/src/java/org/apache/solr/util/SolrPluginUtils.java:
##########
@@ -596,7 +596,9 @@ public static void setMinShouldMatch(BooleanQuery.Builder q, String spec, boolea
optionalDismaxClauses++;
}
} else {
- if (!(c.query() instanceof MatchNoDocsQuery)) optionalClauses++;
+ if (!(mmAutoRelax && (c.query() instanceof MatchNoDocsQuery))) {
Review Comment:
> Can you give an example of a use case you were thinking of?
@janhoy
So if we search for "the brown fox" like this using the BoolQParser: mm=-1 should=field1:the should=field1:brown should=field1:fox, if 'the' is removed by the analyzer and the first should becomes an empty clause, what is mm now? 2, counting the empty clause, or 1 not counting the empty clause.
My interpretation of mm.autoRelax in the context of the BoolQParser was, that it controls if empty clauses are counted or not for mm.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] skip empty BooleanQuery with Occur=SHOULD [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on PR #4475:
URL: https://github.com/apache/solr/pull/4475#issuecomment-4618893215
> The clause with a stopword only result in no documents being returned, before and after my change.
Wouldn't you expect that an empty clause does not have an impact on the documents matching?
TBH I'm not sure. min-should-match with stopwords is a tricky subject... empty BQs is an edge case. dismax/edismax query parsers have lots of nobs for production cases of handling user input. `bool` query parser is a rudimentary/simple query parser -- more nuts & bolts for system developers to use. Definitely not for direct user queries. I'm skeptical it should grow into something like dismax.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] skip empty clause for mm count in BoolQParserPlugin [solr]
Posted by "renatoh (via GitHub)" <gi...@apache.org>.
renatoh commented on PR #4475:
URL: https://github.com/apache/solr/pull/4475#issuecomment-4767805720
> I'm coming around to a null check in org.apache.solr.search.join.FiltersQParser.parseImpl but not so heavily guarded by lots of conditions. And it needs a comment, of course.
My only change left to org.apache.solr.search.join.FiltersQParser.parseImpl is the in-lining of the variable query and occur, which I have reverted just now since how it use how it used to be looks more readable to me.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] skip empty clause for mm count in BoolQParserPlugin [solr]
Posted by "renatoh (via GitHub)" <gi...@apache.org>.
renatoh commented on code in PR #4475:
URL: https://github.com/apache/solr/pull/4475#discussion_r3452389305
##########
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) {
+
+ Query fieldQuery =
+ super.createFieldQuery(analyzer, operator, field, queryText, quoted, phraseSlop);
+ return fieldQuery != null ? fieldQuery : new MatchNoDocsQuery("empty query");
Review Comment:
To make sure we are on the same page: You think of reverting this since this it has broken some tests and hence we can not further follow this approach?
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] skip empty BooleanQuery with Occur=SHOULD [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #4475:
URL: https://github.com/apache/solr/pull/4475#discussion_r3328783280
##########
solr/core/src/java/org/apache/solr/search/join/FiltersQParser.java:
##########
@@ -59,7 +59,14 @@ protected BooleanQuery parseImpl() throws SyntaxError {
BooleanQuery.Builder builder = createBuilder();
for (Map.Entry<QParser, Occur> clause : clauses.entrySet()) {
- builder.add(unwrapQuery(clause.getKey().getQuery(), clause.getValue()), clause.getValue());
+
+ Query query = clause.getKey().getQuery();
+ Occur occur = clause.getValue();
+
+ if (occur == Occur.SHOULD && query instanceof BooleanQuery boolQ && boolQ.clauses().isEmpty()) {
Review Comment:
this string of conditions looks very hacky. Reading the description... I thought there was going to be a null somewhere to check for?
##########
solr/core/src/test/org/apache/solr/search/TestMmBoolQParserPlugin.java:
##########
@@ -109,6 +109,17 @@ public void testMinShouldMatchThresholdsLower() throws Exception {
assertEquals(expected, actual);
}
+ @Test
+ public void testMinShouldMatchWithEmptyClauseCausedByStopWord() throws Exception {
Review Comment:
test fails: `undefined field name_sw`
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] skip empty BooleanQuery with Occur=SHOULD [solr]
Posted by "renatoh (via GitHub)" <gi...@apache.org>.
renatoh commented on code in PR #4475:
URL: https://github.com/apache/solr/pull/4475#discussion_r3339096789
##########
solr/core/src/java/org/apache/solr/search/join/FiltersQParser.java:
##########
@@ -59,7 +59,14 @@ protected BooleanQuery parseImpl() throws SyntaxError {
BooleanQuery.Builder builder = createBuilder();
for (Map.Entry<QParser, Occur> clause : clauses.entrySet()) {
- builder.add(unwrapQuery(clause.getKey().getQuery(), clause.getValue()), clause.getValue());
+
+ Query query = clause.getKey().getQuery();
+ Occur occur = clause.getValue();
+
+ if (occur == Occur.SHOULD && query instanceof BooleanQuery boolQ && boolQ.clauses().isEmpty()) {
Review Comment:
are the test re-run on every commit? if so, it so it looks like no test broke.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] skip empty clause for mm count in BoolQParserPlugin [solr]
Posted by "janhoy (via GitHub)" <gi...@apache.org>.
janhoy commented on PR #4475:
URL: https://github.com/apache/solr/pull/4475#issuecomment-4773188607
I guess for both #4406 and this, we need to ask ourselves whether we want Solr's QParserPlugins to be thin wrappers around Lucene's queries and thus keep mm as integer, or if we want to add our own layers of logic on top, including handling of empty clauses. I know that our eDisMax parser takes the maximalist approach in the name of configurability. But it that the correct call for BoolQParser, or should it have been a SuperDuperBoolQParser intead? 😉
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] skip empty BooleanQuery with Occur=SHOULD [solr]
Posted by "renatoh (via GitHub)" <gi...@apache.org>.
renatoh commented on PR #4475:
URL: https://github.com/apache/solr/pull/4475#issuecomment-4738688392
@dsmiley How do we continue on this one?
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] skip empty BooleanQuery with Occur=SHOULD [solr]
Posted by "renatoh (via GitHub)" <gi...@apache.org>.
renatoh commented on PR #4475:
URL: https://github.com/apache/solr/pull/4475#issuecomment-4606298194
@dsmiley I am wondering about the AND/must case. With dismax the query:
`q.op=AND&q=black and pink&qf=title_txt_en`
turns into:
`+(DisjunctionMaxQuery((title_txt_en:black)) DisjunctionMaxQuery((title_txt_en:pink)))~2 ()`
The stopword is remove entirely and documents with black and pink are returned.
With the Bool-Parser:
`q=({!bool must=$w1 must=$w2 must=$w3})&w1=(title_txt_en:luggage)&w2=(title_txt_en:and)&w3=(title_txt_en:pink)`
before change:
`+title_txt_en:black +() +title_txt_en:pink`
after change:
`+MatchNoDocsQuery(MatchNoDocsQuery(\"empty query\")) +title_txt_en:pink +title_txt_en:black`
The clause with a stopword only result in no documents being returned, before and after my change.
Wouldn't you expect that an empty clause does not have an impact on the documents matching?
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] skip empty clause for mm count in BoolQParserPlugin [solr]
Posted by "github-actions[bot] (via GitHub) via issues" <is...@solr.apache.org>.
github-actions[bot] closed pull request #4475: skip empty clause for mm count in BoolQParserPlugin
URL: https://github.com/apache/solr/pull/4475
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] skip empty clause for mm count in BoolQParserPlugin [solr]
Posted by "janhoy (via GitHub)" <gi...@apache.org>.
janhoy commented on code in PR #4475:
URL: https://github.com/apache/solr/pull/4475#discussion_r3455480400
##########
solr/core/src/test-files/solr/collection1/conf/schema.xml:
##########
@@ -156,6 +156,7 @@
<analyzer type="query">
<tokenizer class="solr.LetterTokenizerFactory"/>
<filter class="solr.LowerCaseFilterFactory"/>
+ <filter class="solr.StopFilterFactory" words="stopwords.txt"/>
Review Comment:
I suppose this schema file is used by a fair amount of tests, are you sure that this change won't have side effects for those other tests?
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] skip empty BooleanQuery with Occur=SHOULD [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #4475:
URL: https://github.com/apache/solr/pull/4475#discussion_r3444285500
##########
solr/core/src/test/org/apache/solr/search/TestMmBoolQParserPlugin.java:
##########
@@ -109,6 +110,20 @@ public void testMinShouldMatchThresholdsLower() throws Exception {
assertEquals(expected, actual);
}
+ @Test
+ public void testMinShouldMatchWithEmptyClauseCausedByStopWord() throws Exception {
+
+ Query actual =
+ parseQuery(req("q", "{!bool should=name:foo should=name:bar should=teststop:to mm=-1}"));
+
+ BooleanQuery expected =
+ shouldBuilder("foo", "bar")
+ .setMinimumNumberShouldMatch(1)
Review Comment:
I'm guessing this is undesirable -- we want 2? If so, a comment e.g. "nocommit" would communicate intent.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] skip empty clause for mm count in BoolQParserPlugin [solr]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #4475:
URL: https://github.com/apache/solr/pull/4475#issuecomment-5472189858
This PR has had no activity for 60 days and is now labeled as stale. Any new activity will remove the stale label. To attract more reviewers, please tag people who might be familiar with the code area and/or notify the dev@solr.apache.org mailing list. To exempt this PR from being marked as stale, make it a draft PR or add the label "exempt-stale". If left unattended, this PR will be closed after another 60 days of inactivity. Thank you for your contribution!
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] skip empty clause for mm count in BoolQParserPlugin [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #4475:
URL: https://github.com/apache/solr/pull/4475#discussion_r3452162210
##########
solr/core/src/java/org/apache/solr/util/SolrPluginUtils.java:
##########
@@ -596,7 +596,9 @@ public static void setMinShouldMatch(BooleanQuery.Builder q, String spec, boolea
optionalDismaxClauses++;
}
} else {
- if (!(c.query() instanceof MatchNoDocsQuery)) optionalClauses++;
+ if (!(mmAutoRelax && (c.query() instanceof MatchNoDocsQuery))) {
Review Comment:
My reading of mm.autoRelax is that it's pertinent when there are multiple qf fields, and, due to varying analysis on different fields, *sometimes* fully analyzes away an input token (e.g. due to a stopword) and sometimes not. The code details in SolrPluginUtils in the way it increments/resets certain counters here reflects that. Of course, feel free to prove me wrong; I didn't do further research. Maybe the original JIRA/discussions show more broad usage.
Even if I'm right, there's maybe a case to be made for generalizing the concept.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] skip empty BooleanQuery with Occur=SHOULD [solr]
Posted by "renatoh (via GitHub)" <gi...@apache.org>.
renatoh commented on code in PR #4475:
URL: https://github.com/apache/solr/pull/4475#discussion_r3444448600
##########
solr/core/src/test/org/apache/solr/search/TestMmBoolQParserPlugin.java:
##########
@@ -109,6 +110,20 @@ public void testMinShouldMatchThresholdsLower() throws Exception {
assertEquals(expected, actual);
}
+ @Test
+ public void testMinShouldMatchWithEmptyClauseCausedByStopWord() throws Exception {
+
+ Query actual =
+ parseQuery(req("q", "{!bool should=name:foo should=name:bar should=teststop:to mm=-1}"));
+
+ BooleanQuery expected =
+ shouldBuilder("foo", "bar")
+ .setMinimumNumberShouldMatch(1)
Review Comment:
We want 2 unless mmAutoRelax=true, so should=teststop:to is counted for mm unless mmAutoRelax=true? Is my understanding correct?
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] skip empty clause for mm count in BoolQParserPlugin [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on PR #4475:
URL: https://github.com/apache/solr/pull/4475#issuecomment-4764979494
I'm coming around to a null check in org.apache.solr.search.join.FiltersQParser.parseImpl but not so heavily guarded by lots of conditions. And it needs a comment, of course.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] skip empty BooleanQuery with Occur=SHOULD [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #4475:
URL: https://github.com/apache/solr/pull/4475#discussion_r3330829251
##########
solr/core/src/java/org/apache/solr/search/join/FiltersQParser.java:
##########
@@ -59,7 +59,14 @@ protected BooleanQuery parseImpl() throws SyntaxError {
BooleanQuery.Builder builder = createBuilder();
for (Map.Entry<QParser, Occur> clause : clauses.entrySet()) {
- builder.add(unwrapQuery(clause.getKey().getQuery(), clause.getValue()), clause.getValue());
+
+ Query query = clause.getKey().getQuery();
+ Occur occur = clause.getValue();
+
+ if (occur == Occur.SHOULD && query instanceof BooleanQuery boolQ && boolQ.clauses().isEmpty()) {
Review Comment:
yes; try it any ways. We'll see. It'll be interesting to see if there are test failures that make assumptions.
Seems SolrQueryParserBase.newFieldQuery is a good spot for this change. Or override Lucene level createFieldQuery as it's the lowest spot we can do and it should cover a wider net.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] skip empty clause for mm count in BoolQParserPlugin [solr]
Posted by "renatoh (via GitHub)" <gi...@apache.org>.
renatoh commented on code in PR #4475:
URL: https://github.com/apache/solr/pull/4475#discussion_r3448998216
##########
solr/core/src/java/org/apache/solr/util/SolrPluginUtils.java:
##########
@@ -596,7 +596,9 @@ public static void setMinShouldMatch(BooleanQuery.Builder q, String spec, boolea
optionalDismaxClauses++;
}
} else {
- if (!(c.query() instanceof MatchNoDocsQuery)) optionalClauses++;
+ if (!(mmAutoRelax && (c.query() instanceof MatchNoDocsQuery))) {
Review Comment:
The problem with mm for BoolQParserPlugin is that empty clauses are counted as they were non-empty clauses. e.g. if we have mm=-1 but one should-clause becomes empty, mm will not be adjusted accordingly. And that is, at least to my understanding, what mmAutoRelax is for. If set to true, the mm-calculation will account for empty clauses.
Here the comment describing the parameter:
/** If set to true, will try to reduce MM if tokens are removed from some clauses but not all */
public static String MM_AUTORELAX = "mm.autoRelax";
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] skip empty clause for mm count in BoolQParserPlugin [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #4475:
URL: https://github.com/apache/solr/pull/4475#discussion_r3449786259
##########
solr/core/src/java/org/apache/solr/util/SolrPluginUtils.java:
##########
@@ -596,7 +596,9 @@ public static void setMinShouldMatch(BooleanQuery.Builder q, String spec, boolea
optionalDismaxClauses++;
}
} else {
- if (!(c.query() instanceof MatchNoDocsQuery)) optionalClauses++;
+ if (!(mmAutoRelax && (c.query() instanceof MatchNoDocsQuery))) {
Review Comment:
I guessed wrong on mm.autoRelax -- see the docs: https://github.com/apache/solr/blob/9809766bd9fda66aa67992ab4036e00a15ea1adf/solr/solr-ref-guide/modules/query-guide/pages/edismax-query-parser.adoc#L54 it's strictly for disjunctionmax and doesn't apply in another context. I can also see that from its implementation now that I look at it longer.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] skip empty clause for mm count in BoolQParserPlugin [solr]
Posted by "janhoy (via GitHub)" <gi...@apache.org>.
janhoy commented on code in PR #4475:
URL: https://github.com/apache/solr/pull/4475#discussion_r3455474719
##########
solr/core/src/java/org/apache/solr/util/SolrPluginUtils.java:
##########
@@ -596,7 +596,9 @@ public static void setMinShouldMatch(BooleanQuery.Builder q, String spec, boolea
optionalDismaxClauses++;
}
} else {
- if (!(c.query() instanceof MatchNoDocsQuery)) optionalClauses++;
+ if (!(mmAutoRelax && (c.query() instanceof MatchNoDocsQuery))) {
Review Comment:
It's right that `autoRelax` was designed for the corner case of dismax, where you have different analysis for two fields in the field list, so "the brown fox" would result in "brown fox" by one ananlyzer and "the brown fox" in another. Then auto relaxing mm will not require all three terms. Not sure if it makes sense in a pure BooleanQuery against same field?
Can you give an example of a use case you were thinking of?
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] skip empty clause for mm count in BoolQParserPlugin [solr]
Posted by "github-actions[bot] (via GitHub) via issues" <is...@solr.apache.org>.
github-actions[bot] commented on PR #4475:
URL: https://github.com/apache/solr/pull/4475#issuecomment-6536272385
This PR is now closed due to 60 days of inactivity after being marked as stale. Re-opening this PR is still possible, in which case it will be marked as active again.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] skip empty BooleanQuery with Occur=SHOULD [solr]
Posted by "renatoh (via GitHub)" <gi...@apache.org>.
renatoh commented on PR #4475:
URL: https://github.com/apache/solr/pull/4475#issuecomment-4629869910
@dsmiley
I had a look at some failing tests, bellow my analysis. Would be great to get your inputs on my findings. Thanks!
org.apache.solr.util.SolrPluginUtilsTest#testDisjunctionMaxQueryParser
Fails since it expects name:the to be dropped entirely from the query, now we get a MatchNoDocsQuery, the number of expected queries does not match anymore. Null query get skipped here:
org/apache/solr/util/SolrPluginUtils.java:896
This test was a pain to analyze and to debug since it tests so many different things and the last assert failed, I broke that test up into several smaller test cases, the failing now is: testDismaxParser_stopWordReducesDMQClauseCount
org.apache.solr.search.TestExtendedDismaxParser#testFocusQueryParser
That’s an interesting one, apparently edismax has a flag alwaysStopwords, which was introduced here:
https://issues.apache.org/jira/projects/SOLR/issues/SOLR-17959
So it expects the stop words only to be dropped from the query if alwaysStopwords=true, but now they are dropped regardless of the flag.
org.apache.solr.search.TestExtendedDismaxParser#testSplitOnWhitespace_shouldRespectMinimumShouldMatch.
The test expect the query below to not return any documents, since only one out of the conditions are matching:
mm=100%&q=Terminator: 100&defType=edismax&df=text&qf=movies_t foo_i
Searching for a text in an integer field foo_i creates a MatchNoDocsQuery, with my changes to org.apache.solr.util.SolrPluginUtils#setMinShouldMatch are ignored when calculating the absolute mm, hence 100% means for this query now 1 out of 1, and not 2 out of 2 as it did before.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] skip empty BooleanQuery with Occur=SHOULD [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #4475:
URL: https://github.com/apache/solr/pull/4475#discussion_r3448934688
##########
solr/core/src/java/org/apache/solr/util/SolrPluginUtils.java:
##########
@@ -596,7 +596,9 @@ public static void setMinShouldMatch(BooleanQuery.Builder q, String spec, boolea
optionalDismaxClauses++;
}
} else {
- if (!(c.query() instanceof MatchNoDocsQuery)) optionalClauses++;
+ if (!(mmAutoRelax && (c.query() instanceof MatchNoDocsQuery))) {
Review Comment:
I'm confused at what problem is being solved at this point.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] skip empty clause for mm count in BoolQParserPlugin [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #4475:
URL: https://github.com/apache/solr/pull/4475#discussion_r3452194631
##########
solr/core/src/java/org/apache/solr/search/join/FiltersQParser.java:
##########
@@ -59,7 +59,11 @@ protected BooleanQuery parseImpl() throws SyntaxError {
BooleanQuery.Builder builder = createBuilder();
for (Map.Entry<QParser, Occur> clause : clauses.entrySet()) {
- builder.add(unwrapQuery(clause.getKey().getQuery(), clause.getValue()), clause.getValue());
+
Review Comment:
Maybe I'm nitpicky but I don't find the blank lines hepful here.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] skip empty BooleanQuery with Occur=SHOULD [solr]
Posted by "renatoh (via GitHub)" <gi...@apache.org>.
renatoh commented on code in PR #4475:
URL: https://github.com/apache/solr/pull/4475#discussion_r3329226127
##########
solr/core/src/java/org/apache/solr/search/join/FiltersQParser.java:
##########
@@ -59,7 +59,14 @@ protected BooleanQuery parseImpl() throws SyntaxError {
BooleanQuery.Builder builder = createBuilder();
for (Map.Entry<QParser, Occur> clause : clauses.entrySet()) {
- builder.add(unwrapQuery(clause.getKey().getQuery(), clause.getValue()), clause.getValue());
+
+ Query query = clause.getKey().getQuery();
+ Occur occur = clause.getValue();
+
+ if (occur == Occur.SHOULD && query instanceof BooleanQuery boolQ && boolQ.clauses().isEmpty()) {
Review Comment:
I can only work with what the LuceneQParser returns, and in this case it is a BooleanQuery with an empty List of clauses. And as I understand it, I can only safely drop a Query with an empty list of clauses if it is a SHOULD. I do not see any attribute on BooleanQuery I could check for null.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] skip empty BooleanQuery with Occur=SHOULD [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #4475:
URL: https://github.com/apache/solr/pull/4475#discussion_r3444353621
##########
solr/core/src/java/org/apache/solr/search/join/FiltersQParser.java:
##########
@@ -59,7 +59,14 @@ protected BooleanQuery parseImpl() throws SyntaxError {
BooleanQuery.Builder builder = createBuilder();
for (Map.Entry<QParser, Occur> clause : clauses.entrySet()) {
- builder.add(unwrapQuery(clause.getKey().getQuery(), clause.getValue()), clause.getValue());
+
+ Query query = clause.getKey().getQuery();
+ Occur occur = clause.getValue();
+
+ if (occur == Occur.SHOULD && query instanceof BooleanQuery boolQ && boolQ.clauses().isEmpty()) {
Review Comment:
no; tests only run for committers for security reasons. And I can approve them to run -- albeit each time. I just did. I recommend you get familiar with how to run a subset of tests that can run quickly -- perhaaps an IntelliJ "run configuration" of a list of tests, for example.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] skip empty clause for mm count in BoolQParserPlugin [solr]
Posted by "renatoh (via GitHub)" <gi...@apache.org>.
renatoh commented on PR #4475:
URL: https://github.com/apache/solr/pull/4475#issuecomment-4776902094
> I guess for both #4406 and this, we need to ask ourselves whether we want Solr's QParserPlugins to be thin wrappers around Lucene's queries and thus keep mm as integer, or if we want to add our own layers of logic on top, including handling of empty clauses. I know that our eDisMax parser takes the maximalist approach in the name of configurability. But it that the correct call for BoolQParser, or should it have been a SuperDuperBoolQParser intead? 😉
@janhoy
I would argue if mm is supported, it should also handle the same expression as eDisMax, maybe a good middle ground would be to support the expressions and the -1 case, but not handle empty-clauses as eDisMax does. A query like should:title_txt_en:to, I would considered that an edge-case. What do you think?
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] skip empty clause for mm count in BoolQParserPlugin [solr]
Posted by "renatoh (via GitHub)" <gi...@apache.org>.
renatoh commented on code in PR #4475:
URL: https://github.com/apache/solr/pull/4475#discussion_r3450954946
##########
solr/core/src/java/org/apache/solr/util/SolrPluginUtils.java:
##########
@@ -596,7 +596,9 @@ public static void setMinShouldMatch(BooleanQuery.Builder q, String spec, boolea
optionalDismaxClauses++;
}
} else {
- if (!(c.query() instanceof MatchNoDocsQuery)) optionalClauses++;
+ if (!(mmAutoRelax && (c.query() instanceof MatchNoDocsQuery))) {
Review Comment:
mm is supported by BoolQParser, but mm.autoRelax is strictly for disjunctionmax? Can you explain why that is?
If mm.autoRleax is not supprted by BoolQParser, how should it behave for relative mm-value likes mm=-1, should it count empty clauses or not?
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] skip empty clause for mm count in BoolQParserPlugin [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #4475:
URL: https://github.com/apache/solr/pull/4475#discussion_r3452198708
##########
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) {
+
+ Query fieldQuery =
+ super.createFieldQuery(analyzer, operator, field, queryText, quoted, phraseSlop);
+ return fieldQuery != null ? fieldQuery : new MatchNoDocsQuery("empty query");
Review Comment:
can be reverted now I think?
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] skip empty clause for mm count in BoolQParserPlugin [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #4475:
URL: https://github.com/apache/solr/pull/4475#discussion_r3452211506
##########
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,
Review Comment:
This isn't delegation, this is overriding; and thus we needn't specifically say the method signature as that's the same as the current method.
Anyway; this is maybe not needed anymore.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] skip empty BooleanQuery with Occur=SHOULD [solr]
Posted by "renatoh (via GitHub)" <gi...@apache.org>.
renatoh commented on code in PR #4475:
URL: https://github.com/apache/solr/pull/4475#discussion_r3447044764
##########
solr/core/src/java/org/apache/solr/util/SolrPluginUtils.java:
##########
@@ -596,7 +596,9 @@ public static void setMinShouldMatch(BooleanQuery.Builder q, String spec, boolea
optionalDismaxClauses++;
}
} else {
- if (!(c.query() instanceof MatchNoDocsQuery)) optionalClauses++;
+ if (!(mmAutoRelax && (c.query() instanceof MatchNoDocsQuery))) {
Review Comment:
@dsmiley Could that be a solution for mmAutoRelax?
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] skip empty clause for mm count in BoolQParserPlugin [solr]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] skip empty clause for mm count in BoolQParserPlugin [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #4475:
URL: https://github.com/apache/solr/pull/4475#discussion_r3455486967
##########
solr/core/src/test-files/solr/collection1/conf/schema.xml:
##########
@@ -156,6 +156,7 @@
<analyzer type="query">
<tokenizer class="solr.LetterTokenizerFactory"/>
<filter class="solr.LowerCaseFilterFactory"/>
+ <filter class="solr.StopFilterFactory" words="stopwords.txt"/>
Review Comment:
I 100% agree and was going to give this critique but was waiting for us to understand what problem is being solved and how, first.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] skip empty BooleanQuery with Occur=SHOULD [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #4475:
URL: https://github.com/apache/solr/pull/4475#discussion_r3329301384
##########
solr/core/src/java/org/apache/solr/search/join/FiltersQParser.java:
##########
@@ -59,7 +59,14 @@ protected BooleanQuery parseImpl() throws SyntaxError {
BooleanQuery.Builder builder = createBuilder();
for (Map.Entry<QParser, Occur> clause : clauses.entrySet()) {
- builder.add(unwrapQuery(clause.getKey().getQuery(), clause.getValue()), clause.getValue());
+
+ Query query = clause.getKey().getQuery();
+ Occur occur = clause.getValue();
+
+ if (occur == Occur.SHOULD && query instanceof BooleanQuery boolQ && boolQ.clauses().isEmpty()) {
Review Comment:
The description says:
> org.apache.solr.search.join.FiltersQParser#parseImpl can return empty-queries
If you dig deeper... this isn't a fault/problem of FiltersQParser specifically or that method, it's that **broadly**, any QParser parse() may return either null or return an empty BooleanQuery. It'd be great if you pursue fixing *that*. But I don't think FiltersQParser needs a modification.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] skip empty BooleanQuery with Occur=SHOULD [solr]
Posted by "renatoh (via GitHub)" <gi...@apache.org>.
renatoh commented on code in PR #4475:
URL: https://github.com/apache/solr/pull/4475#discussion_r3329985105
##########
solr/core/src/java/org/apache/solr/search/join/FiltersQParser.java:
##########
@@ -59,7 +59,14 @@ protected BooleanQuery parseImpl() throws SyntaxError {
BooleanQuery.Builder builder = createBuilder();
for (Map.Entry<QParser, Occur> clause : clauses.entrySet()) {
- builder.add(unwrapQuery(clause.getKey().getQuery(), clause.getValue()), clause.getValue());
+
+ Query query = clause.getKey().getQuery();
+ Occur occur = clause.getValue();
+
+ if (occur == Occur.SHOULD && query instanceof BooleanQuery boolQ && boolQ.clauses().isEmpty()) {
Review Comment:
I see, the root cause is that org.apache.solr.parser.SolrQueryParserBase#newFieldQuery calls org.apache.lucene.util.QueryBuilder#createFieldQuery, and this method returns null
if the queryText is entirely remove due to only containing a stop-word. This null is then propagated upwards.
What would be the alternative, returning a MatchNoDocsQuery instead of null?
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] skip empty BooleanQuery with Occur=SHOULD [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #4475:
URL: https://github.com/apache/solr/pull/4475#discussion_r3330307914
##########
solr/core/src/java/org/apache/solr/search/join/FiltersQParser.java:
##########
@@ -59,7 +59,14 @@ protected BooleanQuery parseImpl() throws SyntaxError {
BooleanQuery.Builder builder = createBuilder();
for (Map.Entry<QParser, Occur> clause : clauses.entrySet()) {
- builder.add(unwrapQuery(clause.getKey().getQuery(), clause.getValue()), clause.getValue());
+
+ Query query = clause.getKey().getQuery();
+ Occur occur = clause.getValue();
+
+ if (occur == Occur.SHOULD && query instanceof BooleanQuery boolQ && boolQ.clauses().isEmpty()) {
Review Comment:
Ahh. IMO it's make sense to put logic in `org.apache.solr.schema.TextField#getFieldQuery` which is on the call chain you reference. Logic to prevent null or an empty boolean query -- either should simply return MatchNoDocsQuery. Ideally the scope would be broader than this getFieldQuery method which is somewhat obscure and thus communicating this change alone would be awkward -- a clue that the scope isn't right. It'd be more useful & clearer to say *any* analysis yielding no tokens results in a MatchNoDocsQuery, if we can make such a claim. Of course, feel free to look further and propose something.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org