javanna commented on code in PR #16247:
URL: https://github.com/apache/lucene/pull/16247#discussion_r3474950888
##########
lucene/grouping/src/test/org/apache/lucene/search/grouping/TestGroupingSearch.java:
##########
@@ -82,22 +82,22 @@ public void testBasic() throws Exception {
doc = new Document();
addGroupField(doc, groupField, "author2", canUseIDV);
doc.add(new TextField("content", "some random text", Field.Store.YES));
- doc.add(new Field("id", "4", customType));
+ doc.add(new Field("id", "3", customType));
doc.add(new StringField("groupend", "x", Field.Store.NO));
w.addDocument(doc);
// 4
doc = new Document();
addGroupField(doc, groupField, "author3", canUseIDV);
doc.add(new TextField("content", "some more random text",
Field.Store.YES));
- doc.add(new Field("id", "5", customType));
+ doc.add(new Field("id", "4", customType));
documents.add(doc);
// 5
doc = new Document();
addGroupField(doc, groupField, "author3", canUseIDV);
doc.add(new TextField("content", "random", Field.Store.YES));
- doc.add(new Field("id", "6", customType));
+ doc.add(new Field("id", "5", customType));
Review Comment:
sorry for being pedantic, but can we revert these changes and leave things
as they are? Or are these changes necessary? Can they otherwise be made as a
followup perhaps?
##########
lucene/CHANGES.txt:
##########
@@ -424,6 +424,8 @@ Improvements
* GITHUB#15660: Introduce LargeNumHitsTopDocsCollectorManager to parallelize
search when using LargeNumHitsTopDocsCollector. (Binlong Gao)
+* GITHUB#16247: Introduce CachingCollectorManager to parallelize search when
using CachingCollector. (Binlong Gao)
Review Comment:
Can you move this to the 10.6 section please, given 10.5 shipped?
##########
lucene/core/src/java/org/apache/lucene/search/CachingCollectorManager.java:
##########
@@ -0,0 +1,131 @@
+/*
+ * 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;
+
+/**
+ * A {@link CollectorManager} that wraps a delegate {@link CollectorManager}
and caches all
+ * collected documents (and optionally scores) per slice, so they can be
replayed to a second-pass
+ * {@link CollectorManager} without re-running the query.
+ *
+ * <p>One {@link CachingCollector} is created per slice. During {@link
#replay}, each cached slice
+ * is replayed into a fresh second-pass collector, and all second-pass
collectors are reduced
+ * together. This works correctly with both sequential and concurrent search.
+ *
+ * <p>Example usage:
+ *
+ * <pre class="prettyprint">
+ * CachingCollectorManager<C1, R1> caching = new
CachingCollectorManager<>(
+ * firstPassManager, cacheScores, maxRAMMB, null);
+ * R1 firstResult = searcher.search(query, caching);
+ *
+ * if (caching.isCached()) {
+ * R2 secondResult = caching.replay(secondPassManager);
+ * } else {
+ * // cache overflowed — re-run the query
+ * R2 secondResult = searcher.search(query, secondPassManager);
+ * }
+ * </pre>
+ *
+ * @lucene.experimental
+ */
+public class CachingCollectorManager<C extends Collector, R>
+ implements CollectorManager<CachingCollector, R> {
+
+ private final CollectorManager<C, R> delegate;
+ private final boolean cacheScores;
+ private final Double maxRAMMB;
+ private final Integer maxDocsToCache;
+
+ // One CachingCollector per slice
+ private final List<CachingCollector> cachingCollectors = new ArrayList<>();
+
+ /**
+ * @param delegate the first-pass {@link CollectorManager}
+ * @param cacheScores whether to cache scores in addition to document IDs
+ * @param maxRAMMB the maximum RAM in MB to use per slice cache, or null if
using maxDocsToCache
+ * @param maxDocsToCache the maximum number of documents to cache per slice,
or null if using
+ * maxRAMMB
+ */
+ public CachingCollectorManager(
+ CollectorManager<C, R> delegate,
+ boolean cacheScores,
+ Double maxRAMMB,
+ Integer maxDocsToCache) {
+ if (maxRAMMB == null && maxDocsToCache == null) {
+ throw new IllegalArgumentException("Either maxRAMMB or maxDocsToCache
must be set");
+ }
Review Comment:
should we also throw if both are non null given only one will be used and
the other silently ignored?
##########
lucene/core/src/test/org/apache/lucene/search/TestCachingCollectorManager.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.search;
+
+import java.io.IOException;
+import org.apache.lucene.document.Document;
+import org.apache.lucene.store.Directory;
+import org.apache.lucene.tests.index.RandomIndexWriter;
+import org.apache.lucene.tests.util.LuceneTestCase;
+
+public class TestCachingCollectorManager extends LuceneTestCase {
+
+ public void testCacheOverflow() throws IOException {
+ Directory dir = newDirectory();
+ RandomIndexWriter iw = new RandomIndexWriter(random(), dir);
+ for (int i = 0; i < atLeast(10); i++) {
+ iw.addDocument(new Document());
+ }
+ IndexSearcher searcher = newSearcher(iw.getReader());
+ iw.close();
+
+ CachingCollectorManager<TopScoreDocCollector, TopDocs> caching =
+ new CachingCollectorManager<>(
+ new TopScoreDocCollectorManager(10, Integer.MAX_VALUE), false,
null, 0);
+
+ searcher.search(MatchAllDocsQuery.INSTANCE, caching);
+ assertFalse(caching.isCached());
+ assertThrows(
+ IllegalStateException.class,
+ () -> caching.replay(new TopScoreDocCollectorManager(10,
Integer.MAX_VALUE)));
+
+ searcher.getIndexReader().close();
+ dir.close();
+ }
+
+ public void testNotCachedBeforeSearch() {
+ CachingCollectorManager<TopScoreDocCollector, TopDocs> caching =
+ new CachingCollectorManager<>(
+ new TopScoreDocCollectorManager(10, Integer.MAX_VALUE), false,
null, Integer.MAX_VALUE);
+ assertFalse(caching.isCached());
+
+ assertThrows(
+ IllegalStateException.class,
+ () -> caching.replay(new TopScoreDocCollectorManager(10,
Integer.MAX_VALUE)));
+ }
+
+ public void testConstructorValidation() {
+ assertThrows(
+ IllegalArgumentException.class,
+ () ->
+ new CachingCollectorManager<>(
+ new TopScoreDocCollectorManager(10, Integer.MAX_VALUE), false,
null, null));
+ }
Review Comment:
Sorry ,where? I mean, these tests never verify normal functioning of the
collector manager, calling search against it without exceptions. Or am I not
looking in the right place?
##########
lucene/grouping/src/java/org/apache/lucene/search/grouping/GroupingSearch.java:
##########
@@ -69,17 +73,17 @@ public class GroupingSearch {
* @param groupField The name of the field to group by.
*/
public GroupingSearch(String groupField) {
- this(new TermGroupSelector(groupField), null);
+ this(() -> new TermGroupSelector(groupField), null);
}
/**
* Constructs a <code>GroupingSearch</code> instance that groups documents
using a {@link
- * GroupSelector}
+ * GroupSelector} factory.
*
- * @param groupSelector a {@link GroupSelector} that defines groups for this
GroupingSearch
+ * @param grouperFactory a factory that creates fresh {@link GroupSelector}
instances
*/
- public GroupingSearch(GroupSelector<?> groupSelector) {
Review Comment:
Removing this constructor is a good call, like we previously discussed.
Could you perhaps mention this explicitly in the changes entry and the PR
description?
--
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]
