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

Revert "Better heuristics for negative numbers"

8 files changed, 79 insertions, 90 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 499cacd89c5..5e994dac5d6 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,8 +13,7 @@ import static com.yahoo.prelude.query.parser.Token.Kind.SPACE;
 /**
  * Parser for queries of type all.
  *
- * @author Steinar Knutsen
- * @author bratseth
+ * @author  <a href="mailto:steinar@yahoo-inc.com">Steinar Knutsen</a>
  */
 public class AllParser extends SimpleParser {
 
@@ -33,37 +32,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);
@@ -79,23 +78,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;
     }
@@ -104,7 +103,8 @@ public class AllParser extends SimpleParser {
         int position = tokens.getPosition();
         Item item = null;
         try {
-            if ( ! tokens.skip(MINUS)) return null;
+            if (!tokens.skipMultiple(MINUS)) return null;
+
             if (tokens.currentIsNoIgnore(SPACE)) return null;
 
             item = indexableItem();
@@ -122,15 +122,8 @@ 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 9ddfea6dffb..291beb40b4c 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,6 +25,7 @@ abstract class SimpleParser extends StructuredParser {
         return anyItems(false); // Nesteds are any even if all on top level
     }
 
+
     protected abstract Item negativeItem();
 
     /**
@@ -162,7 +163,11 @@ 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) {
@@ -179,7 +184,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 8ecd4d8f81c..ec1f79828c1 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();
+            item = number(indexName != null);
 
             if (item == null) {
                 item = phrase();
@@ -147,36 +147,39 @@ abstract class StructuredParser extends AbstractParser {
             List<Token> firstWord = new ArrayList<>();
             List<Token> secondWord = new ArrayList<>();
 
-            tokens.skip(LSQUAREBRACKET);
+            tokens.skip(LSQUAREBRACKET); // For test 93 and 60
 
-            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);
@@ -192,7 +195,8 @@ 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();
@@ -249,11 +253,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() {
@@ -273,19 +277,19 @@ abstract class StructuredParser extends AbstractParser {
         }
     }
 
-    private IntItem number() {
+    private IntItem number(boolean hasIndex) {
         int position = tokens.getPosition();
         IntItem item = null;
 
         try {
-            item = numberRange();
+            if (item == null) {
+                item = numberRange();
+            }
 
-            tokens.skip(LSQUAREBRACKET);
-            if (item == null)
-                tokens.skipNoIgnore(SPACE);
+            tokens.skip(LSQUAREBRACKET); // For test 93 and 60
 
             // TODO: Better definition of start and end of numeric items
-            if (item == null && tokens.currentIsNoIgnore(MINUS) && (tokens.currentNoIgnore(1).kind == NUMBER)) {
+            if (item == null && hasIndex && tokens.currentIsNoIgnore(MINUS) && (tokens.currentNoIgnore(1).kind == NUMBER)) {
                 tokens.skipNoIgnore();
                 Token t = tokens.next();
                 item = new IntItem("-" + t.toString() + decimalPart(), true);
@@ -303,7 +307,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 fbaf1675ff1..42cef67f189 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,8 +1,10 @@
 // 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
@@ -181,7 +183,8 @@ 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();
@@ -195,16 +198,20 @@ 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;
+        }
 
-        skipNoIgnore();
+        skip();
         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 b3d79f65df4..32c386f0e32 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,7 +15,8 @@ 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 3eac1d88784..af095fefc1c 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 73f2ae7eb87..12f9ef2b18f 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,12 +1957,7 @@ public class ParseTestCase {
 
     @Test
     public void testNumbersAndNot() {
-        tester.assertParsed("AND a -12", "a -12", Query.Type.ALL);
-    }
-
-    @Test
-    public void testNumbersAndDoubleNot() {
-        tester.assertParsed("+a --12", "a --12", Query.Type.ALL);
+        tester.assertParsed("+a -12", "a -12", Query.Type.ALL);
     }
 
     @Test
@@ -1972,7 +1967,7 @@ public class ParseTestCase {
 
     @Test
     public void testSingleNegativeNumberLikeTerm() {
-        tester.assertParsed("-12", "-12", Query.Type.ALL);
+        tester.assertParsed(null, "-12", Query.Type.ALL);
     }
 
     @Test
@@ -2009,12 +2004,7 @@ public class ParseTestCase {
 
     @Test
     public void testDecimalNumbersAndNot() {
-        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);
+        tester.assertParsed("+a -12.2", "a -12.2", Query.Type.ALL);
     }
 
     @Test
@@ -2024,7 +2014,7 @@ public class ParseTestCase {
 
     @Test
     public void testSingleNegativeDecimalNumberLikeTerm() {
-        tester.assertParsed("-12.2", "-12.2", Query.Type.ALL);
+        tester.assertParsed(null, "-12.2", Query.Type.ALL);
     }
 
     @Test
@@ -2331,12 +2321,12 @@ public class ParseTestCase {
 
     @Test
     public void testSingleNegativeNumberLikeTermWeb() {
-        tester.assertParsed("-12", "-12", Query.Type.WEB);
+        tester.assertParsed(null, "-12", Query.Type.WEB);
     }
 
     @Test
     public void testSingleNegativeDecimalNumberLikeTermWeb() {
-        tester.assertParsed("-12.2", "-12.2", Query.Type.WEB);
+        tester.assertParsed(null, "-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 6173d710434..b5c4166e4de 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,17 +118,6 @@ 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",