SOLR-15047: Fix collapse parser behavior when collapsing on numeric fields to differentiate '0' group from null group

solr/CHANGES.txt

@@ -291,6 +291,8 @@ Bug Fixes
 * SOLR-15058: Enforce node_name contains colon and port and find first underscore after colon to parse context
   when converting a node_name to a base URL. (Timothy Potter, Su Sasa)
 
+* SOLR-15047: Fix collapse parser behavior when collapsing on numeric fields to differentiate '0' group from null group (hossman)
+
 Other Changes
 ---------------------
 

solr/core/src/java/org/apache/solr/search/CollapsingQParserPlugin.java

@@ -764,13 +764,11 @@ public class CollapsingQParserPlugin extends QParserPlugin {
     private int nullDoc = -1;
     private FloatArrayList nullScores;
     private String field;
-    private int nullValue;
 
     private final BoostedDocsCollector boostedDocsCollector;
     
     public IntScoreCollector(int maxDoc,
                              int segments,
-                             int nullValue,
                              int nullPolicy,
                              int size,
                              String field,
@@ -784,7 +782,6 @@ public class CollapsingQParserPlugin extends QParserPlugin {
       }
 
       this.collapsedSet = new FixedBitSet(maxDoc);
-      this.nullValue = nullValue;
       this.nullPolicy = nullPolicy;
       if(nullPolicy == NullPolicy.EXPAND.getCode()) {
         nullScores = new FloatArrayList();
@@ -806,23 +803,12 @@ public class CollapsingQParserPlugin extends QParserPlugin {
 
     @Override
     public void collect(int contextDoc) throws IOException {
-      int collapseValue;
+      final int globalDoc = docBase+contextDoc;
       if (collapseValues.advanceExact(contextDoc)) {
-        collapseValue = (int) collapseValues.longValue();
+        final int collapseValue = (int) collapseValues.longValue();
-      } else {
+        // Check to see if we have documents boosted by the QueryElevationComponent (skip normal strategy based collection)
-        collapseValue = 0;
-      }
-      int globalDoc = docBase+contextDoc;
-
-      // Check to see of we have documents boosted by the QueryElevationComponent
-      if (collapseValue != nullValue) {
         if (boostedDocsCollector.collectIfBoosted(collapseValue, globalDoc)) return;
-      } else {
-        if (boostedDocsCollector.collectInNullGroupIfBoosted(globalDoc)) return;
-      }
 
-
-      if(collapseValue != nullValue) {
         float score = scorer.score();
         final int idx;
         if((idx = cmap.indexOf(collapseValue)) >= 0) {
@@ -838,16 +824,24 @@ public class CollapsingQParserPlugin extends QParserPlugin {
           long scoreDoc = (((long)Float.floatToRawIntBits(score))<<32)+globalDoc;
           cmap.indexInsert(idx, collapseValue, scoreDoc);
         }
-      } else if(nullPolicy == NullPolicy.COLLAPSE.getCode()) {
+
-        float score = scorer.score();
+      } else { // Null Group...
-        if(score > this.nullScore) {
+        
-          this.nullScore = score;
+        // Check to see if we have documents boosted by the QueryElevationComponent (skip normal strategy based collection)
-          this.nullDoc = globalDoc;
+        if (boostedDocsCollector.collectInNullGroupIfBoosted(globalDoc)) return;
+
+        if(nullPolicy == NullPolicy.COLLAPSE.getCode()) {
+          float score = scorer.score();
+          if(score > this.nullScore) {
+            this.nullScore = score;
+            this.nullDoc = globalDoc;
+          }
+        } else if(nullPolicy == NullPolicy.EXPAND.getCode()) {
+          collapsedSet.set(globalDoc);
+          nullScores.add(scorer.score());
         }
-      } else if(nullPolicy == NullPolicy.EXPAND.getCode()) {
-        collapsedSet.set(globalDoc);
-        nullScores.add(scorer.score());
       }
+
     }
 
     @Override
@@ -895,24 +889,22 @@ public class CollapsingQParserPlugin extends QParserPlugin {
           collapseValues = DocValues.getNumeric(contexts[currentContext].reader(), this.field);
         }
 
-        int contextDoc = globalDoc-currentDocBase;
+        final int contextDoc = globalDoc-currentDocBase;
-        int collapseValue;
         if (collapseValues.advanceExact(contextDoc)) {
-          collapseValue = (int) collapseValues.longValue();
+          final int collapseValue = (int) collapseValues.longValue();
-        } else {
+          final long scoreDoc = cmap.get(collapseValue);
-          collapseValue = 0;
-        }
-
-        if(collapseValue != nullValue) {
-          long scoreDoc = cmap.get(collapseValue);
           dummy.score = Float.intBitsToFloat((int)(scoreDoc>>32));
-        } else if(mergeBoost.boost(globalDoc)) {
+          
-          //It's an elevated doc so no score is needed (and should not have been populated)
+        } else { // Null Group...
-          dummy.score = 0F;
+          
-        } else if (nullPolicy == NullPolicy.COLLAPSE.getCode()) {
+          if(mergeBoost.boost(globalDoc)) {
-          dummy.score = nullScore;
+            //It's an elevated doc so no score is needed (and should not have been populated)
-        } else if(nullPolicy == NullPolicy.EXPAND.getCode()) {
+            dummy.score = 0F;
-          dummy.score = nullScores.get(nullScoreIndex++);
+          } else if (nullPolicy == NullPolicy.COLLAPSE.getCode()) {
+            dummy.score = nullScore;
+          } else if(nullPolicy == NullPolicy.EXPAND.getCode()) {
+            dummy.score = nullScores.get(nullScoreIndex++);
+          }
         }
 
         dummy.docId = contextDoc;
@@ -1143,7 +1135,6 @@ public class CollapsingQParserPlugin extends QParserPlugin {
     private LeafReaderContext[] contexts;
     private NumericDocValues collapseValues;
     private int maxDoc;
-    private int nullValue;
     private int nullPolicy;
 
     private IntFieldValueStrategy collapseStrategy;
@@ -1156,7 +1147,6 @@ public class CollapsingQParserPlugin extends QParserPlugin {
     public IntFieldValueCollector(int maxDoc,
                                   int size,
                                   int segments,
-                                  int nullValue,
                                   int nullPolicy,
                                   String collapseField,
                                   GroupHeadSelector groupHeadSelector,
@@ -1177,7 +1167,6 @@ public class CollapsingQParserPlugin extends QParserPlugin {
         contexts[i] = con.get(i);
       }
       this.collapseField = collapseField;
-      this.nullValue = nullValue;
       this.nullPolicy = nullPolicy;
       this.needsScores4Collapsing = needsScores4Collapsing;
       this.needsScores = needsScores;
@@ -1185,19 +1174,19 @@ public class CollapsingQParserPlugin extends QParserPlugin {
       this.boostedDocsCollector = BoostedDocsCollector.build(boostDocsMap);
       
       if (null != sortSpec) {
-        this.collapseStrategy = new IntSortSpecStrategy(maxDoc, size, collapseField, nullValue, nullPolicy, groupHeadSelector, this.needsScores4Collapsing, this.needsScores, boostedDocsCollector, sortSpec, searcher);
+        this.collapseStrategy = new IntSortSpecStrategy(maxDoc, size, collapseField, nullPolicy, groupHeadSelector, this.needsScores4Collapsing, this.needsScores, boostedDocsCollector, sortSpec, searcher);
       } else if (funcQuery != null) {
-        this.collapseStrategy =  new IntValueSourceStrategy(maxDoc, size, collapseField, nullValue, nullPolicy, groupHeadSelector, this.needsScores4Collapsing, this.needsScores, boostedDocsCollector, funcQuery, searcher);
+        this.collapseStrategy =  new IntValueSourceStrategy(maxDoc, size, collapseField, nullPolicy, groupHeadSelector, this.needsScores4Collapsing, this.needsScores, boostedDocsCollector, funcQuery, searcher);
       } else {
         NumberType numType = fieldType.getNumberType();
         assert null != numType; // shouldn't make it here for non-numeric types
         switch (numType) {
           case INTEGER: {
-            this.collapseStrategy = new IntIntStrategy(maxDoc, size, collapseField, nullValue, nullPolicy, groupHeadSelector, this.needsScores, boostedDocsCollector);
+            this.collapseStrategy = new IntIntStrategy(maxDoc, size, collapseField, nullPolicy, groupHeadSelector, this.needsScores, boostedDocsCollector);
             break;
           }
           case FLOAT: {
-            this.collapseStrategy = new IntFloatStrategy(maxDoc, size, collapseField, nullValue, nullPolicy, groupHeadSelector, this.needsScores, boostedDocsCollector);
+            this.collapseStrategy = new IntFloatStrategy(maxDoc, size, collapseField, nullPolicy, groupHeadSelector, this.needsScores, boostedDocsCollector);
             break;
           }
           default: {
@@ -1223,23 +1212,23 @@ public class CollapsingQParserPlugin extends QParserPlugin {
     }
 
     public void collect(int contextDoc) throws IOException {
-      int collapseKey;
+      final int globalDoc = contextDoc+this.docBase;
       if (collapseValues.advanceExact(contextDoc)) {
-        collapseKey = (int) collapseValues.longValue();
+        final int collapseKey = (int) collapseValues.longValue();
-      } else {
+        // Check to see if we have documents boosted by the QueryElevationComponent (skip normal strategy based collection)
-        collapseKey = 0;
+        if (boostedDocsCollector.collectIfBoosted(collapseKey, globalDoc)) return;
+        collapseStrategy.collapse(collapseKey, contextDoc, globalDoc);
+        
+      } else { // Null Group...
+        
+        // Check to see if we have documents boosted by the QueryElevationComponent (skip normal strategy based collection)
+        if (boostedDocsCollector.collectInNullGroupIfBoosted(globalDoc)) return;
+
+        if (NullPolicy.IGNORE.getCode() != nullPolicy) {
+          collapseStrategy.collapseNullGroup(contextDoc, globalDoc);
+        }
       }
 
-      int globalDoc = contextDoc+this.docBase;
-      
-      // Check to see if we have documents boosted by the QueryElevationComponent (skip normal strategy based collection)
-      if (collapseKey == nullValue) {
-        if (boostedDocsCollector.collectInNullGroupIfBoosted(globalDoc)) return;
-      } else {
-        if (boostedDocsCollector.collectIfBoosted(collapseKey, globalDoc)) return;
-      }
-      
-      collapseStrategy.collapse(collapseKey, contextDoc, globalDoc);
     }
 
     public void finish() throws IOException {
@@ -1274,26 +1263,25 @@ public class CollapsingQParserPlugin extends QParserPlugin {
           this.collapseValues = DocValues.getNumeric(contexts[currentContext].reader(), this.collapseField);
         }
 
-        int contextDoc = globalDoc-currentDocBase;
+        final int contextDoc = globalDoc-currentDocBase;
 
         if(this.needsScores){
-          int collapseValue;
           if (collapseValues.advanceExact(contextDoc)) {
-            collapseValue = (int) collapseValues.longValue();
+            final int collapseValue = (int) collapseValues.longValue();
-          } else {
+            
-            collapseValue = 0;
+            final int pointer = cmap.get(collapseValue);
-          }
-          
-          if(collapseValue != nullValue) {
-            int pointer = cmap.get(collapseValue);
             dummy.score = scores.get(pointer);
-          } else if (mergeBoost.boost(globalDoc)) {
+
-            //Its an elevated doc so no score is needed
+          } else { // Null Group...
-            dummy.score = 0F;
+            
-          } else if (nullPolicy == NullPolicy.COLLAPSE.getCode()) {
+            if (mergeBoost.boost(globalDoc)) {
-            dummy.score = nullScore;
+              //It's an elevated doc so no score is needed (and should not have been populated)
-          } else if(nullPolicy == NullPolicy.EXPAND.getCode()) {
+              dummy.score = 0F;
-            dummy.score = nullScores.get(nullScoreIndex++);
+            } else if (nullPolicy == NullPolicy.COLLAPSE.getCode()) {
+              dummy.score = nullScore;
+            } else if(nullPolicy == NullPolicy.EXPAND.getCode()) {
+              dummy.score = nullScores.get(nullScoreIndex++);
+            }
           }
         }
 
@@ -1330,7 +1318,6 @@ public class CollapsingQParserPlugin extends QParserPlugin {
       FunctionQuery funcQuery = null;
 
       FieldType collapseFieldType = searcher.getSchema().getField(collapseField).getType();
-      String defaultValue = searcher.getSchema().getField(collapseField).getDefaultValue();
 
       if(collapseFieldType instanceof StrField) {
         if(HINT_TOP_FC.equals(hint)) {
@@ -1384,23 +1371,8 @@ public class CollapsingQParserPlugin extends QParserPlugin {
           return new OrdScoreCollector(maxDoc, leafCount, docValuesProducer, nullPolicy, boostDocs, searcher);
 
         } else if (isNumericCollapsible(collapseFieldType)) {
-
+          
-          int nullValue = 0;
+          return new IntScoreCollector(maxDoc, leafCount, nullPolicy, size, collapseField, boostDocs, searcher);
-
-          // must be non-null at this point
-          if (collapseFieldType.getNumberType().equals(NumberType.FLOAT)) {
-            if (defaultValue != null) {
-              nullValue = Float.floatToIntBits(Float.parseFloat(defaultValue));
-            } else {
-              nullValue = Float.floatToIntBits(0.0f);
-            }
-          } else {
-            if (defaultValue != null) {
-              nullValue = Integer.parseInt(defaultValue);
-            }
-          }
-
-          return new IntScoreCollector(maxDoc, leafCount, nullValue, nullPolicy, size, collapseField, boostDocs, searcher);
 
         } else {
           throw new SolrException(SolrException.ErrorCode.BAD_REQUEST,
@@ -1426,25 +1398,9 @@ public class CollapsingQParserPlugin extends QParserPlugin {
 
         } else if (isNumericCollapsible(collapseFieldType)) {
 
-          int nullValue = 0;
-
-          // must be non-null at this point
-          if (collapseFieldType.getNumberType().equals(NumberType.FLOAT)) {
-            if (defaultValue != null) {
-              nullValue = Float.floatToIntBits(Float.parseFloat(defaultValue));
-            } else {
-              nullValue = Float.floatToIntBits(0.0f);
-            }
-          } else {
-            if (defaultValue != null) {
-              nullValue = Integer.parseInt(defaultValue);
-            }
-          }
-
           return new IntFieldValueCollector(maxDoc,
                                             size,
                                             leafCount,
-                                            nullValue,
                                             nullPolicy,
                                             collapseField,
                                             groupHeadSelector,
@@ -2015,22 +1971,20 @@ public class CollapsingQParserPlugin extends QParserPlugin {
     protected boolean needsScores;
     protected String collapseField;
     protected IntIntDynamicMap docs;
-    protected int nullValue;
     
     private final BoostedDocsCollector boostedDocsCollector;
 
+    public abstract void collapseNullGroup(int contextDoc, int globalDoc) throws IOException;
     public abstract void collapse(int collapseKey, int contextDoc, int globalDoc) throws IOException;
     public abstract void setNextReader(LeafReaderContext context) throws IOException;
 
     public IntFieldValueStrategy(int maxDoc,
                                  int size,
                                  String collapseField,
-                                 int nullValue,
                                  int nullPolicy,
                                  boolean needsScores,
                                  BoostedDocsCollector boostedDocsCollector) {
       this.collapseField = collapseField;
-      this.nullValue = nullValue;
       this.nullPolicy = nullPolicy;
       this.needsScores = needsScores;
       this.collapsedSet = new FixedBitSet(maxDoc);
@@ -2109,13 +2063,12 @@ public class CollapsingQParserPlugin extends QParserPlugin {
     public IntIntStrategy(int maxDoc,
                           int size,
                           String collapseField,
-                          int nullValue,
                           int nullPolicy,
                           GroupHeadSelector groupHeadSelector,
                           boolean needsScores,
                           BoostedDocsCollector boostedDocsCollector) throws IOException {
 
-      super(maxDoc, size, collapseField, nullValue, nullPolicy, needsScores, boostedDocsCollector);
+      super(maxDoc, size, collapseField, nullPolicy, needsScores, boostedDocsCollector);
       this.field = groupHeadSelector.selectorText;
       this.testValues = new IntIntDynamicMap(size, 0);
 
@@ -2133,37 +2086,40 @@ public class CollapsingQParserPlugin extends QParserPlugin {
     public void setNextReader(LeafReaderContext context) throws IOException {
       this.minMaxVals = DocValues.getNumeric(context.reader(), this.field);
     }
-
+    private int advanceAndGetCurrentVal(int contextDoc) throws IOException {
-    public void collapse(int collapseKey, int contextDoc, int globalDoc) throws IOException {
-
-      int currentVal;
       if (minMaxVals.advanceExact(contextDoc)) {
-        currentVal = (int) minMaxVals.longValue();
+        return (int) minMaxVals.longValue();
-      } else {
+      } // else...
-        currentVal = 0;
+      return 0; 
-      }
+    }
+    public void collapse(int collapseKey, int contextDoc, int globalDoc) throws IOException {
+      final int currentVal = advanceAndGetCurrentVal(contextDoc);
 
-      if(collapseKey != nullValue) {
+      final int idx;
-        final int idx;
+      if((idx = cmap.indexOf(collapseKey)) >= 0) {
-        if((idx = cmap.indexOf(collapseKey)) >= 0) {
+        int pointer = cmap.indexGet(idx);
-          int pointer = cmap.indexGet(idx);
+        if(comp.test(currentVal, testValues.get(pointer))) {
-          if(comp.test(currentVal, testValues.get(pointer))) {
+          testValues.put(pointer, currentVal);
-            testValues.put(pointer, currentVal);
+          docs.put(pointer, globalDoc);
-            docs.put(pointer, globalDoc);
-            if(needsScores) {
-              scores.put(pointer, scorer.score());
-            }
-          }
-        } else {
-          ++index;
-          cmap.put(collapseKey, index);
-          testValues.put(index, currentVal);
-          docs.put(index, globalDoc);
           if(needsScores) {
-            scores.put(index, scorer.score());
+            scores.put(pointer, scorer.score());
           }
         }
-      } else if(this.nullPolicy == NullPolicy.COLLAPSE.getCode()) {
+      } else {
+        ++index;
+        cmap.put(collapseKey, index);
+        testValues.put(index, currentVal);
+        docs.put(index, globalDoc);
+        if(needsScores) {
+          scores.put(index, scorer.score());
+        }
+      }
+    }
+    public void collapseNullGroup(int contextDoc, int globalDoc) throws IOException {
+      assert NullPolicy.IGNORE.getCode() != this.nullPolicy;
+      
+      final int currentVal = advanceAndGetCurrentVal(contextDoc);
+      if (this.nullPolicy == NullPolicy.COLLAPSE.getCode()) {
         if(comp.test(currentVal, nullCompVal)) {
           nullCompVal = currentVal;
           nullDoc = globalDoc;
@@ -2193,13 +2149,12 @@ public class CollapsingQParserPlugin extends QParserPlugin {
     public IntFloatStrategy(int maxDoc,
                             int size,
                             String collapseField,
-                            int nullValue,
                             int nullPolicy,
                             GroupHeadSelector groupHeadSelector,
                             boolean needsScores,
                             BoostedDocsCollector boostedDocsCollector) throws IOException {
 
-      super(maxDoc, size, collapseField, nullValue, nullPolicy, needsScores, boostedDocsCollector);
+      super(maxDoc, size, collapseField, nullPolicy, needsScores, boostedDocsCollector);
       this.field = groupHeadSelector.selectorText;
       this.testValues = new IntFloatDynamicMap(size, 0.0f);
 
@@ -2217,39 +2172,40 @@ public class CollapsingQParserPlugin extends QParserPlugin {
     public void setNextReader(LeafReaderContext context) throws IOException {
       this.minMaxVals = DocValues.getNumeric(context.reader(), this.field);
     }
+    private float advanceAndGetCurrentVal(int contextDoc) throws IOException {
+      if (minMaxVals.advanceExact(contextDoc)) {
+        return Float.intBitsToFloat((int) minMaxVals.longValue());
+      } // else...
+      return Float.intBitsToFloat(0);
+    }
 
     public void collapse(int collapseKey, int contextDoc, int globalDoc) throws IOException {
+      final float currentVal = advanceAndGetCurrentVal(contextDoc);
 
-      int minMaxVal;
+      final int idx;
-      if (minMaxVals.advanceExact(contextDoc)) {
+      if((idx = cmap.indexOf(collapseKey)) >= 0) {
-        minMaxVal = (int) minMaxVals.longValue();
+        int pointer = cmap.indexGet(idx);
-      } else {
+        if(comp.test(currentVal, testValues.get(pointer))) {
-        minMaxVal = 0;
+          testValues.put(pointer, currentVal);
-      }
+          docs.put(pointer, globalDoc);
-
-      float currentVal = Float.intBitsToFloat(minMaxVal);
-
-      if(collapseKey != nullValue) {
-        final int idx;
-        if((idx = cmap.indexOf(collapseKey)) >= 0) {
-          int pointer = cmap.indexGet(idx);
-          if(comp.test(currentVal, testValues.get(pointer))) {
-            testValues.put(pointer, currentVal);
-            docs.put(pointer, globalDoc);
-            if(needsScores) {
-              scores.put(pointer, scorer.score());
-            }
-          }
-        } else {
-          ++index;
-          cmap.put(collapseKey, index);
-          testValues.put(index, currentVal);
-          docs.put(index, globalDoc);
           if(needsScores) {
-            scores.put(index, scorer.score());
+            scores.put(pointer, scorer.score());
           }
         }
-      } else if(this.nullPolicy == NullPolicy.COLLAPSE.getCode()) {
+      } else {
+        ++index;
+        cmap.put(collapseKey, index);
+        testValues.put(index, currentVal);
+        docs.put(index, globalDoc);
+        if(needsScores) {
+          scores.put(index, scorer.score());
+        }
+      }
+    }
+    public void collapseNullGroup(int contextDoc, int globalDoc) throws IOException {
+      assert NullPolicy.IGNORE.getCode() != this.nullPolicy;
+      final float currentVal = advanceAndGetCurrentVal(contextDoc);
+      if(this.nullPolicy == NullPolicy.COLLAPSE.getCode()) {
         if(comp.test(currentVal, nullCompVal)) {
           nullCompVal = currentVal;
           nullDoc = globalDoc;
@@ -2265,7 +2221,7 @@ public class CollapsingQParserPlugin extends QParserPlugin {
       }
     }
   }
-
+  
   /*
    *  Strategy for collapsing on a 32 bit numeric field and selecting the group head based
    *  on the min/max value of a Value Source Function.
@@ -2287,7 +2243,6 @@ public class CollapsingQParserPlugin extends QParserPlugin {
     public IntValueSourceStrategy(int maxDoc,
                                   int size,
                                   String collapseField,
-                                  int nullValue,
                                   int nullPolicy,
                                   GroupHeadSelector groupHeadSelector,
                                   boolean needsScores4Collapsing,
@@ -2296,7 +2251,7 @@ public class CollapsingQParserPlugin extends QParserPlugin {
                                   FunctionQuery funcQuery,
                                   IndexSearcher searcher) throws IOException {
 
-      super(maxDoc, size, collapseField, nullValue, nullPolicy, needsScores, boostedDocsCollector);
+      super(maxDoc, size, collapseField, nullPolicy, needsScores, boostedDocsCollector);
 
       this.needsScores4Collapsing = needsScores4Collapsing;
       this.testValues = new IntFloatDynamicMap(size, 0.0f);
@@ -2321,44 +2276,51 @@ public class CollapsingQParserPlugin extends QParserPlugin {
     public void setNextReader(LeafReaderContext context) throws IOException {
       functionValues = this.valueSource.getValues(rcontext, context);
     }
-
+    private float computeScoreIfNeeded4Collapse() throws IOException {
-    public void collapse(int collapseKey, int contextDoc, int globalDoc) throws IOException {
-      float score = 0;
-
       if (needsScores4Collapsing) {
-        score = scorer.score();
+        this.collapseScore.score = scorer.score();
-        this.collapseScore.score = score;
+        return this.collapseScore.score;
-      }
+      } // else...
+      return 0F;
+    }
+    public void collapse(int collapseKey, int contextDoc, int globalDoc) throws IOException {
+      
+      float score = computeScoreIfNeeded4Collapse();
+      final float currentVal = functionValues.floatVal(contextDoc);
 
-      float currentVal = functionValues.floatVal(contextDoc);
+      final int idx;
-
+      if((idx = cmap.indexOf(collapseKey)) >= 0) {
-      if(collapseKey != nullValue) {
+        int pointer = cmap.indexGet(idx);
-        final int idx;
+        if(comp.test(currentVal, testValues.get(pointer))) {
-        if((idx = cmap.indexOf(collapseKey)) >= 0) {
+          testValues.put(pointer, currentVal);
-          int pointer = cmap.indexGet(idx);
+          docs.put(pointer, globalDoc);
-          if(comp.test(currentVal, testValues.get(pointer))) {
+          if(needsScores){
-            testValues.put(pointer, currentVal);
-            docs.put(pointer, globalDoc);
-            if(needsScores){
-              if (!needsScores4Collapsing) {
-                score = scorer.score();
-              }
-              scores.put(pointer, score);
-            }
-          }
-        } else {
-          ++index;
-          cmap.put(collapseKey, index);
-          docs.put(index, globalDoc);
-          testValues.put(index, currentVal);
-          if(needsScores) {
             if (!needsScores4Collapsing) {
               score = scorer.score();
             }
-            scores.put(index, score);
+            scores.put(pointer, score);
           }
         }
-      } else if(this.nullPolicy == NullPolicy.COLLAPSE.getCode()) {
+      } else {
+        ++index;
+        cmap.put(collapseKey, index);
+        docs.put(index, globalDoc);
+        testValues.put(index, currentVal);
+        if(needsScores) {
+          if (!needsScores4Collapsing) {
+            score = scorer.score();
+          }
+          scores.put(index, score);
+        }
+      }
+    }
+    public void collapseNullGroup(int contextDoc, int globalDoc) throws IOException {
+      assert NullPolicy.IGNORE.getCode() != this.nullPolicy;
+      
+      float score = computeScoreIfNeeded4Collapse();
+      final float currentVal = functionValues.floatVal(contextDoc);
+
+      if(this.nullPolicy == NullPolicy.COLLAPSE.getCode()) {
         if(comp.test(currentVal, nullCompVal)) {
           nullCompVal = currentVal;
           nullDoc = globalDoc;
@@ -2398,7 +2360,6 @@ public class CollapsingQParserPlugin extends QParserPlugin {
     public IntSortSpecStrategy(int maxDoc,
                                int size,
                                String collapseField,
-                               int nullValue,
                                int nullPolicy,
                                GroupHeadSelector groupHeadSelector,
                                boolean needsScores4Collapsing,
@@ -2407,7 +2368,7 @@ public class CollapsingQParserPlugin extends QParserPlugin {
                                SortSpec sortSpec,
                                IndexSearcher searcher) throws IOException {
 
-      super(maxDoc, size, collapseField, nullValue, nullPolicy, needsScores, boostedDocsCollector);
+      super(maxDoc, size, collapseField, nullPolicy, needsScores, boostedDocsCollector);
       this.needsScores4Collapsing = needsScores4Collapsing;
 
       assert GroupHeadSelectorType.SORT.equals(groupHeadSelector.type);
@@ -2427,42 +2388,48 @@ public class CollapsingQParserPlugin extends QParserPlugin {
       super.setScorer(s);
       this.compareState.setScorer(s);
     }
+    
+    private float computeScoreIfNeeded4Collapse() throws IOException {
+      return needsScores4Collapsing ? scorer.score() : 0F;
+    }
 
     public void collapse(int collapseKey, int contextDoc, int globalDoc) throws IOException {
-      float score = 0;
+      float score = computeScoreIfNeeded4Collapse();
 
-      if (needsScores4Collapsing) {
+      final int idx;
-        score = scorer.score();
+      if ((idx = cmap.indexOf(collapseKey)) >= 0) {
-      }
+        // we've seen this collapseKey before, test to see if it's a new group leader
-
+        int pointer = cmap.indexGet(idx);
-      if (collapseKey != nullValue) {
+        if (compareState.testAndSetGroupValues(pointer, contextDoc)) {
-        final int idx;
+          docs.put(pointer, globalDoc);
-        if ((idx = cmap.indexOf(collapseKey)) >= 0) {
+          if (needsScores) {
-          // we've seen this collapseKey before, test to see if it's a new group leader
-          int pointer = cmap.indexGet(idx);
-          if (compareState.testAndSetGroupValues(pointer, contextDoc)) {
-            docs.put(pointer, globalDoc);
-            if (needsScores) {
-              if (!needsScores4Collapsing) {
-                score = scorer.score();
-              }
-              scores.put(pointer, score);
-            }
-          }
-        } else {
-          // we've never seen this collapseKey before, treat it as group head for now
-          ++index;
-          cmap.put(collapseKey, index);
-          docs.put(index, globalDoc);
-          compareState.setGroupValues(index, contextDoc);
-          if(needsScores) {
             if (!needsScores4Collapsing) {
               score = scorer.score();
             }
-            scores.put(index, score);
+            scores.put(pointer, score);
           }
         }
-      } else if(this.nullPolicy == NullPolicy.COLLAPSE.getCode()) {
+      } else {
+        // we've never seen this collapseKey before, treat it as group head for now
+        ++index;
+        cmap.put(collapseKey, index);
+        docs.put(index, globalDoc);
+        compareState.setGroupValues(index, contextDoc);
+        if(needsScores) {
+          if (!needsScores4Collapsing) {
+            score = scorer.score();
+          }
+          scores.put(index, score);
+        }
+      }
+    }
+
+    public void collapseNullGroup(int contextDoc, int globalDoc) throws IOException {
+      assert NullPolicy.IGNORE.getCode() != this.nullPolicy;
+      
+      float score = computeScoreIfNeeded4Collapse();
+
+      if(this.nullPolicy == NullPolicy.COLLAPSE.getCode()) {
         if (-1 == nullDoc) {
           // we've never seen a doc with null collapse key yet, treat it as the null group head for now
           compareState.setNullGroupValues(contextDoc);

solr/core/src/test/org/apache/solr/search/TestCollapseQParserPlugin.java

@@ -1060,24 +1060,28 @@ public class TestCollapseQParserPlugin extends SolrTestCaseJ4 {
         assertU(commit());
     }
     
-    for (String collapseField : new String[] {collapseFieldInt, collapseFieldFloat, collapseFieldString}) {
+    for (String collapseField : Arrays.asList(collapseFieldInt, collapseFieldFloat, collapseFieldString)) {
-      assertQ(req(
+      // all of our docs have a value in the collapse field(s) so the policy shouldn't matter...
-          "q", "{!cache=false}field_s:1",
+      for (String policy : Arrays.asList("", " nullPolicy=ignore", " nullPolicy=expand", " nullPolicy=collapse")) {
-          "rows", "1",
+        assertQ(req("q", "{!cache=false}field_s:1",
-          "minExactCount", "1",
+                    "rows", "1",
-          // this collapse will end up matching all docs
+                    "minExactCount", "1", // collapse should force this to be ignored
-          "fq", "{!collapse field=" + collapseField + " nullPolicy=expand}"// nullPolicy needed due to a bug when val=0
+                    // this collapse will end up creating a group for each matched doc
-          ),"//*[@numFoundExact='true']"
+                    "fq", "{!collapse field=" + collapseField + policy + "}"
-          ,"//*[@numFound='" + (numDocs/2) + "']"
+                    )
-          );
+                , "//*[@numFoundExact='true']"
+                , "//*[@numFound='" + (numDocs/2) + "']"
+                );
+      }
     }
   }
 
   public void testNullGroupNumericVsStringCollapse() throws Exception {
     // NOTE: group_i and group_s will contain identical content so these need to be "numbers"...
-    // The specific numbers shouldn't matter, but until SOLR-15047 is fixed, we can't use "0"...
+    // The specific numbers shouldn't matter (and we explicitly test '0' to confirm legacy bug/behavior
+    // of treating 0 as null is no longer a problem) ...
     final String A = "-1";
-    final String B = "42"; // TODO: switch to "0" once SOLR-15047 is fixed
+    final String B = "0"; 
     final String C = "1";
 
     // Stub out our documents.  From now on assume highest "id" of each group should be group head...