Introduce FirstPassGroupingCollectorManager#15574
Introduce FirstPassGroupingCollectorManager#15574gaobinlong wants to merge 3 commits intoapache:mainfrom
Conversation
|
This PR has not had activity in the past 2 weeks, labeling it as stale. If the PR is waiting for review, notify the dev@lucene.apache.org list. Thank you for your contribution! |
Signed-off-by: Binlong Gao <gbinlong@amazon.com> Format code Signed-off-by: Binlong Gao <gbinlong@amazon.com>
gaobinlong
force-pushed
the
firstPassManager
branch
from
82f9003 to
d40d5d5
Compare
Signed-off-by: Binlong Gao <gbinlong@amazon.com>
|
Hi @dweiss , mind taking a look at this PR? I think it's simple. |
javanna
left a comment
javanna
left a comment
There was a problem hiding this comment.
Thanks @gaobinlong , I left some comments!
| import org.apache.lucene.search.Sort; | ||
|
|
||
| /** A CollectorManager implementation for FirstPassGroupingCollector. */ | ||
| public class FirstPassGroupingCollectorManager<T> |
There was a problem hiding this comment.
given the corresponding collector is experimental, should this be too?
| return new FirstPassGroupingCollector<>( | ||
| new ValueSourceGroupSelector(vs, new HashMap<>()), groupSort, topDocs); | ||
| return new FirstPassGroupingCollectorManager<>( | ||
| () -> new ValueSourceGroupSelector(vs, new HashMap<>()), groupSort, topDocs) |
There was a problem hiding this comment.
should the hashmap be created for each supplier or shared across them?
| return new FirstPassGroupingCollector<>( | ||
| new ValueSourceGroupSelector(vs, new HashMap<>()), groupSort, topDocs); | ||
| return new FirstPassGroupingCollectorManager<>( | ||
| () -> new ValueSourceGroupSelector(vs, new HashMap<>()), groupSort, topDocs) |
There was a problem hiding this comment.
same as above regarding the map
|
|
||
| if (collectors.size() == 1) { | ||
| return collectors.iterator().next().getTopGroups(0); | ||
| } |
There was a problem hiding this comment.
I have a slight preference to remove these two above conditionals. I wonder how much value they bring.
| public Collection<SearchGroup<T>> reduce(Collection<FirstPassGroupingCollector<T>> collectors) | ||
| throws IOException { | ||
| if (collectors.isEmpty()) { | ||
| return null; |
There was a problem hiding this comment.
Returning null here could cause issues downstream.
| return SearchGroup.merge(allGroups, 0, topNGroups, groupSort); | ||
| } | ||
|
|
||
| public List<FirstPassGroupingCollector<T>> getCollectors() { |
There was a problem hiding this comment.
this looks unused, can we remove it?
| new TermGroupSelector(groupField), groupSort, topDocs); | ||
| return new FirstPassGroupingCollectorManager<>( | ||
| () -> new TermGroupSelector(groupField), groupSort, topDocs) | ||
| .newCollector(); |
There was a problem hiding this comment.
can usages like these also be replaced? Hopefully we will do a search in the test using the collector retrieved via the collector manager? Can we do the search providing the collector manager instead?
2 participants
Description
This pr introduces FirstPassGroupingCollectorManager and switches TestGrouping and BaseGroupSelectorTestCase to use search concurrency and move away from the deprecated search(Query, Collector) method.
Relates to #12892.