Merge pull request #11986 from vespa-engine/geirst/fix-search-for-false-in-imported-bool-attributes

Geirst/fix search for false in imported bool attributes

4 files changed, 111 insertions, 37 deletions

diff --git a/searchlib/src/tests/attribute/searchcontext/CMakeLists.txt b/searchlib/src/tests/attribute/searchcontext/CMakeLists.txt
index a705fd5ecb5..377d91bf634 100644
--- a/searchlib/src/tests/attribute/searchcontext/CMakeLists.txt
+++ b/searchlib/src/tests/attribute/searchcontext/CMakeLists.txt
@@ -1,7 +1,7 @@
 # Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 vespa_add_executable(searchlib_searchcontext_test_app TEST
     SOURCES
-    searchcontext.cpp
+    searchcontext_test.cpp
     DEPENDS
     searchlib
     searchlib_test
diff --git a/searchlib/src/tests/attribute/searchcontext/searchcontext.cpp b/searchlib/src/tests/attribute/searchcontext/searchcontext_test.cpp
index 7d4a2d63355..416ddb5fbc0 100644
--- a/searchlib/src/tests/attribute/searchcontext/searchcontext.cpp
+++ b/searchlib/src/tests/attribute/searchcontext/searchcontext_test.cpp
@@ -1,24 +1,27 @@
 // Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
+
 #include <vespa/searchlib/attribute/attribute.h>
 #include <vespa/searchlib/attribute/attributefactory.h>
 #include <vespa/searchlib/attribute/attributeiterators.h>
+#include <vespa/searchlib/attribute/attributevector.hpp>
+#include <vespa/searchlib/attribute/elementiterator.h>
 #include <vespa/searchlib/attribute/flagattribute.h>
+#include <vespa/searchlib/attribute/multistringattribute.h>
+#include <vespa/searchlib/attribute/singleboolattribute.h>
 #include <vespa/searchlib/attribute/singlenumericattribute.h>
 #include <vespa/searchlib/attribute/singlestringattribute.h>
-#include <vespa/searchlib/attribute/multistringattribute.h>
-#include <vespa/searchlib/attribute/elementiterator.h>
 #include <vespa/searchlib/common/bitvectoriterator.h>
 #include <vespa/searchlib/fef/matchdata.h>
 #include <vespa/searchlib/fef/termfieldmatchdataarray.h>
-#include <vespa/searchlib/queryeval/hitcollector.h>
+#include <vespa/searchlib/parsequery/parse.h>
+#include <vespa/searchlib/query/query_term_simple.h>
 #include <vespa/searchlib/queryeval/emptysearch.h>
 #include <vespa/searchlib/queryeval/executeinfo.h>
+#include <vespa/searchlib/queryeval/hitcollector.h>
+#include <vespa/searchlib/queryeval/simpleresult.h>
+#include <vespa/searchlib/test/searchiteratorverifier.h>
 #include <vespa/vespalib/testkit/testapp.h>
 #include <vespa/vespalib/util/compress.h>
-#include <vespa/searchlib/test/searchiteratorverifier.h>
-#include <vespa/searchlib/query/query_term_simple.h>
-#include <vespa/searchlib/parsequery/parse.h>
-#include <vespa/searchlib/attribute/attributevector.hpp>
 
 #include <vespa/log/log.h>
 LOG_SETUP("searchcontext_test");
@@ -43,22 +46,24 @@ isUnsignedSmallIntAttribute(const AttributeVector &a)
 
 }
 
-typedef AttributeVector::SP AttributePtr;
-typedef std::unique_ptr<AttributeVector::SearchContext> SearchContextPtr;
-typedef AttributeVector::SearchContext SearchContext;
-using attribute::Config;
+using AttributePtr = AttributeVector::SP;
+using ResultSetPtr = std::unique_ptr<ResultSet>;
+using SearchBasePtr = queryeval::SearchIterator::UP;
+using SearchContext = AttributeVector::SearchContext;
+using SearchContextPtr = std::unique_ptr<AttributeVector::SearchContext>;
+using largeint_t = AttributeVector::largeint_t;
+
 using attribute::BasicType;
 using attribute::CollectionType;
