jainankitk commented on code in PR #15168:
URL: https://github.com/apache/lucene/pull/15168#discussion_r2338683632
##########
lucene/grouping/src/java/org/apache/lucene/search/grouping/FirstPassGroupingCollector.java:
##########
@@ -207,39 +206,44 @@ public void collect(int doc) throws IOException {
final CollectedSearchGroup<T> group = groupMap.get(groupValue);
if (group == null) {
+ collectNewGroup(doc);
+ } else {
+ collectExistingGroup(doc, group);
+ }
+ }
- // First time we are seeing this group, or, we've seen
- // it before but it fell out of the top N and is now
- // coming back
+ private void collectNewGroup(final int doc) throws IOException {
+ // First time we are seeing this group, or, we've seen
+ // it before but it fell out of the top N and is now
+ // coming back
- if (groupMap.size() < topNGroups) {
+ if (!isGroupMapFull()) {
- // Still in startup transient: we have not
- // seen enough unique groups to start pruning them;
- // just keep collecting them
+ // Still in startup transient: we have not
+ // seen enough unique groups to start pruning them;
+ // just keep collecting them
- // Add a new CollectedSearchGroup:
- CollectedSearchGroup<T> sg = new CollectedSearchGroup<>();
- sg.groupValue = groupSelector.copyValue();
- sg.comparatorSlot = groupMap.size();
- sg.topDoc = docBase + doc;
- for (LeafFieldComparator fc : leafComparators) {
- fc.copy(sg.comparatorSlot, doc);
- }
- groupMap.put(sg.groupValue, sg);
-
- if (groupMap.size() == topNGroups) {
- // End of startup transient: we now have max
- // number of groups; from here on we will drop
- // bottom group when we insert new one:
- buildSortedSet();
- }
+ // Add a new CollectedSearchGroup:
+ CollectedSearchGroup<T> sg = new CollectedSearchGroup<>();
+ sg.groupValue = groupSelector.copyValue();
+ sg.comparatorSlot = groupMap.size();
+ sg.topDoc = docBase + doc;
+ for (LeafFieldComparator fc : leafComparators) {
+ fc.copy(sg.comparatorSlot, doc);
+ }
+ groupMap.put(sg.groupValue, sg);
- return;
+ if (isGroupMapFull()) {
Review Comment:
nit: can we change this also to `isGroupMapFull() == true`?
##########
lucene/grouping/src/java/org/apache/lucene/search/grouping/FirstPassGroupingCollector.java:
##########
@@ -207,39 +206,44 @@ public void collect(int doc) throws IOException {
final CollectedSearchGroup<T> group = groupMap.get(groupValue);
if (group == null) {
+ collectNewGroup(doc);
+ } else {
+ collectExistingGroup(doc, group);
+ }
+ }
- // First time we are seeing this group, or, we've seen
- // it before but it fell out of the top N and is now
- // coming back
+ private void collectNewGroup(final int doc) throws IOException {
+ // First time we are seeing this group, or, we've seen
+ // it before but it fell out of the top N and is now
+ // coming back
- if (groupMap.size() < topNGroups) {
+ if (!isGroupMapFull()) {
Review Comment:
nit: by convention, lucene explicitly compares the boolean value. Can we
change to `isGroupMapFull() == false`?
--
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]
