Merge pull request #36299 from vespa-engine/arnej/allow-marking-hit-as-fulfilled

Add setUnfillable() API on Hit

Author: bratseth

container-search/abi-spec.json

@@ -8640,6 +8640,7 @@
       "public void setCached(boolean)",
       "public boolean isCached()",
       "public void setFillable()",
+      "public void setUnfillable()",
       "public void setFilled(java.lang.String)",
       "public boolean isFillable()",
       "public java.util.Set getFilled()",
@@ -8746,6 +8747,7 @@
       "public void analyze()",
       "public com.yahoo.search.result.HitGroup clone()",
       "public void setFillable()",
+      "public void setUnfillable()",
       "public void setFilled(java.lang.String)",
       "public boolean isFillable()",
       "public java.util.Set getFilled()",

container-search/src/main/java/com/yahoo/search/grouping/vespa/GroupingExecutor.java

@@ -154,11 +154,8 @@ public void fill(Result result, String summaryClass, Execution execution) {
             Hit hit = it.next();
             if (hit.getSearcherSpecificMetaData(this) instanceof String metaDataString) {
                 if (hit.isFilled(metaDataString)) {
-                    hit.setFilled(null);
-                    // TODO: the above should have been enough
-                    hit.setFilled(summaryClass);
-                    // we could in theory also clear SearcherSpecificMetaData here,
-                    // but there's no gain and some risk involved.
+                    // mark as fully filled:
+                    hit.setUnfillable();
                 }
             }
         }

container-search/src/main/java/com/yahoo/search/result/Hit.java

@@ -329,7 +329,8 @@ public void setRelevance(double relevance) {
      * summaries. This also enables tracking of which summary classes
      * have been used for filling so far. Invoking this method
      * multiple times is allowed and will have no addition
-     * effect. Note that a fillable hit may not be made unfillable.
+     * effect. Call setUnfillable() if there is no more data fill()
+     * can obtain for that hit.
      */
     public void setFillable() {
         if (filled == null) {
@@ -339,6 +340,14 @@ public void setFillable() {
         }
     }
 
+    /**
+     * Tag this hit as no longer fillable.
+     */
+    public void setUnfillable() {
+        filled = null;
+        unmodifiableFilled = null;
+    }
+
     /**
      * Register that this hit has been filled with properties using
      * the given summary class. Note that this method will implicitly
@@ -354,7 +363,6 @@ public void setFilled(String summaryClass) {
         } else if (filled.size() == 1) {
             filled = new HashSet<>(filled);
             unmodifiableFilled = Collections.unmodifiableSet(filled);
-
             filled.add(summaryClass);
         } else {
             filled.add(summaryClass);
@@ -600,7 +608,7 @@ public Object getSearcherSpecificMetaData(Searcher searcher) {
 
     final void setFilledInternal(Set<String> filled) {
         this.filled = filled;
-        unmodifiableFilled = (filled != null) ? Collections.unmodifiableSet(filled) : null;
+        unmodifiableFilled = (filled == null) ? null : Collections.unmodifiableSet(filled);
     }
 
     /**

container-search/src/main/java/com/yahoo/search/result/HitGroup.java

@@ -864,9 +864,16 @@ public HitGroup clone() {
         return hitGroupClone;
     }
 
+    /** Ignored as this should always be derived from the content hits */
     @Override
     public void setFillable() {}
 
+    /** This does not make sense for a HitGroup, must be done on the content hits */
+    @Override
+    public void setUnfillable() {
+        throw new IllegalArgumentException("calling setUnfillable() on a HitGroup does not make sense, it needs to be called on its individual hits");
+    }
+
     /** Ignored as this should always be derived from the content hits */
     @Override
     public void setFilled(String summaryClass) {}

container-search/src/test/java/com/yahoo/prelude/fastsearch/PartialSummaryHandlerTestCase.java

@@ -43,6 +43,14 @@ void testFillNull() {
         assertTrue(hit1.isFilled(null));
         assertTrue(hit1.isFilled("default"));
         assertEquals(1, result.hits().getFilled().size());
+        assertTrue(result.hits().isFilled(null));
+        assertFalse(result.hits().isFilled("foobar"));
+        hit2.setUnfillable();
+        assertTrue(result.hits().isFilled("default"));
+        assertTrue(result.hits().isFilled(null));
+        assertFalse(result.hits().isFilled("foobar"));
+        hit1.setUnfillable();
+        assertTrue(result.hits().isFilled("foobar"));
     }
 
     @Test