Introduce AllGroupHeadsCollectorManager#15565
Conversation
Signed-off-by: Binlong Gao <gbinlong@amazon.com>
Signed-off-by: Binlong Gao <gbinlong@amazon.com>
Signed-off-by: Binlong Gao <gbinlong@amazon.com>
|
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! |
|
Thanks for opening this PR @gaobinlong ! I had a quick look and my observations are similar to those I had at #15557 (comment) . We are introducing a new public collector manager that is only used in tests code, while the grouping collector code ( All in all an important longer term question is how are grouping users going to move from collectors to collector managers. Lucene should probably include the building blocks for them to do so, and we need to define which those are. |
Signed-off-by: Binlong Gao <gbinlong@amazon.com>
|
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>
Signed-off-by: Binlong Gao <gbinlong@amazon.com>
Signed-off-by: Binlong Gao <gbinlong@amazon.com>
javanna
left a comment
javanna
left a comment
There was a problem hiding this comment.
Thanks for working on this! I am unfamiliar with the grouping module and this collector, but I did a review round anyways. Hope this helps.
|
|
||
| private final GroupSelector<T> groupSelector; | ||
| protected final Sort sort; | ||
| protected final boolean fillSortValues; |
There was a problem hiding this comment.
I wonder if this belongs in the new collector manager or in the collector instances. Could we have mixed values for the different collectors created by the same collector manager?
There was a problem hiding this comment.
In collector manager, we always set fillSortValues to true when creating new collector, sort values is required when we merge across collectors.
Could we have mixed values for the different collectors created by the same collector manager?
I think no, the sort are same in all collectors.
There was a problem hiding this comment.
I see. I think I would unify the two paths and always fill sort values, considering that we are working on removing the search method that takes a collector. The only path in the future will be the concurrent one after all. I think that would simplify things.
Signed-off-by: Binlong Gao <gbinlong@amazon.com>
Signed-off-by: Binlong Gao <gbinlong@amazon.com>
javanna
left a comment
javanna
left a comment
There was a problem hiding this comment.
Thanks @gaobinlong I left a few more comments.
| s.search(new TermQuery(new Term("content", searchTerm)), allGroupHeadsCollector); | ||
| AllGroupHeadsCollectorManager<BytesRef> allGroupHeadsCollectorManager = | ||
| new AllGroupHeadsCollectorManager<>( | ||
| () -> new TermGroupSelector("group"), sortWithinGroup); |
There was a problem hiding this comment.
we previously were calling createRandomCollector with more randomization in the mix?
| @SuppressWarnings({"unchecked", "rawtypes"}) | ||
| private int compareValues(Object[] values1, Object[] values2, SortField[] sortFields) { | ||
| for (int i = 0; i < sortFields.length; i++) { | ||
| FieldComparator comparator = sortFields[i].getComparator(1, Pruning.NONE); |
There was a problem hiding this comment.
do we need a fresh new comparator per sort field, per group head? Couldn't we reuse the same instance?
|
|
||
| @Override | ||
| public GroupHeadsResult reduce(Collection<AllGroupHeadsCollector<T>> collectors) { | ||
| Map<Object, GroupHeadWithValues> mergedHeads = new HashMap<>(); |
There was a problem hiding this comment.
Map<T, GroupHeadWithValues> ?
|
|
||
| private final GroupSelector<T> groupSelector; | ||
| protected final Sort sort; | ||
| protected final boolean fillSortValues; |
There was a problem hiding this comment.
I see. I think I would unify the two paths and always fill sort values, considering that we are working on removing the search method that takes a collector. The only path in the future will be the concurrent one after all. I think that would simplify things.
2 participants
Description
AllGroupHeadsCollectorManager in grouping package did not have until now a corresponding collector manager, this pr introduces one and switches TestAllGroupHeadsCollector to use search concurrency and move away from the deprecated search(Query, Collector) method.
Relates to #12892.