From ba415d216374f974dde970a1cc39ba4212a18d01 Mon Sep 17 00:00:00 2001 From: Harald Musum Date: Mon, 8 Apr 2019 11:26:26 +0200 Subject: Revert "Better heuristics for negative numbers" --- .../com/yahoo/prelude/query/parser/AllParser.java | 53 ++++++++++------------ .../yahoo/prelude/query/parser/SimpleParser.java | 13 ++++-- .../prelude/query/parser/StructuredParser.java | 48 +++++++++++--------- .../yahoo/prelude/query/parser/TokenPosition.java | 17 +++++-- .../java/com/yahoo/search/query/parser/Parser.java | 3 +- .../main/java/com/yahoo/search/yql/YqlParser.java | 2 +- .../prelude/query/parser/test/ParseTestCase.java | 22 +++------ .../com/yahoo/search/yql/UserInputTestCase.java | 11 ----- 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 Steinar Knutsen */ 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 not found in the composite, or returns null if there's none */ + + /** + * Removes and returns the first not 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 firstWord = new ArrayList<>(); List 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 @@ -117,17 +117,6 @@ public class UserInputTestCase { query.yqlRepresentation()); } - @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(); -- cgit v1.2.3