Merge pull request #9026 from vespa-engine/bratseth/better-negative-number-heuristics

Better heuristics for negative numbers

8 files changed, 90 insertions, 79 deletions

diff --git a/container-search/src/main/java/com/yahoo/prelude/query/parser/AllParser.java b/container-search/src/main/java/com/yahoo/prelude/query/parser/AllParser.java
index 5e994dac5d6..499cacd89c5 100644
--- a/container-search/src/main/java/com/yahoo/prelude/query/parser/AllParser.java
+++ b/container-search/src/main/java/com/yahoo/prelude/query/parser/AllParser.java
@@ -13,7 +13,8 @@ import static com.yahoo.prelude.query.parser.Token.Kind.SPACE;
 /**
  * Parser for queries of type all.
  *
- * @author  <a href="mailto:steinar@yahoo-inc.com">Steinar Knutsen</a>
+ * @author Steinar Knutsen
+ * @author bratseth
  */
 public class AllParser extends SimpleParser {
 
@@ -32,37 +33,37 @@ public class AllParser extends SimpleParser {
 
     protected Item parseItemsBody() {
         // Algorithm: Collect positive, negative, and and'ed items, then combine.
-        AndItem and=null;
-        NotItem not=null; // Store negatives here as we go
+        AndItem and = null;
+        NotItem not = null; // Store negatives here as we go
         Item current;
 
         // Find all items
         do {
-            current=negativeItem();
-            if (current!=null) {
-                not=addNot(current,not);
+            current = negativeItem();
+            if (current != null) {
+                not = addNot(current, not);
                 continue;
             }
 
-            current=positiveItem();
-            if (current==null)
+            current = positiveItem();
+            if (current == null)
                 current = indexableItem();
             if (current == null)
                 current = compositeItem();
 
-            if (current!=null)
-                and=addAnd(current,and);
+            if (current != null)
+                and = addAnd(current, and);
 
             if (current == null)
                 tokens.skip();
         } while (tokens.hasNext());
 
         // Combine the items
-        Item topLevel=and;
+        Item topLevel = and;
 
-        if (not!=null && topLevel!=null) {
+        if (not != null && topLevel != null) {
             not.setPositiveItem(topLevel);
-            topLevel=not;
+            topLevel = not;
         }
 
         return simplifyUnnecessaryComposites(topLevel);
@@ -78,23 +79,23 @@ public class AllParser extends SimpleParser {
         return root.getRoot() instanceof NullItem ? null : root.getRoot();
     }
 
-    protected AndItem addAnd(Item item,AndItem and) {
-        if (and==null)
-            and=new AndItem();
+    protected AndItem addAnd(Item item, AndItem and) {
+        if (and == null)
+            and = new AndItem();
         and.addItem(item);
         return and;
     }
 
     protected OrItem addOr(Item item,OrItem or) {
-        if (or==null)
-            or=new OrItem();
+        if (or == null)
+            or = new OrItem();
         or.addItem(item);
         return or;
     }
 
     protected NotItem addNot(Item item,NotItem not) {
-        if (not==null)
-            not=new NotItem();
+        if (not == null)
+            not = new NotItem();
         not.addNegativeItem(item);
         return not;
     }
@@ -103,8 +104,7 @@ public class AllParser extends SimpleParser {
         int position = tokens.getPosition();
         Item item = null;
         try {
-            if (!tokens.skipMultiple(MINUS)) return null;
-
+            if ( ! tokens.skip(MINUS)) return null;
             if (tokens.currentIsNoIgnore(SPACE)) return null;
 
             item = indexableItem();
@@ -122,8 +122,15 @@ public class AllParser extends SimpleParser {
                     }
                 }
             }
-            if (item!=null)
+            if (item != null)
                 item.setProtected(true);
+
+            // Heuristic overdrive engaged!
+            // Interpret -N as a positive item matching a negative number (by backtracking out of this)
+            // but interpret --N as a negative item matching a negative number
+            if ( item instanceof IntItem && ! ((IntItem)item).getNumber().startsWith(("-")))
+                item = null;
+
             return item;
         } finally {
             if (item == null) {
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 291beb40b4c..9ddfea6dffb 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
@@ -10,10 +10,10 @@ import static com.yahoo.prelude.query.parser.Token.Kind.PLUS;
 import static com.yahoo.prelude.query.parser.Token.Kind.SPACE;
 
 /**
- * Base class for parsers of the "simple" query languages (query types
- * ANY and ALL).
+ * Base class for parsers of the "simple" query languages (query types ANY and ALL).
  *
  * @author Steinar Knutsen
+ * @author bratseth
  */
 abstract class SimpleParser extends StructuredParser {
 
@@ -25,7 +25,6 @@ abstract class SimpleParser extends StructuredParser {
         return anyItems(false); // Nesteds are any even if all on top level
     }
 
-
     protected abstract Item negativeItem();
 
     /**
@@ -163,11 +162,7 @@ abstract class SimpleParser extends StructuredParser {
         return false;
     }
 
-
-    /**
-     * Removes and returns the first <i>not</i> found in the composite,
-     * or returns null if there's none
-     */
+    /** Removes and returns the first <i>not</i> found in the composite, or returns null if there's none */
     private NotItem removeNot(CompositeItem composite) {
         for (int i = 0; i < composite.getItemCount(); i++) {
             if (composite.getItem(i) instanceof NotItem) {
@@ -184,7 +179,7 @@ abstract class SimpleParser extends StructuredParser {
         Item item = null;
 
         try {
-            if (!tokens.skipMultiple(PLUS)) {
+            if ( ! tokens.skipMultiple(PLUS)) {
                 return null;
             }
 
diff --git a/container-search/src/main/java/com/yahoo/prelude/query/parser/StructuredParser.java b/container-search/src/main/java/com/yahoo/prelude/query/parser/StructuredParser.java
index ec1f79828c1..8ecd4d8f81c 100644
--- a/container-search/src/main/java/com/yahoo/prelude/query/parser/StructuredParser.java
+++ b/container-search/src/main/java/com/yahoo/prelude/query/parser/StructuredParser.java
@@ -60,7 +60,7 @@ abstract class StructuredParser extends AbstractParser {
             String indexName = indexPrefix();
             setSubmodeFromIndex(indexName, indexFacts);
 
-            item = number(indexName != null);
+            item = number();
 
             if (item == null) {
                 item = phrase();
@@ -147,39 +147,36 @@ abstract class StructuredParser extends AbstractParser {
             List<Token> firstWord = new ArrayList<>();
             List<Token> secondWord = new ArrayList<>();
 
-            tokens.skip(LSQUAREBRACKET); // For test 93 and 60
+            tokens.skip(LSQUAREBRACKET);
 
-            if (!tokens.currentIs(WORD) && !tokens.currentIs(NUMBER)
-                && !tokens.currentIs(UNDERSCORE)) {
+            if ( ! tokens.currentIs(WORD) && ! tokens.currentIs(NUMBER) && ! tokens.currentIs(UNDERSCORE)) {
                 return null;
             }
 
             firstWord.add(tokens.next());
 
             while (tokens.currentIsNoIgnore(UNDERSCORE)
-                    || tokens.currentIsNoIgnore(WORD)
-                    || tokens.currentIsNoIgnore(NUMBER)) {
+                   || tokens.currentIsNoIgnore(WORD)
+                   || tokens.currentIsNoIgnore(NUMBER)) {
                 firstWord.add(tokens.next());
             }
 
             if (tokens.currentIsNoIgnore(DOT)) {
                 tokens.skip();
-                if (tokens.currentIsNoIgnore(WORD)
-                        || tokens.currentIsNoIgnore(NUMBER)) {
+                if (tokens.currentIsNoIgnore(WORD) || tokens.currentIsNoIgnore(NUMBER)) {
                     secondWord.add(tokens.next());
                 } else {
                     return null;
                 }
                 while (tokens.currentIsNoIgnore(UNDERSCORE)
-                        || tokens.currentIsNoIgnore(WORD)
-                        || tokens.currentIsNoIgnore(NUMBER)) {
+                       || tokens.currentIsNoIgnore(WORD)
+                       || tokens.currentIsNoIgnore(NUMBER)) {
                     secondWord.add(tokens.next());
                 }
             }
 
-            if (!tokens.skipNoIgnore(COLON)) {
+            if ( ! tokens.skipNoIgnore(COLON))
                 return null;
-            }
 
             if (secondWord.size() == 0) {
                 item = concatenate(firstWord);
@@ -195,8 +192,7 @@ abstract class StructuredParser extends AbstractParser {
                 return null;
             } else {
                 if (nothingAhead(false)) {
-                    // correct index syntax, correct name, but followed
-                    // by noise. Let's skip this.
+                    // correct index syntax, correct name, but followed by noise. Let's skip this.
                     nothingAhead(true);
                     position = tokens.getPosition();
                     item = indexPrefix();
@@ -253,11 +249,11 @@ abstract class StructuredParser extends AbstractParser {
 
     private boolean endOfNumber() {
         return tokens.currentIsNoIgnore(SPACE)
-                || tokens.currentIsNoIgnore(RSQUAREBRACKET)
-                || tokens.currentIsNoIgnore(SEMICOLON)
-                || tokens.currentIsNoIgnore(RBRACE)
-                || tokens.currentIsNoIgnore(EOF)
-                || tokens.currentIsNoIgnore(EXCLAMATION);
+               || tokens.currentIsNoIgnore(RSQUAREBRACKET)
+               || tokens.currentIsNoIgnore(SEMICOLON)
+               || tokens.currentIsNoIgnore(RBRACE)
+               || tokens.currentIsNoIgnore(EOF)
+               || tokens.currentIsNoIgnore(EXCLAMATION);
     }
 
     private String decimalPart() {
@@ -277,19 +273,19 @@ abstract class StructuredParser extends AbstractParser {
         }
     }
 
-    private IntItem number(boolean hasIndex) {
+    private IntItem number() {
         int position = tokens.getPosition();
         IntItem item = null;
 
         try {
-            if (item == null) {
-                item = numberRange();
-            }
+            item = numberRange();
 
-            tokens.skip(LSQUAREBRACKET); // For test 93 and 60
+            tokens.skip(LSQUAREBRACKET);
+            if (item == null)
+                tokens.skipNoIgnore(SPACE);
 
             // TODO: Better definition of start and end of numeric items
-            if (item == null && hasIndex && tokens.currentIsNoIgnore(MINUS) && (tokens.currentNoIgnore(1).kind == NUMBER)) {
+            if (item == null && tokens.currentIsNoIgnore(MINUS) && (tokens.currentNoIgnore(1).kind == NUMBER)) {
                 tokens.skipNoIgnore();
                 Token t = tokens.next();
                 item = new IntItem("-" + t.toString() + decimalPart(), true);
@@ -307,7 +303,7 @@ abstract class StructuredParser extends AbstractParser {
             if (item == null) {
                 item = numberGreater();
             }
-            if (item != null && !endOfNumber()) {
+            if (item != null && ! endOfNumber()) {
                 item = null;
             }
             return item;
diff --git a/container-search/src/main/java/com/yahoo/prelude/query/parser/TokenPosition.java b/container-search/src/main/java/com/yahoo/prelude/query/parser/TokenPosition.java
index 42cef67f189..fbaf1675ff1 100644
--- a/container-search/src/main/java/com/yahoo/prelude/query/parser/TokenPosition.java
+++ b/container-search/src/main/java/com/yahoo/prelude/query/parser/TokenPosition.java
@@ -1,10 +1,8 @@
 // Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 package com.yahoo.prelude.query.parser;
 
-
 import java.util.List;
 
-
 /**
  * An iterator-like view of a list of tokens, but typed, random-accessible
  * and with more convenience methods
@@ -183,8 +181,7 @@ final class TokenPosition {
     /**
      * Skips one or zero items of the given kind.
      *
-     * @return true if one item was skipped, false if none was,
-     *         or if there are no more tokens
+     * @return true if one item was skipped, false if none was, or if there are no more tokens
      */
     public boolean skip(Token.Kind kind) {
         Token current = current();
@@ -198,20 +195,16 @@ final class TokenPosition {
     }
 
     /**
-     * Skips one or zero items of the given kind, without ignoring
-     * spaces
+     * Skips one or zero items of the given kind, without ignoring spaces
      *
-     * @return true if one item was skipped, false if none was,
-     *         or if there are no more tokens
+     * @return true if one item was skipped, false if none was or if there are no more tokens
      */
     public boolean skipNoIgnore(Token.Kind kind) {
         Token current = currentNoIgnore();
 
-        if (current == null || current.kind != kind) {
-            return false;
-        }
+        if (current == null || current.kind != kind) return false;
 
-        skip();
+        skipNoIgnore();
         return true;
     }
     
diff --git a/container-search/src/main/java/com/yahoo/search/query/parser/Parser.java b/container-search/src/main/java/com/yahoo/search/query/parser/Parser.java
index 32c386f0e32..b3d79f65df4 100644
--- a/container-search/src/main/java/com/yahoo/search/query/parser/Parser.java
+++ b/container-search/src/main/java/com/yahoo/search/query/parser/Parser.java
@@ -15,8 +15,7 @@ public interface Parser {
      * {@link QueryTree}. If parsing fails without an exception, the contained
      * root will be an instance of {@link com.yahoo.prelude.query.NullItem}.
      *
-     * @param query
-     *            the Parsable to parse
+     * @param query the Parsable to parse
      * @return the parsed QueryTree, never null
      */
     QueryTree parse(Parsable query);
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 af095fefc1c..3eac1d88784 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
@@ -712,7 +712,7 @@ public class YqlParser implements Parser {
                                                .setLanguage(language)
                                                .setDefaultIndexName(defaultIndex)).getRoot();
         // the null check should be unnecessary, but is there to avoid having to suppress null warnings
-        if ( !allowNullItem && (item == null || item instanceof NullItem))
+        if ( ! allowNullItem && (item == null || item instanceof NullItem))
             throw new IllegalArgumentException("Parsing '" + wordData + "' only resulted in NullItem.");
 
         if (language != Language.ENGLISH) // mark the language used, unless it's the default
diff --git a/container-search/src/test/java/com/yahoo/prelude/query/parser/test/ParseTestCase.java b/container-search/src/test/java/com/yahoo/prelude/query/parser/test/ParseTestCase.java
index 12f9ef2b18f..73f2ae7eb87 100644
--- a/container-search/src/test/java/com/yahoo/prelude/query/parser/test/ParseTestCase.java
+++ b/container-search/src/test/java/com/yahoo/prelude/query/parser/test/ParseTestCase.java
@@ -1957,7 +1957,12 @@ public class ParseTestCase {
 
     @Test
     public void testNumbersAndNot() {
-        tester.assertParsed("+a -12", "a -12", Query.Type.ALL);
+        tester.assertParsed("AND a -12", "a -12", Query.Type.ALL);
+    }
+
+    @Test
+    public void testNumbersAndDoubleNot() {
+        tester.assertParsed("+a --12", "a --12", Query.Type.ALL);
     }
 
     @Test
@@ -1967,7 +1972,7 @@ public class ParseTestCase {
 
     @Test
     public void testSingleNegativeNumberLikeTerm() {
-        tester.assertParsed(null, "-12", Query.Type.ALL);
+        tester.assertParsed("-12", "-12", Query.Type.ALL);
     }
 
     @Test
@@ -2004,7 +2009,12 @@ public class ParseTestCase {
 
     @Test
     public void testDecimalNumbersAndNot() {
-        tester.assertParsed("+a -12.2", "a -12.2", Query.Type.ALL);
+        tester.assertParsed("AND a -12.2", "a -12.2", Query.Type.ALL);
+    }
+
+    @Test
+    public void testDecimalNumbersAndDoubleNot() {
+        tester.assertParsed("+a --12.2", "a --12.2", Query.Type.ALL);
     }
 
     @Test
@@ -2014,7 +2024,7 @@ public class ParseTestCase {
 
     @Test
     public void testSingleNegativeDecimalNumberLikeTerm() {
-        tester.assertParsed(null, "-12.2", Query.Type.ALL);
+        tester.assertParsed("-12.2", "-12.2", Query.Type.ALL);
     }
 
     @Test
@@ -2321,12 +2331,12 @@ public class ParseTestCase {
 
     @Test
     public void testSingleNegativeNumberLikeTermWeb() {
-        tester.assertParsed(null, "-12", Query.Type.WEB);
+        tester.assertParsed("-12", "-12", Query.Type.WEB);
     }
 
     @Test
     public void testSingleNegativeDecimalNumberLikeTermWeb() {
-        tester.assertParsed(null, "-12.2", Query.Type.WEB);
+        tester.assertParsed("-12.2", "-12.2", Query.Type.WEB);
     }
 
     @Test
diff --git a/container-search/src/test/java/com/yahoo/search/yql/UserInputTestCase.java b/container-search/src/test/java/com/yahoo/search/yql/UserInputTestCase.java
index b5c4166e4de..6173d710434 100644
--- a/container-search/src/test/java/com/yahoo/search/yql/UserInputTestCase.java
+++ b/container-search/src/test/java/com/yahoo/search/yql/UserInputTestCase.java
@@ -118,6 +118,17 @@ public class UserInputTestCase {
     }
 
     @Test
+    public void testNegativeNumberComparison() {
+        URIBuilder builder = searchUri();
+        builder.setParameter("myinput", "-5");
+        builder.setParameter("yql",
+                             "select * from ecitem where rank(([{\"defaultIndex\":\"myfield\"}](userInput(@myinput))));");
+        Query query = searchAndAssertNoErrors(builder);
+        assertEquals("select * from ecitem where rank(myfield = (-5));", query.yqlRepresentation());
+        assertEquals("RANK myfield:-5", query.getModel().getQueryTree().getRoot().toString());
+    }
+
+    @Test
     public void testAnnotatedUserInputUnrankedTerms() {
         URIBuilder builder = searchUri();
         builder.setParameter("yql",