Introduce CachingCollectorManager to parallelize search when using CachingCollector by gaobinlong · Pull Request #16247 · apache/lucene

gaobinlong · 2026-06-11T14:02:18Z

Description

This PR introduces CachingCollectorManager, switches GroupingSearch to use search concurrency and move away from the deprecated search(Query, Collector) method.

In addition, the useless constructor GroupingSearch(GroupSelector<?> groupSelector) is removed.

Relates to #12892.

…chingCollector Signed-off-by: Binlong Gao <gbinlong@amazon.com>

Signed-off-by: Binlong Gao <gbinlong@amazon.com>

javanna

Great to see movement in this area, thanks for working on this @gaobinlong ! I left some comments

javanna · 2026-06-15T08:34:44Z

+  private final Integer maxDocsToCache;
+
+  // One CachingCollector per slice, thread-safe for concurrent newCollector() calls.
+  private final List<CachingCollector> cachingCollectors = new CopyOnWriteArrayList<>();


looks like this list is needed for the replay functionality. Note that like we discussed in other PRs, newCollector is never called concurrently. It is called sequentially by the coordinating thread which will also call reduce at the end.

What I do worry about when it comes to concurrency though is the isCached mutable flag, that gets modified by the worker threads and accessed by the main thread at the end. There isn't a concurrent access problem with it, but there may be visibility issues. The list does not need to handle concurrency though.

Signed-off-by: Binlong Gao <gbinlong@amazon.com>

gaobinlong · 2026-06-17T08:57:07Z

@javanna all comments are addressed yet, please help to review again, thanks!

javanna

a couple more comments, this is close! Thanks again!

javanna · 2026-06-19T15:44:57Z

+        () ->
+            new CachingCollectorManager<>(
+                new TopScoreDocCollectorManager(10, Integer.MAX_VALUE), false, null, null));
+  }


could we add also a test for the happy path ?

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?

Oh, sorry for the misunderstanding, added a new test method testBasic() to test the happy path, thanks!

…ager

Signed-off-by: Binlong Gao <gbinlong@amazon.com>

javanna

Thanks for all the work, I left a new batch of comments, I 'd expect these to be the last ones.

javanna · 2026-06-25T13:54:53Z

+        () ->
+            new CachingCollectorManager<>(
+                new TopScoreDocCollectorManager(10, Integer.MAX_VALUE), false, null, null));
+  }


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?

javanna · 2026-06-25T13:57:19Z

    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));


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?

javanna · 2026-06-25T13:57:40Z


 * GITHUB#15660: Introduce LargeNumHitsTopDocsCollectorManager to parallelize search when using LargeNumHitsTopDocsCollector. (Binlong Gao)

+* GITHUB#16247: Introduce CachingCollectorManager to parallelize search when using CachingCollector. (Binlong Gao)


Can you move this to the 10.6 section please, given 10.5 shipped?

javanna · 2026-06-25T14:04:55Z

+      Integer maxDocsToCache) {
+    if (maxRAMMB == null && maxDocsToCache == null) {
+      throw new IllegalArgumentException("Either maxRAMMB or maxDocsToCache must be set");
+    }


should we also throw if both are non null given only one will be used and the other silently ignored?

Changed that, throw exception if both are non-null.

javanna · 2026-06-25T14:08:18Z

-   * @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) {


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?

…ager

Signed-off-by: Binlong Gao <gbinlong@amazon.com>

gaobinlong added 2 commits June 11, 2026 21:59

Introduce CachingCollectorManager to parallelize search when using Ca…

0cde4ba

…chingCollector Signed-off-by: Binlong Gao <gbinlong@amazon.com>

Modify change log

7bc5217

Signed-off-by: Binlong Gao <gbinlong@amazon.com>

github-actions Bot added module:core/search module:grouping labels Jun 11, 2026

github-actions Bot added this to the 10.5.0 milestone Jun 11, 2026

javanna reviewed Jun 15, 2026

View reviewed changes

javanna mentioned this pull request Jun 15, 2026

Remove all deprecated IndexSearcher#search(Query, Collector) usage / methods in the next major release #12892

Open

19 tasks

Optimize code and fix bug

0466d2f

Signed-off-by: Binlong Gao <gbinlong@amazon.com>

github-actions Bot modified the milestones: 10.5.0, 11.0.0 Jun 17, 2026

Fix test issue

22efafa

github-actions Bot modified the milestones: 11.0.0, 10.5.0 Jun 17, 2026

gaobinlong commented Jun 17, 2026

View reviewed changes

Comment thread lucene/grouping/src/java/org/apache/lucene/search/grouping/AllGroupHeadsCollector.java

gaobinlong commented Jun 18, 2026

View reviewed changes

Comment thread lucene/grouping/src/test/org/apache/lucene/search/grouping/BaseGroupSelectorTestCase.java

javanna reviewed Jun 19, 2026

View reviewed changes

gaobinlong added 2 commits June 25, 2026 10:40

Merge remote-tracking branch 'upstream/main' into cachingCollectorMan…

55e79e2

…ager

Add more tests and fix some issue

dab05cc

Signed-off-by: Binlong Gao <gbinlong@amazon.com>

javanna modified the milestones: 10.5.0, 10.6.0 Jun 25, 2026

javanna reviewed Jun 25, 2026

View reviewed changes

gaobinlong added 3 commits June 26, 2026 21:37

Merge remote-tracking branch 'upstream/main' into cachingCollectorMan…

7666028

…ager

Add more tests and fix some issues

5d85a91

Signed-off-by: Binlong Gao <gbinlong@amazon.com>

Revert unnecessary change

4472875

Signed-off-by: Binlong Gao <gbinlong@amazon.com>


		* GITHUB#15660: Introduce LargeNumHitsTopDocsCollectorManager to parallelize search when using LargeNumHitsTopDocsCollector. (Binlong Gao)

		* GITHUB#16247: Introduce CachingCollectorManager to parallelize search when using CachingCollector. (Binlong Gao)

Uh oh!

Conversation

gaobinlong commented Jun 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

javanna left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gaobinlong commented Jun 17, 2026

Uh oh!

Uh oh!

javanna left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

javanna left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

gaobinlong commented Jun 11, 2026 •

edited

Loading