diff options
author | Jon Bratseth <bratseth@oath.com> | 2019-04-26 13:34:54 +0200 |
---|---|---|
committer | GitHub <noreply@github.com> | 2019-04-26 13:34:54 +0200 |
commit | 5f6a5a5d626808459bc65c95eb226e746ed3d18f (patch) | |
tree | 58dfb87b6905a2709c70f19a6470627ff614ca93 /container-search/src | |
parent | 6e1be1f6c681c26594b3f539f41b6e23ee0c446b (diff) | |
parent | 08be9a163a0861901bd832f2d0cbd1c0d4a05ddf (diff) |
Merge pull request #9044 from vespa-engine/revert-9043-revert-9026-bratseth/better-negative-number-heuristics
Revert "Revert "Better heuristics for negative numbers""
Diffstat (limited to 'container-search/src')
9 files changed, 104 insertions, 80 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..72ee4ae2c12 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,18 @@ 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 not if there is an explicit index (such as -a:b) + // but interpret --N as a negative item matching a negative number + if ( item instanceof IntItem && + ((IntItem)item).getIndexName().isEmpty() && + ! ((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/searchchain/Execution.java b/container-search/src/main/java/com/yahoo/search/searchchain/Execution.java index 6a56f94c724..5427da6c06c 100644 --- a/container-search/src/main/java/com/yahoo/search/searchchain/Execution.java +++ b/container-search/src/main/java/com/yahoo/search/searchchain/Execution.java @@ -481,7 +481,7 @@ public class Execution extends com.yahoo.processing.execution.Execution { * if searchChain is null */ @SuppressWarnings("unchecked") - private Execution(Chain<? extends Processor> searchChain,Context context, int searcherIndex) { + private Execution(Chain<? extends Processor> searchChain, Context context, int searcherIndex) { // Create a new Execution which is placed in the context of the execution of the given Context if any // "if any" because a context may, or may not, belong to an execution. // This is decided at the creation time of the Context - Context instances which do not belong 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..dc2f990431a 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 @@ -1966,8 +1971,18 @@ public class ParseTestCase { } @Test + public void testNegativeTermPositiveNumberWithIndex() { + tester.assertParsed("+a -normal:12", "a -normal:12", Query.Type.ALL); + } + + @Test + public void testNegativeTermNegativeNumberWithIndex() { + tester.assertParsed("+a -normal:-12", "a -normal:-12", Query.Type.ALL); + } + + @Test public void testSingleNegativeNumberLikeTerm() { - tester.assertParsed(null, "-12", Query.Type.ALL); + tester.assertParsed("-12", "-12", Query.Type.ALL); } @Test @@ -2004,7 +2019,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 +2034,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 +2341,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", |