Refactor

8 files changed, 145 insertions, 127 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 659e027586f..dc59d9cb3e5 100644
--- a/config-model/src/main/java/com/yahoo/searchdefinition/FeatureNames.java
+++ b/config-model/src/main/java/com/yahoo/searchdefinition/FeatureNames.java
@@ -5,13 +5,10 @@
  */
 package com.yahoo.searchdefinition;
 
-import com.yahoo.searchlib.rankingexpression.rule.ReferenceNode;
+import com.yahoo.searchlib.rankingexpression.Reference;
 
-import java.util.Arrays;
-import java.util.List;
 import java.util.Optional;
 import java.util.regex.Pattern;
-import java.util.stream.Collectors;
 
 /**
  * Utility methods for query, document and constant rank feature names
@@ -22,16 +19,16 @@ public class FeatureNames {
 
     private static final Pattern identifierRegexp = Pattern.compile("[A-Za-z0-9_][A-Za-z0-9_-]*");
 
-    public static ReferenceNode.Reference asConstantFeature(String constantName) {
-        return ReferenceNode.Reference.simple("constant", quoteIfNecessary(constantName));
+    public static Reference asConstantFeature(String constantName) {
+        return Reference.simple("constant", quoteIfNecessary(constantName));
     }
 
-    public static ReferenceNode.Reference asAttributeFeature(String attributeName) {
-        return ReferenceNode.Reference.simple("attribute", quoteIfNecessary(attributeName));
+    public static Reference asAttributeFeature(String attributeName) {
+        return Reference.simple("attribute", quoteIfNecessary(attributeName));
     }
 
-    public static ReferenceNode.Reference asQueryFeature(String propertyName) {
-        return ReferenceNode.Reference.simple("query", quoteIfNecessary(propertyName));
+    public static Reference asQueryFeature(String propertyName) {
+        return Reference.simple("query", quoteIfNecessary(propertyName));
     }
 
     /**
@@ -39,7 +36,7 @@ public class FeatureNames {
      * or empty if it is not a valid query, attribute or constant feature name
      */
     public static Optional<String> argumentOf(String feature) {
-        Optional<ReferenceNode.Reference> reference = ReferenceNode.Reference.simple(feature);
+        Optional<Reference> reference = Reference.simple(feature);
         if ( ! reference.isPresent()) return Optional.empty();
         if ( ! ( reference.get().name().equals("attribute") ||
                  reference.get().name().equals("constant") ||
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 43e8c7db357..6b8f1f64bb2 100644
--- a/config-model/src/main/java/com/yahoo/searchdefinition/MapEvaluationTypeContext.java
+++ b/config-model/src/main/java/com/yahoo/searchdefinition/MapEvaluationTypeContext.java
@@ -3,6 +3,7 @@ package com.yahoo.searchdefinition;
 
 import com.yahoo.searchlib.rankingexpression.ExpressionFunction;
 import com.yahoo.searchlib.rankingexpression.RankingExpression;
+import com.yahoo.searchlib.rankingexpression.Reference;
 import com.yahoo.searchlib.rankingexpression.parser.ParseException;
 import com.yahoo.searchlib.rankingexpression.rule.Arguments;
 import com.yahoo.searchlib.rankingexpression.rule.ExpressionNode;
@@ -29,7 +30,7 @@ import java.util.Optional;
  */
 public class MapEvaluationTypeContext extends FunctionReferenceContext implements TypeContext {
 
-    private final Map<ReferenceNode.Reference, TensorType> featureTypes = new HashMap<>();
+    private final Map<Reference, TensorType> featureTypes = new HashMap<>();
 
     public MapEvaluationTypeContext(Collection<ExpressionFunction> functions) {
         super(functions);
@@ -37,23 +38,23 @@ public class MapEvaluationTypeContext extends FunctionReferenceContext implement
 
     public MapEvaluationTypeContext(Map<String, ExpressionFunction> functions,
                                     Map<String, String> bindings,
-                                    Map<ReferenceNode.Reference, TensorType> featureTypes) {
+                                    Map<Reference, TensorType> featureTypes) {
         super(functions, bindings);
         this.featureTypes.putAll(featureTypes);
     }
 
     public void setType(Name name, TensorType type) {
         // TODO: Use a type parameter if we do this both here and in getType ...
-        if ( ! (name instanceof ReferenceNode.Reference))
+        if ( ! (name instanceof Reference))
             throw new IllegalArgumentException("Not expecting unstructured names here");
-        featureTypes.put((ReferenceNode.Reference)name, type);
+        featureTypes.put((Reference)name, type);
     }
 
     @Override
     public TensorType getType(Name name) {
-        if ( ! (name instanceof ReferenceNode.Reference))
+        if ( ! (name instanceof Reference))
             throw new IllegalArgumentException("Not expecting unstructured names here");
-        ReferenceNode.Reference reference = (ReferenceNode.Reference)name;
+        Reference reference = (Reference)name;
 
         Optional<String> binding = boundIdentifier(reference);
         if (binding.isPresent()) {
@@ -70,7 +71,7 @@ public class MapEvaluationTypeContext extends FunctionReferenceContext implement
         if (isSimpleFeature(reference)) {
             // The argument may be a local identifier bound to the actual value
             String argument = simpleArgument(reference.arguments()).get();
-            reference = ReferenceNode.Reference.simple(reference.name(), bindings.getOrDefault(argument, argument));
+            reference = Reference.simple(reference.name(), bindings.getOrDefault(argument, argument));
             return featureTypes.get(reference);
         }
 
@@ -89,7 +90,7 @@ public class MapEvaluationTypeContext extends FunctionReferenceContext implement
      * Returns the binding if this reference is a simple identifier which is bound in this context.
      * Returns empty otherwise.
      */
-    private Optional<String> boundIdentifier(ReferenceNode.Reference reference) {
+    private Optional<String> boundIdentifier(Reference reference) {
         if ( ! reference.arguments().isEmpty()) return Optional.empty();
         if ( reference.output() != null) return Optional.empty();
         return Optional.ofNullable(bindings.get(reference.name()));
@@ -100,7 +101,7 @@ public class MapEvaluationTypeContext extends FunctionReferenceContext implement
      * ("attribute(name)", "constant(name)" or "query(name)").
      * We disregard the output because all outputs under a simple feature have the same type.
      */
-    private boolean isSimpleFeature(ReferenceNode.Reference reference) {
+    private boolean isSimpleFeature(Reference reference) {
         Optional<String> argument = simpleArgument(reference.arguments());
         if ( ! argument.isPresent()) return false;
         return reference.name().equals("attribute") ||
@@ -124,7 +125,7 @@ public class MapEvaluationTypeContext extends FunctionReferenceContext implement
         return Optional.of(refArgument.getName());
     }
 
-    private Optional<ExpressionFunction> functionInvocation(ReferenceNode.Reference reference) {
+    private Optional<ExpressionFunction> functionInvocation(Reference reference) {
         if (reference.output() != null) return Optional.empty();
         return Optional.ofNullable(functions().get(reference.name()));
     }
@@ -154,7 +155,7 @@ public class MapEvaluationTypeContext extends FunctionReferenceContext implement
         throw new IllegalArgumentException(description + " must be an identifier but is '" + expression + "'");
     }
 
-    public Map<ReferenceNode.Reference, TensorType> featureTypes() {
+    public Map<Reference, TensorType> featureTypes() {
         return Collections.unmodifiableMap(featureTypes);
     }
 
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 0b0c0c9e0ca..bd645422d50 100644
--- a/config-model/src/main/java/com/yahoo/searchdefinition/RankProfile.java
+++ b/config-model/src/main/java/com/yahoo/searchdefinition/RankProfile.java
@@ -12,6 +12,7 @@ import com.yahoo.searchdefinition.parser.ParseException;
 import com.yahoo.searchlib.rankingexpression.ExpressionFunction;
 import com.yahoo.searchlib.rankingexpression.FeatureList;
 import com.yahoo.searchlib.rankingexpression.RankingExpression;
+import com.yahoo.searchlib.rankingexpression.Reference;
 import com.yahoo.searchlib.rankingexpression.evaluation.TensorValue;
 import com.yahoo.searchlib.rankingexpression.evaluation.Value;
 import com.yahoo.searchlib.rankingexpression.rule.ReferenceNode;
@@ -767,7 +768,7 @@ public class RankProfile implements Serializable, Cloneable {
         for (QueryProfileType queryProfileType : queryProfiles.getTypeRegistry().allComponents()) {
             for (FieldDescription field : queryProfileType.declaredFields().values()) {
                 TensorType type = field.getType().asTensorType();
-                Optional<ReferenceNode.Reference> feature = ReferenceNode.Reference.simple(field.getName());
+                Optional<Reference> feature = Reference.simple(field.getName());
                 if ( ! feature.isPresent() || ! feature.get().name().equals("query")) continue;
 
                 TensorType existingType = context.getType(feature.get());
diff --git a/searchlib/src/main/java/com/yahoo/searchlib/rankingexpression/Reference.java b/searchlib/src/main/java/com/yahoo/searchlib/rankingexpression/Reference.java
new file mode 100644
index 00000000000..05562f9c933
--- /dev/null
+++ b/searchlib/src/main/java/com/yahoo/searchlib/rankingexpression/Reference.java
@@ -0,0 +1,104 @@
+// Copyright 2018 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
+package com.yahoo.searchlib.rankingexpression;
+
+import com.yahoo.searchlib.rankingexpression.rule.Arguments;
+import com.yahoo.searchlib.rankingexpression.rule.ExpressionNode;
+import com.yahoo.searchlib.rankingexpression.rule.ReferenceNode;
+import com.yahoo.tensor.evaluation.TypeContext;
+
+import java.util.Objects;
+import java.util.Optional;
+import java.util.stream.Collectors;
+
+/**
+ * A reference to a feature, function, or value in ranking expressions
+ *
+ * @author bratseth
+ */
+public class Reference extends TypeContext.Name {
+
+    private final String name;
+    private final Arguments arguments;
+
+    /** The output, or null if none */
+    private final String output;
+
+    public Reference(String name, Arguments arguments, String output) {
+        super(name);
+        Objects.requireNonNull(name, "name cannot be null");
+        Objects.requireNonNull(arguments, "arguments cannot be null");
+        this.name = name;
+        this.arguments = arguments;
+        this.output = output;
+    }
+
+    public String name() { return name; }
+    public Arguments arguments() { return arguments; }
+    public String output() { return output; }
+
+    /** Creates a reference to a simple feature consisting of a name and a single argument */
+    public static Reference simple(String name, String argumentValue) {
+        return new Reference(name,
+                             new Arguments(new ReferenceNode(argumentValue)),
+                             null);
+    }
+
+    /**
+     * Returns the given simple feature as a reference, or empty if it is not a valid simple
+     * feature string on the form name(argument).
+     */
+    public static Optional<Reference> simple(String feature) {
+        int startParenthesis = feature.indexOf('(');
+        if (startParenthesis < 0)
+            return Optional.empty();
+        int endParenthesis = feature.lastIndexOf(')');
+        String featureName = feature.substring(0, startParenthesis);
+        if (startParenthesis < 1 || endParenthesis < startParenthesis) return Optional.empty();
+        String argument = feature.substring(startParenthesis + 1, endParenthesis);
+        if (argument.startsWith("'") || argument.startsWith("\""))
+            argument = argument.substring(1);
+        if (argument.endsWith("'") || argument.endsWith("\""))
+            argument = argument.substring(0, argument.length() - 1);
+        return Optional.of(simple(featureName, argument));
+    }
+
+    /** Returns whether this is a simple identifier - no arguments or output */
+    public boolean isIdentifier() {
+        return this.arguments.expressions().size() == 0 && output == null;
+    }
+
+    public Reference withArguments(Arguments arguments) {
+        return new Reference(name, arguments, output);
+    }
+
+    public Reference withOutput(String output) {
+        return new Reference(name, arguments, output);
+    }
+
+    @Override
+    public boolean equals(Object o) {
+        if (o == this) return true;
+        if ( ! (o instanceof Reference)) return false;
+        Reference other = (Reference)o;
+        if ( ! Objects.equals(other.name, this.name)) return false;
+        if ( ! Objects.equals(other.arguments, this.arguments)) return false;
+        if ( ! Objects.equals(other.output, this.output)) return false;
+        return true;
+    }
+
+    @Override
+    public int hashCode() {
+        return Objects.hash(name, arguments, output);
+    }
+
+    @Override
+    public String toString() {
+        StringBuilder b = new StringBuilder(name);
+        if (arguments != null && arguments.expressions().size() > 0)
+            b.append("(").append(arguments.expressions().stream().map(ExpressionNode::toString).collect(Collectors.joining(","))).append(")");
+        if (output !=null)
+            b.append(".").append(output);
+        return b.toString();
+    }
+
+}
diff --git a/searchlib/src/main/java/com/yahoo/searchlib/rankingexpression/rule/NameNode.java b/searchlib/src/main/java/com/yahoo/searchlib/rankingexpression/rule/NameNode.java
index f55ed59b65c..759d966e10b 100755
--- a/searchlib/src/main/java/com/yahoo/searchlib/rankingexpression/rule/NameNode.java
+++ b/searchlib/src/main/java/com/yahoo/searchlib/rankingexpression/rule/NameNode.java
@@ -14,6 +14,7 @@ import java.util.Deque;
  *
  * @author Simon Thoresen
  */
+// TODO: This is achieved by ReferenceNode in almost all cases - remove this
 public final class NameNode extends ExpressionNode {
 
     private final String name;
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 8c6bc6a6291..e390745dbc7 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
@@ -3,6 +3,7 @@ package com.yahoo.searchlib.rankingexpression.rule;
 
 import com.yahoo.searchlib.rankingexpression.ExpressionFunction;
 import com.yahoo.searchlib.rankingexpression.RankingExpression;
+import com.yahoo.searchlib.rankingexpression.Reference;
 import com.yahoo.searchlib.rankingexpression.evaluation.Context;
 import com.yahoo.searchlib.rankingexpression.evaluation.Value;
 import com.yahoo.tensor.TensorType;
@@ -11,10 +12,6 @@ import com.yahoo.tensor.evaluation.TypeContext;
 import java.util.ArrayDeque;
 import java.util.Deque;
 import java.util.List;
-import java.util.Objects;
-import java.util.Optional;
-import java.util.regex.Pattern;
-import java.util.stream.Collectors;
 
 /**
  * A node referring either to a value in the context or to a named ranking expression (function aka macro).
@@ -22,8 +19,6 @@ import java.util.stream.Collectors;
  * @author simon
  * @author bratseth
  */
-// TODO: Using the same node to represent formal function argument, the function itself, and to features is confusing.
-//       At least the first two should be split into separate classes.
 public final class ReferenceNode extends CompositeNode {
 
     private final Reference reference;
@@ -133,7 +128,11 @@ public final class ReferenceNode extends CompositeNode {
 
     @Override
     public Value evaluate(Context context) {
-        return context.get(reference.toString());
+        // TODO: Context should accept a Reference instead.
+
+        if (reference.isIdentifier())
+            return context.get(reference.name());
+        return context.get(getName(), getArguments(), getOutput());
     }
 
     @Override
@@ -141,94 +140,4 @@ public final class ReferenceNode extends CompositeNode {
         return setArguments(newChildren);
     }
 
-    /** Wraps the content of this in a form which can be passed to a type context */
-    // TODO: Extract to top level?
-    public static class Reference extends TypeContext.Name {
-
-        private final String name;
-        private final Arguments arguments;
-
-        /** The output, or null if none */
-        private final String output;
-
-        public Reference(String name, Arguments arguments, String output) {
-            super(name);
-            Objects.requireNonNull(name, "name cannot be null");
-            Objects.requireNonNull(arguments, "arguments cannot be null");
-            this.name = name;
-            this.arguments = arguments;
-            this.output = output;
-        }
-
-        public String name() { return name; }
-        public Arguments arguments() { return arguments; }
-        public String output() { return output; }
-
-        /** Creates a reference to a simple feature consisting of a name and a single argument */
-        public static Reference simple(String name, String argumentValue) {
-            return new Reference(name,
-                                 new Arguments(new ReferenceNode(argumentValue)),
-                                 null);
-        }
-
-        /**
-         * Returns the given simple feature as a reference, or empty if it is not a valid simple
-         * feature string on the form name(argument).
-         */
-        public static Optional<Reference> simple(String feature) {
-            int startParenthesis = feature.indexOf('(');
-            if (startParenthesis < 0)
-                return Optional.empty();
-            int endParenthesis = feature.lastIndexOf(')');
-            String featureName = feature.substring(0, startParenthesis);
-            if (startParenthesis < 1 || endParenthesis < startParenthesis) return Optional.empty();
-            String argument = feature.substring(startParenthesis + 1, endParenthesis);
-            if (argument.startsWith("'") || argument.startsWith("\""))
-                argument = argument.substring(1);
-            if (argument.endsWith("'") || argument.endsWith("\""))
-                argument = argument.substring(0, argument.length() - 1);
-            return Optional.of(simple(featureName, argument));
-        }
-
-        /** Returns whether this is a simple identifier - no arguments or output */
-        public boolean isIdentifier() {
-            return this.arguments.expressions().size() == 0 && output == null;
-        }
-
-        public Reference withArguments(Arguments arguments) {
-            return new Reference(name, arguments, output);
-        }
-
-        public Reference withOutput(String output) {
-            return new Reference(name, arguments, output);
-        }
-
-        @Override
-        public boolean equals(Object o) {
-            if (o == this) return true;
-            if ( ! (o instanceof Reference)) return false;
-            Reference other = (Reference)o;
-            if ( ! Objects.equals(other.name, this.name)) return false;
-            if ( ! Objects.equals(other.arguments, this.arguments)) return false;
-            if ( ! Objects.equals(other.output, this.output)) return false;
-            return true;
-        }
-
-        @Override
-        public int hashCode() {
-            return Objects.hash(name, arguments, output);
-        }
-
-        @Override
-        public String toString() {
-            StringBuilder b = new StringBuilder(name);
-            if (arguments != null && arguments.expressions().size() > 0)
-                b.append("(").append(arguments.expressions().stream().map(ExpressionNode::toString).collect(Collectors.joining(","))).append(")");
-            if (output !=null)
-                b.append(".").append(output);
-            return b.toString();
-        }
-
-    }
-
 }
diff --git a/searchlib/src/test/java/com/yahoo/searchlib/rankingexpression/evaluation/EvaluationTestCase.java b/searchlib/src/test/java/com/yahoo/searchlib/rankingexpression/evaluation/EvaluationTestCase.java
index e9030cf5852..f2122bb5da9 100644
--- a/searchlib/src/test/java/com/yahoo/searchlib/rankingexpression/evaluation/EvaluationTestCase.java
+++ b/searchlib/src/test/java/com/yahoo/searchlib/rankingexpression/evaluation/EvaluationTestCase.java
@@ -378,8 +378,13 @@ public class EvaluationTestCase {
     private static class StructuredTestContext extends MapContext {
 
         @Override
+        public Value get(String feature) {
+            throw new RuntimeException("Called simple get for feature " + feature);
+        }
+
+        @Override
         public Value get(String name, Arguments arguments, String output) {
-            if (!name.equals("average")) {
+            if ( ! name.equals("average")) {
                 throw new IllegalArgumentException("Unknown operation '" + name + "'");
             }
             if (arguments.expressions().size() != 2) {
diff --git a/searchlib/src/test/java/com/yahoo/searchlib/rankingexpression/evaluation/TypeResolutionTestCase.java b/searchlib/src/test/java/com/yahoo/searchlib/rankingexpression/evaluation/TypeResolutionTestCase.java
index da546dd30d0..fc73bcd3f79 100644
--- a/searchlib/src/test/java/com/yahoo/searchlib/rankingexpression/evaluation/TypeResolutionTestCase.java
+++ b/searchlib/src/test/java/com/yahoo/searchlib/rankingexpression/evaluation/TypeResolutionTestCase.java
@@ -3,8 +3,8 @@
 package com.yahoo.searchlib.rankingexpression.evaluation;
 
 import com.yahoo.searchlib.rankingexpression.RankingExpression;
+import com.yahoo.searchlib.rankingexpression.Reference;
 import com.yahoo.searchlib.rankingexpression.parser.ParseException;
-import com.yahoo.searchlib.rankingexpression.rule.ReferenceNode;
 import com.yahoo.tensor.TensorType;
 import com.yahoo.tensor.evaluation.TypeContext;
 import org.junit.Test;
@@ -20,15 +20,15 @@ public class TypeResolutionTestCase {
     @Test
     public void testTypeResolution() {
         MapTypeContext context = new MapTypeContext();
-        context.setType(ReferenceNode.Reference.simple("query", "x1"),
+        context.setType(Reference.simple("query", "x1"),
                         TensorType.fromSpec("tensor(x[])"));
-        context.setType(ReferenceNode.Reference.simple("query", "x2"),
+        context.setType(Reference.simple("query", "x2"),
                         TensorType.fromSpec("tensor(x[10])"));
-        context.setType(ReferenceNode.Reference.simple("query", "y1"),
+        context.setType(Reference.simple("query", "y1"),
                         TensorType.fromSpec("tensor(y[])"));
-        context.setType(ReferenceNode.Reference.simple("query", "xy1"),
+        context.setType(Reference.simple("query", "xy1"),
                         TensorType.fromSpec("tensor(x[10],y[])"));
-        context.setType(ReferenceNode.Reference.simple("query", "xy2"),
+        context.setType(Reference.simple("query", "xy2"),
                         TensorType.fromSpec("tensor(x[],y[10])"));
 
         assertType("tensor(x[])", "query(x1)", context);