Use inheritance information from the uncompiled rankprofile to sort out when

to use extrenal files, and when they are overridden.

4 files changed, 133 insertions, 102 deletions

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 39ab443554c..af40dc947c8 100644
--- a/config-model/src/main/java/com/yahoo/searchdefinition/RankProfile.java
+++ b/config-model/src/main/java/com/yahoo/searchdefinition/RankProfile.java
@@ -370,7 +370,8 @@ public class RankProfile implements Cloneable {
      */
     public RankingExpression getFirstPhaseRanking() {
         if (firstPhaseRanking != null) return firstPhaseRanking;
-        if (getInherited() != null) return getInherited().getFirstPhaseRanking();
+        RankProfile inherited = getInherited();
+        if (inherited != null) return inherited.getFirstPhaseRanking();
         return null;
     }
 
@@ -379,15 +380,30 @@ public class RankProfile implements Cloneable {
     }
 
     public String getUniqueExpressionName(String name) {
-        return getName() + "_" + name;
+        return getName().replace('-', '_') + "_" + name;
+    }
+    public String resolveExpressionName(String name) {
+        if (externalFileExpressions.contains(name)) {
+            return getUniqueExpressionName(name);
+        }
+        if (functions.get(name) == null) {
+            RankProfile inherited = getInherited();
+            if (inherited != null) {
+                return inherited.resolveExpressionName(name);
+            }
+        }
+        return name;
     }
     public String getFirstPhaseFile() {
         String name = FIRST_PHASE;
         if (externalFileExpressions.contains(name)) {
             return rankExpressionFiles().get(getUniqueExpressionName(name)).getFileName();
         }
-        if ((firstPhaseRanking == null) && (getInherited() != null)) {
-            return getInherited().getFirstPhaseFile();
+        if (firstPhaseRanking == null) {
+            RankProfile inherited = getInherited();
+            if (inherited != null) {
+                return getInherited().getFirstPhaseFile();
+            }
         }
         return null;
     }
@@ -397,8 +413,11 @@ public class RankProfile implements Cloneable {
         if (externalFileExpressions.contains(name)) {
             return rankExpressionFiles().get(getUniqueExpressionName(name)).getFileName();
         }
-        if ((secondPhaseRanking == null) && (getInherited() != null)) {
-            return getInherited().getSecondPhaseFile();
+        if (secondPhaseRanking == null) {
+            RankProfile inherited = getInherited();
+            if (inherited != null) {
+                return getInherited().getSecondPhaseFile();
+            }
         }
         return null;
     }
@@ -407,8 +426,11 @@ public class RankProfile implements Cloneable {
         if (externalFileExpressions.contains(name)) {
             return rankExpressionFiles().get(getUniqueExpressionName(name)).getFileName();
         }
-        if (getInherited() != null) {
-            return getInherited().getExpressionFile(name);
+        if (functions.get(name) == null) {
+            RankProfile inherited = getInherited();
+            if (inherited != null) {
+                return inherited.getExpressionFile(name);
+            }
         }
         return null;
     }
@@ -428,7 +450,8 @@ public class RankProfile implements Cloneable {
      */
     public RankingExpression getSecondPhaseRanking() {
         if (secondPhaseRanking != null) return secondPhaseRanking;
-        if (getInherited() != null) return getInherited().getSecondPhaseRanking();
+        RankProfile inherited = getInherited();
+        if (inherited != null) return inherited.getSecondPhaseRanking();
         return null;
     }
 
@@ -746,6 +769,7 @@ public class RankProfile implements Cloneable {
             clone.functions = new LinkedHashMap<>(this.functions);
             clone.filterFields = new HashSet<>(this.filterFields);
             clone.constants = new HashMap<>(this.constants);
+            clone.externalFileExpressions = new HashSet(this.externalFileExpressions);
             return clone;
         }
         catch (CloneNotSupportedException e) {
@@ -781,6 +805,7 @@ public class RankProfile implements Cloneable {
         secondPhaseRanking = compile(this.getSecondPhaseRanking(), queryProfiles, featureTypes, importedModels, getConstants(), inlineFunctions, expressionTransforms);
 
         // Function compiling second pass: compile all functions and insert previously compiled inline functions
+        // TODO This merges all functions from inherited profiles too and erases inheritance information. Not good.
         functions = compileFunctions(this::getFunctions, queryProfiles, featureTypes, importedModels, inlineFunctions, expressionTransforms);
 
     }
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 41ac1e17d93..fd2cc2b6a49 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
@@ -22,6 +22,7 @@ import com.yahoo.vespa.config.search.RankProfilesConfig;
 
 import java.nio.charset.StandardCharsets;
 import java.util.ArrayList;
+import java.util.Collection;
 import java.util.HashSet;
 import java.util.Iterator;
 import java.util.LinkedHashMap;
@@ -55,15 +56,18 @@ public class RawRankProfile implements RankProfilesConfig.Producer {
     /**
      * Creates a raw rank profile from the given rank profile
      */
-    public RawRankProfile(RankProfile rankProfile, QueryProfileRegistry queryProfiles, ImportedMlModels importedModels, AttributeFields attributeFields, ModelContext.Properties deployProperties) {
+    public RawRankProfile(RankProfile rankProfile, QueryProfileRegistry queryProfiles, ImportedMlModels importedModels,
+                          AttributeFields attributeFields, ModelContext.Properties deployProperties) {
         this.name = rankProfile.getName();
-        compressedProperties = compress(new Deriver(rankProfile, queryProfiles, importedModels, attributeFields, deployProperties).derive());
+        compressedProperties = compress(new Deriver(rankProfile, rankProfile.compile(queryProfiles, importedModels),
+                                        attributeFields, deployProperties).derive());
     }
 
     /**
      * Only for testing
      */
-    public RawRankProfile(RankProfile rankProfile, QueryProfileRegistry queryProfiles, ImportedMlModels importedModels, AttributeFields attributeFields) {
+    public RawRankProfile(RankProfile rankProfile, QueryProfileRegistry queryProfiles,
+                          ImportedMlModels importedModels, AttributeFields attributeFields) {
         this(rankProfile, queryProfiles, importedModels, attributeFields, new TestProperties());
     }
 
@@ -120,61 +124,81 @@ public class RawRankProfile implements RankProfilesConfig.Producer {
 
     private static class Deriver {
 
-        /**
-         * The field rank settings of this profile
-         */
-        private Map<String, FieldRankSettings> fieldRankSettings = new java.util.LinkedHashMap<>();
-
-        private final RankProfile rankProfile;
-        private RankingExpression firstPhaseRanking = null;
-        private RankingExpression secondPhaseRanking = null;
-
-        private Set<ReferenceNode> summaryFeatures = new LinkedHashSet<>();
+        // Due to compiled rankprofiles flattening inheritance we need to use the uncompiled
+        // to sort out what comes from external files and not.
+        private final RankProfile unCompiledRankProfile;
 
-        private Set<ReferenceNode> rankFeatures = new LinkedHashSet<>();
-
-        private List<RankProfile.RankProperty> rankProperties = new ArrayList<>();
+        private final Map<String, FieldRankSettings> fieldRankSettings = new java.util.LinkedHashMap<>();
+        private final Set<ReferenceNode> summaryFeatures;
+        private final Set<ReferenceNode> rankFeatures;
+        private final List<RankProfile.RankProperty> rankProperties;
 
         /**
          * Rank properties for weight settings to make these available to feature executors
          */
-        private List<RankProfile.RankProperty> boostAndWeightRankProperties = new ArrayList<>();
-
-        private boolean ignoreDefaultRankFeatures = false;
-
-        private RankProfile.MatchPhaseSettings matchPhaseSettings = null;
-
-        private int rerankCount = -1;
-        private int keepRankCount = -1;
-        private int numThreadsPerSearch = -1;
-        private int minHitsPerThread = -1;
-        private int numSearchPartitions = -1;
-        private double termwiseLimit = 1.0;
-        private double rankScoreDropLimit = -Double.MAX_VALUE;
+        private final List<RankProfile.RankProperty> boostAndWeightRankProperties = new ArrayList<>();
+
+        private final boolean ignoreDefaultRankFeatures;
+        private final RankProfile.MatchPhaseSettings matchPhaseSettings;
+        private final int rerankCount;
+        private final int keepRankCount;
+        private final int numThreadsPerSearch;
+        private final int minHitsPerThread;
+        private final int numSearchPartitions;
+        private final double termwiseLimit;
+        private final double rankScoreDropLimit;
 
         /**
          * The rank type definitions used to derive settings for the native rank features
          */
         private final NativeRankTypeDefinitionSet nativeRankTypeDefinitions = new NativeRankTypeDefinitionSet("default");
-
         private final Map<String, String> attributeTypes;
         private final Map<String, String> queryFeatureTypes;
         private final boolean useExternalExpressionFiles;
+        private final Set<String> filterFields = new java.util.LinkedHashSet<>();
 
-        private Set<String> filterFields = new java.util.LinkedHashSet<>();
+        private RankingExpression firstPhaseRanking;
+        private RankingExpression secondPhaseRanking;
 
         /**
          * Creates a raw rank profile from the given rank profile
          */
-        Deriver(RankProfile rankProfile, QueryProfileRegistry queryProfiles, ImportedMlModels importedModels,
-                AttributeFields attributeFields, ModelContext.Properties deployProperties)
+        Deriver(RankProfile unCompiledRankProfile, RankProfile compiled, AttributeFields attributeFields, ModelContext.Properties deployProperties)
         {
-            this.rankProfile = rankProfile;
-            RankProfile compiled = rankProfile.compile(queryProfiles, importedModels);
+            this.unCompiledRankProfile = unCompiledRankProfile;
             attributeTypes = compiled.getAttributeTypes();
             queryFeatureTypes = compiled.getQueryFeatureTypes();
             useExternalExpressionFiles = deployProperties.featureFlags().useExternalRankExpressions();
-            deriveRankingFeatures(compiled, deployProperties);
+            firstPhaseRanking = compiled.getFirstPhaseRanking();
+            secondPhaseRanking = compiled.getSecondPhaseRanking();
+            summaryFeatures = new LinkedHashSet<>(compiled.getSummaryFeatures());
+            rankFeatures = compiled.getRankFeatures();
+            rerankCount = compiled.getRerankCount();
+            matchPhaseSettings = compiled.getMatchPhaseSettings();
+            numThreadsPerSearch = compiled.getNumThreadsPerSearch();
+            minHitsPerThread = compiled.getMinHitsPerThread();
+            numSearchPartitions = compiled.getNumSearchPartitions();
+            termwiseLimit = compiled.getTermwiseLimit().orElse(deployProperties.featureFlags().defaultTermwiseLimit());
+            keepRankCount = compiled.getKeepRankCount();
+            rankScoreDropLimit = compiled.getRankScoreDropLimit();
+            ignoreDefaultRankFeatures = compiled.getIgnoreDefaultRankFeatures();
+            rankProperties = new ArrayList<>(compiled.getRankProperties());
+
+            Map<String, RankProfile.RankingExpressionFunction> functions = compiled.getFunctions();
+            List<ExpressionFunction> functionExpressions = functions.values().stream().map(f -> f.function()).collect(Collectors.toList());
+            Map<String, String> functionProperties = new LinkedHashMap<>();
+            SerializationContext functionSerializationContext = new FunctionSerializationContext(unCompiledRankProfile, functionExpressions, functionProperties);
+
+            if (firstPhaseRanking != null) {
+                functionProperties.putAll(firstPhaseRanking.getRankProperties(functionSerializationContext));
+            }
+            if (secondPhaseRanking != null) {
+                functionProperties.putAll(secondPhaseRanking.getRankProperties(functionSerializationContext));
+            }
+
+            derivePropertiesAndSummaryFeaturesFromFunctions(functions, functionProperties, functionSerializationContext);
+            deriveOnnxModelFunctionsAndSummaryFeatures(compiled);
+
             deriveRankTypeSetting(compiled, attributeFields);
             deriveFilterFields(compiled);
             deriveWeightProperties(compiled);
@@ -184,44 +208,35 @@ public class RawRankProfile implements RankProfilesConfig.Producer {
             filterFields.addAll(rp.allFilterFields());
         }
 
-        private void deriveRankingFeatures(RankProfile rankProfile, ModelContext.Properties deployProperties) {
-            firstPhaseRanking = rankProfile.getFirstPhaseRanking();
-            secondPhaseRanking = rankProfile.getSecondPhaseRanking();
-            summaryFeatures = new LinkedHashSet<>(rankProfile.getSummaryFeatures());
-            rankFeatures = rankProfile.getRankFeatures();
-            rerankCount = rankProfile.getRerankCount();
-            matchPhaseSettings = rankProfile.getMatchPhaseSettings();
-            numThreadsPerSearch = rankProfile.getNumThreadsPerSearch();
-            minHitsPerThread = rankProfile.getMinHitsPerThread();
-            numSearchPartitions = rankProfile.getNumSearchPartitions();
-            termwiseLimit = rankProfile.getTermwiseLimit().orElse(deployProperties.featureFlags().defaultTermwiseLimit());
-            keepRankCount = rankProfile.getKeepRankCount();
-            rankScoreDropLimit = rankProfile.getRankScoreDropLimit();
-            ignoreDefaultRankFeatures = rankProfile.getIgnoreDefaultRankFeatures();
-            rankProperties = new ArrayList<>(rankProfile.getRankProperties());
-            derivePropertiesAndSummaryFeaturesFromFunctions(rankProfile.getFunctions());
-            deriveOnnxModelFunctionsAndSummaryFeatures(rankProfile);
-        }
-
-        private void derivePropertiesAndSummaryFeaturesFromFunctions(Map<String, RankProfile.RankingExpressionFunction> functions) {
-            if (functions.isEmpty()) return;
-
-            List<ExpressionFunction> functionExpressions = functions.values().stream().map(f -> f.function()).collect(Collectors.toList());
-            Map<String, String> functionProperties = new LinkedHashMap<>();
+        private static class FunctionSerializationContext extends SerializationContext {
+            private final RankProfile rankProfile;
+            FunctionSerializationContext(RankProfile rankProfile, Collection<ExpressionFunction> functions,
+                                         Map<String, String> serializedFunctions)
+            {
+                super(functions, null, serializedFunctions);
+                this.rankProfile = rankProfile;
+            }
 
-            if (firstPhaseRanking != null) {
-                functionProperties.putAll(firstPhaseRanking.getRankProperties(functionExpressions));
+            @Override
+            public String uniqueName(String functionName) {
+                return rankProfile.resolveExpressionName(functionName);
             }
-            if (secondPhaseRanking != null) {
-                functionProperties.putAll(secondPhaseRanking.getRankProperties(functionExpressions));
+            @Override
+            public boolean needSerialization(String functionName) {
+                return functionName.equals(uniqueName(functionName)) && super.needSerialization(functionName);
             }
+        }
 
-            SerializationContext context = new SerializationContext(functionExpressions, null, functionProperties);
-            replaceFunctionSummaryFeatures(context);
+        private void derivePropertiesAndSummaryFeaturesFromFunctions(Map<String, RankProfile.RankingExpressionFunction> functions,
+                                                                     Map<String, String> functionProperties,
+                                                                     SerializationContext functionContext) {
+            if (functions.isEmpty()) return;
+
+            replaceFunctionSummaryFeatures(functionContext);
 
             // First phase, second phase and summary features should add all required functions to the context.
             // However, we need to add any functions not referenced in those anyway for model-evaluation.
-            deriveFunctionProperties(functions, functionExpressions, functionProperties);
+            deriveFunctionProperties(functions, functionProperties, functionContext);
 
             for (Map.Entry<String, String> e : functionProperties.entrySet()) {
                 rankProperties.add(new RankProfile.RankProperty(e.getKey(), e.getValue()));
@@ -229,15 +244,16 @@ public class RawRankProfile implements RankProfilesConfig.Producer {
         }
 
         private void deriveFunctionProperties(Map<String, RankProfile.RankingExpressionFunction> functions,
-                                              List<ExpressionFunction> functionExpressions,
-                                              Map<String, String> functionProperties) {
-            SerializationContext context = new SerializationContext(functionExpressions, null, functionProperties);
+                                              Map<String, String> functionProperties,
+                                              SerializationContext context) {
             for (Map.Entry<String, RankProfile.RankingExpressionFunction> e : functions.entrySet()) {
-                if (useExternalExpressionFiles && rankProfile.getExpressionFile(e.getKey()) != null) continue;
+                if (useExternalExpressionFiles && unCompiledRankProfile.getExpressionFile(e.getKey()) != null) {
+                    continue;
+                }
                 String propertyName = RankingExpression.propertyName(e.getKey());
                 if (context.serializedFunctions().containsKey(propertyName)) continue;
 
-                String expressionString = e.getValue().function().getBody().getRoot().toString(new StringBuilder(), context, null, null).toString();
+                String expressionString = e.getValue().function().getBody().getRoot().toString(context);
 
                 context.addFunctionSerialization(propertyName, expressionString);
                 for (Map.Entry<String, TensorType> argumentType : e.getValue().function().argumentTypes().entrySet())
@@ -259,7 +275,7 @@ public class RawRankProfile implements RankProfilesConfig.Producer {
                 ExpressionFunction function = context.getFunction(referenceNode.getName());
                 if (function != null) {
                     String propertyName = RankingExpression.propertyName(referenceNode.getName());
-                    String expressionString = function.getBody().getRoot().toString(new StringBuilder(), context, null, null).toString();
+                    String expressionString = function.getBody().getRoot().toString(context);
                     context.addFunctionSerialization(propertyName, expressionString);
                     ReferenceNode newReferenceNode = new ReferenceNode("rankingExpression(" + referenceNode.getName() + ")", referenceNode.getArguments().expressions(), referenceNode.getOutput());
                     functionSummaryFeatures.put(referenceNode.getName(), newReferenceNode);
@@ -355,8 +371,8 @@ public class RawRankProfile implements RankProfilesConfig.Producer {
                     properties.add(new Pair<>(property.getName(), property.getValue()));
                 }
             }
-            properties.addAll(deriveRankingPhaseRankProperties(firstPhaseRanking, rankProfile.getFirstPhaseFile(), RankProfile.FIRST_PHASE));
-            properties.addAll(deriveRankingPhaseRankProperties(secondPhaseRanking, rankProfile.getSecondPhaseFile(), RankProfile.SECOND_PHASE));
+            properties.addAll(deriveRankingPhaseRankProperties(firstPhaseRanking, unCompiledRankProfile.getFirstPhaseFile(), RankProfile.FIRST_PHASE));
+            properties.addAll(deriveRankingPhaseRankProperties(secondPhaseRanking, unCompiledRankProfile.getSecondPhaseFile(), RankProfile.SECOND_PHASE));
             for (FieldRankSettings settings : fieldRankSettings.values()) {
                 properties.addAll(settings.deriveRankProperties());
             }
@@ -408,9 +424,7 @@ public class RawRankProfile implements RankProfilesConfig.Producer {
             if (ignoreDefaultRankFeatures) {
                 properties.add(new Pair<>("vespa.dump.ignoredefaultfeatures", String.valueOf(true)));
             }
-            Iterator filterFieldsIterator = filterFields.iterator();
-            while (filterFieldsIterator.hasNext()) {
-                String fieldName = (String) filterFieldsIterator.next();
+            for (String fieldName : filterFields) {
                 properties.add(new Pair<>("vespa.isfilterfield." + fieldName, String.valueOf(true)));
             }
             for (Map.Entry<String, String> attributeType : attributeTypes.entrySet()) {
@@ -432,7 +446,7 @@ public class RawRankProfile implements RankProfilesConfig.Producer {
                 name = phase;
 
             if (useExternalExpressionFiles && (fileName != null)) {
-                properties.add(new Pair<>("vespa.rank." + phase, "rankingExpression(" + rankProfile.getUniqueExpressionName(name) + ")"));
+                properties.add(new Pair<>("vespa.rank." + phase, "rankingExpression(" + unCompiledRankProfile.getUniqueExpressionName(name) + ")"));
             } else if (expression.getRoot() instanceof ReferenceNode) {
                 properties.add(new Pair<>("vespa.rank." + phase, expression.getRoot().toString()));
             } else {
diff --git a/config-model/src/test/derived/rankexpression/rank-profiles.cfg b/config-model/src/test/derived/rankexpression/rank-profiles.cfg
index 0ae31a62b4f..d1f6701e7f1 100644
--- a/config-model/src/test/derived/rankexpression/rank-profiles.cfg
+++ b/config-model/src/test/derived/rankexpression/rank-profiles.cfg
@@ -248,7 +248,7 @@ rankprofile[].fef.property[].value "rankingExpression(m1) * 67"
 rankprofile[].fef.property[].name "vespa.rank.secondphase"
 rankprofile[].fef.property[].value "rankingExpression(secondphase)"
 rankprofile[].fef.property[].name "rankingExpression(secondphase).rankingScript"
-rankprofile[].fef.property[].value "40000 * rankingExpression(m2)"
+rankprofile[].fef.property[].value "40000 * rankingExpression(m2) * rankingExpression(macros_refering_macros_m4)"
 rankprofile[].name "macros-refering-macros-inherited"
 rankprofile[].fef.property[].name "rankingExpression(m1).rankingScript"
 rankprofile[].fef.property[].value "700 * fieldMatch(title).completeness"
@@ -259,7 +259,7 @@ rankprofile[].fef.property[].value "if (isNan(attribute(nrtgmp)) == 1, 0.0, rank
 rankprofile[].fef.property[].name "vespa.rank.secondphase"
 rankprofile[].fef.property[].value "rankingExpression(secondphase)"
 rankprofile[].fef.property[].name "rankingExpression(secondphase).rankingScript"
-rankprofile[].fef.property[].value "3000 * rankingExpression(m2)"
+rankprofile[].fef.property[].value "3000 * rankingExpression(m2) * rankingExpression(macros_refering_macros_m4)"
 rankprofile[].name "macros-refering-macros-inherited2"
 rankprofile[].fef.property[].name "rankingExpression(m1).rankingScript"
 rankprofile[].fef.property[].value "700 * fieldMatch(title).completeness"
@@ -276,11 +276,9 @@ rankprofile[].fef.property[].name "rankingExpression(m2).rankingScript"
 rankprofile[].fef.property[].value "rankingExpression(m1) * 67"
 rankprofile[].fef.property[].name "rankingExpression(m3).rankingScript"
 rankprofile[].fef.property[].value "if (isNan(attribute(nrtgmp)) == 1, 0.0, rankingExpression(m2))"
-rankprofile[].fef.property[].name "rankingExpression(m4).rankingScript"
-rankprofile[].fef.property[].value "701 * fieldMatch(title).completeness"
 rankprofile[].fef.property[].name "rankingExpression(m5).rankingScript"
-rankprofile[].fef.property[].value "if (isNan(attribute(glmpfw)) == 1, rankingExpression(m1), rankingExpression(m4))"
+rankprofile[].fef.property[].value "if (isNan(attribute(glmpfw)) == 1, rankingExpression(m1), rankingExpression(macros_refering_macros_m4))"
 rankprofile[].fef.property[].name "vespa.rank.secondphase"
 rankprofile[].fef.property[].value "rankingExpression(secondphase)"
 rankprofile[].fef.property[].name "rankingExpression(secondphase).rankingScript"
-rankprofile[].fef.property[].value "3000 * rankingExpression(m2)"
+rankprofile[].fef.property[].value "3000 * rankingExpression(m2) * rankingExpression(macros_refering_macros_m4)"
diff --git a/config-model/src/test/derived/rankexpression/rankexpression.sd b/config-model/src/test/derived/rankexpression/rankexpression.sd
index d3e0057cfe1..20f9c7a9160 100644
--- a/config-model/src/test/derived/rankexpression/rankexpression.sd
+++ b/config-model/src/test/derived/rankexpression/rankexpression.sd
@@ -276,7 +276,7 @@ search rankexpression {
     
     second-phase {
       expression {
-        40000 * m2
+        40000 * m2 * m4
       }
     }
     
@@ -291,14 +291,9 @@ search rankexpression {
         )
       }
     }
-    macro m4() {
-      expression {
-         701 * fieldMatch(title).completeness
-       }
-    }
     second-phase {
       expression {
-        3000 * m2
+        3000 * m2 * m4
       }
     }
   }
@@ -324,4 +319,3 @@ search rankexpression {
 
 }
 
-