Merge pull request #4907 from vespa-engine/balder/fix-setting-of-top-aka-max-on-all

Propagate max() within nested all to propagate properly.

2 files changed, 61 insertions, 3 deletions

diff --git a/container-search/src/main/java/com/yahoo/search/grouping/vespa/RequestBuilder.java b/container-search/src/main/java/com/yahoo/search/grouping/vespa/RequestBuilder.java
index c6772f449b5..6a35e599eba 100644
--- a/container-search/src/main/java/com/yahoo/search/grouping/vespa/RequestBuilder.java
+++ b/container-search/src/main/java/com/yahoo/search/grouping/vespa/RequestBuilder.java
@@ -3,6 +3,7 @@ package com.yahoo.search.grouping.vespa;
 
 import com.yahoo.search.grouping.Continuation;
 import com.yahoo.search.grouping.GroupingRequest;
+import com.yahoo.search.grouping.request.AllOperation;
 import com.yahoo.search.grouping.request.EachOperation;
 import com.yahoo.search.grouping.request.GroupingExpression;
 import com.yahoo.search.grouping.request.GroupingOperation;
@@ -228,12 +229,14 @@ class RequestBuilder {
         }
     }
 
+    private long computeNewTopN(long oldMax, long newMax) {
+        return (oldMax < 0) ? newMax : Math.min(oldMax, newMax);
+    }
     private void resolveMax(BuildFrame frame) {
-
         if (frame.astNode.hasMax()) {
             int max = frame.astNode.getMax();
-            if (isRootOperation(frame)) {
-                frame.grouping.setTopN(max);
+            if (isTopNAllowed(frame)) {
+                frame.grouping.setTopN(computeNewTopN(frame.grouping.getTopN(), max));
             } else {
                 frame.state.max = max;
             }
@@ -325,6 +328,10 @@ class RequestBuilder {
         return frame.astNode == root && frame.state.groupBy == null;
     }
 
+    private boolean isTopNAllowed(BuildFrame frame) {
+        return (frame.astNode instanceof AllOperation) && (frame.state.groupBy == null);
+    }
+
     private GroupingLevel getLeafGroupingLevel(BuildFrame frame) {
         if (frame.grouping.getLevels().isEmpty()) {
             return null;
diff --git a/container-search/src/test/java/com/yahoo/search/grouping/vespa/RequestBuilderTestCase.java b/container-search/src/test/java/com/yahoo/search/grouping/vespa/RequestBuilderTestCase.java
index cf58dd3929b..33e8dbbbef1 100644
--- a/container-search/src/test/java/com/yahoo/search/grouping/vespa/RequestBuilderTestCase.java
+++ b/container-search/src/test/java/com/yahoo/search/grouping/vespa/RequestBuilderTestCase.java
@@ -40,6 +40,57 @@ public class RequestBuilderTestCase {
         assertEquals(new AttributeNode("foo"), aggr.getExpression());
     }
 
+    private List<Grouping> getRequestList(String selection) {
+        RequestBuilder builder = new RequestBuilder(0);
+        builder.setRootOperation(GroupingOperation.fromString(selection));
+        builder.build();
+        return builder.getRequestList();
+    }
+
+    @Test
+    public void requireThatTopNIsHonoured() {
+        List<Grouping> gl = getRequestList("all(max(3) all(group(product_id) max(5) each(output(sum(price)))))");
+        assertEquals(1, gl.size());
+        assertEquals(3, gl.get(0).getTopN());
+    }
+
+    @Test
+    public void requireThatTopNIsHonouredWhenNested() {
+        List<Grouping> gl = getRequestList("all( all(max(3) all(group(product_id) max(5) each(output(sum(price))))))");
+        assertEquals(1, gl.size());
+        assertEquals(3, gl.get(0).getTopN());
+    }
+
+    @Test
+    public void requireThatTopNIsInherited() {
+        List<Grouping> gl = getRequestList("all(max(7) all( all(group(product_id) max(5) each(output(sum(price))))))");
+        assertEquals(1, gl.size());
+        assertEquals(7, gl.get(0).getTopN());
+    }
+
+    @Test
+    public void requireThatTopNIsMinimum() {
+        List<Grouping> gl = getRequestList("all(max(7) all(max(3) all(group(product_id) max(5) each(output(sum(price))))))");
+        assertEquals(1, gl.size());
+        assertEquals(3, gl.get(0).getTopN());
+        gl = getRequestList("all(max(3) all(max(7) all(group(product_id) max(5) each(output(sum(price))))))");
+        assertEquals(1, gl.size());
+        assertEquals(3, gl.get(0).getTopN());
+    }
+
+    @Test
+    public void requireThatTopNIsIndividual() {
+        List<Grouping> gl = getRequestList("all( all(max(3) all(group(product_id) max(5) each(output(sum(price))))) all(group(filter_cluster3) order(count()) each(output(count()))))");
+        assertEquals(2, gl.size());
+        assertEquals(3, gl.get(0).getTopN());
+        assertEquals(-1, gl.get(1).getTopN());
+
+        gl = getRequestList("all( max(7) all(max(3) all(group(product_id) max(5) each(output(sum(price))))) all(group(filter_cluster3) order(count()) each(output(count()))))");
+        assertEquals(2, gl.size());
+        assertEquals(3, gl.get(0).getTopN());
+        assertEquals(7, gl.get(1).getTopN());
+    }
+
     @Test
     public void requireThatAllExpressionNodesAreSupported() {
         assertLayout("all(group(add(a,b)) each(output(count())))", "[[{ Add, result = [Count] }]]");