Refactor

5 files changed, 123 insertions, 104 deletions

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);