Merge pull request #8061 from vespa-engine/balder/add-multibitvector-iterator-too-conformance-tests-too

Balder/add multibitvector iterator too conformance tests too

12 files changed, 97 insertions, 50 deletions

diff --git a/searchlib/src/tests/diskindex/diskindex/diskindex_test.cpp b/searchlib/src/tests/diskindex/diskindex/diskindex_test.cpp
index ba266a85470..f4aae4e2d73 100644
--- a/searchlib/src/tests/diskindex/diskindex/diskindex_test.cpp
+++ b/searchlib/src/tests/diskindex/diskindex/diskindex_test.cpp
@@ -80,10 +80,11 @@ private:
     FakePosting::SP _fp;
 };
 
-    Verifier::Verifier(FakePosting::SP fp) :
-    _fp(std::move(fp))
-            { }
-    Verifier::~Verifier() {}
+Verifier::Verifier(FakePosting::SP fp)
+    : _fp(std::move(fp))
+{ }
+
+Verifier::~Verifier() = default;
 
 void
 Test::requireThatSearchIteratorsConforms()
diff --git a/searchlib/src/tests/queryeval/multibitvectoriterator/CMakeLists.txt b/searchlib/src/tests/queryeval/multibitvectoriterator/CMakeLists.txt
index c447aeabb76..718b0553fc9 100644
--- a/searchlib/src/tests/queryeval/multibitvectoriterator/CMakeLists.txt
+++ b/searchlib/src/tests/queryeval/multibitvectoriterator/CMakeLists.txt
@@ -4,6 +4,7 @@ vespa_add_executable(searchlib_multibitvectoriterator_test_app TEST
     multibitvectoriterator_test.cpp
     DEPENDS
     searchlib
+    searchlib_test
 )
 vespa_add_test(NAME searchlib_multibitvectoriterator_test_app COMMAND searchlib_multibitvectoriterator_test_app)
 vespa_add_executable(searchlib_multibitvectoriterator_bench_app
diff --git a/searchlib/src/tests/queryeval/multibitvectoriterator/multibitvectoriterator_test.cpp b/searchlib/src/tests/queryeval/multibitvectoriterator/multibitvectoriterator_test.cpp
index 91d39965531..28ba13bb4fc 100644
--- a/searchlib/src/tests/queryeval/multibitvectoriterator/multibitvectoriterator_test.cpp
+++ b/searchlib/src/tests/queryeval/multibitvectoriterator/multibitvectoriterator_test.cpp
@@ -1,6 +1,4 @@
 // Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
-#include <vespa/log/log.h>
-LOG_SETUP("multibitvectoriterator_test");
 #include <vespa/vespalib/testkit/testapp.h>
 #include <vespa/searchlib/queryeval/multibitvectoriterator.h>
 #include <vespa/searchlib/queryeval/emptysearch.h>
@@ -11,10 +9,15 @@ LOG_SETUP("multibitvectoriterator_test");
 #include <vespa/searchlib/queryeval/orsearch.h>
 #include <vespa/searchlib/fef/termfieldmatchdata.h>
 #include <vespa/searchlib/fef/termfieldmatchdataarray.h>
+#include <vespa/searchlib/test/searchiteratorverifier.h>
+
+#include <vespa/log/log.h>
+LOG_SETUP("multibitvectoriterator_test");
 
 using namespace search::queryeval;
 using namespace search::fef;
 using namespace search;
+using vespalib::Trinary;
 
 //-----------------------------------------------------------------------------
 
@@ -29,6 +32,7 @@ public:
     void testOr();
     void testAndWith(bool invert);
     void testEndGuard(bool invert);
+    void testIteratorConformance();
     template<typename T>
     void testThatOptimizePreservesUnpack();
     template <typename T>
@@ -355,7 +359,7 @@ Test::testOptimizeCommon(bool isAnd, bool invert)
         EXPECT_EQUAL(2u, m.getChildren().size());
         EXPECT_TRUE(dynamic_cast<const EmptySearch *>(m.getChildren()[0]) != nullptr);
         EXPECT_TRUE(dynamic_cast<const MultiBitVectorIteratorBase *>(m.getChildren()[1]) != nullptr);
-        EXPECT_FALSE(dynamic_cast<const MultiBitVectorIteratorBase *>(m.getChildren()[1])->isStrict());
+        EXPECT_TRUE(Trinary::False == m.getChildren()[1]->is_strict());
     }
     {
         MultiSearch::Children children;
@@ -371,7 +375,7 @@ Test::testOptimizeCommon(bool isAnd, bool invert)
         EXPECT_EQUAL(2u, m.getChildren().size());
         EXPECT_TRUE(dynamic_cast<const EmptySearch *>(m.getChildren()[0]) != nullptr);
         EXPECT_TRUE(dynamic_cast<const MultiBitVectorIteratorBase *>(m.getChildren()[1]) != nullptr);
-        EXPECT_TRUE(dynamic_cast<const MultiBitVectorIteratorBase *>(m.getChildren()[1])->isStrict());
+        EXPECT_TRUE(Trinary::True == m.getChildren()[1]->is_strict());
     }
     {
         MultiSearch::Children children;
@@ -418,7 +422,7 @@ Test::testOptimizeAndOr(bool invert)
         s = MultiBitVectorIteratorBase::optimize(std::move(s));
         EXPECT_TRUE(s);
         EXPECT_TRUE(dynamic_cast<const MultiBitVectorIteratorBase *>(s.get()) != nullptr);
-        EXPECT_FALSE(dynamic_cast<const MultiBitVectorIteratorBase *>(s.get())->isStrict());
+        EXPECT_TRUE(Trinary::False == s->is_strict());
     }
     {
         MultiSearch::Children children;
@@ -433,7 +437,7 @@ Test::testOptimizeAndOr(bool invert)
         const MultiSearch & m(dynamic_cast<const MultiSearch &>(*s));
         EXPECT_EQUAL(2u, m.getChildren().size());
         EXPECT_TRUE(dynamic_cast<const MultiBitVectorIteratorBase *>(m.getChildren()[0]) != nullptr);
-        EXPECT_FALSE(dynamic_cast<const MultiBitVectorIteratorBase *>(m.getChildren()[0])->isStrict());
+        EXPECT_TRUE(Trinary::False == m.getChildren()[0]->is_strict());
         EXPECT_TRUE(dynamic_cast<const EmptySearch *>(m.getChildren()[1]) != nullptr);
     }
     {
@@ -449,7 +453,7 @@ Test::testOptimizeAndOr(bool invert)
         const MultiSearch & m(dynamic_cast<const MultiSearch &>(*s));
         EXPECT_EQUAL(2u, m.getChildren().size());
         EXPECT_TRUE(dynamic_cast<const MultiBitVectorIteratorBase *>(m.getChildren()[0]) != nullptr);
-        EXPECT_FALSE(dynamic_cast<const MultiBitVectorIteratorBase *>(m.getChildren()[0])->isStrict());
+        EXPECT_TRUE(Trinary::False == m.getChildren()[0]->is_strict());
         EXPECT_TRUE(dynamic_cast<const EmptySearch *>(m.getChildren()[1]) != nullptr);
     }
     {
@@ -465,7 +469,7 @@ Test::testOptimizeAndOr(bool invert)
         const MultiSearch & m(dynamic_cast<const MultiSearch &>(*s));
         EXPECT_EQUAL(2u, m.getChildren().size());
         EXPECT_TRUE(dynamic_cast<const MultiBitVectorIteratorBase *>(m.getChildren()[0]) != nullptr);
-        EXPECT_TRUE(dynamic_cast<const MultiBitVectorIteratorBase *>(m.getChildren()[0])->isStrict());
+        EXPECT_TRUE(Trinary::True == m.getChildren()[0]->is_strict());
         EXPECT_TRUE(dynamic_cast<const EmptySearch *>(m.getChildren()[1]) != nullptr);
     }
     {
@@ -481,7 +485,7 @@ Test::testOptimizeAndOr(bool invert)
         const MultiSearch & m(dynamic_cast<const MultiSearch &>(*s));
         EXPECT_EQUAL(2u, m.getChildren().size());
         EXPECT_TRUE(dynamic_cast<const MultiBitVectorIteratorBase *>(m.getChildren()[0]) != nullptr);
-        EXPECT_TRUE(dynamic_cast<const MultiBitVectorIteratorBase *>(m.getChildren()[0])->isStrict());
+        EXPECT_TRUE(Trinary::True == m.getChildren()[0]->is_strict());
         EXPECT_TRUE(dynamic_cast<const EmptySearch *>(m.getChildren()[1]) != nullptr);
     }
 }
