Merge pull request #2497 from yahoo/bratseth/multiple-fills-plus-grouping

Try to avoid ignoring users fill argument

5 files changed, 57 insertions, 41 deletions

diff --git a/container-search/src/main/java/com/yahoo/search/grouping/vespa/GroupingExecutor.java b/container-search/src/main/java/com/yahoo/search/grouping/vespa/GroupingExecutor.java
index 70942fa2553..0f1322c08d0 100644
--- a/container-search/src/main/java/com/yahoo/search/grouping/vespa/GroupingExecutor.java
+++ b/container-search/src/main/java/com/yahoo/search/grouping/vespa/GroupingExecutor.java
@@ -38,7 +38,7 @@ import com.yahoo.vespa.objects.ObjectPredicate;
  * transformation from the abstract request to Vespa grouping expressions (using {@link RequestBuilder}), and the
  * corresponding transformation of results (using {@link ResultBuilder}).
  *
- * @author <a href="mailto:simon@yahoo-inc.com">Simon Thoresen</a>
+ * @author Simon Thoresen
  */
 @After({ GroupingValidator.GROUPING_VALIDATED,
          "com.yahoo.search.querytransform.WandSearcher",
@@ -83,9 +83,9 @@ public class GroupingExecutor extends Searcher {
 
         // Convert requests to Vespa style grouping.
         Map<Integer, Grouping> groupingMap = new HashMap<>();
-        List<RequestContext> ctxList = new LinkedList<>();
+        List<RequestContext> requestContextList = new LinkedList<>();
         for (GroupingRequest grpRequest : reqList) {
-            ctxList.add(convertRequest(query, grpRequest, groupingMap));
+            requestContextList.add(convertRequest(query, grpRequest, groupingMap));
         }
         if (groupingMap.isEmpty()) {
             return execution.search(query);
@@ -96,10 +96,10 @@ public class GroupingExecutor extends Searcher {
 
         // Convert Vespa style results to hits.
         HitConverter hitConverter = new HitConverter(this, query);
-        for (RequestContext ctx : ctxList) {
-            RootGroup grp = convertResult(ctx, groupingMap, hitConverter);
-            ctx.request.setResultGroup(grp);
-            result.hits().add(grp);
+        for (RequestContext context : requestContextList) {
+            RootGroup group = convertResult(context, groupingMap, hitConverter);
+            context.request.setResultGroup(group);
+            result.hits().add(group);
         }
         return result;
     }
@@ -110,11 +110,21 @@ public class GroupingExecutor extends Searcher {
         for (Iterator<Hit> it = result.hits().unorderedDeepIterator(); it.hasNext(); ) {
             Hit hit = it.next();
             Object metaData = hit.getSearcherSpecificMetaData(this);
-            String hitSummary = (metaData instanceof String) ? (String)metaData : summaryClass;
-            Result summaryResult = summaryMap.get(hitSummary);
+            if (metaData != null && metaData instanceof String) {
+                // Use the summary class specified by grouping, set in HitConverter, for the first fill request
+                // after grouping. This assumes the first fill request is using the default summary class,
+                // which may be a fragile assumption. But currently we cannot do better because the difference
+                // between explicit and implicit summary class in fill is erased by the Execution.
+                // 
+                // We reset the summary class here such that following fill calls will execute with the
+                // summary class they specify
+                summaryClass = (String) metaData;
+                hit.setSearcherSpecificMetaData(this, null);
+            }
+            Result summaryResult = summaryMap.get(summaryClass);
             if (summaryResult == null) {
                 summaryResult = new Result(result.getQuery());
-                summaryMap.put(hitSummary, summaryResult);
+                summaryMap.put(summaryClass, summaryResult);
             }
             summaryResult.hits().add(hit);
         }
@@ -169,18 +179,18 @@ public class GroupingExecutor extends Searcher {
     /**
      * Converts the results of the given request context into a single {@link Group}.
      *
-     * @param requestCtx   The context that identifies the results to convert.
+     * @param requestContext   The context that identifies the results to convert.
      * @param groupingMap  The map of all {@link Grouping} objects available.
      * @param hitConverter The converter to use for {@link Hit} conversion.
      * @return The corresponding root RootGroup.
      */
-    private RootGroup convertResult(RequestContext requestCtx, Map<Integer, Grouping> groupingMap,
+    private RootGroup convertResult(RequestContext requestContext, Map<Integer, Grouping> groupingMap,
                                     HitConverter hitConverter) {
         ResultBuilder builder = new ResultBuilder();
         builder.setHitConverter(hitConverter);
-        builder.setTransform(requestCtx.transform);
-        builder.setRequestId(requestCtx.request.getRequestId());
-        for (Integer grpId : requestCtx.idList) {
+        builder.setTransform(requestContext.transform);
+        builder.setRequestId(requestContext.request.getRequestId());
+        for (Integer grpId : requestContext.idList) {
             builder.addGroupingResult(groupingMap.get(grpId));
         }
         builder.build();
diff --git a/container-search/src/main/java/com/yahoo/search/grouping/vespa/HitConverter.java b/container-search/src/main/java/com/yahoo/search/grouping/vespa/HitConverter.java
index 81ae100b84f..388f528b535 100644
--- a/container-search/src/main/java/com/yahoo/search/grouping/vespa/HitConverter.java
+++ b/container-search/src/main/java/com/yahoo/search/grouping/vespa/HitConverter.java
@@ -14,7 +14,7 @@ import com.yahoo.searchlib.aggregation.VdsHit;
 /**
  * Implementation of the {@link ResultBuilder.HitConverter} interface for {@link GroupingExecutor}.
  *
- * @author <a href="mailto:simon@yahoo-inc.com">Simon Thoresen</a>
+ * @author Simon Thoresen
  */
 class HitConverter implements ResultBuilder.HitConverter {
 
@@ -43,32 +43,32 @@ class HitConverter implements ResultBuilder.HitConverter {
         }
     }
 
-    private Hit convertFs4Hit(String summaryClass, FS4Hit grpHit) {
-        FastHit ret = new FastHit();
-        ret.setRelevance(grpHit.getRank());
-        ret.setGlobalId(grpHit.getGlobalId());
-        ret.setPartId(grpHit.getPath(), 0);
-        ret.setDistributionKey(grpHit.getDistributionKey());
-        ret.setFillable();
-        ret.setSearcherSpecificMetaData(searcher, summaryClass);
+    private Hit convertFs4Hit(String summaryClass, FS4Hit groupHit) {
+        FastHit hit = new FastHit();
+        hit.setRelevance(groupHit.getRank());
+        hit.setGlobalId(groupHit.getGlobalId());
+        hit.setPartId(groupHit.getPath(), 0);
+        hit.setDistributionKey(groupHit.getDistributionKey());
+        hit.setFillable();
+        hit.setSearcherSpecificMetaData(searcher, summaryClass);
 
-        Hit ctxHit = (Hit)grpHit.getContext();
+        Hit ctxHit = (Hit)groupHit.getContext();
         if (ctxHit == null) {
             throw new NullPointerException("Hit has no context.");
         }
-        ret.setSource(ctxHit.getSource());
-        ret.setSourceNumber(ctxHit.getSourceNumber());
-        ret.setQuery(ctxHit.getQuery());
+        hit.setSource(ctxHit.getSource());
+        hit.setSourceNumber(ctxHit.getSourceNumber());
+        hit.setQuery(ctxHit.getQuery());
 
         if (ctxHit instanceof GroupingListHit) {
             // in a live system the ctxHit can only by GroupingListHit, but because the code used Hit prior to version
             // 5.10 we need to check to avoid breaking existing unit tests -- both internally and with customers
             QueryPacketData queryPacketData = ((GroupingListHit)ctxHit).getQueryPacketData();
             if (queryPacketData != null) {
-                ret.setQueryPacketData(queryPacketData);
+                hit.setQueryPacketData(queryPacketData);
             }
         }
-        return ret;
+        return hit;
     }
 
     private Hit convertVdsHit(String summaryClass, VdsHit grpHit) {
diff --git a/container-search/src/main/java/com/yahoo/search/grouping/vespa/ResultBuilder.java b/container-search/src/main/java/com/yahoo/search/grouping/vespa/ResultBuilder.java
index 6caffc0d043..9b3601d35cb 100644
--- a/container-search/src/main/java/com/yahoo/search/grouping/vespa/ResultBuilder.java
+++ b/container-search/src/main/java/com/yahoo/search/grouping/vespa/ResultBuilder.java
@@ -52,7 +52,7 @@ import java.util.Map;
  * This class implements the necessary logic to build a {@link RootGroup} from a list of {@link Grouping} objects. It is
  * used by the {@link GroupingExecutor}.
  *
- * @author <a href="mailto:simon@yahoo-inc.com">Simon Thoresen</a>
+ * @author Simon Thoresen
  */
 class ResultBuilder {
 
@@ -383,7 +383,7 @@ class ResultBuilder {
      * Defines a helper interface to convert Vespa style grouping hits into corresponding instances of {@link Hit}. It
      * is an interface to simplify testing.
      *
-     * @author <a href="mailto:simon@yahoo-inc.com">Simon Thoresen</a>
+     * @author Simon Thoresen
      */
     public interface HitConverter {
 
diff --git a/container-search/src/main/java/com/yahoo/search/result/Hit.java b/container-search/src/main/java/com/yahoo/search/result/Hit.java
index 97c245ef5da..cfc7ba7bce7 100644
--- a/container-search/src/main/java/com/yahoo/search/result/Hit.java
+++ b/container-search/src/main/java/com/yahoo/search/result/Hit.java
@@ -709,10 +709,12 @@ public class Hit extends ListenableFreezableClass implements Data, Comparable<Hi
         }
     }
 
-    // TODO: Move out? If not, delegate here from subclass
     /**
+     * @deprecated do not use
+     * 
      * @return a field without bolding markup
      */
+    @Deprecated // TODO: Remove on Vespa 7
     public String getUnboldedField(String key, boolean escape) {
         Object p = getField(key);
 
@@ -732,7 +734,7 @@ public class Hit extends ListenableFreezableClass implements Data, Comparable<Hi
     }
 
     /**
-     * set meta data describing how a given searcher should treat this hit.
+     * Set meta data describing how a given searcher should treat this hit.
      * It is currently recommended that the invoker == searcher.
      * <b>Internal. Do not use!</b>
      */
diff --git a/container-search/src/test/java/com/yahoo/search/grouping/vespa/GroupingExecutorTestCase.java b/container-search/src/test/java/com/yahoo/search/grouping/vespa/GroupingExecutorTestCase.java
index 386e8346cae..8d524ae011e 100644
--- a/container-search/src/test/java/com/yahoo/search/grouping/vespa/GroupingExecutorTestCase.java
+++ b/container-search/src/test/java/com/yahoo/search/grouping/vespa/GroupingExecutorTestCase.java
@@ -53,16 +53,10 @@ import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
 
 /**
- * @author <a href="mailto:simon@yahoo-inc.com">Simon Thoresen</a>
+ * @author Simon Thoresen
  */
 public class GroupingExecutorTestCase {
 
-    // --------------------------------------------------------------------------------
-    //
-    // Tests
-    //
-    // --------------------------------------------------------------------------------
-
     @Test
     public void requireThatNullRequestsPass() {
         Result res = newExecution(new GroupingExecutor()).search(newQuery());
@@ -262,12 +256,22 @@ public class GroupingExecutorTestCase {
                                               new GroupingListHit(Arrays.asList(grp1), null))),
                                       new FillRequestThrower());
         Result res = exec.search(query);
+        
+        // Fill with summary specified in grouping
         try {
             exec.fill(res);
             fail();
         } catch (FillRequestException e) {
             assertEquals("bar", e.summaryClass);
         }
+
+        // Fill again, with another summary
+        try {
+            exec.fill(res, "otherSummary");
+            fail();
+        } catch (FillRequestException e) {
+            assertEquals("otherSummary", e.summaryClass);
+        }
     }
 
     @Test