epotyom commented on code in PR #16446: URL: https://github.com/apache/lucene/pull/16446#discussion_r3970502907
########## lucene/queryparser/src/test/org/apache/lucene/queryparser/classic/TestMultiFieldQueryParserBoostAndOperator.java: ########## @@ -0,0 +1,126 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.lucene.queryparser.classic; + +import java.util.Map; +import org.apache.lucene.analysis.standard.StandardAnalyzer; +import org.apache.lucene.queryparser.classic.QueryParser.Operator; +import org.apache.lucene.search.Query; +import org.apache.lucene.tests.util.LuceneTestCase; + +/** + * Reproduces and guards against a bug GITHUB#16441 in {@link QueryParserBase#addMultiTermClauses}: Review Comment: This reminds me: can we deprecate addMultiTermClauses? It has no callers left after this change, but it is protected on a public class, so any subclass overriding it would silently stop being called. I would suggest removing it on main and marking it @deprecated in the backport, and updating the javadocs that still refer to it. ########## lucene/queryparser/src/test/org/apache/lucene/queryparser/classic/TestQueryParser.java: ########## @@ -1026,4 +1027,13 @@ private boolean isAHit(Query q, String content, Analyzer analyzer) throws IOExce return false; } } + + @Override + public void testSimpleDAO() throws Exception { + assertQueryEqualsDOA("term term term", null, "+term +term +term"); + assertQueryEqualsDOA("term +term term", null, "+term +term +term"); + assertQueryEqualsDOA("term term +term", null, "+(+term +term) +term"); + assertQueryEqualsDOA("term +term +term", null, "+term +term +term"); + assertQueryEqualsDOA("-term term term", null, "-term +(+term +term)"); Review Comment: I used IndexSearcher.rewrite to check that the queries before and after this change end up the same, and this case stood out: it now rewrites to `-term +(term)^2.0`, whereas it used to collapse to MatchNoDocsQuery because the same term appeared as both MUST and MUST_NOT. So while the change is purely cosmetic semantically, there may be cases where it costs us at query time. Would it be worth running the https://github.com/mikemccand/luceneutil benchmarks just to confirm there is no regression? WDYT? ########## lucene/queryparser/src/test/org/apache/lucene/queryparser/flexible/standard/TestStandardQP.java: ########## @@ -113,6 +115,51 @@ public TokenStreamComponents createComponents(String fieldName) { assertQueryEquals("a ! b", a, "a -b"); } + @Override + public void testRange() throws Exception { Review Comment: This is a big test, and overriding it is fragile since we then have to remember to keep the two copies in sync. WDYT about adding a protected method to the test base class, e.g. `multiTermGroup`, that takes an expected query string and returns it unchanged by default but wraps it in parentheses in TestQueryParser? I think we could then use it in both testRange and testSimpleDAO instead of overriding them. -- 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]
