Fix Select and grouping bugs

- Deep copy the grouping structure on query copy.
  It is mutable but we have neglected doing this right until now.
- Fix a bug in the previous commit where the last constructed Query.Model was shared between all instances.
- Fix a bug in the previous commit where the query string instead of the query tree was reset when a new select
  expression is set.
- Don't use deprecated method.
- Clean up Javadoc and formatting.

2 files changed, 76 insertions, 49 deletions

diff --git a/container-search/src/main/java/com/yahoo/search/query/Model.java b/container-search/src/main/java/com/yahoo/search/query/Model.java
index bd0c229085b..cbc15bf39a1 100644
--- a/container-search/src/main/java/com/yahoo/search/query/Model.java
+++ b/container-search/src/main/java/com/yahoo/search/query/Model.java
@@ -237,12 +237,13 @@ public class Model implements Cloneable {
     public void setQueryString(String queryString) {
         if (queryString == null) queryString="";
         this.queryString = queryString;
-        queryTree = null; // Cause parsing of the new query string next time the tree is accessed
+        clearQueryTree();
     }
 
     /**
      * Returns the query string which caused the original query tree of this model to come about.
      * Note that changes to the query tree are <b>not</b> reflected in this query string.
+     * Note that changes to the query tree are <b>not</b> reflected in this query string.
      *
      * @return the original (or reassigned) query string - never null
      */
@@ -265,6 +266,14 @@ public class Model implements Cloneable {
     }
 
     /**
+     * Clears the parsed query such that it will be created anew from the textual representation (a query string or
+     * select.where expression) on the next access.
+     */
+    public void clearQueryTree() {
+        queryTree = null;
+    }
+
+    /**
      * Returns the filter string set for this query.
      * The filter is included in the query tree at the time the query tree is parsed
      */
@@ -337,25 +346,25 @@ public class Model implements Cloneable {
                QueryHelper.combineHash(encoding,filter,language,getQueryTree(),sources,restrict,defaultIndex,type,searchPath);
     }
 
-
+    @Override
     public Object clone() {
         try {
-            Model clone = (Model) super.clone();
+            Model clone = (Model)super.clone();
             if (queryTree != null)
                 clone.queryTree = this.queryTree.clone();
-            if (sources !=null)
+            if (sources != null)
                 clone.sources = new LinkedHashSet<>(this.sources);
-            if (restrict !=null)
+            if (restrict != null)
                 clone.restrict = new LinkedHashSet<>(this.restrict);
             return clone;
         }
         catch (CloneNotSupportedException e) {
-            throw new RuntimeException("Someone inserted a noncloneable superclass",e);
+            throw new RuntimeException("Someone inserted a noncloneable superclass", e);
         }
     }
 
     public Model cloneFor(Query q)  {
-        Model model = (Model) this.clone();
+        Model model = (Model)this.clone();
         model.setParent(q);
         return model;
     }
@@ -365,7 +374,7 @@ public class Model implements Cloneable {
 
     /** Assigns the query owning this */
     public void setParent(Query parent) {
-        if (parent==null) throw new NullPointerException("A query models owner cannot be null");
+        if (parent == null) throw new NullPointerException("A query models owner cannot be null");
         this.parent = parent;
     }
 
@@ -403,7 +412,7 @@ public class Model implements Cloneable {
 
     /** Sets the execution working on this. For internal use. */
     public void setExecution(Execution execution) {
-        if (execution==this.execution) return;
+        if (execution == this.execution) return;
 
         // If not already coupled, bind the trace of the new execution into the existing execution trace
         if (execution.trace().traceNode().isRoot()
@@ -425,7 +434,7 @@ public class Model implements Cloneable {
     /** Returns the Execution working on this, or a null execution if none. For internal use. */
     public Execution getExecution() { return execution; }
 
-    private void setFromString(String string,Set<String> set) {
+    private void setFromString(String string, Set<String> set) {
         set.clear();
         for (String item : string.split(","))
             set.add(item.trim());
@@ -520,7 +529,6 @@ public class Model implements Cloneable {
         return false;
     }
 
-
     /**
      * Set the YTrace header value to use when transmitting this model to a
      * search backend (of some kind).
diff --git a/container-search/src/main/java/com/yahoo/search/query/Select.java b/container-search/src/main/java/com/yahoo/search/query/Select.java
index ef6a7fe8272..bbc152c6391 100644
--- a/container-search/src/main/java/com/yahoo/search/query/Select.java
+++ b/container-search/src/main/java/com/yahoo/search/query/Select.java
@@ -12,7 +12,9 @@ import com.yahoo.search.yql.VespaGroupingStep;
 
 import java.util.ArrayList;
 import java.util.Collections;
+import java.util.LinkedHashSet;
 import java.util.List;
+import java.util.Objects;
 
 
 /**
@@ -30,69 +32,75 @@ public class Select implements Cloneable {
     public static final String WHERE = "where";
     public static final String GROUPING = "grouping";
 
-    private static Model model;
-    private Query parent;
-    private String where = "";
-    private String grouping = "";
-    private List<GroupingRequest> groupingRequests = new ArrayList<>();
+    private final Query parent;
+    private final List<GroupingRequest> groupingRequests;
+
+    private String where;
+    private String grouping;
 
     static {
         argumentType = new QueryProfileType(SELECT);
         argumentType.setStrict(true);
         argumentType.setBuiltin(true);
-        argumentType.addField(new FieldDescription(WHERE, "string", "where"));
-        argumentType.addField(new FieldDescription(GROUPING, "string", "grouping"));
+        argumentType.addField(new FieldDescription(WHERE, "string"));
+        argumentType.addField(new FieldDescription(GROUPING, "string"));
         argumentType.freeze();
-        argumentTypeName=new CompoundName(argumentType.getId().getName());
+        argumentTypeName = new CompoundName(argumentType.getId().getName());
     }
 
     public static QueryProfileType getArgumentType() { return argumentType; }
 
-    public Select(String where, String grouping){
-        this.where = where;
-        this.grouping = grouping;
-    }
-
+    /** Creates an empty select statement */
     public Select(Query query) {
-        setParent(query);
-        model = query.getModel();
+        this("", "", query);
     }
 
+    public Select(String where, String grouping, Query query) {
+        this(where, grouping, query, Collections.emptyList());
+    }
 
-    /** Returns the query owning this, never null */
-    private Query getParent() { return parent; }
-
-
-    /** Assigns the query owning this */
-    public void setParent(Query parent) {
-        if (parent==null) throw new NullPointerException("A query models owner cannot be null");
-        this.parent = parent;
+    private Select(String where, String grouping, Query query, List<GroupingRequest> groupingRequests) {
+        this.where = Objects.requireNonNull(where, "A Select must have a where string (possibly the empty string)");
+        this.grouping = Objects.requireNonNull(grouping, "A Select must have a select string (possibly the empty string)");
+        this.parent = Objects.requireNonNull(query, "A Select must have a parent query");
+        this.groupingRequests = deepCopy(groupingRequests, this);
     }
 
+    private static List<GroupingRequest> deepCopy(List<GroupingRequest> groupingRequests, Select parentOfCopy) {
+        List<GroupingRequest> copy = new ArrayList<>(groupingRequests.size());
+        for (GroupingRequest request : groupingRequests)
+            copy.add(request.copy(parentOfCopy));
+        return copy;
+    }
 
-    /** Set the where-clause for the query. Must be a JSON-string, with the format described in the Select Reference doc:
-     * @see <a href="https://docs.vespa.ai/documentation/reference/select-reference.html">https://docs.vespa.ai/documentation/reference/select-reference.html</a>
+    /**
+     * Sets the document selection criterion of the query.
+     *
+     * @param where the documents to select as a JSON string on the format specified in
+     *        <a href="https://docs.vespa.ai/documentation/reference/select-reference.html">the select reference doc</a>
      */
     public void setWhereString(String where) {
         this.where = where;
-        model.setType(SELECT);
+        parent.getModel().setType(SELECT);
 
-        // Setting the queryTree to null
-        model.setQueryString(null);
+        // This replaces the current query
+        parent.getModel().clearQueryTree();
     }
 
-
-    /** Returns the where-clause in the query */
+    /** Returns the where clause string previously assigned, or an empty string if none */
     public String getWhereString(){ return where; }
 
 
-    /** Set the grouping-string for the query. Must be a JSON-string, with the format described in the Select Reference doc:
-     * @see <a href="https://docs.vespa.ai/documentation/reference/select-reference.html">https://docs.vespa.ai/documentation/reference/select-reference.html</a>
-     * */
-    public void setGroupingString(String grouping){
+    /**
+     * Sets the grouping operation of the query.
+     *
+     * @param grouping the grouping to perform as a JSON string on the format specified in
+     *        <a href="https://docs.vespa.ai/documentation/reference/select-reference.html">the select reference doc</a>
+     */
+    public void setGroupingString(String grouping) {
+        groupingRequests.clear();
         this.grouping = grouping;
         SelectParser parser = (SelectParser) ParserFactory.newInstance(Query.Type.SELECT, new ParserEnvironment());
-
         for (VespaGroupingStep step : parser.getGroupingSteps(grouping)) {
             GroupingRequest.newInstance(parent)
                     .setRootOperation(step.getOperation())
@@ -106,9 +114,11 @@ public class Select implements Cloneable {
         return grouping;
     }
 
-
-    /** Returns the query's {@link GroupingRequest} objects, as mutable list */
-    public List<GroupingRequest> getGrouping(){ return groupingRequests; }
+    /**
+     * Returns the query's {@link GroupingRequest} as a mutable list. Changing this directly changes the grouping
+     * operations which will be performed by this query.
+     */
+    public List<GroupingRequest> getGrouping() { return groupingRequests; }
 
 
     @Override
@@ -116,4 +126,13 @@ public class Select implements Cloneable {
         return "where: [" + where + "], grouping: [" + grouping+ "]";
     }
 
+    @Override
+    public Object clone() {
+        return new Select(where, grouping, parent, groupingRequests);
+    }
+
+    public Select cloneFor(Query parent)  {
+        return new Select(where, grouping, parent, groupingRequests);
+    }
+
 }