zacharymorn commented on a change in pull request #240: URL: https://github.com/apache/lucene/pull/240#discussion_r692722544
########## File path: lucene/core/src/java/org/apache/lucene/search/IndexSearcher.java ########## @@ -527,7 +503,10 @@ public TopDocs search(Query query, int n) throws IOException { * * @throws TooManyClauses If a query would exceed {@link IndexSearcher#getMaxClauseCount()} * clauses. + * @deprecated This method is being deprecated in favor of {@link IndexSearcher#search(Query, Review comment: > I think we like the @lucene.deprecated tag? Do you mean `@lucene.deprecated` should be created and used instead of using `@deprecated`? I can't seems to find that tag is being used in lucene actually...I think `@lucene.deprecated` most likely won't be recognized by IDE or build tool to signal / warn the use of deprecated methods though. > Is your reasoning behind keeping this in there for now that all of our uses of this (internal to Lucene) haven't yet migrated as part of this change? Would the plan me to migrate all usages off of this internally and then actually remove it on main/9.0, or are you thinking of keeping it around until 10.0? I think our backwards compatibility policy is such that we could just directly remove this on main/9.0, but then leave it like you have it (marked deprecated) if you choose to backport this to 8x. Since this method is so fundamental though, I could easily see an argument to keep it around for an extra major release to give users more time to migrate. Then again, the migration path seems pretty straight-forward. What do you think? As explained in https://issues.apache.org/jira/browse/LUCENE-10002?focusedCommentId=17397827&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-17397827, I use deprecation instead of direct removal in this PR exactly for the reasons you mentioned: 1. Not all internal uses have been migrated in this PR (which may require another few thousand lines of changes). 1. I personally feel that we should give users more time for migration and testing out the changes, so 10.0 release might be a good timing for complete removal. I would be interested in seeing how folks feel about the timing of removal, but after #1 is completed, complete removal of IndexSearcher#search(Query, Collector) in lucene codebase should require only straightforward deletion changes. ########## File path: lucene/core/src/java/org/apache/lucene/search/IndexSearcher.java ########## @@ -659,9 +614,12 @@ public TopFieldDocs reduce(Collection<TopFieldCollector> collectors) throws IOEx */ public <C extends Collector, T> T search(Query query, CollectorManager<C, T> collectorManager) throws IOException { - if (executor == null || leafSlices.length <= 1) { + if (executor == null || leafSlices.length == 0) { Review comment: Technically speaking in this block, `collectorManager#newCollector` & `collectorManager#reduce` etc are still used. But I couldn't remember why I made this change exactly (maybe was just trying to still use the thread pool executor to process this case as well), so I reverted this change. ########## File path: lucene/core/src/java/org/apache/lucene/search/TopFieldCollector.java ########## @@ -383,9 +382,13 @@ protected void updateMinCompetitiveScore(Scorable scorer) throws IOException { * count of the result will be accurate. {@link Integer#MAX_VALUE} may be used to make the hit * count accurate, but this will also make query processing slower. * @return a {@link TopFieldCollector} instance which will sort the results by the sort criteria. + * @deprecated This method is being deprecated in favor of using the constructor of {@link + * TopFieldCollectorManager} due to its support for concurrency in IndexSearcher */ + @Deprecated Review comment: Please see above. ########## File path: lucene/core/src/java/org/apache/lucene/search/TopFieldCollector.java ########## @@ -407,97 +410,14 @@ public static TopFieldCollector create(Sort sort, int numHits, int totalHitsThre * field is indexed both with doc values and points. In this case, there is an assumption that * the same data is stored in these points and doc values. * @return a {@link TopFieldCollector} instance which will sort the results by the sort criteria. + * @deprecated This method is being deprecated in favor of using the constructor of {@link + * TopFieldCollectorManager} due to its support for concurrency in IndexSearcher */ + @Deprecated Review comment: Please see above. ########## File path: lucene/test-framework/src/java/org/apache/lucene/util/FixedBitSetCollector.java ########## @@ -0,0 +1,68 @@ +/* + * 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.util; + +import java.util.Collection; +import org.apache.lucene.index.LeafReaderContext; +import org.apache.lucene.search.CollectorManager; +import org.apache.lucene.search.ScoreMode; +import org.apache.lucene.search.SimpleCollector; + +/** Test utility collector that uses FixedBitSet to record hits. */ +public class FixedBitSetCollector extends SimpleCollector { + private FixedBitSet hits; Review comment: Updated. ########## File path: lucene/test-framework/src/java/org/apache/lucene/util/FixedBitSetCollector.java ########## @@ -0,0 +1,68 @@ +/* + * 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.util; + +import java.util.Collection; +import org.apache.lucene.index.LeafReaderContext; +import org.apache.lucene.search.CollectorManager; +import org.apache.lucene.search.ScoreMode; +import org.apache.lucene.search.SimpleCollector; + +/** Test utility collector that uses FixedBitSet to record hits. */ +public class FixedBitSetCollector extends SimpleCollector { + private FixedBitSet hits; + private int docBase; + + public FixedBitSetCollector(int maxDoc) { + hits = new FixedBitSet(maxDoc); + } + + @Override + public ScoreMode scoreMode() { + return ScoreMode.COMPLETE_NO_SCORES; + } + + @Override + protected void doSetNextReader(LeafReaderContext context) { + docBase = context.docBase; + } + + @Override + public void collect(int doc) { + hits.set(docBase + doc); + } + + public FixedBitSet getHits() { + return hits; + } + + public static CollectorManager<FixedBitSetCollector, FixedBitSet> create(int maxDoc) { + return new CollectorManager<>() { + @Override + public FixedBitSetCollector newCollector() { + return new FixedBitSetCollector(maxDoc); + } + + @Override + public FixedBitSet reduce(Collection<FixedBitSetCollector> collectors) { + FixedBitSet result = new FixedBitSet(maxDoc); + collectors.stream().forEach(c -> result.or(c.getHits())); Review comment: You are right! Updated. ########## File path: lucene/test-framework/src/java/org/apache/lucene/util/FixedBitSetCollector.java ########## @@ -0,0 +1,68 @@ +/* + * 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.util; + +import java.util.Collection; +import org.apache.lucene.index.LeafReaderContext; +import org.apache.lucene.search.CollectorManager; +import org.apache.lucene.search.ScoreMode; +import org.apache.lucene.search.SimpleCollector; + +/** Test utility collector that uses FixedBitSet to record hits. */ +public class FixedBitSetCollector extends SimpleCollector { + private FixedBitSet hits; + private int docBase; + + public FixedBitSetCollector(int maxDoc) { + hits = new FixedBitSet(maxDoc); + } + + @Override + public ScoreMode scoreMode() { + return ScoreMode.COMPLETE_NO_SCORES; + } + + @Override + protected void doSetNextReader(LeafReaderContext context) { + docBase = context.docBase; + } + + @Override + public void collect(int doc) { + hits.set(docBase + doc); + } + + public FixedBitSet getHits() { + return hits; + } + + public static CollectorManager<FixedBitSetCollector, FixedBitSet> create(int maxDoc) { Review comment: Makes sense. Updated. ########## File path: lucene/core/src/java/org/apache/lucene/search/TopScoreDocCollectorManager.java ########## @@ -0,0 +1,142 @@ +/* + * 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.search; + +import java.io.IOException; +import java.util.Collection; + +/** + * Create a TopScoreDocCollectorManager which uses a shared hit counter to maintain number of hits + * and a shared {@link MaxScoreAccumulator} to propagate the minimum score across segments + * + * <p>Note that a new collectorManager should be created for each search due to its internal states. + */ +public class TopScoreDocCollectorManager + implements CollectorManager<TopScoreDocCollector, TopDocs> { + private final int numHits; + private final ScoreDoc after; + private final HitsThresholdChecker hitsThresholdChecker; + private final MaxScoreAccumulator minScoreAcc; + + /** + * Creates a new {@link TopScoreDocCollectorManager} given the number of hits to collect and the + * number of hits to count accurately. + * + * <p><b>NOTE</b>: If the total hit count of the top docs is less than or exactly {@code + * totalHitsThreshold} then this value is accurate. On the other hand, if the {@link + * TopDocs#totalHits} value is greater than {@code totalHitsThreshold} then its value is a lower + * bound of the hit count. A value of {@link Integer#MAX_VALUE} will make the hit count accurate + * but will also likely make query processing slower. + * + * <p><b>NOTE</b>: The instances returned by this method pre-allocate a full array of length + * <code>numHits</code>, and fill the array with sentinel objects. + * + * @param numHits the number of results to collect. + * @param after the previous doc after which matching docs will be collected. + * @param totalHitsThreshold the number of docs to count accurately. If the query matches more + * than {@code totalHitsThreshold} hits then its hit count will be a lower bound. On the other + * hand if the query matches less than or exactly {@code totalHitsThreshold} hits then the hit + * count of the result will be accurate. {@link Integer#MAX_VALUE} may be used to make the hit + * count accurate, but this will also make query processing slower. + * @param supportsConcurrency to use thread-safe and slower internal states for count tracking. + */ + public TopScoreDocCollectorManager( + int numHits, ScoreDoc after, int totalHitsThreshold, boolean supportsConcurrency) { Review comment: Yes the concern is indeed valid. Technically speaking this is probably an existing issue, as you can accidentally pass in not thread-safe internals to collectorManager to be used in a concurrent index searcher (the example below is thread-safe and internal to lucene's method, but the user can implement similar logic and accidentally pass in not thread-safe `HitsThresholdChecker` and nothing would check for thread-safety afterward) : https://github.com/apache/lucene/blob/efb7b2a5e8c1bdc19dfd65f7095f70a142343472/lucene/core/src/java/org/apache/lucene/search/IndexSearcher.java#L482-L510 On the other hand, not all collectorManager requires concurrency support, for example: https://github.com/apache/lucene/blob/efb7b2a5e8c1bdc19dfd65f7095f70a142343472/lucene/core/src/java/org/apache/lucene/search/IndexSearcher.java#L430-L447 However, given we are trying to promote IndexSearch#search(Query, CollectorManager) to be the main method for collection, and to guard against hard to debug concurrency related errors, I feel we should definitely do what you suggested. Maybe we can add a new method `supportsConcurrency` to CollectorManager's API like below to "force" the user to specify it, and to allow `IndexSearcher` to check for it: ``` public interface CollectorManager<C extends Collector, T> { C newCollector() throws IOException; T reduce(Collection<C> collectors) throws IOException; boolean supportsConcurrency(); } ``` What do you think? ########## File path: lucene/facet/src/java/org/apache/lucene/facet/DrillSideways.java ########## @@ -520,10 +458,39 @@ private DrillDownQuery getDrillDownQuery( } @SuppressWarnings("unchecked") - private <R> ConcurrentDrillSidewaysResult<R> searchSequentially( - final DrillDownQuery query, final CollectorManager<?, R> hitCollectorManager) + private <C extends Collector, R> ConcurrentDrillSidewaysResult<R> searchSequentially( + final DrillDownQuery query, final CollectorManager<C, R> hitCollectorManager) throws IOException { + // This mirrors a similar hack from DrillSideways#search(query, collector). + // Without this cache, LRU cache will be used, causing acceptDocs to be null during collection Review comment: Sorry about the typo there, I meant `Without this hack`. The caching decision happens here https://github.com/apache/lucene/blob/5896e5389a83f657781875a852120615ba4763dc/lucene/core/src/java/org/apache/lucene/search/IndexSearcher.java#L863-L866 so the hack of returning `ScoreMode.COMPLETE` basically would cause `scoreMode.needsScores() == true` in this logic. Without this hack, eventually this logic will be invoked, passing `null` for `acceptDocs`, causing some deleted docs to be collected as well. https://github.com/apache/lucene/blob/5896e5389a83f657781875a852120615ba4763dc/lucene/core/src/java/org/apache/lucene/search/LRUQueryCache.java#L509-L521 -- 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-unsubscr...@lucene.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org