mikemccand commented on code in PR #240:
URL: https://github.com/apache/lucene/pull/240#discussion_r1386407230
##########
lucene/core/src/java/org/apache/lucene/search/IndexSearcher.java:
##########
@@ -602,34 +579,10 @@ private TopFieldDocs searchAfter(
final Sort rewrittenSort = sort.rewrite(this);
final LeafSlice[] leafSlices = getSlices();
+ final boolean supportsConcurrency = leafSlices.length > 1;
final CollectorManager<TopFieldCollector, TopFieldDocs> manager =
- new CollectorManager<>() {
-
- private final HitsThresholdChecker hitsThresholdChecker =
- leafSlices.length <= 1
- ? HitsThresholdChecker.create(Math.max(TOTAL_HITS_THRESHOLD,
numHits))
- :
HitsThresholdChecker.createShared(Math.max(TOTAL_HITS_THRESHOLD, numHits));
-
- private final MaxScoreAccumulator minScoreAcc =
- leafSlices.length <= 1 ? null : new MaxScoreAccumulator();
-
- @Override
- public TopFieldCollector newCollector() throws IOException {
- // TODO: don't pay the price for accurate hit counts by default
- return TopFieldCollector.create(
- rewrittenSort, cappedNumHits, after, hitsThresholdChecker,
minScoreAcc);
- }
-
- @Override
- public TopFieldDocs reduce(Collection<TopFieldCollector> collectors)
throws IOException {
- final TopFieldDocs[] topDocs = new TopFieldDocs[collectors.size()];
- int i = 0;
- for (TopFieldCollector collector : collectors) {
- topDocs[i++] = collector.topDocs();
- }
- return TopDocs.merge(rewrittenSort, 0, cappedNumHits, topDocs);
- }
- };
+ new TopFieldCollectorManager(
Review Comment:
Wonderful to see all this redundant code go away!
##########
lucene/core/src/java/org/apache/lucene/search/TopFieldCollector.java:
##########
@@ -429,106 +432,29 @@ 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
Review Comment:
Remove `being` and `using`? Makes the language more punchy (avoids passive
voice).
##########
lucene/core/src/java/org/apache/lucene/search/TopDocs.java:
##########
@@ -232,8 +232,8 @@ public static TopDocs merge(
/**
* Returns a new TopFieldDocs, containing topN results across the provided
TopFieldDocs, sorting
* by the specified {@link Sort}. Each of the TopDocs must have been sorted
by the same Sort, and
- * sort field values must have been filled (ie, <code>fillFields=true</code>
must be passed to
- * {@link TopFieldCollector#create}).
+ * sort field values must have been filled (ie, <code>fillFields=true</code>
must be passed to the
+ * constructor of {@link TopFieldCollectorManager}).
Review Comment:
Hmm I don't think I see a `boolean fillFields` parameter to
`TopFieldCollectorManager`?
##########
lucene/core/src/java/org/apache/lucene/search/TopScoreDocCollector.java:
##########
@@ -44,7 +43,7 @@ public void setScorer(Scorable scorer) throws IOException {
}
Review Comment:
Should we add a comment to the javadoc for this class that one should use
`TopScoreDocCollectorManager` instead of the static factory methods here? Is
it too soon to deprecate this class (so we can make it package private
eventually)?
##########
lucene/benchmark/src/java/org/apache/lucene/benchmark/byTask/tasks/SearchWithCollectorTask.java:
##########
@@ -45,20 +43,6 @@ public boolean withCollector() {
return true;
}
- @Override
- protected Collector createCollector() throws Exception {
Review Comment:
Hmm the purpose of this benchmark component is explicitly to test ones own
`Collector` impl -- which I think is no longer possible with this diff
(neutering it). Maybe we can move this to a `SearchWithCollectorManagerTask`
instead? Or we could defer this to a later PR -- this one is already great.
Progress not perfection (PNP)!
##########
lucene/core/src/test/org/apache/lucene/document/BaseSpatialTestCase.java:
##########
@@ -695,8 +695,8 @@ protected void verifyRandomDistanceQueries(IndexReader
reader, Object... shapes)
}
}
- private FixedBitSet searchIndex(IndexSearcher s, Query query, int maxDoc)
throws IOException {
- return s.search(query, FixedBitSetCollector.createManager(maxDoc));
+ private CollectorManager<FixedBitSetCollector, FixedBitSet> searchIndex(int
maxDoc) {
Review Comment:
Hmm rename method to `createCollectorManager` or so? It's no longer
searching.
##########
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:
> As for my comment about `@lucene.deprecated`, I was getting this mixed up
with something else. Apologies for any confusion!
Though, I think there are both `@deprecated` (javadoc tag) and `@Deprecated`
(Java class annotation) -- we do both of them typically (looks like you did
here @zacharymorn, great!).
##########
lucene/core/src/java/org/apache/lucene/search/TopFieldCollectorManager.java:
##########
@@ -0,0 +1,198 @@
+/*
+ * 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.ArrayList;
+import java.util.Collection;
+import java.util.List;
+
+/**
+ * Create a TopFieldCollectorManager which uses a shared hit counter to
maintain number of hits and
+ * a shared {@link MaxScoreAccumulator} to propagate the minimum score across
segments if the
+ * primary sort is by relevancy.
+ *
+ * <p>Note that a new collectorManager should be created for each search due
to its internal states.
+ */
+public class TopFieldCollectorManager implements
CollectorManager<TopFieldCollector, TopFieldDocs> {
+ private final Sort sort;
+ private final int numHits;
+ private final FieldDoc after;
+ private final HitsThresholdChecker hitsThresholdChecker;
+ private final MaxScoreAccumulator minScoreAcc;
+ private final List<TopFieldCollector> collectors;
+ private final boolean supportsConcurrency;
+ private boolean collectorCreated;
+
+ /**
+ * Creates a new {@link TopFieldCollectorManager} from the given arguments.
+ *
+ * <p><b>NOTE</b>: The instances returned by this method pre-allocate a full
array of length
+ * <code>numHits</code>.
+ *
+ * @param sort the sort criteria (SortFields).
+ * @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 TopFieldCollectorManager(
+ Sort sort, int numHits, FieldDoc after, int totalHitsThreshold, boolean
supportsConcurrency) {
Review Comment:
I wonder what the cost is if you `supportsConcurrency=true` but use only a
single thread (single `Collector`), vs `supportsConcurrency=false`. No need to
answer this before merging this, just curious.
E.g. in `reduce` do we fastpath the case when there happens to be only one
`Collector`?
##########
lucene/core/src/java/org/apache/lucene/search/TopFieldCollectorManager.java:
##########
@@ -0,0 +1,198 @@
+/*
+ * 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.ArrayList;
+import java.util.Collection;
+import java.util.List;
+
+/**
+ * Create a TopFieldCollectorManager which uses a shared hit counter to
maintain number of hits and
+ * a shared {@link MaxScoreAccumulator} to propagate the minimum score across
segments if the
+ * primary sort is by relevancy.
+ *
+ * <p>Note that a new collectorManager should be created for each search due
to its internal states.
+ */
+public class TopFieldCollectorManager implements
CollectorManager<TopFieldCollector, TopFieldDocs> {
+ private final Sort sort;
+ private final int numHits;
+ private final FieldDoc after;
+ private final HitsThresholdChecker hitsThresholdChecker;
+ private final MaxScoreAccumulator minScoreAcc;
+ private final List<TopFieldCollector> collectors;
+ private final boolean supportsConcurrency;
+ private boolean collectorCreated;
+
+ /**
+ * Creates a new {@link TopFieldCollectorManager} from the given arguments.
+ *
+ * <p><b>NOTE</b>: The instances returned by this method pre-allocate a full
array of length
+ * <code>numHits</code>.
+ *
+ * @param sort the sort criteria (SortFields).
+ * @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 TopFieldCollectorManager(
+ Sort sort, int numHits, FieldDoc after, int totalHitsThreshold, boolean
supportsConcurrency) {
+ if (totalHitsThreshold < 0) {
+ throw new IllegalArgumentException(
+ "totalHitsThreshold must be >= 0, got " + totalHitsThreshold);
+ }
+
+ if (numHits <= 0) {
+ throw new IllegalArgumentException(
+ "numHits must be > 0; please use TotalHitCountCollector if you just
need the total hit count");
+ }
+
+ if (sort.getSort().length == 0) {
+ throw new IllegalArgumentException("Sort must contain at least one
field");
+ }
+
+ if (after != null) {
+ if (after.fields == null) {
+ throw new IllegalArgumentException(
+ "after.fields wasn't set; you must pass fillFields=true for the
previous search");
+ }
+
+ if (after.fields.length != sort.getSort().length) {
+ throw new IllegalArgumentException(
+ "after.fields has "
+ + after.fields.length
+ + " values but sort has "
+ + sort.getSort().length);
+ }
+ }
+
+ this.sort = sort;
+ this.numHits = numHits;
+ this.after = after;
+ this.supportsConcurrency = supportsConcurrency;
+ this.hitsThresholdChecker =
+ supportsConcurrency
+ ? HitsThresholdChecker.createShared(Math.max(totalHitsThreshold,
numHits))
+ : HitsThresholdChecker.create(Math.max(totalHitsThreshold,
numHits));
+ this.minScoreAcc = supportsConcurrency ? new MaxScoreAccumulator() : null;
+ this.collectors = new ArrayList<>();
+ }
+
+ /**
+ * Creates a new {@link TopFieldCollectorManager} from the given arguments,
with thread-safe
+ * internal states.
+ *
+ * <p><b>NOTE</b>: The instances returned by this method pre-allocate a full
array of length
+ * <code>numHits</code>.
+ *
+ * @param sort the sort criteria (SortFields).
+ * @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.
+ */
+ public TopFieldCollectorManager(Sort sort, int numHits, FieldDoc after, int
totalHitsThreshold) {
+ this(sort, numHits, after, totalHitsThreshold, true);
+ }
+
+ /**
+ * Creates a new {@link TopFieldCollectorManager} from the given arguments,
with thread-safe
+ * internal states.
+ *
+ * <p><b>NOTE</b>: The instances returned by this method pre-allocate a full
array of length
+ * <code>numHits</code>.
+ *
+ * @param sort the sort criteria (SortFields).
+ * @param numHits the number of results to collect.
+ * @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.
+ */
+ public TopFieldCollectorManager(Sort sort, int numHits, int
totalHitsThreshold) {
+ this(sort, numHits, null, totalHitsThreshold, true);
+ }
+
+ @Override
+ public TopFieldCollector newCollector() {
+ if (collectorCreated && supportsConcurrency == false) {
+ throw new IllegalStateException(
+ "The instantiated TopFieldCollectorManager does not support
concurrency, but multiple collectors are being created");
Review Comment:
Could we sharpen the message to `this TopFieldCollectorManager was created
without concurrency (supportsConcurrency=false), ...`? Make it clear which
ctor parameter caused this ...
##########
lucene/core/src/java/org/apache/lucene/search/TopFieldCollector.java:
##########
@@ -174,7 +173,7 @@ private static boolean canEarlyTerminateOnPrefix(Sort
searchSort, Sort indexSort
* Implements a TopFieldCollector over one SortField criteria, with tracking
Review Comment:
Same question for this class's ctor -- can we warn that one should not
extend it / use its factory methods but rather the `TopFieldCollectorManager`?
--
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
