Use make_for_lookup() member function on existing comparator

to make a new comparator which is used for lookup.

8 files changed, 71 insertions, 51 deletions

diff --git a/searchlib/src/vespa/searchlib/attribute/enumcomparator.h b/searchlib/src/vespa/searchlib/attribute/enumcomparator.h
index 52d9ebad902..255719ad1c1 100644
--- a/searchlib/src/vespa/searchlib/attribute/enumcomparator.h
+++ b/searchlib/src/vespa/searchlib/attribute/enumcomparator.h
@@ -18,14 +18,24 @@ public:
     using ParentType = vespalib::datastore::UniqueStoreComparator<EntryT, IEnumStore::InternalIndex>;
     using DataStoreType = typename ParentType::DataStoreType;
 
-    EnumStoreComparator(const DataStoreType& data_store, const EntryT& lookup_value)
-        : ParentType(data_store, lookup_value)
-    {}
     EnumStoreComparator(const DataStoreType& data_store)
         : ParentType(data_store)
     {}
 
+private:
+    EnumStoreComparator(const DataStoreType& data_store, const EntryT& lookup_value)
+        : ParentType(data_store, lookup_value)
+    {}
+
+public:
     static bool equal_helper(const EntryT& lhs, const EntryT& rhs);
+
+    EnumStoreComparator<EntryT> make_folded() const {
+        return *this;
+    }
+    EnumStoreComparator<EntryT> make_for_lookup(const EntryT& lookup_value) const {
+        return EnumStoreComparator<EntryT>(this->_store, lookup_value);
+    }
 };
 
 /**
@@ -45,22 +55,31 @@ public:
     EnumStoreStringComparator(const DataStoreType& data_store)
         : EnumStoreStringComparator(data_store, false)
     {}
+
+private:
     EnumStoreStringComparator(const DataStoreType& data_store, bool fold);
 
     /**
      * Creates a comparator using the given low-level data store and that uses the
      * given value during compare if the enum index is invalid.
      */
-    EnumStoreStringComparator(const DataStoreType& data_store, const char* lookup_value)
-        : EnumStoreStringComparator(data_store, false, lookup_value)
-    {}
     EnumStoreStringComparator(const DataStoreType& data_store, bool fold, const char* lookup_value);
     EnumStoreStringComparator(const DataStoreType& data_store, bool fold, const char* lookup_value, bool prefix);
 
+public:
     bool less(const vespalib::datastore::EntryRef lhs, const vespalib::datastore::EntryRef rhs) const override;
     bool equal(const vespalib::datastore::EntryRef lhs, const vespalib::datastore::EntryRef rhs) const override;
+    EnumStoreStringComparator make_folded() const {
+        return EnumStoreStringComparator(_store, true);
+    }
+    EnumStoreStringComparator make_for_lookup(const char* lookup_value) const {
+        return EnumStoreStringComparator(_store, _fold, lookup_value);
+    }
+    EnumStoreStringComparator make_for_prefix_lookup(const char* lookup_value) const {
+        return EnumStoreStringComparator(_store, _fold, lookup_value, true);
+    }
 private:
-    inline bool use_prefix() const { return _prefix; }
+    inline bool use_prefix() const noexcept { return _prefix; }
     const bool _fold;
     const bool _prefix;
     uint32_t   _prefix_len;
