Merge pull request #12486 from vespa-engine/bratseth/nn-query-type-resolving

Bratseth/nn query type resolving

9 files changed, 154 insertions, 64 deletions

diff --git a/config-model/src/test/derived/nearestneighbor/query-profiles/default.xml b/config-model/src/test/derived/nearestneighbor/query-profiles/default.xml
new file mode 100644
index 00000000000..b8140f34617
--- /dev/null
+++ b/config-model/src/test/derived/nearestneighbor/query-profiles/default.xml
@@ -0,0 +1 @@
+<query-profile id="default" type="root" />
diff --git a/config-model/src/test/derived/nearestneighbor/query-profiles/types/root.xml b/config-model/src/test/derived/nearestneighbor/query-profiles/types/root.xml
new file mode 100644
index 00000000000..895e0663181
--- /dev/null
+++ b/config-model/src/test/derived/nearestneighbor/query-profiles/types/root.xml
@@ -0,0 +1,3 @@
+<query-profile-type id="root" inherits="native">
+  <field name="ranking.features.query(q_vec)" type="tensor&lt;float&gt;(x[5])" />
+</query-profile-type>
diff --git a/config-model/src/test/derived/nearestneighbor/test.sd b/config-model/src/test/derived/nearestneighbor/test.sd
new file mode 100644
index 00000000000..ab5f6d85448
--- /dev/null
+++ b/config-model/src/test/derived/nearestneighbor/test.sd
@@ -0,0 +1,27 @@
+search test {
+  document test {
+    field id type int {
+      indexing: attribute | summary
+    }
+    field vec type tensor<float>(x[5]) {
+      indexing: attribute | summary
+    }
+    field vec_hnsw type tensor<float>(x[5]) {
+      indexing: attribute | index | summary
+      index {
+        hnsw {
+          max-links-per-node: 16
+          neighbors-to-explore-at-insert: 200
+        }
+      }
+    }
+  }
+  rank-profile default {
+    first-phase {
+      expression: 10000 - itemRawScore(nns)
+    }
+  }
+  document-summary minimal {
+    summary id type int {}
+  }
+}
diff --git a/config-model/src/test/java/com/yahoo/searchdefinition/derived/NearestNeighborTestCase.java b/config-model/src/test/java/com/yahoo/searchdefinition/derived/NearestNeighborTestCase.java
new file mode 100644
index 00000000000..ead4e586d9f
--- /dev/null
+++ b/config-model/src/test/java/com/yahoo/searchdefinition/derived/NearestNeighborTestCase.java
@@ -0,0 +1,37 @@
+// Copyright 2019 Oath Inc. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
+package com.yahoo.searchdefinition.derived;
+
+import com.yahoo.component.ComponentId;
+import com.yahoo.prelude.query.QueryException;
+import com.yahoo.search.Query;
+import com.yahoo.search.query.profile.compiled.CompiledQueryProfileRegistry;
+import com.yahoo.search.query.profile.config.QueryProfileConfigurer;
+import com.yahoo.searchdefinition.parser.ParseException;
+import com.yahoo.vespa.model.container.search.QueryProfiles;
+import org.junit.Test;
+
+import java.io.IOException;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.fail;
+
+public class NearestNeighborTestCase extends AbstractExportingTestCase {
+
+    @Test
+    public void testNearestNeighbor() throws IOException, ParseException {
+        try {
+            ComponentId.resetGlobalCountersForTests();
+            DerivedConfiguration c = assertCorrectDeriving("nearestneighbor");
+
+            CompiledQueryProfileRegistry queryProfiles =
+                    QueryProfileConfigurer.createFromConfig(new QueryProfiles(c.getQueryProfiles(), (level, message) -> {}).getConfig()).compile();
+            Query q = new Query("?ranking.features.query(q_vec)=[1,2,3,4,5,6]", // length is 6, not 5
+                                queryProfiles.getComponent("default"));
+            fail("This should fail when q_vec is parsed as a tensor");
+        } catch (QueryException e) {
+            // success
+            assertEquals("Invalid request parameter", e.getMessage());
+        }
+    }
+
+}
diff --git a/container-search/src/main/java/com/yahoo/prelude/query/NearestNeighborItem.java b/container-search/src/main/java/com/yahoo/prelude/query/NearestNeighborItem.java
index 836107138d0..e8fa70afd1b 100644
--- a/container-search/src/main/java/com/yahoo/prelude/query/NearestNeighborItem.java
+++ b/container-search/src/main/java/com/yahoo/prelude/query/NearestNeighborItem.java
@@ -85,4 +85,5 @@ public class NearestNeighborItem extends SimpleTaggableItem {
         buffer.append(",approximate=").append(String.valueOf(approximate));
         buffer.append(",targetNumHits=").append(targetNumHits).append("}");
     }
