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
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.
Signed-off-by: Binlong Gao <gbinlong@amazon.com>
Signed-off-by: Binlong Gao <gbinlong@amazon.com>
javanna
left a comment
There was a problem hiding this comment.
Thanks @gaobinlong I left a few more comments.
| @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?
There was a problem hiding this comment.
SortingGroupHead have comparator which can be reused but ScoringGroupHead not, at a second thought, I reverted this change back to the original version, that comparing values directly, please help to check.
Signed-off-by: Binlong Gao <gbinlong@amazon.com>
Sure, removed the fillSortValues variable yet. |
| cmp = values2[i] == null ? 0 : -1; | ||
| } else if (values2[i] == null) { | ||
| cmp = 1; | ||
| } else if (values1[i] instanceof Comparable) { |
There was a problem hiding this comment.
Is is possible in practice that values are not comparable? I wonder if it's ok to deal with that silently.
There was a problem hiding this comment.
According to this comment:
, the values maybe not comparable in a custom FieldComparator, so I think we need to cover this case, now change the code to expose comparators in GroupHead, use comparator.compareValues() instead for SortingGroupHead and compare float values directly for ScoringGroupHead, please help to review, thanks!
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
There was a problem hiding this comment.
Last round, few minor comments, thanks!
Signed-off-by: Binlong Gao <gbinlong@amazon.com>
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. --------- Signed-off-by: Binlong Gao <gbinlong@amazon.com>
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.