Merge pull request #3788 from vespa-engine/bratseth/dont-remove-recall-neutral-terms

Bratseth/dont remove recall neutral terms

10 files changed, 131 insertions, 99 deletions

diff --git a/container-search/src/main/java/com/yahoo/prelude/query/AndSegmentItem.java b/container-search/src/main/java/com/yahoo/prelude/query/AndSegmentItem.java
index de66d4c4408..91378eeca21 100644
--- a/container-search/src/main/java/com/yahoo/prelude/query/AndSegmentItem.java
+++ b/container-search/src/main/java/com/yahoo/prelude/query/AndSegmentItem.java
@@ -9,7 +9,7 @@ import edu.umd.cs.findbugs.annotations.NonNull;
  * An immutable and'ing of a collection of sub-expressions. It does not extend
  * AndItem to avoid code using instanceof handling it as an AndItem.
  *
- * @author <a href="mailto:steinar@yahoo-inc.com">Steinar Knutsen</a>
+ * @author Steinar Knutsen
  */
 public class AndSegmentItem extends SegmentItem implements BlockItem {
 
diff --git a/container-search/src/main/java/com/yahoo/prelude/query/Item.java b/container-search/src/main/java/com/yahoo/prelude/query/Item.java
index 96d113723e8..e8e0a07941e 100644
--- a/container-search/src/main/java/com/yahoo/prelude/query/Item.java
+++ b/container-search/src/main/java/com/yahoo/prelude/query/Item.java
@@ -149,7 +149,7 @@ public abstract class Item implements Cloneable {
     /**
      * Sets whether this is a filter term.
      * This indicates that the term origins from the filter parameter in the search API.
-     * The search backend does to handle filter terms any different than non-filter terms.
+     * The search backend does not handle filter terms any different than non-filter terms.
      */
     public void setFilter(boolean filter) {
         if (filter) {
diff --git a/container-search/src/main/java/com/yahoo/prelude/query/PhraseSegmentItem.java b/container-search/src/main/java/com/yahoo/prelude/query/PhraseSegmentItem.java
index ff310cafc2e..a19a6e53963 100644
--- a/container-search/src/main/java/com/yahoo/prelude/query/PhraseSegmentItem.java
+++ b/container-search/src/main/java/com/yahoo/prelude/query/PhraseSegmentItem.java
@@ -91,8 +91,7 @@ public class PhraseSegmentItem extends IndexedSegmentItem {
         if (item instanceof WordItem) {
             addWordItem((WordItem) item);
         } else {
-            throw new IllegalArgumentException(
-                    "Can not add " + item + " to a segment phrase");
+            throw new IllegalArgumentException("Can not add " + item + " to a segment phrase");
         }
     }
 
@@ -117,6 +116,7 @@ public class PhraseSegmentItem extends IndexedSegmentItem {
         super.encodeThis(buffer); // takes care of index bytes
     }
 
+    @Override
     public int encode(ByteBuffer buffer) {
         encodeThis(buffer);
         return encodeContent(buffer, 1);
@@ -199,4 +199,5 @@ public class PhraseSegmentItem extends IndexedSegmentItem {
         super.disclose(discloser);
         discloser.addProperty("explicit", explicit);
     }
+
 }
diff --git a/container-search/src/main/java/com/yahoo/prelude/querytransform/QueryRewrite.java b/container-search/src/main/java/com/yahoo/prelude/querytransform/QueryRewrite.java
index 2c724e2b27b..d4b3175aac9 100644
--- a/container-search/src/main/java/com/yahoo/prelude/querytransform/QueryRewrite.java
+++ b/container-search/src/main/java/com/yahoo/prelude/querytransform/QueryRewrite.java
@@ -22,18 +22,14 @@ import com.yahoo.search.result.Hit;
 // TODO: This overlaps with QueryCanonicalizer
 public class QueryRewrite {
 
-    private enum Recall {
-        RECALLS_EVERYTHING,
-        RECALLS_NOTHING,
-        UNKNOWN_RECALL
-    }
+    private enum Recall { RECALLS_EVERYTHING, RECALLS_NOTHING, UNKNOWN_RECALL }
 
+    // ------------------- Start public API
+    
     /**
      * Optimize multiple NotItems under and or by collapsing them in to one and leaving
      * the positive ones behind in its place and moving itself with the original and as its positive item
      * and the union of all the negative items of all the original NotItems as its negative items.
-     *
-     * @param query to optimize
      */
     public static void optimizeAndNot(Query query) {
         Item root = query.getModel().getQueryTree().getRoot();
@@ -42,12 +38,52 @@ public class QueryRewrite {
             query.getModel().getQueryTree().setRoot(possibleNewRoot);
         }
     }
+
+    /**
+     * Optimizes the given query tree based on its {@link Model#getRestrict()} parameter, if any.
+     */
+    public static void optimizeByRestrict(Query query) {
+        if (query.getModel().getRestrict().size() != 1) {
+            return;
+        }
+        Item root = query.getModel().getQueryTree().getRoot();
+        if (optimizeByRestrict(root, query.getModel().getRestrict().iterator().next()) == Recall.RECALLS_NOTHING) {
+            query.getModel().getQueryTree().setRoot(new NullItem());
+        }
+    }
+
+    /**
+     * Collapses all single-child {@link CompositeItem}s into their parent item.
+     */
+    public static void collapseSingleComposites(Query query) {
+        Item oldRoot = query.getModel().getQueryTree().getRoot();
+        Item newRoot = collapseSingleComposites(oldRoot);
+        if (oldRoot != newRoot) {
+            query.getModel().getQueryTree().setRoot(newRoot);
+        }
+    }
+
+    /**
+     * Replaces and {@link SimpleIndexedItem} searching in the {@link Hit#SDDOCNAME_FIELD} with an item
+     * appropriate for the search node.
+     */
+    public static void rewriteSddocname(Query query) {
+        Item oldRoot = query.getModel().getQueryTree().getRoot();
+        Item newRoot = rewriteSddocname(oldRoot);
+        if (oldRoot != newRoot) {
+            query.getModel().getQueryTree().setRoot(newRoot);
+        }
+    }
+
+    // ------------------- End public API
+
     private static Item optimizeAndNot(Item node) {
         if (node instanceof CompositeItem) {
             return extractAndNotRecursively((CompositeItem) node);
         }
         return node;
     }
+
     private static CompositeItem extractAndNotRecursively(CompositeItem parent) {
         for (int i = 0; i < parent.getItemCount(); i++) {
             Item child = parent.getItem(i);
@@ -61,6 +97,7 @@ public class QueryRewrite {
         }
         return parent;
     }
+
     private static CompositeItem extractAndNot(AndItem parent) {
         NotItem theOnlyNot = null;
         for (int i = 0; i < parent.getItemCount(); i++) {
@@ -80,20 +117,6 @@ public class QueryRewrite {
         }
         return (theOnlyNot != null) ? theOnlyNot : parent;
     }
-    /**
-     * Optimizes the given query tree based on its {@link Model#getRestrict()} parameter, if any.
-     *
-     * @param query to optimize.
-     */
-    public static void optimizeByRestrict(Query query) {
-        if (query.getModel().getRestrict().size() != 1) {
-            return;
-        }
-        Item root = query.getModel().getQueryTree().getRoot();
-        if (optimizeByRestrict(root, query.getModel().getRestrict().iterator().next()) == Recall.RECALLS_NOTHING) {
-            query.getModel().getQueryTree().setRoot(new NullItem());
-        }
-    }
 
     private static Recall optimizeByRestrict(Item item, String restrictParam) {
         if (item instanceof SimpleIndexedItem) {
@@ -127,68 +150,57 @@ public class QueryRewrite {
         for (int i = item.getItemCount(); --i >= 1; ) {
             Item child = item.getItem(i);
             switch (optimizeByRestrict(child, restrictParam)) {
-            case RECALLS_EVERYTHING:
-                return Recall.RECALLS_NOTHING;
-            case RECALLS_NOTHING:
-                item.removeItem(i);
-                break;
+                case RECALLS_EVERYTHING:
+                    return Recall.RECALLS_NOTHING;
+                case RECALLS_NOTHING:
+                    item.removeItem(i);
+                    break;
             }
         }
         return Recall.UNKNOWN_RECALL;
     }
 
     private static Recall optimizeCompositeItemByRestrict(CompositeItem item, String restrictParam) {
+        Recall recall = Recall.UNKNOWN_RECALL;
         for (int i = item.getItemCount(); --i >= 0; ) {
             switch (optimizeByRestrict(item.getItem(i), restrictParam)) {
-            case RECALLS_EVERYTHING:
-                if ((item instanceof OrItem) || (item instanceof EquivItem)) {
-                    retainChild(item, i);
-                    return Recall.RECALLS_EVERYTHING;
-                } else if ((item instanceof AndItem) || (item instanceof NearItem)) {
-                    item.removeItem(i);
-                } else if (item instanceof RankItem) {
-                    // empty
-                } else {
-                    throw new UnsupportedOperationException(item.getClass().getName());
-                }
-                break;
-            case RECALLS_NOTHING:
-                if ((item instanceof OrItem) || (item instanceof EquivItem)) {
-                    item.removeItem(i);
-                } else if ((item instanceof AndItem) || (item instanceof NearItem)) {
-                    return Recall.RECALLS_NOTHING;
-                } else if (item instanceof RankItem) {
-                    item.removeItem(i);
-                } else {
-                    throw new UnsupportedOperationException(item.getClass().getName());
-                }
-                break;
+                case RECALLS_EVERYTHING:
+                    if ((item instanceof OrItem) || (item instanceof EquivItem)) {
+                        removeOtherNonrankedChildren(item, i);
+                        recall = Recall.RECALLS_EVERYTHING;
+                    } else if ((item instanceof AndItem) || (item instanceof NearItem)) {
+                        item.removeItem(i);
+                    } else if (item instanceof RankItem) {
+                        // empty
+                    } else {
+                        throw new UnsupportedOperationException(item.getClass().getName());
+                    }
+                    break;
+                case RECALLS_NOTHING:
+                    if ((item instanceof OrItem) || (item instanceof EquivItem)) {
+                        item.removeItem(i);
+                    } else if ((item instanceof AndItem) || (item instanceof NearItem)) {
+                        return Recall.RECALLS_NOTHING;
+                    } else if (item instanceof RankItem) {
+                        item.removeItem(i);
+                    } else {
+                        throw new UnsupportedOperationException(item.getClass().getName());
+                    }
+                    break;
             }
         }
-        return Recall.UNKNOWN_RECALL;
-    }
-
-    private static void retainChild(CompositeItem item, int childIdx) {
-        Item child = item.removeItem(childIdx);
-        for (int i = item.getItemCount(); --i >= 0; ) {
-            item.removeItem(i);
-        }
-        item.addItem(child);
+        return recall;
     }
 
-    /**
-     * Collapses all single-child {@link CompositeItem}s into their parent item.
-     *
-     * @param query The query whose composites to collapse.
-     */
-    public static void collapseSingleComposites(Query query) {
-        Item oldRoot = query.getModel().getQueryTree().getRoot();
-        Item newRoot = collapseSingleComposites(oldRoot);
-        if (oldRoot != newRoot) {
-            query.getModel().getQueryTree().setRoot(newRoot);
+    private static void removeOtherNonrankedChildren(CompositeItem parent, int indexOfChildToKeep) {
+        Item childToKeep = parent.getItem(indexOfChildToKeep);
+        for (int i = parent.getItemCount(); --i >= 0; ) {
+            Item child = parent.getItem(i);
+            if ( child != childToKeep && ! parent.getItem(i).isRanked())
+                parent.removeItem(i);
         }
     }
-
+    
     private static Item collapseSingleComposites(Item item) {
         if (!(item instanceof CompositeItem)) {
             return item;
@@ -205,20 +217,6 @@ public class QueryRewrite {
         return numChildren == 1 ? parent.getItem(0) : item;
     }
 
-    /**
-     * Replaces and {@link SimpleIndexedItem} searching in the {@link Hit#SDDOCNAME_FIELD} with an item
-     * appropriate for the search node.
-     *
-     * @param query The query to rewrite.
-     */
-    public static void rewriteSddocname(Query query) {
-        Item oldRoot = query.getModel().getQueryTree().getRoot();
-        Item newRoot = rewriteSddocname(oldRoot);
-        if (oldRoot != newRoot) {
-            query.getModel().getQueryTree().setRoot(newRoot);
-        }
-    }
-
     private static Item rewriteSddocname(Item item) {
         if (item instanceof CompositeItem) {
             CompositeItem parent = (CompositeItem)item;
@@ -239,4 +237,5 @@ public class QueryRewrite {
         }
         return item;
     }
+
 }
diff --git a/container-search/src/main/java/com/yahoo/prelude/querytransform/RecallSearcher.java b/container-search/src/main/java/com/yahoo/prelude/querytransform/RecallSearcher.java
index 4d9e47d3c15..4490d3c9b1e 100644
--- a/container-search/src/main/java/com/yahoo/prelude/querytransform/RecallSearcher.java
+++ b/container-search/src/main/java/com/yahoo/prelude/querytransform/RecallSearcher.java
@@ -29,7 +29,7 @@ import static com.yahoo.prelude.querytransform.StemmingSearcher.STEMMING;
  *
  * If the "recall" property is unset, this searcher does nothing.
  *
- * @author <a href="mailto:simon@yahoo-inc.com">Simon Thoresen</a>
+ * @author Simon Thoresen
  */
 @After("com.yahoo.search.querytransform.WandSearcher")
 @Before({STEMMING, ACCENT_REMOVAL})
diff --git a/container-search/src/test/java/com/yahoo/prelude/querytransform/test/QueryRewriteTestCase.java b/container-search/src/test/java/com/yahoo/prelude/querytransform/test/QueryRewriteTestCase.java
index d11fc15fe90..11922cf640a 100644
--- a/container-search/src/test/java/com/yahoo/prelude/querytransform/test/QueryRewriteTestCase.java
+++ b/container-search/src/test/java/com/yahoo/prelude/querytransform/test/QueryRewriteTestCase.java
@@ -3,6 +3,7 @@ package com.yahoo.prelude.querytransform.test;
 
 import com.yahoo.prelude.query.AndItem;
 import com.yahoo.prelude.query.NotItem;
+import com.yahoo.prelude.query.OrItem;
 import com.yahoo.prelude.query.WordItem;
 import com.yahoo.prelude.querytransform.QueryRewrite;
 import com.yahoo.search.Query;
@@ -17,14 +18,14 @@ import static org.junit.Assert.assertTrue;
 public class QueryRewriteTestCase {
 
     @Test
-    public void requireThatOptimizeByRestrictSimplifiesORItemsThatHaveFullRecall() {
+    public void requireThatOptimizeByRestrictSimplifiesORItemsThatHaveFullRecallAndDontImpactRank() {
         assertRewritten("sddocname:foo OR sddocname:bar OR sddocname:baz", "foo", "sddocname:foo");
         assertRewritten("sddocname:foo OR sddocname:bar OR sddocname:baz", "bar", "sddocname:bar");
         assertRewritten("sddocname:foo OR sddocname:bar OR sddocname:baz", "baz", "sddocname:baz");
 
-        assertRewritten("lhs OR (sddocname:foo OR sddocname:bar OR sddocname:baz)", "foo", "sddocname:foo");
-        assertRewritten("lhs OR (sddocname:foo OR sddocname:bar OR sddocname:baz)", "bar", "sddocname:bar");
-        assertRewritten("lhs OR (sddocname:foo OR sddocname:bar OR sddocname:baz)", "baz", "sddocname:baz");
+        assertRewritten("lhs OR (sddocname:foo OR sddocname:bar OR sddocname:baz)", "foo", "OR lhs sddocname:foo");
+        assertRewritten("lhs OR (sddocname:foo OR sddocname:bar OR sddocname:baz)", "bar", "OR lhs sddocname:bar");
+        assertRewritten("lhs OR (sddocname:foo OR sddocname:bar OR sddocname:baz)", "baz", "OR lhs sddocname:baz");
 
         assertRewritten("lhs AND (sddocname:foo OR sddocname:bar OR sddocname:baz)", "foo", "lhs");
         assertRewritten("lhs AND (sddocname:foo OR sddocname:bar OR sddocname:baz)", "bar", "lhs");
@@ -32,6 +33,17 @@ public class QueryRewriteTestCase {
     }
 
     @Test
+    public void testRestrictRewriteDoesNotRemoveRankContributingTerms() {
+        Query query = query("sddocname:per OR foo OR bar", "per");
+        assertRewritten(query, "OR sddocname:per foo bar");
+        ((OrItem)query.getModel().getQueryTree().getRoot()).getItem(2).setRanked(false); // set 'bar' unranked
+        assertRewritten(query, "OR sddocname:per foo");
+
+        assertRewritten("sddocname:per OR foo OR (bar AND fuz)", "per", "OR sddocname:per foo (AND bar fuz)");
+
+    }
+
+    @Test
     public void requireThatOptimizeByRestrictSimplifiesANDItemsThatHaveZeroRecall() {
         assertRewritten("sddocname:foo AND bar AND baz", "cox", "NULL");
         assertRewritten("foo AND sddocname:bar AND baz", "cox", "NULL");
@@ -59,15 +71,22 @@ public class QueryRewriteTestCase {
         assertRewritten("sddocname:perder ANDNOT b", "per", "NULL");
         assertRewritten("a ANDNOT sddocname:per a b", "per", "NULL");
     }
-
+    
     @Test
     public void testRestrictRank() {
         assertRewritten("sddocname:per&filter=abc", "espen", "|abc");
         assertRewritten("sddocname:per&filter=abc", "per", "RANK sddocname:per |abc");
     }
 
-    private static void assertRewritten(String queryParam, String restrictParam, String expectedOptimizedQuery) {
-        Query query = new Query("?type=adv&query=" + queryParam.replace(" ", "%20") + "&restrict=" + restrictParam);
+    private static Query query(String queryString, String restrict) {
+        return new Query("?type=adv&query=" + queryString.replace(" ", "%20") + "&restrict=" + restrict);
+    }
+
+    private static void assertRewritten(String query, String restrict, String expectedOptimizedQuery) {
+        assertRewritten(query(query, restrict), expectedOptimizedQuery);
+    }
+
+    private static void assertRewritten(Query query, String expectedOptimizedQuery) {
         QueryRewrite.optimizeByRestrict(query);
         QueryRewrite.collapseSingleComposites(query);
         assertEquals(expectedOptimizedQuery, query.getModel().getQueryTree().toString());
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/DeploymentMetrics.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/DeploymentMetrics.java
index 6812e4cb468..2cfbeaddb42 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/DeploymentMetrics.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/DeploymentMetrics.java
@@ -20,7 +20,7 @@ public class DeploymentMetrics {
     }
 
     public DeploymentMetrics(double queriesPerSecond, double writesPerSecond, double documentCount,
-                      double queryLatencyMillis, double writeLatencyMills) {
+                             double queryLatencyMillis, double writeLatencyMills) {
         this.queriesPerSecond = queriesPerSecond;
         this.writesPerSecond = writesPerSecond;
         this.documentCount = documentCount;
@@ -47,4 +47,5 @@ public class DeploymentMetrics {
     public double writeLatencyMillis() {
         return writeLatencyMills;
     }
+
 }
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/DeploymentMetricsMaintainer.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/DeploymentMetricsMaintainer.java
index 7a771464957..52ffa67c565 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/DeploymentMetricsMaintainer.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/DeploymentMetricsMaintainer.java
@@ -27,7 +27,6 @@ public class DeploymentMetricsMaintainer extends Maintainer {
 
     @Override
     protected void maintain() {
-
         for (Application application : controller().applications().asList()) {
             for (Deployment deployment : application.deployments().values()) {
                 try {
diff --git a/vespajlib/src/main/java/com/yahoo/tensor/Tensor.java b/vespajlib/src/main/java/com/yahoo/tensor/Tensor.java
index 5ad1a8f1e17..5e3af70cba4 100644
--- a/vespajlib/src/main/java/com/yahoo/tensor/Tensor.java
+++ b/vespajlib/src/main/java/com/yahoo/tensor/Tensor.java
@@ -112,6 +112,10 @@ public interface Tensor {
                                                     Collections.singletonList(toDimension)).evaluate();
     }
 
+    default Tensor concat(double argument, String dimension) {
+        return concat(Tensor.Builder.of(TensorType.empty).cell(argument).build(), dimension);
+    }
+
     default Tensor concat(Tensor argument, String dimension) {
         return new Concat(new ConstantTensor(this), new ConstantTensor(argument), dimension).evaluate();
     }
diff --git a/vespajlib/src/test/java/com/yahoo/tensor/TensorTestCase.java b/vespajlib/src/test/java/com/yahoo/tensor/TensorTestCase.java
index c47bfe84373..30078b4a826 100644
--- a/vespajlib/src/test/java/com/yahoo/tensor/TensorTestCase.java
+++ b/vespajlib/src/test/java/com/yahoo/tensor/TensorTestCase.java
@@ -71,9 +71,18 @@ public class TensorTestCase {
         Tensor y =  Tensor.from("{{y:1}:3}");
         Tensor x =  Tensor.from("{{x:0}:5,{x:1}:7}");
         Tensor xy = Tensor.from("{{x:0,y:1}:11, {x:1,y:1}:13}");
-        double nest = y.multiply(x.multiply(xy).sum("x")).sum("y").asDouble();
+        double nest1 = y.multiply(x.multiply(xy).sum("x")).sum("y").asDouble();
+        double nest2 = x.multiply(xy).sum("x").multiply(y).sum("y").asDouble();
         double flat = y.multiply(x).multiply(xy).sum(ImmutableList.of("x","y")).asDouble();
-        assertEquals(nest, flat, 0.000000001);
+        assertEquals(nest1, flat, 0.000000001);
+        assertEquals(nest2, flat, 0.000000001);
+    }
+
+    @Test
+    public void testCombineInDimensionIndexed() {
+        Tensor input =  Tensor.from("tensor(input[]):{{input:0}:3, {input:1}:7}");
+        Tensor result = input.concat(11, "input");
+        assertEquals("{{input:0}:3.0,{input:1}:7.0,{input:2}:11.0}", result.toString());
     }
 
     /** All functions are more throughly tested in searchlib EvaluationTestCase */