diff options
author | Jon Bratseth <bratseth@oath.com> | 2022-01-07 09:11:13 +0100 |
---|---|---|
committer | GitHub <noreply@github.com> | 2022-01-07 09:11:13 +0100 |
commit | c1be0bf948923cfe91fd57ed6b353d747abf694b (patch) | |
tree | c45b8cfc32a43a2b07e215b069b10a700eca8a46 | |
parent | 9ad2d2cbdcb8cf716cf30ae76e9a3e62cee822bb (diff) | |
parent | 00ba52f4900ec8c78c5647f4d0e67499f86fd86d (diff) |
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"); |