SOLR-8295: Fix NPE in collapse QParser when collapse field is missing from all docs in a segment

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

solr/CHANGES.txt

@@ -394,6 +394,7 @@ Bug Fixes
 * SOLR-8284: JSON Facet API - fix NPEs when short form "sort:index" or "sort:count" 
   are used. (Michael Sun via yonik)
 
+* SOLR-8295: Fix NPE in collapse QParser when collapse field is missing from all docs in a segment (hossman)
 
 Optimizations
 ----------------------

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

@@ -822,7 +822,7 @@ public class CollapsingQParserPlugin extends QParserPlugin {
       int currentContext = 0;
       int currentDocBase = 0;
 
-      collapseValues = contexts[currentContext].reader().getNumericDocValues(this.field);
+      collapseValues = DocValues.getNumeric(contexts[currentContext].reader(), this.field);
       int nextDocBase = currentContext+1 < contexts.length ? contexts[currentContext+1].docBase : maxDoc;
       leafDelegate = delegate.getLeafCollector(contexts[currentContext]);
       DummyScorer dummy = new DummyScorer();
@@ -838,7 +838,7 @@ public class CollapsingQParserPlugin extends QParserPlugin {
           nextDocBase = currentContext+1 < contexts.length ? contexts[currentContext+1].docBase : maxDoc;
           leafDelegate = delegate.getLeafCollector(contexts[currentContext]);
           leafDelegate.setScorer(dummy);
-          collapseValues = contexts[currentContext].reader().getNumericDocValues(this.field);
+          collapseValues = DocValues.getNumeric(contexts[currentContext].reader(), this.field);
         }
 
         int contextDoc = globalDoc-currentDocBase;
@@ -1101,7 +1101,7 @@ public class CollapsingQParserPlugin extends QParserPlugin {
       this.contexts[context.ord] = context;
       this.docBase = context.docBase;
       this.collapseStrategy.setNextReader(context);
-      this.collapseValues = context.reader().getNumericDocValues(this.collapseField);
+      this.collapseValues = DocValues.getNumeric(context.reader(), this.collapseField);
     }
 
     public void collect(int contextDoc) throws IOException {
@@ -1117,7 +1117,7 @@ public class CollapsingQParserPlugin extends QParserPlugin {
 
       int currentContext = 0;
       int currentDocBase = 0;
-      this.collapseValues = contexts[currentContext].reader().getNumericDocValues(this.collapseField);
+      this.collapseValues = DocValues.getNumeric(contexts[currentContext].reader(), this.collapseField);
       int nextDocBase = currentContext+1 < contexts.length ? contexts[currentContext+1].docBase : maxDoc;
       leafDelegate = delegate.getLeafCollector(contexts[currentContext]);
       DummyScorer dummy = new DummyScorer();
@@ -1140,7 +1140,7 @@ public class CollapsingQParserPlugin extends QParserPlugin {
           nextDocBase = currentContext+1 < contexts.length ? contexts[currentContext+1].docBase : maxDoc;
           leafDelegate = delegate.getLeafCollector(contexts[currentContext]);
           leafDelegate.setScorer(dummy);
-          this.collapseValues = contexts[currentContext].reader().getNumericDocValues(this.collapseField);
+          this.collapseValues = DocValues.getNumeric(contexts[currentContext].reader(), this.collapseField);
         }
 
         int contextDoc = globalDoc-currentDocBase;

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

@@ -653,6 +653,26 @@ public class TestCollapseQParserPlugin extends SolrTestCaseJ4 {
         "//result/doc[1]/float[@name='id'][.='5.0']",
         "//result/doc[2]/float[@name='id'][.='1.0']");
     
+    // Test collapse using selector field in no docs
+    // tie selector in all of these cases, so index order applies
+    for (String selector : new String[] {
+        " min=bogus_ti ", " sort='bogus_ti asc' ",
+        " max=bogus_ti ", " sort='bogus_ti desc' ",
+        " min=bogus_tf ", " sort='bogus_tf asc' ",
+        " max=bogus_tf ", " sort='bogus_tf desc' ",
+        " sort='bogus_td asc' ", " sort='bogus_td desc' ",
+        " sort='bogus_s asc' ", " sort='bogus_s desc' ", 
+      }) {
+      params = new ModifiableSolrParams();
+      params.add("q", "*:*");
+      params.add("fq", "{!collapse field="+group + selector + hint+"}");
+      params.add("sort", "id asc");
+      assertQ(req(params),
+              "*[count(//doc)=2]",
+              "//result/doc[1]/float[@name='id'][.='1.0']",
+              "//result/doc[2]/float[@name='id'][.='5.0']");
+    }
+    
     // attempting to use cscore() in sort local param should fail
     assertQEx("expected error trying to sort on a function that includes cscore()",
               req(params("q", "{!func}sub(sub(test_tl,1000),id)",
@@ -773,6 +793,74 @@ public class TestCollapseQParserPlugin extends SolrTestCaseJ4 {
     assertQ(req(params), "*[count(//doc)=0]");
   }
 
+  public void testNoDocsHaveGroupField() throws Exception {
+    // as unlikely as this test seems, it's important for the possibility that a segment exists w/o
+    // any live docs that have DocValues for the group field -- ie: every doc in segment is in null group.
+    
+    assertU(adoc("id", "1", "group_s", "group1", "test_ti", "5", "test_tl", "10"));
+    assertU(commit());
+    assertU(adoc("id", "2", "group_s", "group1", "test_ti", "5", "test_tl", "1000"));
+    assertU(adoc("id", "3", "group_s", "group1", "test_ti", "5", "test_tl", "1000"));
+    assertU(adoc("id", "4", "group_s", "group1", "test_ti", "10", "test_tl", "100"));
+    //
+    assertU(adoc("id", "5", "group_s", "group2", "test_ti", "5", "test_tl", "10", "term_s", "YYYY"));
+    assertU(commit());
+    assertU(adoc("id", "6", "group_s", "group2", "test_ti", "5", "test_tl","1000"));
+    assertU(adoc("id", "7", "group_s", "group2", "test_ti", "5", "test_tl","1000", "term_s", "XXXX"));
+    assertU(adoc("id", "8", "group_s", "group2", "test_ti", "10","test_tl", "100"));
+    assertU(commit());
+    
+    // none of these grouping fields are in any doc
+    for (String group : new String[] {
+        "field=bogus_s", "field=bogus_s_dv",
+        "field=bogus_s hint=top_fc", // alternative docvalues codepath w/ hint
+        "field=bogus_s_dv hint=top_fc", // alternative docvalues codepath w/ hint
+        "field=bogus_ti", "field=bogus_tf" }) {
+      
+      // for any of these selectors, behavior of these checks should be consistent
+      for (String selector : new String[] {
+          "", " sort='score desc' ",
+          " min=test_ti ", " max=test_ti ", " sort='test_ti asc' ",  " sort='test_ti desc' ",
+          " min=test_tf ", " max=test_tf ", " sort='test_tf asc' ",  " sort='test_tf desc' ",
+          " sort='group_s asc' ",  " sort='group_s desc' ",
+          // fields that don't exist
+          " min=bogus_sort_ti ", " max=bogus_sort_ti ",
+          " sort='bogus_sort_ti asc' ",  " sort='bogus_sort_ti desc' ",
+          " sort='bogus_sort_s asc' ",  " sort='bogus_sort_s desc' ",
+        }) {
+          
+          
+        ModifiableSolrParams params = null;
+
+        // w/default nullPolicy, no groups found
+        params = new ModifiableSolrParams();
+        params.add("q", "*:*");
+        params.add("sort", "id desc");
+        params.add("fq", "{!collapse "+group+" "+selector+"}");
+        assertQ(req(params), "*[count(//doc)=0]");
+
+        // w/nullPolicy=expand, every doc found
+        params = new ModifiableSolrParams();
+        params.add("q", "*:*");
+        params.add("sort", "id desc");
+        params.add("fq", "{!collapse field="+group+" nullPolicy=expand "+selector+"}");
+        assertQ(req(params)
+                , "*[count(//doc)=8]"
+                ,"//result/doc[1]/float[@name='id'][.='8.0']"
+                ,"//result/doc[2]/float[@name='id'][.='7.0']"
+                ,"//result/doc[3]/float[@name='id'][.='6.0']"
+                ,"//result/doc[4]/float[@name='id'][.='5.0']"
+                ,"//result/doc[5]/float[@name='id'][.='4.0']"
+                ,"//result/doc[6]/float[@name='id'][.='3.0']"
+                ,"//result/doc[7]/float[@name='id'][.='2.0']"
+                ,"//result/doc[8]/float[@name='id'][.='1.0']"
+                );
+
+        
+      }
+    }
+  }
+
   public void testGroupHeadSelector() {
     GroupHeadSelector s;
     

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

@@ -160,49 +160,53 @@ public class TestRandomCollapseQParserPlugin extends SolrTestCaseJ4 {
                    "rows", "200",
                    "fq", ("{!collapse" + csize + nullPs +
                           " field="+collapseField+" sort='"+collapseSort+"'}"));
-        
-        final QueryResponse mainRsp = SOLR.query(SolrParams.wrapDefaults(collapseP, mainP));
 
-        for (SolrDocument doc : mainRsp.getResults()) {
+        try {
-          final Object groupHeadId = doc.getFieldValue("id");
+          final QueryResponse mainRsp = SOLR.query(SolrParams.wrapDefaults(collapseP, mainP));
-          final Object collapseVal = doc.getFieldValue(collapseField);
+
-          
+          for (SolrDocument doc : mainRsp.getResults()) {
-          if (null == collapseVal) {
+            final Object groupHeadId = doc.getFieldValue("id");
-            if (NULL_EXPAND.equals(nullPolicy)) {
+            final Object collapseVal = doc.getFieldValue(collapseField);
-              // nothing to check for this doc, it's in it's own group
+            
-              continue;
+            if (null == collapseVal) {
+              if (NULL_EXPAND.equals(nullPolicy)) {
+                // nothing to check for this doc, it's in it's own group
+                continue;
+              }
+              
+              assertFalse(groupHeadId + " has null collapseVal but nullPolicy==ignore; " + 
+                          "mainP: " + mainP + ", collapseP: " + collapseP,
+                          NULL_IGNORE.equals(nullPolicy));
             }
             
-            assertFalse(groupHeadId + " has null collapseVal but nullPolicy==ignore; " + 
+            // work arround for SOLR-8082...
-                        "mainP: " + mainP + ", collapseP: " + collapseP,
+            //
-                        NULL_IGNORE.equals(nullPolicy));
+            // what's important is that we already did the collapsing on the *real* collapseField
+            // to verify the groupHead returned is really the best our verification filter
+            // on docs with that value in a differnet ifeld containing the exact same values
+            final String checkField = collapseField.replace("float_dv", "float");
+            
+            final String checkFQ = ((null == collapseVal)
+                                    ? ("-" + checkField + ":[* TO *]")
+                                    : ("{!field f="+checkField+"}" + collapseVal.toString()));
+            
+            final SolrParams checkP = params("fq", checkFQ,
+                                             "rows", "1",
+                                             "sort", collapseSort);
+            
+            final QueryResponse checkRsp = SOLR.query(SolrParams.wrapDefaults(checkP, mainP));
+            
+            assertTrue("not even 1 match for sanity check query? expected: " + doc,
+                       ! checkRsp.getResults().isEmpty());
+            final SolrDocument firstMatch = checkRsp.getResults().get(0);
+            final Object firstMatchId = firstMatch.getFieldValue("id");
+            assertEquals("first match for filtered group '"+ collapseVal +
+                         "' not matching expected group head ... " +
+                         "mainP: " + mainP + ", collapseP: " + collapseP + ", checkP: " + checkP,
+                         groupHeadId, firstMatchId);
           }
-
+        } catch (Exception e) {
-          // work arround for SOLR-8082...
+          throw new RuntimeException("BUG using params: " + collapseP + " + " + mainP, e);
-          //
-          // what's important is that we already did the collapsing on the *real* collapseField
-          // to verify the groupHead returned is really the best our verification filter
-          // on docs with that value in a differnet ifeld containing the exact same values
-          final String checkField = collapseField.replace("float_dv", "float");
-          
-          final String checkFQ = ((null == collapseVal)
-                                  ? ("-" + checkField + ":[* TO *]")
-                                  : ("{!field f="+checkField+"}" + collapseVal.toString()));
-          
-          final SolrParams checkP = params("fq", checkFQ,
-                                           "rows", "1",
-                                           "sort", collapseSort);
-          
-          final QueryResponse checkRsp = SOLR.query(SolrParams.wrapDefaults(checkP, mainP));
-
-          assertTrue("not even 1 match for sanity check query? expected: " + doc,
-                     ! checkRsp.getResults().isEmpty());
-          final SolrDocument firstMatch = checkRsp.getResults().get(0);
-          final Object firstMatchId = firstMatch.getFieldValue("id");
-          assertEquals("first match for filtered group '"+ collapseVal +
-                       "' not matching expected group head ... " +
-                       "mainP: " + mainP + ", collapseP: " + collapseP + ", checkP: " + checkP,
-                       groupHeadId, firstMatchId);
         }
       }
     }