Merge pull request #21898 from vespa-engine/bjorncs/grouping-default-precision-factor

Add query parameter for default precision factor

7 files changed, 86 insertions, 27 deletions

diff --git a/container-search/abi-spec.json b/container-search/abi-spec.json
index 7cc7168c79a..52a12f1e9a7 100644
--- a/container-search/abi-spec.json
+++ b/container-search/abi-spec.json
@@ -2347,7 +2347,9 @@
       "public static final com.yahoo.processing.request.CompoundName PARAM_REQUEST",
       "public static final com.yahoo.processing.request.CompoundName PARAM_TIMEZONE",
       "public static final com.yahoo.processing.request.CompoundName PARAM_DEFAULT_MAX_HITS",
-      "public static final com.yahoo.processing.request.CompoundName PARAM_DEFAULT_MAX_GROUPS"
+      "public static final com.yahoo.processing.request.CompoundName PARAM_DEFAULT_MAX_GROUPS",
+      "public static final com.yahoo.processing.request.CompoundName PARAM_DEFAULT_PRECISION_FACTOR",
+      "public static final com.yahoo.processing.request.CompoundName GROUPING_GLOBAL_MAX_GROUPS"
     ]
   },
   "com.yahoo.search.grouping.GroupingRequest": {
@@ -2371,6 +2373,7 @@
       "public void setDefaultMaxGroups(int)",
       "public java.util.OptionalLong globalMaxGroups()",
       "public void setGlobalMaxGroups(long)",
+      "public java.util.OptionalDouble defaultPrecisionFactor()",
       "public static com.yahoo.search.grouping.GroupingRequest newInstance(com.yahoo.search.Query)",
       "public java.lang.String toString()"
     ],
@@ -6632,7 +6635,6 @@
     "fields": [
       "public static final com.yahoo.processing.request.CompoundName MAX_OFFSET",
       "public static final com.yahoo.processing.request.CompoundName MAX_HITS",
-      "public static final com.yahoo.processing.request.CompoundName GROUPING_GLOBAL_MAX_GROUPS",
       "public static final com.yahoo.search.query.profile.types.QueryProfileType argumentType"
     ]
   },
diff --git a/container-search/src/main/java/com/yahoo/search/grouping/GroupingQueryParser.java b/container-search/src/main/java/com/yahoo/search/grouping/GroupingQueryParser.java
index ee78e41d0d8..36a8b7812ba 100644
--- a/container-search/src/main/java/com/yahoo/search/grouping/GroupingQueryParser.java
+++ b/container-search/src/main/java/com/yahoo/search/grouping/GroupingQueryParser.java
@@ -12,7 +12,6 @@ import com.yahoo.search.Result;
 import com.yahoo.search.Searcher;
 import com.yahoo.search.grouping.request.GroupingOperation;
 import com.yahoo.search.query.Select;
-import com.yahoo.search.query.properties.DefaultProperties;
 import com.yahoo.search.searchchain.Execution;
 import com.yahoo.search.searchchain.PhaseNames;
 
@@ -21,6 +20,9 @@ import java.util.LinkedHashMap;
 import java.util.LinkedList;
 import java.util.List;
 import java.util.Map;