-typedef AttributeVector::largeint_t largeint_t;
-typedef queryeval::SearchIterator::UP SearchBasePtr;
-typedef std::unique_ptr<ResultSet> ResultSetPtr;
-
-using queryeval::HitCollector;
-using queryeval::SearchIterator;
+using attribute::Config;
+using attribute::SearchContextParams;
 using fef::MatchData;
 using fef::TermFieldMatchData;
 using fef::TermFieldMatchDataArray;
 using fef::TermFieldMatchDataPosition;
+using queryeval::HitCollector;
+using queryeval::SearchIterator;
+using queryeval::SimpleResult;
 
 class DocSet : public std::set<uint32_t>
 {
@@ -267,6 +272,9 @@ private:
     void requireThatOutOfBoundsSearchTermGivesZeroHits(const vespalib::string &name, const Config &cfg, int64_t maxValue);
     void requireThatOutOfBoundsSearchTermGivesZeroHits();
 
+    void single_bool_attribute_search_context_handles_true_and_false_queries();
+    void single_bool_attribute_search_iterator_handles_true_and_false_queries();
+
     // init maps with config objects
     void initIntegerConfig();
     void initFloatConfig();
@@ -1825,6 +1833,87 @@ SearchContextTest::requireThatOutOfBoundsSearchTermGivesZeroHits()
     }
 }
 
+class BoolAttributeFixture {
+private:
+    search::SingleBoolAttribute _attr;
+
+public:
+    BoolAttributeFixture(const SimpleResult& true_docs, uint32_t num_docs)
+        : _attr("bool_attr", search::GrowStrategy())
+    {
+        _attr.addDocs(num_docs);
+        for (uint32_t i = 0; i < true_docs.getHitCount(); ++i) {
+            uint32_t docid = true_docs.getHit(i);
+            _attr.update(docid, 1);
+        }
+        _attr.commit();
+    }
+    search::AttributeVector::SearchContext::UP create_search_context(const std::string& term) const {
+        return _attr.getSearch(std::make_unique<search::QueryTermSimple>(term, search::QueryTermSimple::WORD),
+                               SearchContextParams().useBitVector(true));
+    }
+    SimpleResult search_context(const std::string& term) const {
+        auto search_ctx = create_search_context(term);
+        SimpleResult result;
+        int32_t weight = 10;
+        for (uint32_t docid = 1; docid < _attr.getNumDocs(); ++docid) {
+            bool match_1 = search_ctx->matches(docid);
+            bool match_2 = search_ctx->matches(docid, weight);
+            EXPECT_EQUAL(match_1, match_2);
+            EXPECT_EQUAL(match_2 ? 1 : 0, weight);
+            if (match_1) {
+                result.addHit(docid);
+            }
+            weight = 10;
+        }
+        return result;
+    }
+    SimpleResult search_iterator(const std::string& term, bool strict) const {
+        auto search_ctx = create_search_context(term);
+        TermFieldMatchData tfmd;
+        auto itr = search_ctx->createIterator(&tfmd, strict);
+        SimpleResult result;
+        if (strict) {
+            result.searchStrict(*itr, _attr.getNumDocs());
+        } else {
+            result.search(*itr, _attr.getNumDocs());
+        }
+        return result;
+    }
+};
+
+void
+SearchContextTest::single_bool_attribute_search_context_handles_true_and_false_queries()
+{
+    BoolAttributeFixture f(SimpleResult().addHit(3).addHit(5).addHit(7), 9);
+
+    auto true_exp = SimpleResult().addHit(3).addHit(5).addHit(7);
+    EXPECT_EQUAL(true_exp, f.search_context("true"));
+    EXPECT_EQUAL(true_exp, f.search_context("1"));
+
+    auto false_exp = SimpleResult().addHit(1).addHit(2).addHit(4).addHit(6).addHit(8);
+    EXPECT_EQUAL(false_exp, f.search_context("false"));
+    EXPECT_EQUAL(false_exp, f.search_context("0"));
+}
+
+void
+SearchContextTest::single_bool_attribute_search_iterator_handles_true_and_false_queries()
+{
+    BoolAttributeFixture f(SimpleResult().addHit(3).addHit(5).addHit(7), 9);
+
+    auto true_exp = SimpleResult().addHit(3).addHit(5).addHit(7);
+    EXPECT_EQUAL(true_exp, f.search_iterator("true", false));
+    EXPECT_EQUAL(true_exp, f.search_iterator("1", false));
+    EXPECT_EQUAL(true_exp, f.search_iterator("true", true));
+    EXPECT_EQUAL(true_exp, f.search_iterator("1", true));
+
+    auto false_exp = SimpleResult().addHit(1).addHit(2).addHit(4).addHit(6).addHit(8);
+    EXPECT_EQUAL(false_exp, f.search_iterator("false", false));
+    EXPECT_EQUAL(false_exp, f.search_iterator("0", false));
+    EXPECT_EQUAL(false_exp, f.search_iterator("false", true));
+    EXPECT_EQUAL(false_exp, f.search_iterator("0", true));
+}
+
 void
 SearchContextTest::initIntegerConfig()
 {
@@ -1955,6 +2044,8 @@ SearchContextTest::Main()
     TEST_DO(requireThatInvalidSearchTermGivesZeroHits());
     TEST_DO(requireThatFlagAttributeHandlesTheByteRange());
     TEST_DO(requireThatOutOfBoundsSearchTermGivesZeroHits());
+    TEST_DO(single_bool_attribute_search_context_handles_true_and_false_queries());
+    TEST_DO(single_bool_attribute_search_iterator_handles_true_and_false_queries());
 
     TEST_DONE();
 }
