cpoerschke commented on a change in pull request #300: SOLR-11831: Skip second
grouping step if group.limit is 1 (aka Las Vegas Patch)
URL: https://github.com/apache/lucene-solr/pull/300#discussion_r349748411
##########
File path:
lucene/grouping/src/java/org/apache/lucene/search/grouping/FirstPassGroupingCollector.java
##########
@@ -132,17 +132,28 @@ public ScoreMode scoreMode() {
final Collection<SearchGroup<T>> result = new ArrayList<>();
int upto = 0;
final int sortFieldCount = comparators.length;
+ assert sortFieldCount > 0; // this must always be true because fields Sort
must contain at least a field
for(CollectedSearchGroup<T> group : orderedGroups) {
if (upto++ < groupOffset) {
continue;
}
// System.out.println(" group=" + (group.groupValue == null ? "null" :
group.groupValue.toString()));
SearchGroup<T> searchGroup = new SearchGroup<>();
searchGroup.groupValue = group.groupValue;
+ // We pass this around so that we can get the corresponding solr id when
serializing the search group to send to the federator
+ searchGroup.topDocLuceneId = group.topDoc;
searchGroup.sortValues = new Object[sortFieldCount];
for(int sortFieldIDX=0;sortFieldIDX<sortFieldCount;sortFieldIDX++) {
searchGroup.sortValues[sortFieldIDX] =
comparators[sortFieldIDX].value(group.comparatorSlot);
}
+ searchGroup.topDocScore = Float.NaN;
+ // if there is the score comparator we want to return the score
+ for (FieldComparator comparator: comparators){
+ if (comparator instanceof FieldComparator.RelevanceComparator){
+ searchGroup.topDocScore =
(Float)comparator.value(group.comparatorSlot);
+ }
Review comment:
The `searchGroup.topDocScore` being filled in conditionally here i.e. only
if it's a field comparator 'jumps out' a little.
Have not yet looked into what other comparators there are and if removing of
the conditional might be practical.
But this also re-surfaced the `SearchGroup.java` question around adding or
not adding additional fields and what (if any) alternative might be possible.
With this in mind
https://github.com/cpoerschke/lucene-solr/commits/github-bloomberg-SOLR-11831-cpoerschke-13
explores the 'extend FirstPassGroupingCollector and SearchGroup' route a
little further:
* presume LUCENE-8728 changes or equivalent are available
* add SolrFirstPassGroupingCollector and SolrSearchGroup classes
* on the `group.skip.second.step` code paths always have SolrSearchGroup
instead of SearchGroup
* not yet done:
* creation of SolrFirstPassGroupingCollector instead of
FirstPassGroupingCollector if-and-only-if appropriate
* tests need to pass again
* consider if this would actually be comprehensible, maintainable and
helpful e.g. w.r.t. (Solr)SearchGroup fields always being filled in.
What do you think?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
