Merge pull request #20681 from vespa-engine/bratseth/not

Bratseth/not

14 files changed, 77 insertions, 81 deletions

diff --git a/container-search/src/main/java/com/yahoo/prelude/query/CompositeItem.java b/container-search/src/main/java/com/yahoo/prelude/query/CompositeItem.java
index aaa4d33c6dc..d3fbeb020f8 100644
--- a/container-search/src/main/java/com/yahoo/prelude/query/CompositeItem.java
+++ b/container-search/src/main/java/com/yahoo/prelude/query/CompositeItem.java
@@ -80,11 +80,6 @@ public abstract class CompositeItem extends Item {
         subitems.add(index, item);
     }
 
-    /** For NOT items, which may wish to insert nulls */
-    void insertNullFirstItem() {
-        subitems.add(0, null);
-    }
-
     /**
      * Returns a subitem
      *
@@ -109,7 +104,7 @@ public abstract class CompositeItem extends Item {
 
         adding(item);
         Item old = subitems.set(index, item);
-        if (old!=item)
+        if (old != item)
             removing(old);
         return old;
     }
@@ -188,9 +183,7 @@ public abstract class CompositeItem extends Item {
         return itemCount;
     }
 
-    /**
-     * Encodes just this item, not it's usual subitems, to the given buffer.
-     */
+    /** Encodes just this item, not its regular subitems, to the given buffer. */
     protected void encodeThis(ByteBuffer buffer) {
         super.encodeThis(buffer);
         IntegerCompressor.putCompressedPositiveNumber(encodingArity(), buffer);
@@ -279,10 +272,7 @@ public abstract class CompositeItem extends Item {
         return code;
     }
 
-    /**
-     * Returns whether this item is of the same class and
-     * contains the same state as the given item
-     */
+    /** Returns whether this item is of the same class and contains the same state as the given item. */
     @Override
     public boolean equals(Object object) {
         if (!super.equals(object)) return false;
@@ -303,17 +293,12 @@ public abstract class CompositeItem extends Item {
     @Override
     public int getTermCount() {
         int terms = 0;
-        for (Item item : subitems) {
+        for (Item item : subitems)
             terms += item.getTermCount();
-        }
         return terms;
     }
 
-    /**
-     * Will return its single child if itself can safely be omitted.
-     *
-     * @return a valid Item or empty Optional if it can not be done
-     */
+    /** Returns the single child of this, if this can be omitted without changes to recall semantics. */
     public Optional<Item> extractSingleChild() {
         return getItemCount() == 1 ? Optional.of(getItem(0)) : Optional.empty();
     }
diff --git a/container-search/src/main/java/com/yahoo/prelude/query/NotItem.java b/container-search/src/main/java/com/yahoo/prelude/query/NotItem.java
index 833b8635f61..6985ed0913c 100644
--- a/container-search/src/main/java/com/yahoo/prelude/query/NotItem.java
+++ b/container-search/src/main/java/com/yahoo/prelude/query/NotItem.java
@@ -7,11 +7,12 @@ import java.util.Objects;
 
 /**
  * A composite item where the first item is positive and the following
- * items are negative items which should be excluded from the result.
+ * items are negative items where matches should exclude the document should from the result.
+ * The default positive item, if only negatives are added, is TrueItem: Meaning that all documents are matched
+ * except those matching the negative terms added.
  *
  * @author bratseth
  */
-// TODO: Handle nulls by creating nullItem or checking in encode/toString
 public class NotItem extends CompositeItem {
 
     @Override
@@ -25,6 +26,7 @@ public class NotItem extends CompositeItem {
     }
 
     /** Adds an item. The first item is the positive, the rest are negative */
+    @Override
     public void addItem(Item item) {
         super.addItem(item);
     }
@@ -34,27 +36,25 @@ public class NotItem extends CompositeItem {
      * (position 0) if it is not already set.
      */
     public void addNegativeItem(Item negative) {
-        if (getItemCount() == 0) {
-            insertNullFirstItem();
-        }
+        if (getItemCount() == 0)
+            insertTrueFirstItem();
         addItem(negative);
     }
 
     /** Returns the negative items of this: All child items except the first */
     public List<Item> negativeItems() { return items().subList(1, getItemCount()); }
 
-    /** Returns the positive item (the first subitem), or null if no positive items has been added. */
+    /** Returns the positive item (the first subitem), or TrueItem if no positive items has been added. */
     public Item getPositiveItem() {
-        if (getItemCount() == 0) {
-            return null;
-        }
+        if (getItemCount() == 0)
+            return new TrueItem();
         return getItem(0);
     }
 
     /**
      * Sets the positive item (the first item)
      *
-     * @return the old positive item, or null if there was none
+     * @return the old positive item, or TrueItem if there was none
      */
     public Item setPositiveItem(Item item) {
         Objects.requireNonNull(item, () -> "Positive item of " + this);
@@ -72,7 +72,7 @@ public class NotItem extends CompositeItem {
      * the positive item becomes an AndItem with the items added
      */
     public void addPositiveItem(Item item) {
-        if (getPositiveItem() == null) {
+        if (getPositiveItem() instanceof TrueItem) {
             setPositiveItem(item);
         } else if (getPositiveItem() instanceof AndItem) {
             ((AndItem) getPositiveItem()).addItem(item);
@@ -90,7 +90,7 @@ public class NotItem extends CompositeItem {
         boolean removed = super.removeItem(item);
 
         if (removed && removedIndex == 0) {
-            insertNullFirstItem();
+            insertTrueFirstItem();
         }
         return removed;
     }
@@ -99,35 +99,35 @@ public class NotItem extends CompositeItem {
         Item removed = super.removeItem(index);
 
         if (index == 0) { // Don't make the first negative the positive
-            insertNullFirstItem();
+            insertTrueFirstItem();
         }
         return removed;
     }
 
+    private void insertTrueFirstItem() {
+        addItem(0, new TrueItem());
+    }
+
     /** Not items uses a empty heading instead of "NOT " */
     protected void appendHeadingString(StringBuilder buffer) {}
 
     /**
-     * Overridden to tolerate nulls and to append "+"
+     * Overridden to skip the positive TrueItem and (otherwise) append "+"
      * to the first item and "-" to the rest
      */
+    @Override
     protected void appendBodyString(StringBuilder buffer) {
-        boolean isFirstItem = true;
-
-        for (Iterator<Item> i = getItemIterator(); i.hasNext();) {
-            Item item = i.next();
-
-            if (isFirstItem) {
-                buffer.append("+");
-            } else {
-                buffer.append(" -");
-            }
-            if (item == null) {
-                buffer.append("(null)");
-            } else {
-                buffer.append(item.toString());
-            }
-            isFirstItem = false;
+        if (items().isEmpty()) return;
+        if (items().size() == 1) {
+            buffer.append(items().get(0));
+            return;
+        }
+        for (int i = 0; i < items().size(); i++) {
+            if (i == 0 && items().get(i) instanceof TrueItem) continue; // skip positive true
+
+            buffer.append(i == 0 ? "+" : "-").append(items().get(i));
+            if ( i < items().size() - 1)
+                buffer.append(" ");
         }
     }
 
diff --git a/container-search/src/main/java/com/yahoo/prelude/query/QueryCanonicalizer.java b/container-search/src/main/java/com/yahoo/prelude/query/QueryCanonicalizer.java
index 1f30833b3db..8c4c5c84a28 100644
--- a/container-search/src/main/java/com/yahoo/prelude/query/QueryCanonicalizer.java
+++ b/container-search/src/main/java/com/yahoo/prelude/query/QueryCanonicalizer.java
@@ -77,11 +77,6 @@ public class QueryCanonicalizer {
         else if (composite instanceof RankItem) {
             makeDuplicatesCheap((RankItem)composite);
         }
-        else if (composite instanceof NotItem) {
-            if (((NotItem) composite).getPositiveItem() == null)
-                return CanonicalizationResult.error("Can not search for only negative items");
-        }
-
         if (composite.getItemCount() == 0)
             parentIterator.remove();
 
diff --git a/container-search/src/main/java/com/yahoo/prelude/query/parser/SimpleParser.java b/container-search/src/main/java/com/yahoo/prelude/query/parser/SimpleParser.java
index 087da13a937..020d93d951c 100644
--- a/container-search/src/main/java/com/yahoo/prelude/query/parser/SimpleParser.java
+++ b/container-search/src/main/java/com/yahoo/prelude/query/parser/SimpleParser.java
@@ -121,7 +121,7 @@ abstract class SimpleParser extends StructuredParser {
                 return combineItems(topLevelItem, not.getPositiveItem());
             }
         }
-        if (not != null && not.getPositiveItem() == null) {
+        if (not != null && not.getPositiveItem() instanceof TrueItem) {
             // Incomplete not, only negatives -
 
             if (topLevelItem != null && topLevelItem != not) {
diff --git a/container-search/src/main/java/com/yahoo/prelude/querytransform/LiteralBoostSearcher.java b/container-search/src/main/java/com/yahoo/prelude/querytransform/LiteralBoostSearcher.java
index e9e5818cefe..47a5213c041 100644
--- a/container-search/src/main/java/com/yahoo/prelude/querytransform/LiteralBoostSearcher.java
+++ b/container-search/src/main/java/com/yahoo/prelude/querytransform/LiteralBoostSearcher.java
@@ -71,8 +71,6 @@ public class LiteralBoostSearcher extends Searcher {
     }
 
     private void addLiterals(RankItem rankTerms, Item item, IndexFacts.Session indexFacts) {
-        if (item == null) return;
-
         if (item instanceof NotItem) {
             addLiterals(rankTerms, ((NotItem) item).getPositiveItem(), indexFacts);
         }
diff --git a/container-search/src/main/java/com/yahoo/search/query/textserialize/item/AndNotRestConverter.java b/container-search/src/main/java/com/yahoo/search/query/textserialize/item/AndNotRestConverter.java
index b64809e8071..795a78157c5 100644
--- a/container-search/src/main/java/com/yahoo/search/query/textserialize/item/AndNotRestConverter.java
+++ b/container-search/src/main/java/com/yahoo/search/query/textserialize/item/AndNotRestConverter.java
@@ -13,6 +13,7 @@ import static com.yahoo.search.query.textserialize.item.ListUtil.first;
  * @author Tony Vaagenes
  */
 public class AndNotRestConverter extends CompositeConverter<NotItem> {
+
     static final String andNotRest = "AND-NOT-REST";
 
     public AndNotRestConverter() {
@@ -51,4 +52,5 @@ public class AndNotRestConverter extends CompositeConverter<NotItem> {
     protected String getFormName(Item item) {
         return andNotRest;
     }
+
 }
diff --git a/container-search/src/main/java/com/yahoo/search/yql/YqlParser.java b/container-search/src/main/java/com/yahoo/search/yql/YqlParser.java
index 8334775b8e2..199cf7bb2a9 100644
--- a/container-search/src/main/java/com/yahoo/search/yql/YqlParser.java
+++ b/container-search/src/main/java/com/yahoo/search/yql/YqlParser.java
@@ -356,6 +356,8 @@ public class YqlParser implements Parser {
                     return buildFunctionCall(ast);
                 case LITERAL:
                     return buildLiteral(ast);
+                case NOT:
+                    return buildNot(ast);
                 default:
                     throw newUnexpectedArgumentException(ast.getOperator(),
                                                          ExpressionOperator.AND, ExpressionOperator.CALL,
@@ -1096,17 +1098,21 @@ public class YqlParser implements Parser {
         AndItem andItem = new AndItem();
         NotItem notItem = new NotItem();
         convertVarArgsAnd(ast, 0, andItem, notItem);
-        Preconditions
-                .checkArgument(andItem.getItemCount() > 0,
-                        "Vespa does not support AND with no logically positive branches.");
         if (notItem.getItemCount() == 0) {
             return andItem;
         }
         if (andItem.getItemCount() == 1) {
             notItem.setPositiveItem(andItem.getItem(0));
-        } else {
+        } else if (andItem.getItemCount() > 1) {
             notItem.setPositiveItem(andItem);
-        }
+        } // else no positives, which is ok
+        return notItem;
+    }
+
+    /** Build a "pure" not, without any positive terms. */
+    private CompositeItem buildNot(OperatorNode<ExpressionOperator> ast) {
+        NotItem notItem = new NotItem();
+        notItem.addNegativeItem(convertExpression(ast.getArgument(0)));
         return notItem;
     }
 
diff --git a/container-search/src/test/java/com/yahoo/prelude/query/test/QueryCanonicalizerTestCase.java b/container-search/src/test/java/com/yahoo/prelude/query/test/QueryCanonicalizerTestCase.java
index 1d2f92063fe..11424cc7e4e 100644
--- a/container-search/src/test/java/com/yahoo/prelude/query/test/QueryCanonicalizerTestCase.java
+++ b/container-search/src/test/java/com/yahoo/prelude/query/test/QueryCanonicalizerTestCase.java
@@ -291,22 +291,22 @@ public class QueryCanonicalizerTestCase {
     }
 
     @Test
-    public void testNegativeMustHavePositive() {
+    public void testNegative() {
         NotItem root = new NotItem();
 
         root.addNegativeItem(new WordItem("negative"));
-        assertCanonicalized("+(null) -negative","Can not search for only negative items", root);
+        assertCanonicalized("-negative",null, root);
     }
 
     @Test
-    public void testNegativeMustHavePositiveNested() {
+    public void testNegativeOnly() {
         CompositeItem root = new AndItem();
         NotItem not = new NotItem();
 
         root.addItem(not);
         root.addItem(new WordItem("word"));
         not.addNegativeItem(new WordItem("negative"));
-        assertCanonicalized("AND (+(null) -negative) word","Can not search for only negative items", root);
+        assertCanonicalized("AND (-negative) word",null, root);
     }
 
     @Test
diff --git a/container-search/src/test/java/com/yahoo/prelude/semantics/test/ExactMatchTrickTestCase.java b/container-search/src/test/java/com/yahoo/prelude/semantics/test/ExactMatchTrickTestCase.java
index a2ff6ae5b82..c566b05405d 100644
--- a/container-search/src/test/java/com/yahoo/prelude/semantics/test/ExactMatchTrickTestCase.java
+++ b/container-search/src/test/java/com/yahoo/prelude/semantics/test/ExactMatchTrickTestCase.java
@@ -21,13 +21,13 @@ public class ExactMatchTrickTestCase extends RuleBaseAbstractTestCase {
 
     @Test
     public void testCompleteMatchWithNegative() { // Notice ordering bug
-        assertSemantics("+(AND default:primetime default:in default:time default:no) -regionexcl:us",
+        assertSemantics("+(AND default:primetime default:in default:time default:no TRUE) -regionexcl:us",
                         new Query(QueryTestCase.httpEncode("?query=primetime ANDNOT regionexcl:us&type=adv")));
     }
 
     @Test
     public void testCompleteMatchWithFilterAndNegative() {
-        assertSemantics("AND (+(AND default:primetime default:in default:time default:no) -regionexcl:us) |lang:en",
+        assertSemantics("AND (+(AND default:primetime default:in default:time default:no TRUE) -regionexcl:us) |lang:en",
                         new Query(QueryTestCase.httpEncode("?query=primetime ANDNOT regionexcl:us&type=adv&filter=+lang:en")));
     }
 
diff --git a/container-search/src/test/java/com/yahoo/prelude/semantics/test/NoStemmingTestCase.java b/container-search/src/test/java/com/yahoo/prelude/semantics/test/NoStemmingTestCase.java
index 7acecdcf00b..fbdd72fe6ac 100644
--- a/container-search/src/test/java/com/yahoo/prelude/semantics/test/NoStemmingTestCase.java
+++ b/container-search/src/test/java/com/yahoo/prelude/semantics/test/NoStemmingTestCase.java
@@ -17,13 +17,13 @@ public class NoStemmingTestCase extends RuleBaseAbstractTestCase {
     /** Should rewrite correctly */
     @Test
     public void testCorrectRewriting1() {
-        assertSemantics("+(AND i:arts i:sciences) -i:b","i:as -i:b");
+        assertSemantics("+(AND i:arts i:sciences TRUE) -i:b","i:as -i:b");
     }
 
     /** Should rewrite correctly too */
     @Test
     public void testCorrectRewriting2() {
-        assertSemantics("+(AND i:arts i:sciences i:crafts) -i:b","i:asc -i:b");
+        assertSemantics("+(AND i:arts i:sciences i:crafts TRUE) -i:b","i:asc -i:b");
     }
 
     /** Should not rewrite */
diff --git a/container-search/src/test/java/com/yahoo/prelude/semantics/test/StemmingTestCase.java b/container-search/src/test/java/com/yahoo/prelude/semantics/test/StemmingTestCase.java
index 136381df552..6702a1ca1d9 100644
--- a/container-search/src/test/java/com/yahoo/prelude/semantics/test/StemmingTestCase.java
+++ b/container-search/src/test/java/com/yahoo/prelude/semantics/test/StemmingTestCase.java
@@ -16,17 +16,17 @@ public class StemmingTestCase extends RuleBaseAbstractTestCase {
 
     @Test
     public void testRewritingDueToStemmingInQuery() {
-        assertSemantics("+i:vehicle -i:s","i:cars -i:s");
+        assertSemantics("+(AND i:vehicle TRUE) -i:s","i:cars -i:s");
     }
 
     @Test
     public void testRewritingDueToStemmingInRule() {
-        assertSemantics("+i:animal -i:s","i:horse -i:s");
+        assertSemantics("+(AND i:animal TRUE) -i:s","i:horse -i:s");
     }
 
     @Test
     public void testRewritingDueToExactMatch() {
-        assertSemantics("+(AND i:arts i:sciences) -i:s","i:as -i:s");
+        assertSemantics("+(AND i:arts i:sciences TRUE) -i:s","i:as -i:s");
     }
 
     @Test
diff --git a/container-search/src/test/java/com/yahoo/search/grouping/vespa/GroupingExecutorTestCase.java b/container-search/src/test/java/com/yahoo/search/grouping/vespa/GroupingExecutorTestCase.java
index 2fda56c7454..4a7c6179db7 100644
--- a/container-search/src/test/java/com/yahoo/search/grouping/vespa/GroupingExecutorTestCase.java
+++ b/container-search/src/test/java/com/yahoo/search/grouping/vespa/GroupingExecutorTestCase.java
@@ -9,6 +9,7 @@ import com.yahoo.document.GlobalId;
 import com.yahoo.prelude.fastsearch.FastHit;
 import com.yahoo.prelude.fastsearch.GroupingListHit;
 import com.yahoo.prelude.query.NotItem;
+import com.yahoo.prelude.query.NullItem;
 import com.yahoo.prelude.query.WordItem;
 import com.yahoo.search.Query;
 import com.yahoo.search.Result;
@@ -535,18 +536,14 @@ public class GroupingExecutorTestCase {
         Execution exc = newExecution(new GroupingExecutor());
 
         Query query = new Query();
-        NotItem notItem = new NotItem();
-
-        notItem.addNegativeItem(new WordItem("negative"));
-        query.getModel().getQueryTree().setRoot(notItem);
+        query.getModel().getQueryTree().setRoot(new NullItem());
 
         Result result = exc.search(query);
         com.yahoo.search.result.ErrorMessage message = result.hits().getError();
 
         assertNotNull("Got error", message);
         assertEquals("Illegal query", message.getMessage());
-        assertEquals("Can not search for only negative items",
-                message.getDetailedMessage());
+        assertEquals("No query", message.getDetailedMessage());
         assertEquals(3, message.getCode());
     }
 
diff --git a/container-search/src/test/java/com/yahoo/search/query/textserialize/item/test/ParseItemTestCase.java b/container-search/src/test/java/com/yahoo/search/query/textserialize/item/test/ParseItemTestCase.java
index aa0d692ec92..b351bfb927a 100644
--- a/container-search/src/test/java/com/yahoo/search/query/textserialize/item/test/ParseItemTestCase.java
+++ b/container-search/src/test/java/com/yahoo/search/query/textserialize/item/test/ParseItemTestCase.java
@@ -13,6 +13,7 @@ import com.yahoo.prelude.query.PrefixItem;
 import com.yahoo.prelude.query.RankItem;
 import com.yahoo.prelude.query.SubstringItem;
 import com.yahoo.prelude.query.SuffixItem;
+import com.yahoo.prelude.query.TrueItem;
 import com.yahoo.prelude.query.WordItem;
 import com.yahoo.search.query.textserialize.item.ItemContext;
 import com.yahoo.search.query.textserialize.item.ItemFormHandler;
@@ -71,7 +72,7 @@ public class ParseItemTestCase {
     @Test
     public void parse_and_not_rest_with_only_negated_children() throws ParseException {
         NotItem notItem = (NotItem) parse("(AND-NOT-REST null (WORD 'negated-item'))");
-        assertNull(notItem.getPositiveItem());
+        assertTrue(notItem.getPositiveItem() instanceof TrueItem);
         assertTrue(notItem.getItem(1) instanceof WordItem);
     }
 
diff --git a/container-search/src/test/java/com/yahoo/search/yql/YqlParserTestCase.java b/container-search/src/test/java/com/yahoo/search/yql/YqlParserTestCase.java
index 55fb53b4460..881101a7bda 100644
--- a/container-search/src/test/java/com/yahoo/search/yql/YqlParserTestCase.java
+++ b/container-search/src/test/java/com/yahoo/search/yql/YqlParserTestCase.java
@@ -193,6 +193,18 @@ public class YqlParserTestCase {
     }
 
     @Test
+    public void testSingleNot() {
+        assertParse("select foo from bar where !(title contains \"saint\")",
+                    "-title:saint");
+    }
+
+    @Test
+    public void testMultipleNot() {
+        assertParse("select foo from bar where !(title contains \"saint\") AND !(title contains \"etienne\")",
+                    "-title:saint -title:etienne");
+    }
+
+    @Test
     public void testLessThan() {
         assertParse("select foo from bar where price < 500", "price:<500");
         assertParse("select foo from bar where 500 < price", "price:>500");