diff --git a/searchlib/src/tests/queryeval/queryeval.cpp b/searchlib/src/tests/queryeval/queryeval.cpp
index 32dc9b1fa7e..5601baa9113 100644
--- a/searchlib/src/tests/queryeval/queryeval.cpp
+++ b/searchlib/src/tests/queryeval/queryeval.cpp
@@ -346,29 +346,12 @@ public:
         (void) tfmda;
         return _sc->createIterator(&_tfmd, strict);
     }
-    const search::AttributeVector::SearchContext & getSearchContext() const { return *_sc; }
 private:
     search::SingleBoolAttribute     _a;
     search::AttributeVector::SearchContext::UP _sc;
     mutable TermFieldMatchData _tfmd;
 };
 
-TEST("test bool attribute searchcontext") {
-    SimpleResult a;
-    a.addHit(5).addHit(17).addHit(30);
-    auto bp = std::make_unique<DummySingleValueBitNumericAttributeBlueprint>(a);
-    const search::AttributeVector::SearchContext & sc = bp->getSearchContext();
-    EXPECT_FALSE(sc.matches(7));
-    EXPECT_TRUE(sc.matches(17));
-    int32_t weight(0);
-    EXPECT_FALSE(sc.matches(7, weight));
-    EXPECT_EQUAL(0, weight);
-    EXPECT_TRUE(sc.matches(17, weight));
-    EXPECT_EQUAL(1, weight);
-    EXPECT_FALSE(sc.matches(27, weight));
-    EXPECT_EQUAL(0, weight);
-}
-
 
 TEST("testAndNot") {
     {
diff --git a/searchlib/src/vespa/searchlib/attribute/singleboolattribute.cpp b/searchlib/src/vespa/searchlib/attribute/singleboolattribute.cpp
index fd6e68f3473..e1ab47ed434 100644
--- a/searchlib/src/vespa/searchlib/attribute/singleboolattribute.cpp
+++ b/searchlib/src/vespa/searchlib/attribute/singleboolattribute.cpp
@@ -104,7 +104,7 @@ private:
     bool _valid;
     bool valid() const override { return _valid; }
     int32_t onFind(DocId docId, int32_t elemId, int32_t & weight) const override final {
-        if ((elemId == 0) && _bv.testBit(docId)) {
+        if ((elemId == 0) && (_invert != _bv.testBit(docId))) {
             weight = 1;
             return 0;
         }
@@ -113,7 +113,7 @@ private:
     }
 
     int32_t onFind(DocId docId, int32_t elemId) const override final {
-        return ((elemId == 0) && _bv.testBit(docId)) ? 0 : -1;
+        return ((elemId == 0) && (_invert != _bv.testBit(docId))) ? 0 : -1;
     }
 
 public: