LUCENE-6912: Grouping's Collectors now calculate a needsScores() instead of always 'true'.

In core, CachingCollector should have always returned true when scores are cached (small bug).

git-svn-id: https://svn.apache.org/repos/asf/lucene/dev/trunk@1718009 13f79535-47bb-0310-9956-ffa450edef68

lucene/CHANGES.txt

@@ -119,6 +119,9 @@ Optimizations
   particular to rewrite queries that look like: "+*:* #filter" to a
   "ConstantScore(filter)". (Adrien Grand)
 
+* LUCENE-6912: Grouping's Collectors now calculate a response to needsScores()
+  instead of always 'true'. (David Smiley)
+
 Bug Fixes
 
 * LUCENE-6918: LRUQueryCache.onDocIdSetEviction is only called when at least

lucene/core/src/java/org/apache/lucene/search/CachingCollector.java

@@ -99,6 +99,9 @@ public abstract class CachingCollector extends FilterCollector {
       return new NoScoreCachingLeafCollector(in, maxDocsToCache);
     }
 
+    // note: do *not* override needScore to say false. Just because we aren't caching the score doesn't mean the
+    //   wrapped collector doesn't need it to do its job.
+
     public LeafCollector getLeafCollector(LeafReaderContext context) throws IOException {
       postCollection();
       final LeafCollector in = this.in.getLeafCollector(context);
@@ -177,6 +180,13 @@ public abstract class CachingCollector extends FilterCollector {
       scores.add(coll.cachedScores());
     }
 
+    /** Ensure the scores are collected so they can be replayed, even if the wrapped collector doesn't need them. */
+    @Override
+    public boolean needsScores() {
+      return true;
+    }
+
+    @Override
     protected void collect(LeafCollector collector, int i) throws IOException {
       final int[] docs = this.docs.get(i);
       final float[] scores = this.scores.get(i);
@@ -189,7 +199,6 @@ public abstract class CachingCollector extends FilterCollector {
         collector.collect(scorer.doc);
       }
     }
-
   }
 
   private class NoScoreCachingLeafCollector extends FilterLeafCollector {

lucene/grouping/src/java/org/apache/lucene/search/grouping/AbstractAllGroupsCollector.java

@@ -61,4 +61,8 @@ public abstract class AbstractAllGroupsCollector<GROUP_VALUE_TYPE> extends Simpl
   @Override
   public void setScorer(Scorer scorer) throws IOException {}
 
+  @Override
+  public boolean needsScores() {
+    return false; // the result is unaffected by relevancy
+  }
 }

lucene/grouping/src/java/org/apache/lucene/search/grouping/AbstractDistinctValuesCollector.java

@@ -52,4 +52,9 @@ public abstract class AbstractDistinctValuesCollector<GC extends AbstractDistinc
     }
   }
 
+  @Override
+  public boolean needsScores() {
+    return false; // not needed to fetch all values
+  }
+
 }

lucene/grouping/src/java/org/apache/lucene/search/grouping/AbstractFirstPassGroupingCollector.java

@@ -39,6 +39,7 @@ abstract public class AbstractFirstPassGroupingCollector<GROUP_VALUE_TYPE> exten
   private final LeafFieldComparator[] leafComparators;
   private final int[] reversed;
   private final int topNGroups;
+  private final boolean needsScores;
   private final HashMap<GROUP_VALUE_TYPE, CollectedSearchGroup<GROUP_VALUE_TYPE>> groupMap;
   private final int compIDXEnd;
 
@@ -70,7 +71,7 @@ abstract public class AbstractFirstPassGroupingCollector<GROUP_VALUE_TYPE> exten
     // and specialize it?
 
     this.topNGroups = topNGroups;
-
+    this.needsScores = groupSort.needsScores();
     final SortField[] sortFields = groupSort.getSort();
     comparators = new FieldComparator[sortFields.length];
     leafComparators = new LeafFieldComparator[sortFields.length];
@@ -88,6 +89,11 @@ abstract public class AbstractFirstPassGroupingCollector<GROUP_VALUE_TYPE> exten
     groupMap = new HashMap<>(topNGroups);
   }
 
+  @Override
+  public boolean needsScores() {
+    return needsScores;
+  }
+
   /**
    * Returns top groups, starting from offset.  This may
    * return null, if no groups were collected, or if the

lucene/grouping/src/java/org/apache/lucene/search/grouping/AbstractGroupFacetCollector.java

@@ -109,6 +109,11 @@ public abstract class AbstractGroupFacetCollector extends SimpleCollector {
   public void setScorer(Scorer scorer) throws IOException {
   }
 
+  @Override
+  public boolean needsScores() {
+    return false;
+  }
+
   /**
    * The grouped facet result. Containing grouped facet entries, total count and total missing count.
    */

lucene/grouping/src/java/org/apache/lucene/search/grouping/AbstractSecondPassGroupingCollector.java