@@ -506,6 +510,60 @@ Test::testEndGuard(bool invert)
     EXPECT_FALSE(m.seek(_bvs[0]->size()+987));
 }
 
+class Verifier : public search::test::SearchIteratorVerifier {
+public:
+    Verifier(size_t numBv, bool is_and);
+    ~Verifier();
+
+    SearchIterator::UP create(bool strict) const override;
+
+private:
+    bool _is_and;
+    mutable TermFieldMatchData _tfmd;
+    std::vector<BitVector::UP> _bvs;
+};
+
+Verifier::Verifier(size_t numBv, bool is_and)
+    : _is_and(is_and),
+      _bvs()
+{
+    for (size_t i(0); i < numBv; i++) {
+        _bvs.push_back(BitVector::create(getDocIdLimit()));
+    }
+    for (uint32_t docId: getExpectedDocIds()) {
+        if (_is_and) {
+            for (auto & bv : _bvs) {
+                bv->setBit(docId);
+            }
+        } else {
+            _bvs[docId%_bvs.size()]->setBit(docId);
+        }
+    }
+}
+Verifier::~Verifier() = default;
+
+SearchIterator::UP
+Verifier::create(bool strict) const {
+    MultiSearch::Children bvs;
+    for (const auto & bv : _bvs) {
+        bvs.push_back(BitVectorIterator::create(bv.get(), getDocIdLimit(), _tfmd, strict, false).release());
+    }
+    SearchIterator::UP iter(_is_and ? AndSearch::create(bvs, strict) : OrSearch::create(bvs, strict));
+    auto mbvit = MultiBitVectorIteratorBase::optimize(std::move(iter));
+    EXPECT_TRUE((bvs.size() < 2) || (dynamic_cast<const MultiBitVectorIteratorBase *>(mbvit.get()) != nullptr));
+    EXPECT_EQUAL(strict, Trinary::True == mbvit->is_strict());
+    return mbvit;
+}
+
+void Test::testIteratorConformance() {
+    for (bool is_and : {false, true}) {
+        for (size_t i(1); i < 6; i++) {
+            Verifier searchIteratorVerifier(i, is_and);
+            searchIteratorVerifier.verify();
+        }
+    }
+}
+
 int
 Test::Main()
 {
@@ -527,6 +585,8 @@ Test::Main()
     testAndWith(false);
     testAndWith(true);
     TEST_FLUSH();
+    testIteratorConformance();
+    TEST_FLUSH();
     TEST_DONE();
 }
 
