javanna commented on code in PR #16247:
URL: https://github.com/apache/lucene/pull/16247#discussion_r3497834792
##########
lucene/CHANGES.txt:
##########
@@ -287,7 +287,8 @@ New Features
Improvements
---------------------
-(No changes)
+
+* GITHUB#16247: Introduce CachingCollectorManager to parallelize search when
using CachingCollector and remove useless GroupingSearch constructor (Binlong
Gao)
Review Comment:
Can you reword this to:
> Introduce CachingCollectorManager to parallelize search when using
CachingCollector.
> Remove experimental GroupingSearch constructor that takes a GroupSelector
as argument
> in favour of providing a GroupSelector supplier.
##########
lucene/grouping/src/test/org/apache/lucene/search/grouping/BaseGroupSelectorTestCase.java:
##########
@@ -437,6 +435,13 @@ public void testIgnoreDocsWithoutGroupField() throws
IOException {
shard.close();
}
+ protected static void assertScoreDocsEquals(ScoreDoc[] expected, ScoreDoc[]
actual) {
Review Comment:
This shadows the method with same name from the parent class. Is that on
purpose? Should we use a different name for it?
##########
lucene/grouping/src/java/org/apache/lucene/search/grouping/GroupingSearch.java:
##########
@@ -134,59 +138,82 @@ public <T> TopGroups<T> search(
}
@SuppressWarnings({"unchecked", "rawtypes"})
- protected TopGroups groupByFieldOrFunction(
+ protected <T> TopGroups<T> groupByFieldOrFunction(
IndexSearcher searcher, Query query, int groupOffset, int groupLimit)
throws IOException {
- int topN = groupOffset + groupLimit;
+ @SuppressWarnings("unchecked")
+ Supplier<GroupSelector<T>> typedGrouperFactory =
+ (Supplier<GroupSelector<T>>) (Supplier<?>) grouperFactory;
+ FirstPassGroupingCollectorManager<T> firstPassManager =
+ new FirstPassGroupingCollectorManager<>(
+ typedGrouperFactory, groupSort, groupOffset, groupLimit,
ignoreDocsWithoutGroupField);
+ List<CollectorManager<? extends Collector, ?>> firstRoundManagers = new
ArrayList<>();
+ firstRoundManagers.add(firstPassManager);
+ AllGroupsCollectorManager<T> allGroupsManager;
+ if (allGroups) {
+ allGroupsManager = new AllGroupsCollectorManager<>(typedGrouperFactory);
+ firstRoundManagers.add(allGroupsManager);
+ }
- final FirstPassGroupingCollector firstPassCollector =
- new FirstPassGroupingCollector(grouper, groupSort, topN,
ignoreDocsWithoutGroupField);
- final AllGroupsCollector allGroupsCollector =
- allGroups ? new AllGroupsCollector(grouper) : null;
- final AllGroupHeadsCollector allGroupHeadsCollector =
- allGroupHeads ? AllGroupHeadsCollector.newCollector(grouper,
sortWithinGroup) : null;
+ AllGroupHeadsCollectorManager<T> allGroupHeadsManager;
+ if (allGroupHeads) {
+ allGroupHeadsManager =
+ new AllGroupHeadsCollectorManager<>(typedGrouperFactory,
sortWithinGroup);
+ firstRoundManagers.add(allGroupHeadsManager);
+ }
- final Collector firstRound =
- MultiCollector.wrap(firstPassCollector, allGroupsCollector,
allGroupHeadsCollector);
+ CollectorManager<?, Object[]> firstRoundManager =
+ new
MultiCollectorManager(firstRoundManagers.toArray(CollectorManager[]::new));
- CachingCollector cachedCollector = null;
+ CachingCollectorManager<?, Object[]> cachingManager = null;
+ Object[] firstRoundResults;
if (maxCacheRAMMB != null || maxDocsToCache != null) {
- if (maxCacheRAMMB != null) {
- cachedCollector = CachingCollector.create(firstRound, cacheScores,
maxCacheRAMMB);
- } else {
- cachedCollector = CachingCollector.create(firstRound, cacheScores,
maxDocsToCache);
- }
- searcher.search(query, cachedCollector);
+ cachingManager =
+ new CachingCollectorManager<>(
+ firstRoundManager, cacheScores, maxCacheRAMMB, maxDocsToCache);
+ firstRoundResults = searcher.search(query, cachingManager);
} else {
- searcher.search(query, firstRound);
+ firstRoundResults = searcher.search(query, firstRoundManager);
+ }
+
+ int resultIdx = 0;
+ Collection<SearchGroup<T>> topSearchGroups =
+ (Collection<SearchGroup<T>>) firstRoundResults[resultIdx++];
+ if (topSearchGroups.isEmpty()) {
+ return new TopGroups<>(new SortField[0], new SortField[0], 0, 0, new
GroupDocs[0], Float.NaN);
Review Comment:
when we exit early in this scenario, matchingGroups and matchingGroupHeads
are not set. I suspect this will cause NPEs down the line for users relying on
those two being always non-null.
--
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]