@@ -40,12 +40,14 @@ import java.util.Objects;
  */
 public abstract class AbstractSecondPassGroupingCollector<GROUP_VALUE_TYPE> extends SimpleCollector {
 
-  protected final Map<GROUP_VALUE_TYPE, SearchGroupDocs<GROUP_VALUE_TYPE>> groupMap;
-  private final int maxDocsPerGroup;
-  protected SearchGroupDocs<GROUP_VALUE_TYPE>[] groupDocs;
   private final Collection<SearchGroup<GROUP_VALUE_TYPE>> groups;
-  private final Sort withinGroupSort;
   private final Sort groupSort;
+  private final Sort withinGroupSort;
+  private final int maxDocsPerGroup;
+  private final boolean needsScores;
+  protected final Map<GROUP_VALUE_TYPE, SearchGroupDocs<GROUP_VALUE_TYPE>> groupMap;
+
+  protected SearchGroupDocs<GROUP_VALUE_TYPE>[] groupDocs;
 
   private int totalHitCount;
   private int totalGroupedHitCount;
@@ -59,12 +61,13 @@ public abstract class AbstractSecondPassGroupingCollector<GROUP_VALUE_TYPE> exte
       throw new IllegalArgumentException("no groups to collect (groups is empty)");
     }
 
+    this.groups = Objects.requireNonNull(groups);
     this.groupSort = Objects.requireNonNull(groupSort);
     this.withinGroupSort = Objects.requireNonNull(withinGroupSort);
-    this.groups = Objects.requireNonNull(groups);
     this.maxDocsPerGroup = maxDocsPerGroup;
-    this.groupMap = new HashMap<>(groups.size());
+    this.needsScores = getScores || getMaxScores || withinGroupSort.needsScores();
 
+    this.groupMap = new HashMap<>(groups.size());
     for (SearchGroup<GROUP_VALUE_TYPE> group : groups) {
       //System.out.println("  prep group=" + (group.groupValue == null ? "null" : group.groupValue.utf8ToString()));
       final TopDocsCollector<?> collector;
@@ -79,6 +82,11 @@ public abstract class AbstractSecondPassGroupingCollector<GROUP_VALUE_TYPE> exte
     }
   }
 
+  @Override
+  public boolean needsScores() {
+    return needsScores;
+  }
+
   @Override
   public void setScorer(Scorer scorer) throws IOException {
     for (SearchGroupDocs<GROUP_VALUE_TYPE> group : groupMap.values()) {

lucene/grouping/src/java/org/apache/lucene/search/grouping/function/FunctionAllGroupHeadsCollector.java

@@ -155,6 +155,6 @@ public class FunctionAllGroupHeadsCollector extends AbstractAllGroupHeadsCollect
 
   @Override
   public boolean needsScores() {
-    return true; // TODO, maybe we don't: e.g. return sortWithinGroup.needsScores()
+    return sortWithinGroup.needsScores();
   }
 }

lucene/grouping/src/java/org/apache/lucene/search/grouping/function/FunctionAllGroupsCollector.java

@@ -80,8 +80,4 @@ public class FunctionAllGroupsCollector extends AbstractAllGroupsCollector<Mutab
     mval = filler.getValue();
   }
 
-  @Override
-  public boolean needsScores() {
-    return true; // TODO, maybe we don't?
-  }
 }

lucene/grouping/src/java/org/apache/lucene/search/grouping/function/FunctionDistinctValuesCollector.java

@@ -90,8 +90,4 @@ public class FunctionDistinctValuesCollector extends AbstractDistinctValuesColle
 
   }
 
-  @Override
-  public boolean needsScores() {
-    return true; // TODO, maybe we don't?
-  }
 }

lucene/grouping/src/java/org/apache/lucene/search/grouping/function/FunctionFirstPassGroupingCollector.java

@@ -84,8 +84,4 @@ public class FunctionFirstPassGroupingCollector extends AbstractFirstPassGroupin
     mval = filler.getValue();
   }
 
-  @Override
-  public boolean needsScores() {
-    return true; // TODO, maybe we don't?
-  }
 }

lucene/grouping/src/java/org/apache/lucene/search/grouping/function/FunctionSecondPassGroupingCollector.java

@@ -78,8 +78,4 @@ public class FunctionSecondPassGroupingCollector extends AbstractSecondPassGroup
     mval = filler.getValue();
   }
 
-  @Override
-  public boolean needsScores() {
-    return true; // TODO, maybe we don't?
-  }
 }

lucene/grouping/src/java/org/apache/lucene/search/grouping/term/TermAllGroupHeadsCollector.java

@@ -170,6 +170,11 @@ public abstract class TermAllGroupHeadsCollector<GH extends AbstractAllGroupHead
       }
     }
 
+    @Override
+    public boolean needsScores() {
+      return sortWithinGroup.needsScores();
+    }
+
     @Override
     public void setScorer(Scorer scorer) throws IOException {
       this.scorer = scorer;
@@ -249,6 +254,11 @@ public abstract class TermAllGroupHeadsCollector<GH extends AbstractAllGroupHead
       return collectedGroups;
     }
 