+
 }
diff --git a/container-search/src/main/java/com/yahoo/search/query/Properties.java b/container-search/src/main/java/com/yahoo/search/query/Properties.java
index a1a70b4c3ba..a0cd4137e9f 100644
--- a/container-search/src/main/java/com/yahoo/search/query/Properties.java
+++ b/container-search/src/main/java/com/yahoo/search/query/Properties.java
@@ -30,8 +30,9 @@ public abstract class Properties extends com.yahoo.processing.request.Properties
         return (Properties)super.clone();
     }
 
-    /** The query owning this property object.
-     *  Only guaranteed to work if this instance is accessible as query.properties()
+    /**
+     * Returns the query owning this property object.
+     * Only guaranteed to work if this instance is accessible as query.properties()
      */
     public Query getParentQuery() {
         if (chained() == null) {
@@ -48,4 +49,5 @@ public abstract class Properties extends com.yahoo.processing.request.Properties
         if (chained() != null)
             chained().setParentQuery(query);
     }
+
 }
diff --git a/container-search/src/main/java/com/yahoo/search/searchers/ValidateNearestNeighborSearcher.java b/container-search/src/main/java/com/yahoo/search/searchers/ValidateNearestNeighborSearcher.java
index 8cae081cada..0f7f163dce0 100644
--- a/container-search/src/main/java/com/yahoo/search/searchers/ValidateNearestNeighborSearcher.java
+++ b/container-search/src/main/java/com/yahoo/search/searchers/ValidateNearestNeighborSearcher.java
@@ -4,14 +4,19 @@ package com.yahoo.search.searchers;
 
 import com.google.common.annotations.Beta;
 
-import com.yahoo.container.QrSearchersConfig;
 import com.yahoo.prelude.query.Item;
 import com.yahoo.prelude.query.NearestNeighborItem;
 import com.yahoo.prelude.query.QueryCanonicalizer;
 import com.yahoo.prelude.query.ToolBox;
+import com.yahoo.processing.request.CompoundName;
 import com.yahoo.search.Query;
 import com.yahoo.search.Result;
 import com.yahoo.search.Searcher;
+import com.yahoo.search.query.profile.QueryProfileProperties;
+import com.yahoo.search.query.profile.compiled.CompiledQueryProfile;
+import com.yahoo.search.query.profile.types.FieldDescription;
+import com.yahoo.search.query.profile.types.QueryProfileFieldType;
+import com.yahoo.search.query.profile.types.QueryProfileType;
 import com.yahoo.search.query.ranking.RankProperties;
 import com.yahoo.search.result.ErrorMessage;
 import com.yahoo.search.searchchain.Execution;
@@ -25,16 +30,15 @@ import java.util.List;
 import java.util.Map;
 import java.util.Optional;
 
-// This depends on tensors in query.getRanking which are moved to rank.properties during query.prepare()
-// Query.prepare is done at the same time as canonicalization (by GroupingExecutor), so use that constraint.
-@After(QueryCanonicalizer.queryCanonicalization)
-
 /**
  * Validates any NearestNeighborItem query items.
  *
  * @author arnej
  */
 @Beta