+import java.util.OptionalDouble;
+import java.util.OptionalInt;
+import java.util.OptionalLong;
 import java.util.TimeZone;
 
 /**
@@ -41,13 +43,15 @@ public class GroupingQueryParser extends Searcher {
     public static final CompoundName PARAM_TIMEZONE = new CompoundName("timezone");
     @Beta public static final CompoundName PARAM_DEFAULT_MAX_HITS = new CompoundName("grouping.defaultMaxHits");
     @Beta public static final CompoundName PARAM_DEFAULT_MAX_GROUPS = new CompoundName("grouping.defaultMaxGroups");
+    @Beta public static final CompoundName PARAM_DEFAULT_PRECISION_FACTOR = new CompoundName("grouping.defaultPrecisionFactor");
+    @Beta public static final CompoundName GROUPING_GLOBAL_MAX_GROUPS = new CompoundName("grouping.globalMaxGroups");
     private static final ThreadLocal<ZoneCache> zoneCache = new ThreadLocal<>();
 
     @Override
     public Result search(Query query, Execution execution) {
         try {
-            if (query.getHttpRequest().getProperty(DefaultProperties.GROUPING_GLOBAL_MAX_GROUPS.toString()) != null) {
-                throw new IllegalInputException(DefaultProperties.GROUPING_GLOBAL_MAX_GROUPS + " must be specified in a query profile.");
+            if (query.getHttpRequest().getProperty(GROUPING_GLOBAL_MAX_GROUPS.toString()) != null) {
+                throw new IllegalInputException(GROUPING_GLOBAL_MAX_GROUPS + " must be specified in a query profile.");
             }
 
             String reqParam = query.properties().getString(PARAM_REQUEST);
@@ -60,9 +64,10 @@ public class GroupingQueryParser extends Searcher {
                 grpRequest.setRootOperation(op);
                 grpRequest.setTimeZone(zone);
                 grpRequest.continuations().addAll(continuations);
-                grpRequest.setDefaultMaxGroups(query.properties().getInteger(PARAM_DEFAULT_MAX_GROUPS, -1));
-                grpRequest.setDefaultMaxHits(query.properties().getInteger(PARAM_DEFAULT_MAX_HITS, -1));
-                grpRequest.setGlobalMaxGroups(query.properties().getLong(DefaultProperties.GROUPING_GLOBAL_MAX_GROUPS));
+                intProperty(query, PARAM_DEFAULT_MAX_GROUPS).ifPresent(grpRequest::setDefaultMaxGroups);
+                intProperty(query, PARAM_DEFAULT_MAX_HITS).ifPresent(grpRequest::setDefaultMaxHits);
+                longProperty(query, GROUPING_GLOBAL_MAX_GROUPS).ifPresent(grpRequest::setGlobalMaxGroups);
+                doubleProperty(query, PARAM_DEFAULT_PRECISION_FACTOR).ifPresent(grpRequest::setDefaultPrecisionFactor);
             }
             return execution.search(query);
         }
@@ -96,6 +101,21 @@ public class GroupingQueryParser extends Searcher {
         return timeZone;
     }
 
+    private static OptionalInt intProperty(Query q, CompoundName name) {
+        Integer val = q.properties().getInteger(name);
+        return val != null ? OptionalInt.of(val) : OptionalInt.empty();
+    }
+
+    private static OptionalLong longProperty(Query q, CompoundName name) {
+        Long val = q.properties().getLong(name);
+        return val != null ? OptionalLong.of(val) : OptionalLong.empty();
+    }
+
+    private static OptionalDouble doubleProperty(Query q, CompoundName name) {
+        Double val = q.properties().getDouble(name);
+        return val != null ? OptionalDouble.of(val) : OptionalDouble.empty();
+    }
+
     @SuppressWarnings("serial")
     private static class ZoneCache extends LinkedHashMap<String, TimeZone> {
 
diff --git a/container-search/src/main/java/com/yahoo/search/grouping/GroupingRequest.java b/container-search/src/main/java/com/yahoo/search/grouping/GroupingRequest.java
index 9f5deb482db..a211d4960f4 100644
--- a/container-search/src/main/java/com/yahoo/search/grouping/GroupingRequest.java
+++ b/container-search/src/main/java/com/yahoo/search/grouping/GroupingRequest.java
@@ -14,6 +14,7 @@ import com.yahoo.search.result.Hit;
 
 import java.util.ArrayList;
 import java.util.List;
+import java.util.OptionalDouble;
 import java.util.OptionalInt;
 import java.util.OptionalLong;
 import java.util.TimeZone;
@@ -33,9 +34,10 @@ public class GroupingRequest {
     private final List<Continuation> continuations = new ArrayList<>();
     private GroupingOperation root;
     private TimeZone timeZone;
-    private int defaultMaxHits = -1;
-    private int defaultMaxGroups = -1;
-    private long globalMaxGroups = -1;
+    private Integer defaultMaxHits;
+    private Integer defaultMaxGroups;
+    private Long globalMaxGroups;
+    private Double defaultPrecisionFactor;
 
     private GroupingRequest(Select parent) {
         this.parent = parent;
@@ -45,9 +47,10 @@ public class GroupingRequest {
                             List<Continuation> continuations,
                             GroupingOperation root,
                             TimeZone timeZone,
-                            int defaultMaxHits,
-                            int defaultMaxGroups,
-                            long globalMaxGroups) {
+                            Integer defaultMaxHits,
+                            Integer defaultMaxGroups,
+                            Long globalMaxGroups,
+                            Double defaultPrecisionFactor) {
         this.parent = parent;
         continuations.forEach(item -> this.continuations.add(item.copy()));
         this.root = root != null ? root.copy(null) : null;
@@ -55,11 +58,13 @@ public class GroupingRequest {
         this.defaultMaxHits = defaultMaxHits;
         this.defaultMaxGroups = defaultMaxGroups;
         this.globalMaxGroups = globalMaxGroups;
+        this.defaultPrecisionFactor = defaultPrecisionFactor;
     }
 
     /** Returns a deep copy of this */
     public GroupingRequest copy(Select parentOfCopy) {
-        return new GroupingRequest(parentOfCopy, continuations, root, timeZone, defaultMaxHits, defaultMaxGroups, globalMaxGroups);
+        return new GroupingRequest(parentOfCopy, continuations, root, timeZone, defaultMaxHits, defaultMaxGroups,
+                globalMaxGroups, defaultPrecisionFactor);
     }
 
     /**
@@ -145,25 +150,32 @@ public class GroupingRequest {
 
     @Beta
     public OptionalInt defaultMaxHits() {
-        return defaultMaxHits >= 0 ? OptionalInt.of(defaultMaxHits) : OptionalInt.empty();
+        return defaultMaxHits != null ? OptionalInt.of(defaultMaxHits) : OptionalInt.empty();
     }
 
     @Beta public void setDefaultMaxHits(int v) { this.defaultMaxHits = v; }
 
     @Beta
     public OptionalInt defaultMaxGroups() {
-        return defaultMaxGroups >= 0 ? OptionalInt.of(defaultMaxGroups) : OptionalInt.empty();
+        return defaultMaxGroups != null ? OptionalInt.of(defaultMaxGroups) : OptionalInt.empty();
     }
 
     @Beta public void setDefaultMaxGroups(int v) { this.defaultMaxGroups = v; }
 
     @Beta
     public OptionalLong globalMaxGroups() {
-        return globalMaxGroups >= 0 ? OptionalLong.of(globalMaxGroups) : OptionalLong.empty();
+        return globalMaxGroups != null ? OptionalLong.of(globalMaxGroups) : OptionalLong.empty();
     }
 
     @Beta public void setGlobalMaxGroups(long v) { this.globalMaxGroups = v; }
 
+    @Beta
+    public OptionalDouble defaultPrecisionFactor() {
+        return defaultPrecisionFactor != null ? OptionalDouble.of(defaultPrecisionFactor) : OptionalDouble.empty();
+    }
+
+    @Beta void setDefaultPrecisionFactor(double v) { this.defaultPrecisionFactor = v; }
+
     /**
      * Creates a new grouping request and adds it to the query.getSelect().getGrouping() list
      *
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 c7000c0cdcf..b3c10a67091 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
@@ -48,6 +48,12 @@ public class GroupingExecutor extends Searcher {
     private final static CompoundName PROP_GROUPINGLIST = newCompoundName(GROUPING_LIST);
     private final static Logger log = Logger.getLogger(GroupingExecutor.class.getName());
 
+    // TODO Vespa 8: Modify defaults
+    private static final double DEFAULT_PRECISION_FACTOR = 1;
+    private static final int DEFAULT_MAX_GROUPS = -1;
+    private static final int DEFAULT_MAX_HITS = -1;
+    private static final long DEFAULT_GLOBAL_MAX_GROUPS = -1;
+
     /**
      * Constructs a new instance of this searcher without configuration.
      * This makes the searcher completely useless for searching purposes,
@@ -150,9 +156,10 @@ public class GroupingExecutor extends Searcher {
         builder.setDefaultSummaryName(query.getPresentation().getSummary());
         builder.setTimeZone(req.getTimeZone());
         builder.addContinuations(req.continuations());
-        req.defaultMaxGroups().ifPresent(builder::setDefaultMaxGroups);
-        req.defaultMaxHits().ifPresent(builder::setDefaultMaxHits);
-        req.globalMaxGroups().ifPresent(builder::setGlobalMaxGroups);
+        builder.setDefaultMaxGroups(req.defaultMaxGroups().orElse(DEFAULT_MAX_GROUPS));
+        builder.setDefaultMaxHits(req.defaultMaxHits().orElse(DEFAULT_MAX_HITS));
+        builder.setGlobalMaxGroups(req.globalMaxGroups().orElse(DEFAULT_GLOBAL_MAX_GROUPS));
+        builder.setDefaultPrecisionFactor(req.defaultPrecisionFactor().orElse(DEFAULT_PRECISION_FACTOR));
         builder.build();
 
         RequestContext ctx = new RequestContext(req, builder.getTransform());
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 b013e87fb24..a88450c7d1d 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
@@ -44,6 +44,7 @@ class RequestBuilder {
     private int defaultMaxGroups = -1;
     private long globalMaxGroups = -1;
     private long totalGroupsAndSummaries = -1;
+    private double defaultPrecisionFactor = -1;
 
     /**
      * Constructs a new instance of this class.
@@ -162,6 +163,8 @@ class RequestBuilder {
 
     public RequestBuilder setGlobalMaxGroups(long v) { this.globalMaxGroups = v; return this; }
 
+    public RequestBuilder setDefaultPrecisionFactor(double v) { this.defaultPrecisionFactor = v; return this; }
+
     OptionalLong totalGroupsAndSummaries() {
         return totalGroupsAndSummaries != -1 ? OptionalLong.of(totalGroupsAndSummaries) : OptionalLong.empty();
     }
@@ -337,6 +340,8 @@ class RequestBuilder {
         int precision = frame.astNode.getPrecision();
         if (precision > 0) {
             frame.state.precision = precision;
+        } else if (frame.state.max != null && defaultPrecisionFactor > 0) {
+            frame.state.precision = Math.max(1, (int) Math.ceil(frame.state.max * defaultPrecisionFactor));
         }
     }
 
diff --git a/container-search/src/main/java/com/yahoo/search/query/properties/DefaultProperties.java b/container-search/src/main/java/com/yahoo/search/query/properties/DefaultProperties.java
index f25b61c3265..b94ddde4733 100644
--- a/container-search/src/main/java/com/yahoo/search/query/properties/DefaultProperties.java
+++ b/container-search/src/main/java/com/yahoo/search/query/properties/DefaultProperties.java
@@ -1,12 +1,10 @@
 // Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 package com.yahoo.search.query.properties;
 
-import com.yahoo.api.annotations.Beta;
 import com.yahoo.processing.request.CompoundName;
 import com.yahoo.search.query.Properties;
 import com.yahoo.search.query.profile.types.FieldDescription;
 import com.yahoo.search.query.profile.types.QueryProfileType;
-import com.yahoo.search.query.profile.types.QueryProfileTypeRegistry;
 
 import java.util.Map;
 
@@ -19,7 +17,6 @@ public final class DefaultProperties extends Properties  {
 
     public static final CompoundName MAX_OFFSET = new CompoundName("maxOffset");
     public static final CompoundName MAX_HITS = new CompoundName("maxHits");
-    @Beta public static final CompoundName GROUPING_GLOBAL_MAX_GROUPS = new CompoundName("grouping.globalMaxGroups");
 
     public static final QueryProfileType argumentType = new QueryProfileType("DefaultProperties");
 
@@ -28,7 +25,6 @@ public final class DefaultProperties extends Properties  {
 
         argumentType.addField(new FieldDescription(MAX_OFFSET.toString(), "integer"));
         argumentType.addField(new FieldDescription(MAX_HITS.toString(), "integer"));
-        argumentType.addField(new FieldDescription(GROUPING_GLOBAL_MAX_GROUPS.toString(), "long"), new QueryProfileTypeRegistry());
 
         argumentType.freeze();
     }
@@ -39,8 +35,6 @@ public final class DefaultProperties extends Properties  {
             return 1000;
         } else if (MAX_HITS.equals(name)) {
             return 400;
-        } else if (GROUPING_GLOBAL_MAX_GROUPS.equals(name)) {
-            return -1; // TODO Vespa 8: use default from Vespa 8 release notes
         } else {
             return super.get(name, context, substitution);
         }
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 ccf11d82541..5fa086c2ed1 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
@@ -789,6 +789,25 @@ public class RequestBuilderTestCase {
         assertQueryFailsOnGlobalMax(Long.MAX_VALUE, "all(group(a) max(5) each(each(output(summary()))))", "unbounded number of summaries");
     }
 
+    @Test
+    public void require_that_default_precision_factor_overrides_implicit_precision() {
+        int factor = 3;
+        RequestBuilder builder = new RequestBuilder(0)
+                .setDefaultPrecisionFactor(factor)
+                .setRootOperation(GroupingOperation.fromString("all(group(foo)max(5)each(output(count())))"));
+        builder.build();
+        assertEquals(5 * factor, builder.getRequestList().get(0).getLevels().get(0).getPrecision());
+    }
+
+    @Test
+    public void require_that_explicit_precision_has_precedence() {
+        RequestBuilder builder = new RequestBuilder(0)
+                .setDefaultPrecisionFactor(3)
+                .setRootOperation(GroupingOperation.fromString("all(group(foo)max(5)precision(10)each(output(count())))"));
+        builder.build();
+        assertEquals(10, builder.getRequestList().get(0).getLevels().get(0).getPrecision());
+    }
+
     private static void assertTotalGroupsAndSummaries(long expected, String query) {
         RequestBuilder builder = new RequestBuilder(0)
                 .setRootOperation(GroupingOperation.fromString(query)).setGlobalMaxGroups(Long.MAX_VALUE);