+    @Override
+    public boolean needsScores() {
+      return true;
+    }
+
     @Override
     public void setScorer(Scorer scorer) throws IOException {
       this.scorer = scorer;
@@ -409,6 +419,11 @@ public abstract class TermAllGroupHeadsCollector<GH extends AbstractAllGroupHead
       return collectedGroups;
     }
 
+    @Override
+    public boolean needsScores() {
+      return false;
+    }
+
     @Override
     public void setScorer(Scorer scorer) throws IOException {
     }
@@ -540,6 +555,11 @@ public abstract class TermAllGroupHeadsCollector<GH extends AbstractAllGroupHead
       return collectedGroups;
     }
 
+    @Override
+    public boolean needsScores() {
+      return true;
+    }
+
     @Override
     public void setScorer(Scorer scorer) throws IOException {
       this.scorer = scorer;
@@ -627,8 +647,4 @@ public abstract class TermAllGroupHeadsCollector<GH extends AbstractAllGroupHead
 
   }
 
-  @Override
-  public boolean needsScores() {
-    return true; // TODO, maybe we don't?
-  }
 }

lucene/grouping/src/java/org/apache/lucene/search/grouping/term/TermAllGroupsCollector.java

@@ -118,8 +118,4 @@ public class TermAllGroupsCollector extends AbstractAllGroupsCollector<BytesRef>
     }
   }
 
-  @Override
-  public boolean needsScores() {
-    return true; // TODO, maybe we don't?
-  }
 }

lucene/grouping/src/java/org/apache/lucene/search/grouping/term/TermDistinctValuesCollector.java

@@ -141,8 +141,4 @@ public class TermDistinctValuesCollector extends AbstractDistinctValuesCollector
     }
   }
 
-  @Override
-  public boolean needsScores() {
-    return true; // TODO, maybe we don't?
-  }
 }

lucene/grouping/src/java/org/apache/lucene/search/grouping/term/TermFirstPassGroupingCollector.java

@@ -92,8 +92,4 @@ public class TermFirstPassGroupingCollector extends AbstractFirstPassGroupingCol
     index = DocValues.getSorted(readerContext.reader(), groupField);
   }
 
-  @Override
-  public boolean needsScores() {
-    return true; // TODO, maybe we don't?
-  }
 }

lucene/grouping/src/java/org/apache/lucene/search/grouping/term/TermGroupFacetCollector.java

@@ -76,11 +76,6 @@ public abstract class TermGroupFacetCollector extends AbstractGroupFacetCollecto
     segmentGroupedFacetHits = new SentinelIntSet(initialSize, Integer.MIN_VALUE);
   }
 
-  @Override
-  public boolean needsScores() {
-    return true; // TODO, maybe we don't?
-  }
-
   // Implementation for single valued facet fields.
   static class SV extends TermGroupFacetCollector {
 

lucene/grouping/src/java/org/apache/lucene/search/grouping/term/TermSecondPassGroupingCollector.java

@@ -78,8 +78,4 @@ public class TermSecondPassGroupingCollector extends AbstractSecondPassGroupingC
     return null;
   }
 
-  @Override
-  public boolean needsScores() {
-    return true; // TODO, maybe we don't?
-  }
 }

lucene/grouping/src/test/org/apache/lucene/search/grouping/TestGrouping.java

@@ -839,22 +839,9 @@ public class TestGrouping extends LuceneTestCase {
         final boolean getMaxScores = random().nextBoolean();
         final Sort groupSort = getRandomSort();
         //final Sort groupSort = new Sort(new SortField[] {new SortField("sort1", SortField.STRING), new SortField("id", SortField.INT)});
-        // TODO: also test null (= sort by relevance)
         final Sort docSort = getRandomSort();
         
-        for(SortField sf : docSort.getSort()) {
+        getScores |= (groupSort.needsScores() || docSort.needsScores());
-          if (sf.getType() == SortField.Type.SCORE) {
-            getScores = true;
-            break;
-          }
-        }
-        
-        for(SortField sf : groupSort.getSort()) {
-          if (sf.getType() == SortField.Type.SCORE) {
-            getScores = true;
-            break;
-          }
-        }
         
         final int topNGroups = TestUtil.nextInt(random(), 1, 30);
         //final int topNGroups = 10;
@@ -1172,7 +1159,7 @@ public class TestGrouping extends LuceneTestCase {
       System.out.println("TEST: " + subSearchers.length + " shards: " + Arrays.toString(subSearchers) + " canUseIDV=" + canUseIDV);
     }
     // Run 1st pass collector to get top groups per shard
-    final Weight w = topSearcher.createNormalizedWeight(query, true);
+    final Weight w = topSearcher.createNormalizedWeight(query, getScores);
     final List<Collection<SearchGroup<BytesRef>>> shardGroups = new ArrayList<>();
     List<AbstractFirstPassGroupingCollector<?>> firstPassGroupingCollectors = new ArrayList<>();
     AbstractFirstPassGroupingCollector<?> firstPassCollector = null;