diff --git a/searchlib/src/vespa/searchlib/attribute/enumstore.h b/searchlib/src/vespa/searchlib/attribute/enumstore.h
index 8cafd04586e..6560decf42f 100644
--- a/searchlib/src/vespa/searchlib/attribute/enumstore.h
+++ b/searchlib/src/vespa/searchlib/attribute/enumstore.h
@@ -37,15 +37,16 @@ namespace search {
 template <class EntryT>
 class EnumStoreT : public IEnumStore {
 public:
-    using ComparatorType = std::conditional_t<std::is_same_v<EntryT, const char *>,
+    using EntryType = EntryT;
+    static constexpr bool has_string_type = std::is_same_v<EntryType, const char *>;
+    using ComparatorType = std::conditional_t<has_string_type,
                                               EnumStoreStringComparator,
                                               EnumStoreComparator<EntryT>>;
-    using AllocatorType = std::conditional_t<std::is_same_v<EntryT, const char *>,
+    using AllocatorType = std::conditional_t<has_string_type,
                                              vespalib::datastore::UniqueStoreStringAllocator<InternalIndex>,
                                              vespalib::datastore::UniqueStoreAllocator<EntryT, InternalIndex>>;
     using UniqueStoreType = vespalib::datastore::UniqueStore<EntryT, InternalIndex, ComparatorType, AllocatorType>;
 
-    using EntryType = EntryT;
     using EnumStoreType = EnumStoreT<EntryT>;
     using EntryRef = vespalib::datastore::EntryRef;
     using EntryComparator = vespalib::datastore::EntryComparator;
@@ -69,10 +70,6 @@ private:
         return _store.get_allocator().get_wrapped(idx);
     }
 
-    static bool has_string_type() {
-        return std::is_same_v<EntryType, const char *>;
-    }
-
     ssize_t load_unique_values_internal(const void* src, size_t available, IndexVector& idx);
     ssize_t load_unique_value(const void* src, size_t available, Index& idx);
 
@@ -191,7 +188,7 @@ public:
     }
 
     ComparatorType make_comparator(const EntryType& lookup_value) const {
-        return ComparatorType(_store.get_data_store(), lookup_value);
+        return _store.get_comparator().make_for_lookup(lookup_value);
     }
 
     const EntryComparator & get_folded_comparator() const {
@@ -225,12 +222,12 @@ public:
     template <typename Type>
     ComparatorType
     make_folded_comparator(const Type& lookup_value) const {
-        return ComparatorType(_store.get_data_store(), is_folded(), lookup_value);
+        return _foldedComparator.make_for_lookup(lookup_value);
     }
     template<typename Type>
     ComparatorType
     make_folded_comparator_prefix(const Type& lookup_value) const {
-        return ComparatorType(_store.get_data_store(), is_folded(), lookup_value, true);
+        return _foldedComparator.make_for_prefix_lookup(lookup_value);
     }
     template<typename Type>
     std::vector<IEnumStore::EnumHandle>
diff --git a/searchlib/src/vespa/searchlib/attribute/enumstore.hpp b/searchlib/src/vespa/searchlib/attribute/enumstore.hpp
index 85cf676182e..140ac207d32 100644
--- a/searchlib/src/vespa/searchlib/attribute/enumstore.hpp
+++ b/searchlib/src/vespa/searchlib/attribute/enumstore.hpp
@@ -305,15 +305,15 @@ template <typename EntryT>
 std::unique_ptr<EntryComparator>
 EnumStoreT<EntryT>::allocate_comparator() const
 {
-    return std::make_unique<ComparatorType>(_store.get_data_store());
+    return std::make_unique<ComparatorType>(_store.get_comparator());
 }
 
 template <typename EntryT>
 std::unique_ptr<EntryComparator>
 EnumStoreT<EntryT>::allocate_optionally_folded_comparator(bool folded) const
 {
-    return (has_string_type() && folded)
-            ? std::make_unique<ComparatorType>(_store.get_data_store(), true)
+    return (has_string_type && folded)
+        ? std::make_unique<ComparatorType>(_store.get_comparator().make_folded())
             : std::unique_ptr<EntryComparator>();
 }
 
@@ -321,9 +321,9 @@ template <typename EntryT>
 typename EnumStoreT<EntryT>::ComparatorType
 EnumStoreT<EntryT>::make_optionally_folded_comparator(bool folded) const
 {
-    return (has_string_type() && folded)
-           ? ComparatorType(_store.get_data_store(), true)
-           : ComparatorType(_store.get_data_store());
+    return (has_string_type && folded)
+        ? _store.get_comparator().make_folded()
+        : _store.get_comparator();
 }
 
 }
diff --git a/vespalib/src/tests/datastore/fixed_size_hash_map/fixed_size_hash_map_test.cpp b/vespalib/src/tests/datastore/fixed_size_hash_map/fixed_size_hash_map_test.cpp
index 4f4c3ac94eb..58fd5b4c356 100644
--- a/vespalib/src/tests/datastore/fixed_size_hash_map/fixed_size_hash_map_test.cpp
+++ b/vespalib/src/tests/datastore/fixed_size_hash_map/fixed_size_hash_map_test.cpp
@@ -20,7 +20,7 @@ using vespalib::datastore::EntryRef;
 using RefT = vespalib::datastore::EntryRefT<22>;
 using MyAllocator = vespalib::datastore::UniqueStoreAllocator<uint32_t, RefT>;
 using MyDataStore = vespalib::datastore::DataStoreT<RefT>;
