Refactor: Initialize all macros state at once

7 files changed, 59 insertions, 44 deletions

diff --git a/config-model/src/main/java/com/yahoo/searchdefinition/MapEvaluationTypeContext.java b/config-model/src/main/java/com/yahoo/searchdefinition/MapEvaluationTypeContext.java
index afd33da369f..e094dd0ebcd 100644
--- a/config-model/src/main/java/com/yahoo/searchdefinition/MapEvaluationTypeContext.java
+++ b/config-model/src/main/java/com/yahoo/searchdefinition/MapEvaluationTypeContext.java
@@ -21,7 +21,6 @@ import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.Optional;
-import java.util.Stack;
 import java.util.stream.Collectors;
 
 /**
@@ -117,7 +116,7 @@ public class MapEvaluationTypeContext extends FunctionReferenceContext implement
     }
 
     /**
-     * Returns the default type for this simple feature, or nullif it does not have a default
+     * Returns the default type for this simple feature, or null if it does not have a default
      */
     public TensorType defaultTypeOf(Reference reference) {
         if ( ! FeatureNames.isSimpleFeature(reference))
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 b7e1f9d4538..2affcf5ca7d 100644
--- a/config-model/src/main/java/com/yahoo/searchdefinition/RankProfile.java
+++ b/config-model/src/main/java/com/yahoo/searchdefinition/RankProfile.java
@@ -95,11 +95,11 @@ public class RankProfile implements Serializable, Cloneable {
     /** The properties of this - a multimap */
     private Map<String, List<RankProperty>> rankProperties = new LinkedHashMap<>();
 
-    private Boolean ignoreDefaultRankFeatures=null;
+    private Boolean ignoreDefaultRankFeatures = null;
 
-    private String secondPhaseRankingString=null;
+    private String secondPhaseRankingString = null;
 
-    private String firstPhaseRankingString=null;
+    private String firstPhaseRankingString = null;
 
     private Map<String, Macro> macros= new LinkedHashMap<>();
 
@@ -339,8 +339,8 @@ public class RankProfile implements Serializable, Cloneable {
      * Returns null if no expression is set.
      */
     public RankingExpression getFirstPhaseRanking() {
-        if (firstPhaseRanking!=null) return firstPhaseRanking;
-        if (getInherited()!=null) return getInherited().getFirstPhaseRanking();
+        if (firstPhaseRanking != null) return firstPhaseRanking;
+        if (getInherited() != null) return getInherited().getFirstPhaseRanking();
         return null;
     }
 
@@ -353,13 +353,13 @@ public class RankProfile implements Serializable, Cloneable {
      * Returns null if no expression is set.
      */
     public RankingExpression getSecondPhaseRanking() {
-        if (secondPhaseRanking!=null) return secondPhaseRanking;
-        if (getInherited()!=null) return getInherited().getSecondPhaseRanking();
+        if (secondPhaseRanking != null) return secondPhaseRanking;
+        if (getInherited() != null) return getInherited().getSecondPhaseRanking();
         return null;
     }
 
     public void setSecondPhaseRanking(RankingExpression rankingExpression) {
-        this.secondPhaseRanking=rankingExpression;
+        this.secondPhaseRanking = rankingExpression;
     }
 
     /**
@@ -412,8 +412,8 @@ public class RankProfile implements Serializable, Cloneable {
     }
 
     public void addRankFeature(ReferenceNode feature) {
-        if (rankFeatures==null)
-            rankFeatures=new LinkedHashSet<>();
+        if (rankFeatures == null)
+            rankFeatures = new LinkedHashSet<>();
         rankFeatures.add(feature);
     }
 
@@ -548,9 +548,24 @@ public class RankProfile implements Serializable, Cloneable {
         return null;
     }
 
-    /** Creates a new (empty) macro and returns it */
-    public Macro addMacro(String name, boolean inline) {
-        Macro macro = new Macro(name, inline);
+    /** Adds a new macro and returns it */
+    public Macro addMacro(String name, RankingExpression expression, boolean inline) {
+        return addMacro(name, Collections.emptyList(), expression, inline);
+    }
+
+    /** Adds a new macro and returns it */
+    public Macro addMacro(String name, List<String> arguments, String expression, boolean inline) {
+        try {
+            return addMacro(name, arguments, parseRankingExpression(name, expression), inline);
+        }
+        catch (ParseException e) {
+            throw new IllegalArgumentException("Could not parse macro '" + name + "'", e);
+        }
+    }
+
+    /** Adds a new macro and returns it */
+    public Macro addMacro(String name, List<String> arguments, RankingExpression expression, boolean inline) {
+        Macro macro = new Macro(name, arguments, expression, inline);
         macros.put(name, macro);
         return macro;
     }
@@ -622,10 +637,6 @@ public class RankProfile implements Serializable, Cloneable {
         }
     }
 
-    /**
-     * Passes the contents of macros on to parser. Then put all the implied rank properties
-     * from those macros into the profile's props map.
-     */
     private void parseMacros() throws ParseException {
         for (Map.Entry<String, Macro> e : getMacros().entrySet()) {
             String macroName = e.getKey();
@@ -650,15 +661,15 @@ public class RankProfile implements Serializable, Cloneable {
             setSecondPhaseRanking(parseRankingExpression("secondphase", getSecondPhaseRankingString()));
     }
 
-    private RankingExpression parseRankingExpression(String expressionName, String exp) throws ParseException {
-        if (exp.trim().length() == 0)
+    private RankingExpression parseRankingExpression(String expressionName, String expression) throws ParseException {
+        if (expression.trim().length() == 0)
             throw new ParseException("Encountered an empty ranking expression in " + getName()+ ", " + expressionName + ".");
 
-        try (Reader rankingExpressionReader = openRankingExpressionReader(expressionName, exp.trim())) {
+        try (Reader rankingExpressionReader = openRankingExpressionReader(expressionName, expression.trim())) {
             return new RankingExpression(expressionName, rankingExpressionReader);
         }
         catch (com.yahoo.searchlib.rankingexpression.parser.ParseException e) {
-            ParseException exception = new ParseException("Could not parse ranking expression '" + exp.trim() +
+            ParseException exception = new ParseException("Could not parse ranking expression '" + expression.trim() +
                                                           "' in " + getName()+ ", " + expressionName + ".");
             throw (ParseException)exception.initCause(e);
         }
@@ -936,7 +947,7 @@ public class RankProfile implements Serializable, Cloneable {
 
         @Override
         public int hashCode() {
-            return name.hashCode() + 17*value.hashCode();
+            return name.hashCode() + 17 * value.hashCode();
         }
 
         @Override
@@ -960,23 +971,26 @@ public class RankProfile implements Serializable, Cloneable {
 
         private final String name;
         private String textualExpression = null;
-        private RankingExpression expression = null;
-        private List<String> formalParams = new ArrayList<>();
+        private RankingExpression expression;
+        private List<String> arguments;
 
         /** True if this should be inlined into calling expressions. Useful for very cheap macros. */
         private final boolean inline;
 
-        public Macro(String name, boolean inline) {
+        public Macro(String name, List<String> arguments, RankingExpression expression, boolean inline) {
             this.name = name;
+            this.arguments = arguments;
+            this.textualExpression = expression.getRoot().toString();
+            this.expression = expression;
             this.inline = inline;
         }
 
         public void addParam(String name) {
-            formalParams.add(name);
+            arguments.add(name);
         }
 
         public List<String> getFormalParams() {
-            return formalParams;
+            return arguments;
         }
 
         public String getTextualExpression() {
@@ -1000,7 +1014,7 @@ public class RankProfile implements Serializable, Cloneable {
         }
 
         public boolean getInline() {
-            return inline && formalParams.size() == 0; // only inline no-arg macros;
+            return inline && arguments.size() == 0; // only inline no-arg macros;
         }
 
         public ExpressionFunction asExpressionFunction() {
diff --git a/config-model/src/main/java/com/yahoo/searchdefinition/derived/RawRankProfile.java b/config-model/src/main/java/com/yahoo/searchdefinition/derived/RawRankProfile.java
index 2bfdbd070b3..ffd7f74cfe0 100644
--- a/config-model/src/main/java/com/yahoo/searchdefinition/derived/RawRankProfile.java
+++ b/config-model/src/main/java/com/yahoo/searchdefinition/derived/RawRankProfile.java
@@ -193,6 +193,7 @@ public class RawRankProfile implements RankProfilesConfig.Producer {
             if (secondPhaseRanking != null) {
                 macroProperties.putAll(secondPhaseRanking.getRankProperties(new ArrayList<>(expressionMacros.values())));
             }
+
             for (Map.Entry<String, String> e : macroProperties.entrySet()) {
                 rankProperties.add(new RankProfile.RankProperty(e.getKey(), e.getValue()));
             }
@@ -205,7 +206,6 @@ public class RawRankProfile implements RankProfilesConfig.Producer {
             for (Map.Entry<String, ExpressionFunction> e : eMacros.entrySet()) {
                 String expression = e.getValue().getBody().getRoot().toString(new StringBuilder(), context, null, null).toString();
                 context.addFunctionSerialization(RankingExpression.propertyName(e.getKey()), expression);
-
             }
             return context.serializedFunctions();
         }
diff --git a/config-model/src/main/java/com/yahoo/vespa/model/VespaModel.java b/config-model/src/main/java/com/yahoo/vespa/model/VespaModel.java
index 282e5a29962..15c02290811 100644
--- a/config-model/src/main/java/com/yahoo/vespa/model/VespaModel.java
+++ b/config-model/src/main/java/com/yahoo/vespa/model/VespaModel.java
@@ -236,7 +236,7 @@ public final class VespaModel extends AbstractConfigProducerRoot implements Seri
                 ConvertedModel convertedModel = ConvertedModel.fromSource(new ModelName(model.name()),
                                                                           model.name(), profile, queryProfiles, model);
                 for (Map.Entry<String, RankingExpression> entry : convertedModel.expressions().entrySet()) {
-                    profile.addMacro(entry.getKey(), false).setRankingExpression(entry.getValue());
+                    profile.addMacro(entry.getKey(), entry.getValue(), false);
                 }
             }
         }
@@ -248,7 +248,7 @@ public final class VespaModel extends AbstractConfigProducerRoot implements Seri
                 rankProfileRegistry.add(profile);
                 ConvertedModel convertedModel = ConvertedModel.fromStore(new ModelName(modelName), modelName, profile);
                 for (Map.Entry<String, RankingExpression> entry : convertedModel.expressions().entrySet()) {
-                    profile.addMacro(entry.getKey(), false).setRankingExpression(entry.getValue());
+                    profile.addMacro(entry.getKey(), entry.getValue(), false).setRankingExpression(entry.getValue());
                 }
             }
         }
diff --git a/config-model/src/main/java/com/yahoo/vespa/model/ml/ConvertedModel.java b/config-model/src/main/java/com/yahoo/vespa/model/ml/ConvertedModel.java
index e2236feb336..807235d22dd 100644
--- a/config-model/src/main/java/com/yahoo/vespa/model/ml/ConvertedModel.java
+++ b/config-model/src/main/java/com/yahoo/vespa/model/ml/ConvertedModel.java
@@ -285,9 +285,7 @@ public class ConvertedModel {
                                                    "\nwant to add " + expression + "\n");
             return;
         }
-        RankProfile.Macro macro = profile.addMacro(macroName, false);  // TODO: Inline if only used once
-        macro.setRankingExpression(expression);
-        macro.setTextualExpression(expression.getRoot().toString());
+        profile.addMacro(macroName, expression, false);  // TODO: Inline if only used once
     }
 
     /**
diff --git a/config-model/src/main/javacc/SDParser.jj b/config-model/src/main/javacc/SDParser.jj
index 63d3926afad..2c60ab3771b 100644
--- a/config-model/src/main/javacc/SDParser.jj
+++ b/config-model/src/main/javacc/SDParser.jj
@@ -58,6 +58,8 @@ import com.yahoo.language.Linguistics;
 import com.yahoo.language.simple.SimpleLinguistics;
 import com.yahoo.search.query.ranking.Diversity;
 import java.util.Map;
+import java.util.List;
+import java.util.ArrayList;
 import java.util.LinkedHashMap;
 import java.util.logging.Level;
 import org.apache.commons.lang.StringUtils;
@@ -1920,17 +1922,18 @@ void inheritsRankProfile(RankProfile profile) :
  */
 void macro(RankProfile profile) :
 {
-    String macro, param, expr;
+    String name, expression, parameter;
+    List parameters = new ArrayList();
     boolean inline = false;
 }
 {
-    ( <MACRO> inline = inline() macro = identifier() [ "$" { macro = macro + token.image; } ]
-      "("                                { profile.addMacro(macro, inline); }
-          [ param = identifier()         { profile.getMacros().get(macro).addParam(param); }
-          ( <COMMA> param = identifier() { profile.getMacros().get(macro).addParam(param); } )* ]
+    ( <MACRO> inline = inline() name = identifier() [ "$" { name = name + token.image; } ]
+      "("
+          [ parameter = identifier()         { parameters.add(parameter); }
+          ( <COMMA> parameter = identifier() { parameters.add(parameter); } )* ]
       ")"
-      lbrace() expr = expression() (<NL>)* <RBRACE> )
-    { profile.getMacros().get(macro).setTextualExpression(expr); }
+      lbrace() expression = expression() (<NL>)* <RBRACE> )
+    { profile.addMacro(name, parameters, expression, inline); }
 }
 
 boolean inline() :
diff --git a/config-model/src/test/java/com/yahoo/searchdefinition/RankProfileRegistryTest.java b/config-model/src/test/java/com/yahoo/searchdefinition/RankProfileRegistryTest.java
index 28559f351ac..e8bc4f44738 100644
--- a/config-model/src/test/java/com/yahoo/searchdefinition/RankProfileRegistryTest.java
+++ b/config-model/src/test/java/com/yahoo/searchdefinition/RankProfileRegistryTest.java
@@ -4,6 +4,7 @@ package com.yahoo.searchdefinition;
 import com.yahoo.config.model.application.provider.FilesApplicationPackage;
 import com.yahoo.config.model.test.TestDriver;
 import com.yahoo.config.model.test.TestRoot;
+import com.yahoo.searchlib.rankingexpression.RankingExpression;
 import com.yahoo.vespa.config.search.RankProfilesConfig;
 import org.junit.Test;
 
@@ -45,7 +46,7 @@ public class RankProfileRegistryTest {
         for (String rankProfileName : RankProfileRegistry.overridableRankProfileNames) {
             assertNull(rankProfileRegistry.get(search, rankProfileName).getMacros().get("foo"));
             RankProfile rankProfileWithAddedMacro = new RankProfile(rankProfileName, search, rankProfileRegistry);
-            rankProfileWithAddedMacro.addMacro("foo", true);
+            rankProfileWithAddedMacro.addMacro("foo", RankingExpression.from("1+2"), true);
             rankProfileRegistry.add(rankProfileWithAddedMacro);
             assertNotNull(rankProfileRegistry.get(search, rankProfileName).getMacros().get("foo"));
         }