+// This depends on tensors in query.getRanking which are moved to rank.properties during query.prepare()
+// Query.prepare is done at the same time as canonicalization (by GroupingExecutor), so use that constraint.
+@After(QueryCanonicalizer.queryCanonicalization)
 public class ValidateNearestNeighborSearcher extends Searcher {
 
     private Map<String, TensorType> validAttributes = new HashMap<>();
@@ -56,7 +60,7 @@ public class ValidateNearestNeighborSearcher extends Searcher {
     }
 
     private Optional<ErrorMessage> validate(Query query) {
-        NNVisitor visitor = new NNVisitor(query.getRanking().getProperties(), validAttributes);
+        NNVisitor visitor = new NNVisitor(query.getRanking().getProperties(), validAttributes, query);
         ToolBox.visit(visitor, query.getModel().getQueryTree().getRoot());
         return visitor.errorMessage;
     }
@@ -65,26 +69,26 @@ public class ValidateNearestNeighborSearcher extends Searcher {
 
         public Optional<ErrorMessage> errorMessage = Optional.empty();
 
-        private RankProperties rankProperties;
-        private Map<String, TensorType> validAttributes;
+        private final RankProperties rankProperties;
+        private final Map<String, TensorType> validAttributes;
+        private final Query query;
 
-        public NNVisitor(RankProperties rankProperties, Map<String, TensorType> validAttributes) {
+        public NNVisitor(RankProperties rankProperties, Map<String, TensorType> validAttributes, Query query) {
             this.rankProperties = rankProperties;
             this.validAttributes = validAttributes;
+            this.query = query;
         }
 
         @Override
         public boolean visit(Item item) {
             if (item instanceof NearestNeighborItem) {
-                validate((NearestNeighborItem) item);
+                String error = validate((NearestNeighborItem)item);
+                if (error != null)
+                    errorMessage = Optional.of(ErrorMessage.createIllegalQuery(error));
             }
             return true;
         }
 
-        private void setError(String description) {
-            errorMessage = Optional.of(ErrorMessage.createIllegalQuery(description));
-        }
-
         private static boolean isCompatible(TensorType lhs, TensorType rhs) {
             return lhs.dimensions().equals(rhs.dimensions());
         }
@@ -98,55 +102,78 @@ public class ValidateNearestNeighborSearcher extends Searcher {
             return true;
         }
 
-        private void validate(NearestNeighborItem item) {
+        /** Returns an error message if this is invalid, or null if it is valid */
+        private String validate(NearestNeighborItem item) {
             int target = item.getTargetNumHits();
-            if (target < 1) {
-                setError(item.toString() + " has invalid targetNumHits");
-                return;
-            }
-            String qprop = item.getQueryTensorName();
-            List<Object> rankPropValList = rankProperties.asMap().get(qprop);
-            if (rankPropValList == null) {
-                setError(item.toString() + " query tensor not found");
-                return;
-            }
-            if (rankPropValList.size() != 1) {
-                setError(item.toString() + " query tensor does not have a single value");
-                return;
-            }
+            if (target < 1) return item + " has invalid targetNumHits";
+
+            List<Object> rankPropValList = rankProperties.asMap().get(item.getQueryTensorName());
+            if (rankPropValList == null) return item + " query tensor not found";
+            if (rankPropValList.size() != 1) return item + " query tensor does not have a single value";
+
             Object rankPropValue = rankPropValList.get(0);
             if (! (rankPropValue instanceof Tensor)) {
-                setError(item.toString() + " query tensor should be a tensor, was: "+
-                    (rankPropValue == null ? "null" : rankPropValue.getClass().toString()));
-                return;
+                return item + " expected a query tensor but got " +
+                       (rankPropValue == null ? "null" : rankPropValue.getClass()) +
+                       resolvedTypeInfo();
             }
-            Tensor qTensor = (Tensor)rankPropValue;
-            TensorType qTensorType = qTensor.type();
 
             String field = item.getIndexName();
-            if (validAttributes.containsKey(field)) {
-                TensorType fTensorType = validAttributes.get(field);
-                if (fTensorType == null) {
-                    setError(item.toString() + " field is not a tensor");
-                    return;
-                }
-                if (! isCompatible(fTensorType, qTensorType)) {
-                    setError(item.toString() + " field type "+fTensorType+" does not match query tensor type "+qTensorType);
-                    return;
-                }
-                if (! isDenseVector(fTensorType)) {
-                    setError(item.toString() + " tensor type "+fTensorType+" is not a dense vector");
-                    return;
-                }
-            } else {
-                setError(item.toString() + " field is not an attribute");
-                return;
+            if ( ! validAttributes.containsKey(field)) return item + " field is not an attribute";
+
+            TensorType fTensorType = validAttributes.get(field);
+            TensorType qTensorType = ((Tensor)rankPropValue).type();
+            if (fTensorType == null) return item + " field is not a tensor";
+            if ( ! isCompatible(fTensorType, qTensorType)) {
+                return item + " field type " + fTensorType + " does not match query tensor type " + qTensorType;
             }
+            if (! isDenseVector(fTensorType)) return item + " tensor type " + fTensorType+" is not a dense vector";
+            return null;
         }
 
         @Override
         public void onExit() {}
 
+        // TODO: Remove
+        private String resolvedTypeInfo() {
+            StringBuilder b = new StringBuilder();
+            QueryProfileProperties properties = query.properties().getInstance(QueryProfileProperties.class);
+            if (properties == null) return b.toString();
+            CompiledQueryProfile profile = properties.getQueryProfile();
+            b.append(", profile: ").append(profile);
+
+            CompoundName name = new CompoundName("ranking.features.query(q_vec)");
+
+            if ( ! profile.getTypes().isEmpty()) {
+                QueryProfileType type = null;
+                for (int i = 0; i < name.size(); i++) {
+                    if (type == null) // We're on the first iteration, or no type is explicitly specified
+                        type = profile.getType(name.first(i), new HashMap<>());
+                    if (type == null) continue;
+                    String localName = name.get(i);
+                    FieldDescription fieldDescription = type.getField(localName);
+                    if (fieldDescription == null && type.isStrict())
+                        throw new IllegalArgumentException("'" + localName + "' is not declared in " + type + ", and the type is strict");
+
+                    // TODO: In addition to strictness, check legality along the way
+
+                    if (fieldDescription != null) {
+                        if (i == name.size() - 1) { // at the end of the path, check the assignment type
+                            b.append(", field description: ").append(fieldDescription);
+                        } else if (fieldDescription.getType() instanceof QueryProfileFieldType) {
+                            // If a type is specified, use that instead of the type implied by the name
+                            type = ((QueryProfileFieldType) fieldDescription.getType()).getQueryProfileType();
+                        }
+                    }
+
+                }
+            }
+            else {
+                b.append(", profile types is empty");
+            }
+            return b.toString();
+        }
+
     }
 
 }
diff --git a/container-search/src/test/java/com/yahoo/search/searchers/ValidateNearestNeighborTestCase.java b/container-search/src/test/java/com/yahoo/search/searchers/ValidateNearestNeighborTestCase.java
index c6233ffa50b..0abb8d281b4 100644
--- a/container-search/src/test/java/com/yahoo/search/searchers/ValidateNearestNeighborTestCase.java
+++ b/container-search/src/test/java/com/yahoo/search/searchers/ValidateNearestNeighborTestCase.java
@@ -1,14 +1,11 @@
 // Copyright 2019 Oath Inc. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
-
-package com.yahoo.prelude.searcher;
+package com.yahoo.search.searchers;
 
 import com.google.common.util.concurrent.MoreExecutors;
 
 import com.yahoo.config.subscription.ConfigGetter;
 import com.yahoo.config.subscription.RawSource;
-import com.yahoo.language.Linguistics;
 import com.yahoo.language.simple.SimpleLinguistics;
-import com.yahoo.prelude.Index;
 import com.yahoo.prelude.IndexFacts;
 import com.yahoo.prelude.IndexModel;
 import com.yahoo.prelude.SearchDefinition;
@@ -20,15 +17,11 @@ import com.yahoo.search.rendering.RendererRegistry;
 import com.yahoo.search.Result;
 import com.yahoo.search.result.ErrorMessage;
 import com.yahoo.search.searchchain.Execution;
-import com.yahoo.search.Searcher;
-import com.yahoo.search.searchers.ValidateNearestNeighborSearcher;
 import com.yahoo.search.yql.YqlParser;
 import com.yahoo.tensor.Tensor;
 import com.yahoo.tensor.TensorType;
 import com.yahoo.vespa.config.search.AttributesConfig;
 
-import java.util.*;
-
 import org.junit.Test;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertNull;
@@ -171,9 +164,9 @@ public class ValidateNearestNeighborTestCase {
     public void testQueryTensorWrongType() {
         String q = makeQuery("dvector", "qvector");
         Result r = doSearch(searcher, q, "tensor string");
-        assertErrMsg(desc("dvector", "qvector", 1, "query tensor should be a tensor, was: class java.lang.String"), r);
+        assertErrMsg(desc("dvector", "qvector", 1, "expected a query tensor but got class java.lang.String"), r);
         r = doSearch(searcher, q, null);
-        assertErrMsg(desc("dvector", "qvector", 1, "query tensor should be a tensor, was: null"), r);
+        assertErrMsg(desc("dvector", "qvector", 1, "expected a query tensor but got null"), r);
     }
 
     @Test
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/CapacityPolicies.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/CapacityPolicies.java
index 179d7f2703c..c0664086498 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/CapacityPolicies.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/CapacityPolicies.java
@@ -103,7 +103,6 @@ public class CapacityPolicies {
     /**
      * Throw if the node count is 1 for container and content clusters and we're in a production zone
      *
-     * @return the argument node count
      * @throws IllegalArgumentException if only one node is requested and we can fail
      */
     private void ensureRedundancy(int nodeCount, ClusterSpec.Type clusterType, boolean canFail) {