diff --git a/searchlib/src/vespa/searchlib/common/bitvectoriterator.h b/searchlib/src/vespa/searchlib/common/bitvectoriterator.h
index be19b1e1343..6200837d449 100644
--- a/searchlib/src/vespa/searchlib/common/bitvectoriterator.h
+++ b/searchlib/src/vespa/searchlib/common/bitvectoriterator.h
@@ -28,7 +28,6 @@ public:
     const void *getBitValues() const { return _bv.getStart(); }
 
     Trinary is_strict() const override { return Trinary::False; }
-    virtual bool isStrict() const { return (is_strict() == Trinary::True); }
     uint32_t getDocIdLimit() const { return _docIdLimit; }
     static UP create(const BitVector *const other, fef::TermFieldMatchData &matchData, bool strict, bool inverted = false);
     static UP create(const BitVector *const other, uint32_t docIdLimit,
diff --git a/searchlib/src/vespa/searchlib/queryeval/iterator_pack.cpp b/searchlib/src/vespa/searchlib/queryeval/iterator_pack.cpp
index b11f85e09f9..2f9242bb3fd 100644
--- a/searchlib/src/vespa/searchlib/queryeval/iterator_pack.cpp
+++ b/searchlib/src/vespa/searchlib/queryeval/iterator_pack.cpp
@@ -6,7 +6,7 @@
 
 namespace search::queryeval {
 
-SearchIteratorPack::~SearchIteratorPack() { }
+SearchIteratorPack::~SearchIteratorPack() = default;
 
 SearchIteratorPack::SearchIteratorPack() : _children(), _childMatch(), _md() {}
 
diff --git a/searchlib/src/vespa/searchlib/queryeval/iterators.cpp b/searchlib/src/vespa/searchlib/queryeval/iterators.cpp
index 7b8485ad2f5..32f6122558e 100644
--- a/searchlib/src/vespa/searchlib/queryeval/iterators.cpp
+++ b/searchlib/src/vespa/searchlib/queryeval/iterators.cpp
@@ -2,9 +2,7 @@
 
 #include "iterators.h"
 
-namespace search {
-
-namespace queryeval {
+namespace search::queryeval {
 
 RankedSearchIteratorBase::
 RankedSearchIteratorBase(const fef::TermFieldMatchDataArray &matchData)
@@ -13,6 +11,4 @@ RankedSearchIteratorBase(const fef::TermFieldMatchDataArray &matchData)
       _needUnpack(1)
 { }
 
-} // namespace queryeval
-
-} // namespace search
+}
diff --git a/searchlib/src/vespa/searchlib/queryeval/iterators.h b/searchlib/src/vespa/searchlib/queryeval/iterators.h
index 79c91f6be39..43d046a78b7 100644
--- a/searchlib/src/vespa/searchlib/queryeval/iterators.h
+++ b/searchlib/src/vespa/searchlib/queryeval/iterators.h
@@ -5,9 +5,7 @@
 #include "searchiterator.h"
 #include <vespa/searchlib/fef/termfieldmatchdataarray.h>
 
-namespace search {
-
-namespace queryeval {
+namespace search::queryeval {
 
 class DocIdAndFeatures;
 
@@ -28,7 +26,4 @@ public:
     RankedSearchIteratorBase(const fef::TermFieldMatchDataArray &matchData);
 };
 
-} // namespace queryeval
-
-} // namespace search
-
+}
diff --git a/searchlib/src/vespa/searchlib/queryeval/multibitvectoriterator.cpp b/searchlib/src/vespa/searchlib/queryeval/multibitvectoriterator.cpp
index cf660624cff..bba331284f9 100644
--- a/searchlib/src/vespa/searchlib/queryeval/multibitvectoriterator.cpp
+++ b/searchlib/src/vespa/searchlib/queryeval/multibitvectoriterator.cpp
@@ -13,6 +13,8 @@
 
 namespace search::queryeval {
 
+using vespalib::Trinary;
+
 namespace {
 
 template<typename Update>
@@ -25,7 +27,7 @@ protected:
     void strictSeek(uint32_t docId);
 private:
     void doSeek(uint32_t docId) override;
-    bool isStrict() const override { return false; }
+    Trinary is_strict() const override { return Trinary::False; }
     bool acceptExtraFilter() const override { return Update::isAnd(); }
     Update                  _update;
 };
@@ -37,7 +39,7 @@ public:
     MultiBitVectorIteratorStrict(const MultiSearch::Children & children) : MultiBitVectorIterator<Update>(children) { }
 private:
     void doSeek(uint32_t docId) override { this->strictSeek(docId); }
-    bool isStrict() const override { return true; }
+    Trinary is_strict() const override { return Trinary::True; }
 };
 
 template<typename Update>
@@ -211,7 +213,7 @@ MultiBitVectorIteratorBase::optimizeMultiSearch(SearchIterator::UP parentIt)
                     _unpackIndex.push_back(stolen.size());
                 }
                 SearchIterator::UP bit = parent.remove(it);