-using MyCompare = vespalib::datastore::UniqueStoreComparator<uint32_t, RefT>;
+using MyComparator = vespalib::datastore::UniqueStoreComparator<uint32_t, RefT>;
 using GenerationHandler = vespalib::GenerationHandler;
 using vespalib::makeLambdaTask;
 using vespalib::GenerationHolder;
@@ -48,7 +48,7 @@ struct DataStoreFixedSizeHashTest : public ::testing::Test
     GenerationHolder  _generation_holder;
     MyAllocator       _allocator;
     MyDataStore&      _store;
-    std::unique_ptr<const vespalib::datastore::EntryComparator> _comp;
+    const MyComparator _comparator;
     std::unique_ptr<FixedSizeHashMap> _hash_map;
     vespalib::Rand48 _rnd;
 
@@ -70,7 +70,7 @@ DataStoreFixedSizeHashTest::DataStoreFixedSizeHashTest()
       _generation_holder(),
       _allocator({}),
       _store(_allocator.get_data_store()),
-      _comp(std::make_unique<MyCompare>(_store)),
+      _comparator(_store),
       _hash_map(),
       _rnd()
 {
@@ -106,7 +106,7 @@ DataStoreFixedSizeHashTest::size() const noexcept
 void
 DataStoreFixedSizeHashTest::insert(uint32_t key)
 {
-    MyCompare comp(_store, key);
+    auto comp = _comparator.make_for_lookup(key);
     std::function<EntryRef(void)> insert_entry([this, key]() -> EntryRef { return _allocator.allocate(key); });
     auto& result = _hash_map->add(_hash_map->get_comp(comp), insert_entry);
     auto ref = result.first.load_relaxed();
@@ -117,7 +117,7 @@ DataStoreFixedSizeHashTest::insert(uint32_t key)
 void
 DataStoreFixedSizeHashTest::remove(uint32_t key)
 {
-    MyCompare comp(_store, key);
+    auto comp = _comparator.make_for_lookup(key);
     auto result = _hash_map->remove(_hash_map->get_comp(comp));
     if (result != nullptr) {
         auto ref = result->first.load_relaxed();
@@ -130,7 +130,7 @@ DataStoreFixedSizeHashTest::remove(uint32_t key)
 bool
 DataStoreFixedSizeHashTest::has_key(uint32_t key)
 {
-    MyCompare comp(_store, key);
+    auto comp = _comparator.make_for_lookup(key);
     auto result = _hash_map->find(_hash_map->get_comp(comp));
     if (result != nullptr) {
         auto ref = result->first.load_relaxed();
@@ -270,7 +270,7 @@ TEST_F(DataStoreFixedSizeHashTest, lookups_works_after_insert_and_remove)
         commit();
     }
     for (auto &kv : expected) {
-        MyCompare comp(_store, kv.first);
+        auto comp = _comparator.make_for_lookup(kv.first);
         EXPECT_EQ(kv.second, _hash_map->find(_hash_map->get_comp(comp)) != nullptr);
     }
 }
diff --git a/vespalib/src/tests/datastore/sharded_hash_map/sharded_hash_map_test.cpp b/vespalib/src/tests/datastore/sharded_hash_map/sharded_hash_map_test.cpp
index eec3a6e6188..9c2369dea08 100644
--- a/vespalib/src/tests/datastore/sharded_hash_map/sharded_hash_map_test.cpp
+++ b/vespalib/src/tests/datastore/sharded_hash_map/sharded_hash_map_test.cpp
@@ -25,7 +25,7 @@ using vespalib::datastore::ICompactable;
 using RefT = vespalib::datastore::EntryRefT<22>;
 using MyAllocator = vespalib::datastore::UniqueStoreAllocator<uint32_t, RefT>;
 using MyDataStore = vespalib::datastore::DataStoreT<RefT>;
-using MyCompare = vespalib::datastore::UniqueStoreComparator<uint32_t, RefT>;
+using MyComparator = vespalib::datastore::UniqueStoreComparator<uint32_t, RefT>;
 using MyHashMap = vespalib::datastore::ShardedHashMap;
 using GenerationHandler = vespalib::GenerationHandler;
 using vespalib::makeLambdaTask;
@@ -101,6 +101,7 @@ struct DataStoreShardedHashTest : public ::testing::Test
     GenerationHandler _generationHandler;
     MyAllocator       _allocator;
     MyDataStore&      _store;
+    const MyComparator _comparator;
     MyHashMap         _hash_map;
     vespalib::ThreadStackExecutor _writer; // 1 write thread
     vespalib::ThreadStackExecutor _readers; // multiple reader threads
@@ -134,7 +135,8 @@ DataStoreShardedHashTest::DataStoreShardedHashTest()
     : _generationHandler(),
       _allocator({}),
       _store(_allocator.get_data_store()),
-      _hash_map(std::make_unique<MyCompare>(_store)),
+      _comparator(_store),
+      _hash_map(std::make_unique<MyComparator>(_comparator)),
       _writer(1),
       _readers(4),
       _rnd(),
@@ -178,7 +180,7 @@ DataStoreShardedHashTest::commit()
 void
 DataStoreShardedHashTest::insert(uint32_t key)
 {
-    MyCompare comp(_store, key);
+    auto comp = _comparator.make_for_lookup(key);
     std::function<EntryRef(void)> insert_entry([this, key]() -> EntryRef { return _allocator.allocate(key); });
     auto& result = _hash_map.add(comp, EntryRef(), insert_entry);
     auto ref = result.first.load_relaxed();
@@ -189,7 +191,7 @@ DataStoreShardedHashTest::insert(uint32_t key)
 void
 DataStoreShardedHashTest::remove(uint32_t key)
 {
-    MyCompare comp(_store, key);
+    auto comp = _comparator.make_for_lookup(key);
     auto result = _hash_map.remove(comp, EntryRef());
     if (result != nullptr) {
         auto ref = result->first.load_relaxed();
@@ -210,7 +212,7 @@ DataStoreShardedHashTest::read_work(uint32_t cnt)
     for (i = 0; i < cnt && _stop_read.load() == 0; ++i) {
         auto guard = _generationHandler.takeGuard();
         uint32_t key = rnd.lrand48() % (_keyLimit + 1);
-        MyCompare comp(_store, key);
+        auto comp = _comparator.make_for_lookup(key);
         auto result = _hash_map.find(comp, EntryRef());
         if (result != nullptr) {
             auto ref = result->first.load_relaxed();
@@ -264,7 +266,7 @@ void
 DataStoreShardedHashTest::populate_sample_values(uint32_t cnt)
 {
     for (uint32_t i = 0; i < cnt; ++i) {
-        MyCompare comp(_store, i);
+        auto comp = _comparator.make_for_lookup(i);
         auto result = _hash_map.find(comp, EntryRef());
         ASSERT_NE(result, nullptr);
         EXPECT_EQ(i, _allocator.get_wrapped(result->first.load_relaxed()).value());
@@ -276,7 +278,7 @@ void
 DataStoreShardedHashTest::clear_sample_values(uint32_t cnt)
 {
     for (uint32_t i = 0; i < cnt; ++i) {
-        MyCompare comp(_store, i);
+        auto comp = _comparator.make_for_lookup(i);
         auto result = _hash_map.find(comp, EntryRef());
         ASSERT_NE(result, nullptr);
         EXPECT_EQ(i, _allocator.get_wrapped(result->first.load_relaxed()).value());
@@ -312,7 +314,7 @@ DataStoreShardedHashTest::test_normalize_values(bool use_filter, bool one_filter
         EXPECT_TRUE(_hash_map.normalize_values([](EntryRef ref) noexcept { RefT iref(ref); return RefT(iref.offset() + 300, iref.bufferId()); }));
     }
     for (uint32_t i = 0; i < large_population; ++i) {
-        MyCompare comp(_store, i);
+        auto comp = _comparator.make_for_lookup(i);
         auto result = _hash_map.find(comp, EntryRef());
         ASSERT_NE(result, nullptr);
         EXPECT_EQ(i, _allocator.get_wrapped(result->first.load_relaxed()).value());
diff --git a/vespalib/src/vespa/vespalib/datastore/unique_store.hpp b/vespalib/src/vespa/vespalib/datastore/unique_store.hpp
index fc912382cf8..d197af92fd9 100644
--- a/vespalib/src/vespa/vespalib/datastore/unique_store.hpp
+++ b/vespalib/src/vespa/vespalib/datastore/unique_store.hpp
@@ -50,7 +50,7 @@ template <typename EntryT, typename RefT, typename Comparator, typename Allocato
 UniqueStoreAddResult
 UniqueStore<EntryT, RefT, Comparator, Allocator>::add(EntryConstRefType value)
 {
-    Comparator comp(_store, value);
+    auto comp = _comparator.make_for_lookup(value);
     UniqueStoreAddResult result = _dict->add(comp, [this, &value]() -> EntryRef { return _allocator.allocate(value); });
     _allocator.get_wrapped(result.ref()).inc_ref_count();
     return result;
@@ -60,7 +60,7 @@ template <typename EntryT, typename RefT, typename Comparator, typename Allocato
 EntryRef
 UniqueStore<EntryT, RefT, Comparator, Allocator>::find(EntryConstRefType value)
 {
-    Comparator comp(_store, value);
+    auto comp = _comparator.make_for_lookup(value);
     return _dict->find(comp);
 }
 
diff --git a/vespalib/src/vespa/vespalib/datastore/unique_store_comparator.h b/vespalib/src/vespa/vespalib/datastore/unique_store_comparator.h
index 38ae8c0c442..b238b42c54e 100644
--- a/vespalib/src/vespa/vespalib/datastore/unique_store_comparator.h
+++ b/vespalib/src/vespa/vespalib/datastore/unique_store_comparator.h
@@ -106,14 +106,12 @@ protected:
             return _lookup_value;
         }
     }
-
-public:
     UniqueStoreComparator(const DataStoreType &store, const EntryType &lookup_value)
         : _store(store),
           _lookup_value(lookup_value)
     {
     }
-
+public:
     UniqueStoreComparator(const DataStoreType &store)
         : _store(store),
           _lookup_value()
@@ -134,6 +132,10 @@ public:
         const EntryType &rhsValue = get(rhs);
         return UniqueStoreComparatorHelper<EntryT>::hash(rhsValue);
     }
+
+    UniqueStoreComparator<EntryT, RefT> make_for_lookup(const EntryType& lookup_value) const {
+        return UniqueStoreComparator<EntryT, RefT>(_store, lookup_value);
+    }
 };
 
 }
diff --git a/vespalib/src/vespa/vespalib/datastore/unique_store_string_comparator.h b/vespalib/src/vespa/vespalib/datastore/unique_store_string_comparator.h
index 4ce9923b549..afdbf4b31fa 100644
--- a/vespalib/src/vespa/vespalib/datastore/unique_store_string_comparator.h
+++ b/vespalib/src/vespa/vespalib/datastore/unique_store_string_comparator.h
@@ -37,20 +37,17 @@ protected:
             return _lookup_value;
         }
     }
-
-public:
-    UniqueStoreStringComparator(const DataStoreType &store)
+    UniqueStoreStringComparator(const DataStoreType &store, const char *lookup_value)
         : _store(store),
-          _lookup_value(nullptr)
+          _lookup_value(lookup_value)
     {
     }
-
-    UniqueStoreStringComparator(const DataStoreType &store, const char *lookup_value)
+public:
+    UniqueStoreStringComparator(const DataStoreType &store)
         : _store(store),
-          _lookup_value(lookup_value)
+          _lookup_value(nullptr)
     {
     }
-
     bool less(const EntryRef lhs, const EntryRef rhs) const override {
         const char *lhs_value = get(lhs);
         const char *rhs_value = get(rhs);
@@ -66,6 +63,9 @@ public:
         vespalib::hash<const char *> hasher;
         return hasher(rhs_value);
     }
+    UniqueStoreStringComparator<RefT> make_for_lookup(const char* lookup_value) const {
+        return UniqueStoreStringComparator<RefT>(_store, lookup_value);
+    }
 };
 
 }