Typecheck all ranking expressions

8 files changed, 90 insertions, 21 deletions

diff --git a/config-model/src/main/java/com/yahoo/searchdefinition/FeatureNames.java b/config-model/src/main/java/com/yahoo/searchdefinition/FeatureNames.java
index dd03cb8b2a7..c01b009e93b 100644
--- a/config-model/src/main/java/com/yahoo/searchdefinition/FeatureNames.java
+++ b/config-model/src/main/java/com/yahoo/searchdefinition/FeatureNames.java
@@ -101,6 +101,18 @@ public class FeatureNames {
         return canonicalize("query(\"" + propertyName + "\")");
     }
 
+    public static boolean isConstantFeature(String feature) {
+        return feature.startsWith("constant(");
+    }
+
+    public static boolean isAttributeFeature(String feature) {
+        return feature.startsWith("attribute(");
+    }
+
+    public static boolean isQueryFeature(String feature) {
+        return feature.startsWith("query(");
+    }
+
     /**
      * Returns the single argument of the given feature name, without any quotes,
      * or empty if it is not a valid query, attribute or constant feature name
diff --git a/config-model/src/main/java/com/yahoo/searchdefinition/RankProfile.java b/config-model/src/main/java/com/yahoo/searchdefinition/RankProfile.java
index 2f28d9adb8b..238f22eb17c 100644
--- a/config-model/src/main/java/com/yahoo/searchdefinition/RankProfile.java
+++ b/config-model/src/main/java/com/yahoo/searchdefinition/RankProfile.java
@@ -2,15 +2,9 @@
 package com.yahoo.searchdefinition;
 
 import com.yahoo.config.application.api.ApplicationPackage;
-import com.yahoo.config.model.deploy.DeployState;
-import com.yahoo.io.reader.NamedReader;
-import com.yahoo.processing.request.CompoundName;
-import com.yahoo.search.query.profile.QueryProfile;
 import com.yahoo.search.query.profile.QueryProfileRegistry;
-import com.yahoo.search.query.profile.config.QueryProfileXMLReader;
 import com.yahoo.search.query.profile.types.FieldDescription;
 import com.yahoo.search.query.profile.types.QueryProfileType;
-import com.yahoo.search.query.profile.types.TensorFieldType;
 import com.yahoo.search.query.ranking.Diversity;
 import com.yahoo.searchdefinition.document.SDField;
 import com.yahoo.searchdefinition.expressiontransforms.RankProfileTransformContext;
@@ -19,7 +13,6 @@ import com.yahoo.searchlib.rankingexpression.ExpressionFunction;
 import com.yahoo.searchlib.rankingexpression.FeatureList;
 import com.yahoo.searchlib.rankingexpression.RankingExpression;
 import com.yahoo.searchlib.rankingexpression.evaluation.TensorValue;
-import com.yahoo.searchlib.rankingexpression.evaluation.TypeMapContext;
 import com.yahoo.searchlib.rankingexpression.evaluation.Value;
 import com.yahoo.searchlib.rankingexpression.rule.ReferenceNode;
 import com.yahoo.tensor.TensorType;
@@ -908,7 +901,7 @@ public class RankProfile implements Serializable, Cloneable {
      */
     public static class Macro implements Serializable, Cloneable {
 
-        private String name=null;
+        private final String name;
         private String textualExpression=null;
         private RankingExpression expression=null;
         private List<String> formalParams = new ArrayList<>();
diff --git a/config-model/src/main/java/com/yahoo/searchdefinition/SearchBuilder.java b/config-model/src/main/java/com/yahoo/searchdefinition/SearchBuilder.java
index 762c0fec838..f63e24f65a6 100644
--- a/config-model/src/main/java/com/yahoo/searchdefinition/SearchBuilder.java
+++ b/config-model/src/main/java/com/yahoo/searchdefinition/SearchBuilder.java
@@ -34,7 +34,6 @@ import java.util.List;
  * expressions, using the setRankXXX() methods, 3) invoke the {@link #build()} method, and 4) retrieve the built
  * search objects using the {@link #getSearch(String)} method.
  */
-// TODO: This should be cleaned up and more or maybe completely taken over by MockApplicationPackage
 public class SearchBuilder {
 
     private final DocumentTypeManager docTypeMgr = new DocumentTypeManager();
@@ -212,9 +211,8 @@ public class SearchBuilder {
      * @param queryProfiles The query profiles contained in the application this search is part of.
      */
     public void build(DeployLogger deployLogger, QueryProfiles queryProfiles) {
-        if (isBuilt) {
-            throw new IllegalStateException("Searches already built.");
-        }
+        if (isBuilt) throw new IllegalStateException("Model already built");
+
         List<Search> built = new ArrayList<>();
         List<SDDocumentType> sdocs = new ArrayList<>();
         sdocs.add(SDDocumentType.VESPA_DOCUMENT);
@@ -254,8 +252,6 @@ public class SearchBuilder {
     /**
      * Processes and returns the given {@link Search} object. This method has been factored out of the {@link
      * #build()} method so that subclasses can choose not to build anything.
-     *
-     * @param search The object to build.
      */
     protected void process(Search search, DeployLogger deployLogger, QueryProfiles queryProfiles) {
         Processing.process(search, deployLogger, rankProfileRegistry, queryProfiles);
diff --git a/config-model/src/main/java/com/yahoo/searchdefinition/TypeMapContext.java b/config-model/src/main/java/com/yahoo/searchdefinition/TypeMapContext.java
index 40e9db1413f..1e569ac7016 100644
--- a/config-model/src/main/java/com/yahoo/searchdefinition/TypeMapContext.java
+++ b/config-model/src/main/java/com/yahoo/searchdefinition/TypeMapContext.java
@@ -10,6 +10,9 @@ import java.util.Map;
 
 /**
  * A context which only contains type information.
+ * This returns empty tensor types (double) for unknown features which are not
+ * query, attribute or constant features, as we do not have information about which such
+ * features exist (but we know those that exist are doubles).
  *
  * @author bratseth
  */
@@ -23,10 +26,12 @@ public class TypeMapContext implements TypeContext {
 
     @Override
     public TensorType getType(String name) {
-        return featureTypes.get(FeatureNames.canonicalize(name));
+        if (FeatureNames.isConstantFeature(name) ||
+            FeatureNames.isAttributeFeature(name) ||
+            FeatureNames.isQueryFeature(name))
+            return featureTypes.get(FeatureNames.canonicalize(name));
+        else
+            return TensorType.empty; // we do not have type information for these. Correct would be either empty or null
     }
 
-    /** Returns an unmodifiable map of the bindings in this */
-    public Map<String, TensorType> bindings() { return Collections.unmodifiableMap(featureTypes); }
-
 }
diff --git a/config-model/src/main/java/com/yahoo/searchdefinition/processing/IndexingValues.java b/config-model/src/main/java/com/yahoo/searchdefinition/processing/IndexingValues.java
index ee65c9bec02..cc634abef01 100644
--- a/config-model/src/main/java/com/yahoo/searchdefinition/processing/IndexingValues.java
+++ b/config-model/src/main/java/com/yahoo/searchdefinition/processing/IndexingValues.java
@@ -13,7 +13,7 @@ import com.yahoo.vespa.indexinglanguage.expressions.OutputExpression;
 import com.yahoo.vespa.model.container.search.QueryProfiles;
 
 /**
- * @author <a href="mailto:simon@yahoo-inc.com">Simon Thoresen Hult</a>
+ * @author Simon Thoresen Hult
  */
 public class IndexingValues extends Processor {
 
diff --git a/config-model/src/main/java/com/yahoo/searchdefinition/processing/Processing.java b/config-model/src/main/java/com/yahoo/searchdefinition/processing/Processing.java
index 90183848094..d9b995fbcd6 100644
--- a/config-model/src/main/java/com/yahoo/searchdefinition/processing/Processing.java
+++ b/config-model/src/main/java/com/yahoo/searchdefinition/processing/Processing.java
@@ -77,7 +77,8 @@ public class Processing {
                 FastAccessValidator::new,
                 ReservedMacroNames::new,
 
-                // These two should be last.
+                // These should be last.
+                RankingExpressionTypeValidator::new,
                 IndexingValidation::new,
                 IndexingValues::new);
     }
diff --git a/config-model/src/main/java/com/yahoo/searchdefinition/processing/RankingExpressionTypeValidator.java b/config-model/src/main/java/com/yahoo/searchdefinition/processing/RankingExpressionTypeValidator.java
new file mode 100644
index 00000000000..6a0dbce362d
--- /dev/null
+++ b/config-model/src/main/java/com/yahoo/searchdefinition/processing/RankingExpressionTypeValidator.java
@@ -0,0 +1,59 @@
+// Copyright 2018 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
+package com.yahoo.searchdefinition.processing;
+
+import com.yahoo.config.application.api.DeployLogger;
+import com.yahoo.search.query.profile.QueryProfileRegistry;
+import com.yahoo.searchdefinition.RankProfile;
+import com.yahoo.searchdefinition.RankProfileRegistry;
+import com.yahoo.searchdefinition.Search;
+import com.yahoo.searchlib.rankingexpression.RankingExpression;
+import com.yahoo.tensor.TensorType;
+import com.yahoo.tensor.evaluation.TypeContext;
+import com.yahoo.vespa.model.container.search.QueryProfiles;
+
+public class RankingExpressionTypeValidator extends Processor {
+
+    private final QueryProfileRegistry queryProfiles;
+
+    public RankingExpressionTypeValidator(Search search,
+                                          DeployLogger deployLogger,
+                                          RankProfileRegistry rankProfileRegistry,
+                                          QueryProfiles queryProfiles) {
+        super(search, deployLogger, rankProfileRegistry, queryProfiles);
+        this.queryProfiles = queryProfiles.getRegistry();
+    }
+
+    @Override
+    public void process() {
+        for (RankProfile profile : rankProfileRegistry.allRankProfiles()) {
+            try {
+                validate(profile);
+            }
+            catch (IllegalArgumentException e) {
+                throw new IllegalArgumentException(search + ", " + profile + " is invalid", e);
+            }
+        }
+    }
+
+    /** Throws an IllegalArgumentException if the given rank profile does not produce valid type */
+    private void validate(RankProfile profile) {
+        TypeContext context = profile.typeContext(queryProfiles);
+        for (RankProfile.Macro macro : profile.getMacros().values())
+            if (macro.getRankingExpression() != null)
+                macro.getRankingExpression().type(context); // type infer to throw on type conflicts
+        ensureProducesDouble(profile.getFirstPhaseRanking(), "first-phase", context);
+        ensureProducesDouble(profile.getSecondPhaseRanking(), "second-phase", context);
+    }
+
+    private void ensureProducesDouble(RankingExpression expression, String expressionDescription, TypeContext context) {
+        if (expression == null) return;
+
+        TensorType type = expression.type(context);
+        if (type == null) // Not expected to happen
+            throw new IllegalStateException("Could not determine the type produced by " + expressionDescription);
+        if ( ! type.equals(TensorType.empty))
+            throw new IllegalArgumentException(expressionDescription + " ranking expression must produce a double " +
+                                               "but produces " + type);
+    }
+
+}
diff --git a/searchlib/src/main/java/com/yahoo/searchlib/rankingexpression/rule/ReferenceNode.java b/searchlib/src/main/java/com/yahoo/searchlib/rankingexpression/rule/ReferenceNode.java
index 05a6773c5cb..56914143e25 100755
--- a/searchlib/src/main/java/com/yahoo/searchlib/rankingexpression/rule/ReferenceNode.java
+++ b/searchlib/src/main/java/com/yahoo/searchlib/rankingexpression/rule/ReferenceNode.java
@@ -108,7 +108,10 @@ public final class ReferenceNode extends CompositeNode {
     @Override
     public TensorType type(TypeContext context) {
         // Don't support outputs of different type, for simplicity
-        return context.getType(toString());
+        TensorType type = context.getType(toString());
+        if (type == null)
+            throw new IllegalArgumentException("Could not determine type of feature " + toString());
+        return type;
     }
 
     @Override