-                if ( ! strict && static_cast<const BitVectorIterator &>(*bit).isStrict()) {
+                if ( ! strict && (bit->is_strict() == Trinary::True)) {
                     strict = true;
                 }
                 stolen.push_back(bit.release());
diff --git a/searchlib/src/vespa/searchlib/queryeval/multibitvectoriterator.h b/searchlib/src/vespa/searchlib/queryeval/multibitvectoriterator.h
index f233ee66fd0..cc40f834114 100644
--- a/searchlib/src/vespa/searchlib/queryeval/multibitvectoriterator.h
+++ b/searchlib/src/vespa/searchlib/queryeval/multibitvectoriterator.h
@@ -12,7 +12,6 @@ class MultiBitVectorIteratorBase : public MultiSearch, protected BitWord
 {
 public:
     ~MultiBitVectorIteratorBase();
-    virtual bool isStrict() const = 0;
     void initRange(uint32_t beginId, uint32_t endId) override;
     void addUnpackIndex(size_t index) { _unpackInfo.add(index); }
     /**
diff --git a/searchlib/src/vespa/searchlib/test/document_weight_attribute_helper.h b/searchlib/src/vespa/searchlib/test/document_weight_attribute_helper.h
index 308c305634a..163007b1828 100644
--- a/searchlib/src/vespa/searchlib/test/document_weight_attribute_helper.h
+++ b/searchlib/src/vespa/searchlib/test/document_weight_attribute_helper.h
@@ -8,8 +8,7 @@
 #include <vespa/searchlib/attribute/attributefactory.h>
 #include <vespa/vespalib/testkit/test_kit.h>
 
-namespace search {
-namespace test {
+namespace search::test {
 
 class DocumentWeightAttributeHelper
 {
@@ -51,5 +50,4 @@ public:
     const IDocumentWeightAttribute &dwa() const { return *_dwa; }
 };
 
-} // namespace search::test
-} // namespace search
+}
diff --git a/searchlib/src/vespa/searchlib/test/fakedata/fpfactory.cpp b/searchlib/src/vespa/searchlib/test/fakedata/fpfactory.cpp
index 712e44002a8..f10af286dd7 100644
--- a/searchlib/src/vespa/searchlib/test/fakedata/fpfactory.cpp
+++ b/searchlib/src/vespa/searchlib/test/fakedata/fpfactory.cpp
@@ -12,9 +12,7 @@ namespace search::fakedata {
 
 using index::Schema;
 
-FPFactory::~FPFactory(void)
-{
-}
+FPFactory::~FPFactory() = default;
 
 void
 FPFactory::setup(const FakeWordSet &fws)
@@ -43,7 +41,7 @@ FPFactory::setup(const std::vector<const FakeWord *> &fws)
 typedef std::map<const std::string, FPFactoryMaker *const>
 FPFactoryMap;
 
-static FPFactoryMap *fpFactoryMap = NULL;
+static FPFactoryMap *fpFactoryMap = nullptr;
 
 /*
  * Posting list factory glue.
@@ -52,15 +50,15 @@ static FPFactoryMap *fpFactoryMap = NULL;
 FPFactory *
 getFPFactory(const std::string &name, const Schema &schema)
 {
-    if (fpFactoryMap == NULL)
-        return NULL;
+    if (fpFactoryMap == nullptr)
+        return nullptr;
 
     FPFactoryMap::const_iterator i(fpFactoryMap->find(name));
 
     if (i != fpFactoryMap->end())
         return i->second(schema);
     else
-        return NULL;
+        return nullptr;
 }
 
 
@@ -69,7 +67,7 @@ getPostingTypes()
 {
     std::vector<std::string> res;
 
-    if (fpFactoryMap != NULL)
+    if (fpFactoryMap != nullptr)
         for (FPFactoryMap::const_iterator i(fpFactoryMap->begin());
              i != fpFactoryMap->end();
              ++i)
@@ -81,20 +79,20 @@ getPostingTypes()
 FPFactoryInit::FPFactoryInit(const FPFactoryMapEntry &fpFactoryMapEntry)
     : _key(fpFactoryMapEntry.first)
 {
-    if (fpFactoryMap == NULL)
+    if (fpFactoryMap == nullptr)
         fpFactoryMap = new FPFactoryMap;
     fpFactoryMap->insert(fpFactoryMapEntry);
 }
 
 FPFactoryInit::~FPFactoryInit()
 {
-    assert(fpFactoryMap != NULL);
+    assert(fpFactoryMap != nullptr);
     size_t eraseRes = fpFactoryMap->erase(_key);
     assert(eraseRes == 1);
     (void) eraseRes;
     if (fpFactoryMap->empty()) {
         delete fpFactoryMap;
-        fpFactoryMap = NULL;
+        fpFactoryMap = nullptr;
     }
 }
 
diff --git a/searchlib/src/vespa/searchlib/test/initrange.h b/searchlib/src/vespa/searchlib/test/initrange.h
index 4237a0ac0f9..9431740ac08 100644
--- a/searchlib/src/vespa/searchlib/test/initrange.h
+++ b/searchlib/src/vespa/searchlib/test/initrange.h
@@ -5,8 +5,7 @@
 #include <vespa/searchlib/fef/termfieldmatchdata.h>
 #include <vector>
 
-namespace search {
-namespace test {
+namespace search::test {
 
 class InitRangeVerifier {
 public:
@@ -37,4 +36,3 @@ private:
 };
 
 }
-}