Merge pull request #7889 from vespa-engine/balder/test-and-fix-postinglist-fallback-iterator

Balder/test and fix postinglist fallback iterator

6 files changed, 45 insertions, 37 deletions

diff --git a/searchlib/src/tests/attribute/searchcontext/searchcontext.cpp b/searchlib/src/tests/attribute/searchcontext/searchcontext.cpp
index 167ffec7084..29849139fc5 100644
--- a/searchlib/src/tests/attribute/searchcontext/searchcontext.cpp
+++ b/searchlib/src/tests/attribute/searchcontext/searchcontext.cpp
@@ -71,8 +71,8 @@ public:
     }
 };
 
-DocSet::DocSet() : std::set<uint32_t>() {}
-DocSet::~DocSet() {}
+DocSet::DocSet() = default;
+DocSet::~DocSet() = default;
 
 template <typename V, typename T>
 class PostingList
@@ -97,7 +97,7 @@ template <typename V, typename T>
 PostingList<V, T>::PostingList(V & vec, T value) : _vec(&vec), _value(value), _hits() {}
 
 template <typename V, typename T>
-PostingList<V, T>::~PostingList() {}
+PostingList<V, T>::~PostingList() = default;
 
 class DocRange
 {
@@ -145,7 +145,7 @@ private:
     void checkResultSet(const ResultSet & rs, const DocSet & exp, bool bitVector);
 
     template<typename T, typename A>
-    void testSearchIterator(T key, const vespalib::string &keyAsString, const ConfigMap &cfgs);
+    void testSearchIterator(const std::vector<T> & keys, const vespalib::string &keyAsString, const ConfigMap &cfgs);
     void testSearchIteratorConformance();
     // test search functionality
     template <typename V, typename T>
@@ -169,25 +169,25 @@ private:
     class AttributeIteratorTester : public IteratorTester
     {
     public:
-        virtual bool matches(const SearchIterator & base) const override {
-            return dynamic_cast<const AttributeIterator *>(&base) != NULL;
+        bool matches(const SearchIterator & base) const override {
+            return dynamic_cast<const AttributeIterator *>(&base) != nullptr;
         }
     };
     class FlagAttributeIteratorTester : public IteratorTester
     {
     public:
-        virtual bool matches(const SearchIterator & base) const override {
-            return (dynamic_cast<const FlagAttributeIterator *>(&base) != NULL) ||
-                   (dynamic_cast<const BitVectorIterator *>(&base) != NULL) ||
-                   (dynamic_cast<const queryeval::EmptySearch *>(&base) != NULL);
+        bool matches(const SearchIterator & base) const override {
+            return (dynamic_cast<const FlagAttributeIterator *>(&base) != nullptr) ||
+                   (dynamic_cast<const BitVectorIterator *>(&base) != nullptr) ||
+                   (dynamic_cast<const queryeval::EmptySearch *>(&base) != nullptr);
         }
     };
     class AttributePostingListIteratorTester : public IteratorTester
     {
     public:
-        virtual bool matches(const SearchIterator & base) const override {
-            return dynamic_cast<const AttributePostingListIterator *>(&base) != NULL ||
-                dynamic_cast<const queryeval::EmptySearch *>(&base) != NULL;
+        bool matches(const SearchIterator & base) const override {
+            return dynamic_cast<const AttributePostingListIterator *>(&base) != nullptr ||
+                dynamic_cast<const queryeval::EmptySearch *>(&base) != nullptr;
                 
         }
     };
@@ -473,7 +473,7 @@ SearchContextTest::checkResultSet(const ResultSet & rs, const DocSet & expected,
     if (bitVector) {
         const BitVector * vec = rs.getBitOverflow();
         if (expected.size() != 0) {
-            ASSERT_TRUE(vec != NULL);
+            ASSERT_TRUE(vec != nullptr);
             for (const auto & expect : expected) {
                 EXPECT_TRUE(vec->testBit(expect));
             }
@@ -481,7 +481,7 @@ SearchContextTest::checkResultSet(const ResultSet & rs, const DocSet & expected,
     } else {
         const RankedHit * array = rs.getArray();
         if (expected.size() != 0) {
-            ASSERT_TRUE(array != NULL);
+            ASSERT_TRUE(array != nullptr);
             uint32_t i = 0;
             for (DocSet::const_iterator iter = expected.begin();
                  iter != expected.end(); ++iter, ++i)
@@ -618,11 +618,12 @@ void SearchContextTest::testSearch(const ConfigMap & cfgs) {
 template<typename T, typename A>
 class Verifier : public search::test::SearchIteratorVerifier {
 public:
-    Verifier(T key, const vespalib::string & keyAsString, const vespalib::string & name,
+    Verifier(const std::vector<T> & keys, const vespalib::string & keyAsString, const vespalib::string & name,
              const Config & cfg, bool withElementId);
     ~Verifier();
     SearchIterator::UP
     create(bool strict) const override {
+        _sc->fetchPostings(strict);
         auto search = _sc->createIterator(&_dummy, strict);
         if (_withElementId) {
             search = std::make_unique<attribute::ElementIterator>(std::move(search), *_sc, _dummy);
@@ -637,42 +638,51 @@ private:
 };
 
 template<typename T, typename A>
-Verifier<T, A>::Verifier(T key, const vespalib::string & keyAsString, const vespalib::string & name,
+Verifier<T, A>::Verifier(const std::vector<T> & keys, const vespalib::string & keyAsString, const vespalib::string & name,
                          const Config & cfg, bool withElementId)
     : _withElementId(withElementId),
       _attribute(AttributeFactory::createAttribute(name + "-initrange", cfg)),
       _sc()
 {
     SearchContextTest::addDocs(*_attribute, getDocIdLimit());
+    size_t i(0);
     for (uint32_t doc : getExpectedDocIds()) {
         EXPECT_TRUE(nullptr != dynamic_cast<A *>(_attribute.get()));
-        EXPECT_TRUE(dynamic_cast<A *>(_attribute.get())->update(doc, key));
+        EXPECT_TRUE(dynamic_cast<A *>(_attribute.get())->update(doc, keys[(i++)%keys.size()]));
     }
     _attribute->commit(true);
     _sc = SearchContextTest::getSearch(*_attribute, keyAsString);
     ASSERT_TRUE(_sc->valid());
-    _sc->fetchPostings(true);
 }
 
 template<typename T, typename A>
 Verifier<T, A>::~Verifier() = default;
 
 template<typename T, typename A>
-void SearchContextTest::testSearchIterator(T key, const vespalib::string &keyAsString, const ConfigMap &cfgs) {
+void SearchContextTest::testSearchIterator(const std::vector<T> & keys, const vespalib::string &keyAsString, const ConfigMap &cfgs) {
 
     for (bool withElementId : {false, true} ) {
         for (const auto & cfg : cfgs) {
-            Verifier<T, A> verifier(key, keyAsString, cfg.first, cfg.second, withElementId);
-            verifier.verify();
+            {
+                Verifier<T, A> verifier(keys, keyAsString, cfg.first, cfg.second, withElementId);
+                verifier.verify();
+            }
+            {
+                Config withFilter(cfg.second);
+                withFilter.setIsFilter(true);
+                Verifier<T, A> verifier(keys, keyAsString, cfg.first + "-filter", withFilter, withElementId);
+                verifier.verify();
+            }
         }
     }
 
 }
 
 void SearchContextTest::testSearchIteratorConformance() {
-    testSearchIterator<AttributeVector::largeint_t, IntegerAttribute>(42, "42", _integerCfg);
-    testSearchIterator<double, FloatingPointAttribute>(42.42, "42.42", _floatCfg);
-    testSearchIterator<vespalib::string, StringAttribute>("any-key", "any-key", _stringCfg);
+    testSearchIterator<AttributeVector::largeint_t, IntegerAttribute>({42,45,46}, "[0;100]", _integerCfg);
+    testSearchIterator<AttributeVector::largeint_t, IntegerAttribute>({42}, "42", _integerCfg);
+    testSearchIterator<double, FloatingPointAttribute>({42.42}, "42.42", _floatCfg);
+    testSearchIterator<vespalib::string, StringAttribute>({"any-key"}, "any-key", _stringCfg);
 }
 
 void
diff --git a/searchlib/src/vespa/searchlib/attribute/multinumericenumattribute.h b/searchlib/src/vespa/searchlib/attribute/multinumericenumattribute.h
index b25438f8f2a..08233c09ef8 100644
--- a/searchlib/src/vespa/searchlib/attribute/multinumericenumattribute.h
+++ b/searchlib/src/vespa/searchlib/attribute/multinumericenumattribute.h
@@ -135,7 +135,7 @@ protected:
             return -1;
         }
 
-        bool
+        int32_t
         find(DocId doc, int32_t elemId) const
         {
             WeightedIndexArrayRef indices(_toBeSearched._mvMapping.get(doc));
diff --git a/searchlib/src/vespa/searchlib/attribute/multinumericenumattribute.hpp b/searchlib/src/vespa/searchlib/attribute/multinumericenumattribute.hpp
index d94a59555bd..6a212b8d8b6 100644
--- a/searchlib/src/vespa/searchlib/attribute/multinumericenumattribute.hpp
+++ b/searchlib/src/vespa/searchlib/attribute/multinumericenumattribute.hpp
@@ -192,9 +192,9 @@ MultiValueNumericEnumAttribute<B, M>::ArraySearchContext::createFilterIterator(f
 
 template <typename B, typename M>
 MultiValueNumericEnumAttribute<B, M>::ArraySearchContext::ArraySearchContext(QueryTermSimpleUP qTerm, const NumericAttribute & toBeSearched) :
-        NumericAttribute::Range<T>(*qTerm),
-        SearchContext(toBeSearched),
-        _toBeSearched(static_cast<const MultiValueNumericEnumAttribute<B, M> &>(toBeSearched))
+    NumericAttribute::Range<T>(*qTerm),
+    SearchContext(toBeSearched),
+    _toBeSearched(static_cast<const MultiValueNumericEnumAttribute<B, M> &>(toBeSearched))
 { }
 
 template <typename B, typename M>
diff --git a/searchlib/src/vespa/searchlib/attribute/postinglistsearchcontext.cpp b/searchlib/src/vespa/searchlib/attribute/postinglistsearchcontext.cpp
index 4c2c0043de8..b520b53551c 100644
--- a/searchlib/src/vespa/searchlib/attribute/postinglistsearchcontext.cpp
+++ b/searchlib/src/vespa/searchlib/attribute/postinglistsearchcontext.cpp
@@ -40,9 +40,7 @@ PostingListSearchContext(const Dictionary &dictionary,
 }
 
 
-PostingListSearchContext::~PostingListSearchContext()
-{
-}
+PostingListSearchContext::~PostingListSearchContext() = default;
 
 
 void
diff --git a/searchlib/src/vespa/searchlib/attribute/postinglistsearchcontext.hpp b/searchlib/src/vespa/searchlib/attribute/postinglistsearchcontext.hpp
index cf9450cbcca..15ff9c1ea7a 100644
--- a/searchlib/src/vespa/searchlib/attribute/postinglistsearchcontext.hpp
+++ b/searchlib/src/vespa/searchlib/attribute/postinglistsearchcontext.hpp
@@ -113,12 +113,12 @@ template <typename DataT>
 void
 PostingListSearchContextT<DataT>::fetchPostings(bool strict)
 {
-    assert (! _fetchPostingsDone);
+    if (_fetchPostingsDone) return;
 
     _fetchPostingsDone = true;
-    if (_uniqueValues < 2u) {
-        return;
-    }
+
+    if (_uniqueValues < 2u) return;
+
     if (strict && !fallbackToFiltering()) {
         size_t sum(countHits());
         if (sum < _docIdLimit / 64) {
diff --git a/searchlib/src/vespa/searchlib/test/searchiteratorverifier.cpp b/searchlib/src/vespa/searchlib/test/searchiteratorverifier.cpp
index 59b4e2ab64c..fce768ab0d2 100644
--- a/searchlib/src/vespa/searchlib/test/searchiteratorverifier.cpp
+++ b/searchlib/src/vespa/searchlib/test/searchiteratorverifier.cpp
@@ -114,7 +114,7 @@ SearchIteratorVerifier::createFullIterator() const
 {
     return make_unique<TrueSearch>(_trueTfmd);
 }
-SearchIteratorVerifier::~SearchIteratorVerifier() { }
+SearchIteratorVerifier::~SearchIteratorVerifier() = default;
 
 void
 SearchIteratorVerifier::verify() const {