Avoid dependency on weighted set ordering in unit tests

Weighted sets are not guaranteed to be ordered, but thus far they
have been in practice due to implementation details of the update logic.

5 files changed, 133 insertions, 41 deletions

diff --git a/searchlib/src/tests/attribute/attribute_test.cpp b/searchlib/src/tests/attribute/attribute_test.cpp
index 2f3df28cc6f..ce1344f5c26 100644
--- a/searchlib/src/tests/attribute/attribute_test.cpp
+++ b/searchlib/src/tests/attribute/attribute_test.cpp
@@ -19,6 +19,7 @@
 #include <vespa/searchlib/attribute/singlestringattribute.h>
 #include <vespa/searchlib/index/dummyfileheadercontext.h>
 #include <vespa/searchlib/util/randomgenerator.h>
+#include <vespa/searchlib/test/weighted_type_test_utils.h>
 #include <vespa/vespalib/io/fileutil.h>
 #include <vespa/vespalib/testkit/testapp.h>
 #include <cmath>
@@ -137,6 +138,20 @@ replace_suffix(AttributeVector &v, const vespalib::string &suffix)
     return name + suffix;
 }
 
+template <typename Container, typename V>
+bool contains(const Container& c, size_t elems, const V& value) {
+    auto end = c.begin() + elems;
+    return (std::find(c.begin(), end, value) != end);
+}
+
+template <typename Container, typename V>
+bool contains_value(const Container& c, size_t elems, const V& value) {
+    auto end = c.begin() + elems;
+    return (std::find_if(c.begin(), end, [&value](const auto& ws_elem) {
+        return (ws_elem.getValue() == value);
+    }) != end);
+}
+
 }
 
 namespace search {
@@ -411,7 +426,13 @@ void AttributeTest::compare(VectorType & a, VectorType & b)
         ASSERT_TRUE(a.getValueCount(i) == b.getValueCount(i));
         EXPECT_EQUAL(static_cast<const AttributeVector &>(a).get(i, av, asz), static_cast<uint32_t>(a.getValueCount(i)));
         EXPECT_EQUAL(static_cast<const AttributeVector &>(b).get(i, bv, bsz), static_cast<uint32_t>(b.getValueCount(i)));
-        for(size_t j(0), k(std::min(a.getValueCount(i), b.getValueCount(i))); j < k; j++) {
+        const size_t min_common_value_count = std::min(a.getValueCount(i), b.getValueCount(i));
+        if (a.hasWeightedSetType()) {
+            ASSERT_TRUE(b.hasWeightedSetType());
+            std::sort(av, av + min_common_value_count, order_by_value());
+            std::sort(bv, bv + min_common_value_count, order_by_value());
+        }
+        for(size_t j = 0; j < min_common_value_count; j++) {
             EXPECT_EQUAL(av[j], bv[j]);
         }
     }
@@ -1076,6 +1097,9 @@ AttributeTest::testArray()
 // CollectionType::WSET
 //-----------------------------------------------------------------------------
 
+// This function makes the assumption that weights are unique, so that is has a way
+// of creating a deterministic comparison ordering of weighted sets without caring about
+// the templated values themselvecs.
 template <typename VectorType, typename BufferType>
 void
 AttributeTest::testWeightedSet(const AttributePtr & ptr, const std::vector<BufferType> & values)
@@ -1089,6 +1113,9 @@ AttributeTest::testWeightedSet(const AttributePtr & ptr, const std::vector<Buffe
     uint32_t bufferSize = numDocs + 10;
     std::vector<BufferType> buffer(bufferSize);
 
+    std::vector<BufferType> ordered_values(values.begin(), values.end());
+    std::sort(ordered_values.begin(), ordered_values.end(), order_by_weight());
+
     // fill and check
     EXPECT_EQUAL(ptr->getStatus().getUpdateCount(), 0u);
     EXPECT_EQUAL(ptr->getStatus().getNonIdempotentUpdateCount(), 0u);
@@ -1099,10 +1126,11 @@ AttributeTest::testWeightedSet(const AttributePtr & ptr, const std::vector<Buffe
             EXPECT_TRUE(v.append(doc, values[j].getValue(), values[j].getWeight()));
         }
         commit(ptr);
-        EXPECT_TRUE(ptr->get(doc, &buffer[0], buffer.size()) == valueCount);
+        ASSERT_TRUE(ptr->get(doc, &buffer[0], buffer.size()) == valueCount);
+        std::sort(buffer.begin(), buffer.begin() + valueCount, order_by_weight());
         for (uint32_t j = 0; j < valueCount; ++j) {
-            EXPECT_TRUE(buffer[j].getValue() == values[j].getValue());
-            EXPECT_TRUE(buffer[j].getWeight() == values[j].getWeight());
+            EXPECT_TRUE(buffer[j].getValue() == ordered_values[j].getValue());
+            EXPECT_TRUE(buffer[j].getWeight() == ordered_values[j].getWeight());
         }
     }
     EXPECT_EQUAL(ptr->getStatus().getUpdateCount(), numDocs + (numDocs*(numDocs-1))/2);
@@ -1115,24 +1143,21 @@ AttributeTest::testWeightedSet(const AttributePtr & ptr, const std::vector<Buffe
         // append non-existent value
         EXPECT_TRUE(v.append(doc, values[doc].getValue(), values[doc].getWeight()));
         commit(ptr);
-        EXPECT_TRUE(ptr->get(doc, &buffer[0], buffer.size()) == valueCount + 1);
-        EXPECT_TRUE(buffer[doc].getValue() == values[doc].getValue());
-        EXPECT_TRUE(buffer[doc].getWeight() == values[doc].getWeight());
+        ASSERT_TRUE(ptr->get(doc, &buffer[0], buffer.size()) == valueCount + 1);
+        EXPECT_TRUE(contains(buffer, valueCount + 1, values[doc]));
 
         // append existent value
         EXPECT_TRUE(v.append(doc, values[doc].getValue(), values[doc].getWeight() + 10));
         commit(ptr);
-        EXPECT_TRUE(ptr->get(doc, &buffer[0], buffer.size()) == valueCount + 1);
-        EXPECT_TRUE(buffer[doc].getValue() == values[doc].getValue());
-        EXPECT_TRUE(buffer[doc].getWeight() == values[doc].getWeight() + 10);
+        ASSERT_TRUE(ptr->get(doc, &buffer[0], buffer.size()) == valueCount + 1);
+        EXPECT_TRUE(contains(buffer, valueCount + 1, BufferType(values[doc].getValue(), values[doc].getWeight() + 10)));
 
         // append non-existent value two times
         EXPECT_TRUE(v.append(doc, values[doc + 1].getValue(), values[doc + 1].getWeight()));
         EXPECT_TRUE(v.append(doc, values[doc + 1].getValue(), values[doc + 1].getWeight() + 10));
         commit(ptr);
-        EXPECT_TRUE(ptr->get(doc, &buffer[0], buffer.size()) == valueCount + 2);
-        EXPECT_TRUE(buffer[doc + 1].getValue() == values[doc + 1].getValue());
-        EXPECT_TRUE(buffer[doc + 1].getWeight() == values[doc + 1].getWeight() + 10);
+        ASSERT_TRUE(ptr->get(doc, &buffer[0], buffer.size()) == valueCount + 2);
+        EXPECT_TRUE(contains(buffer, valueCount + 2, BufferType(values[doc + 1].getValue(), values[doc + 1].getWeight() + 10)));
     }
     EXPECT_EQUAL(ptr->getStatus().getUpdateCount(), numDocs + (numDocs*(numDocs-1))/2 + numDocs*4);
     EXPECT_EQUAL(ptr->getStatus().getNonIdempotentUpdateCount(), 0u);
@@ -1148,14 +1173,12 @@ AttributeTest::testWeightedSet(const AttributePtr & ptr, const std::vector<Buffe
         EXPECT_TRUE(static_cast<uint32_t>(v.getValueCount(doc)) == valueCount + 2);
 
         // remove existent value
-        EXPECT_TRUE(ptr->get(doc, &buffer[0], buffer.size()) == valueCount + 2);
-        EXPECT_TRUE(buffer[doc + 1].getValue() == values[doc + 1].getValue());
+        ASSERT_TRUE(ptr->get(doc, &buffer[0], buffer.size()) == valueCount + 2);
+        EXPECT_TRUE(contains_value(buffer, valueCount + 2, values[doc + 1].getValue()));
         EXPECT_TRUE(v.remove(doc, values[doc + 1].getValue(), 0));
         commit(ptr);
-        EXPECT_TRUE(ptr->get(doc, &buffer[0], buffer.size()) == valueCount + 1);
-        for (uint32_t i = 0; i < valueCount + 1; ++i) {
-            EXPECT_TRUE(buffer[i].getValue() != values[doc + 1].getValue());
-        }
+        ASSERT_TRUE(ptr->get(doc, &buffer[0], buffer.size()) == valueCount + 1);
+        EXPECT_FALSE(contains_value(buffer, valueCount + 1, values[doc + 1].getValue()));
     }
     EXPECT_EQUAL(ptr->getStatus().getUpdateCount(), numDocs + (numDocs*(numDocs-1))/2 + numDocs*4 + numDocs * 2);
     EXPECT_EQUAL(ptr->getStatus().getNonIdempotentUpdateCount(), 0u);
@@ -1512,8 +1535,9 @@ AttributeTest::testMapValueUpdate(const AttributePtr & ptr, BufferType initValue
     ptr->commit();
     if (createIfNonExistant) {
         EXPECT_EQUAL(ptr->get(5, &buf[0], 2), uint32_t(2));
-        EXPECT_EQUAL(buf[0].getWeight(), 100);
-        EXPECT_EQUAL(buf[1].getWeight(), 10);
+        std::sort(buf.begin(), buf.begin() + 2, order_by_weight());
+        EXPECT_EQUAL(buf[0].getWeight(), 10);
+        EXPECT_EQUAL(buf[1].getWeight(), 100);
     } else {
         EXPECT_EQUAL(ptr->get(5, &buf[0], 2), uint32_t(1));
         EXPECT_EQUAL(buf[0].getWeight(), 100);
diff --git a/searchlib/src/tests/attribute/enum_attribute_compaction/enum_attribute_compaction_test.cpp b/searchlib/src/tests/attribute/enum_attribute_compaction/enum_attribute_compaction_test.cpp
index e30b41f477d..31af5945337 100644
--- a/searchlib/src/tests/attribute/enum_attribute_compaction/enum_attribute_compaction_test.cpp
+++ b/searchlib/src/tests/attribute/enum_attribute_compaction/enum_attribute_compaction_test.cpp
@@ -6,6 +6,7 @@
 #include <vespa/searchlib/attribute/attributevector.hpp>
 #include <vespa/searchlib/attribute/integerbase.h>
 #include <vespa/searchlib/attribute/stringbase.h>
+#include <vespa/searchlib/test/weighted_type_test_utils.h>
 
 #include <vespa/log/log.h>
 LOG_SETUP("enum_attribute_compaction_test");
@@ -141,8 +142,13 @@ CompactionTest<VectorType>::check_values(uint32_t doc_id)
     buffer.fill(*_v, doc_id);
     if (_v->hasMultiValue()) {
         EXPECT_EQ(2u, buffer.size());
-        EXPECT_EQ(CheckType(buffer[0]), MyTestData::make_value(doc_id, 0));
-        EXPECT_EQ(CheckType(buffer[1]), MyTestData::make_value(doc_id, 1));
+        int i = 0, j = 1;
+        if (_v->hasWeightedSetType() && !(CheckType(buffer[0]) == MyTestData::make_value(doc_id, 0))) {
+            i = 1;
+            j = 0;
+        }
+        EXPECT_EQ(CheckType(buffer[i]), MyTestData::make_value(doc_id, 0));
+        EXPECT_EQ(CheckType(buffer[j]), MyTestData::make_value(doc_id, 1));
     } else {
         EXPECT_EQ(1u, buffer.size());
         EXPECT_EQ(CheckType(buffer[0]), MyTestData::make_value(doc_id, 0));
diff --git a/searchlib/src/tests/attribute/stringattribute/stringattribute_test.cpp b/searchlib/src/tests/attribute/stringattribute/stringattribute_test.cpp
index 5c73a60f6ef..a9c6cd1fd6c 100644
--- a/searchlib/src/tests/attribute/stringattribute/stringattribute_test.cpp
+++ b/searchlib/src/tests/attribute/stringattribute/stringattribute_test.cpp
@@ -105,6 +105,22 @@ StringAttributeTest::testMultiValue()
 
 }
 
+namespace {
+
+template <typename T0, typename T1>
+auto zipped_and_sorted_by_first(const std::vector<T0>& a, const std::vector<T1>& b) -> std::vector<std::pair<T0, T1>> {
+    std::vector<std::pair<T0, T1>> combined;
+    assert(a.size() == b.size());
+    for (size_t i = 0; i < a.size(); ++i) {
+        combined.emplace_back(a[i], b[i]);
+    }
+    std::sort(combined.begin(), combined.end(), [](const auto& lhs, const auto& rhs){
+        return (lhs.first < rhs.first);
+    });
+    return combined;
+}
+
+}
 
 template <typename Attribute>
 void
@@ -118,14 +134,16 @@ StringAttributeTest::testMultiValue(Attribute & attr, uint32_t numDocs)
     for (uint32_t i = 0; i < numDocs - 1; ++i) {
         char unique[16];
         sprintf(unique, i < 10 ? "enum0%u" : "enum%u", i);
-        uniqueStrings.push_back(vespalib::string(unique));
+        uniqueStrings.emplace_back(unique);
     }
+    ASSERT_TRUE(std::is_sorted(uniqueStrings.begin(), uniqueStrings.end()));
+
     std::vector<vespalib::string> newUniques;
     newUniques.reserve(numDocs - 1);
     for (uint32_t i = 0; i < numDocs - 1; ++i) {
         char unique[16];
         sprintf(unique, i < 10 ? "unique0%u" : "unique%u", i);
-        newUniques.push_back(vespalib::string(unique));
+        newUniques.emplace_back(unique);
     }
 
     // add docs
@@ -147,10 +165,10 @@ StringAttributeTest::testMultiValue(Attribute & attr, uint32_t numDocs)
 
         // test get first
         if (valueCount == 0) {
-            EXPECT_TRUE(attr.get(doc) == NULL);
+            EXPECT_TRUE(attr.get(doc) == nullptr);
             EXPECT_TRUE(attr.getEnum(doc) == std::numeric_limits<uint32_t>::max());
-        } else {
-            EXPECT_TRUE(strcmp(attr.get(doc), uniqueStrings[0].c_str()) == 0);
+        } else if (!attr.hasWeightedSetType()) {
+            EXPECT_EQUAL(vespalib::string(attr.get(doc)), uniqueStrings[0]);
             uint32_t e;
             EXPECT_TRUE(attr.findEnum(uniqueStrings[0].c_str(), e));
             EXPECT_EQUAL(1u, attr.findFoldedEnums(uniqueStrings[0].c_str()).size());
@@ -160,17 +178,17 @@ StringAttributeTest::testMultiValue(Attribute & attr, uint32_t numDocs)
 
         // test get all
         std::vector<vespalib::string> values(valueCount);
-        EXPECT_TRUE(attr.get(doc, &values[0], valueCount) == valueCount);
+        ASSERT_TRUE(attr.get(doc, &values[0], valueCount) == valueCount);
 
         std::vector<uint32_t> enums(valueCount);
-        EXPECT_TRUE((static_cast<search::attribute::IAttributeVector &>(attr)).get(doc, &enums[0], valueCount) == valueCount);
+        ASSERT_TRUE((static_cast<search::attribute::IAttributeVector &>(attr)).get(doc, &enums[0], valueCount) == valueCount);
 
+        auto combined = zipped_and_sorted_by_first(values, enums);
         for (uint32_t j = 0; j < valueCount; ++j) {
-            //LOG(info, "doc[%u][%u] = %s", doc, j, values[j].c_str());
-            EXPECT_TRUE(values[j] == uniqueStrings[j]);
+            EXPECT_TRUE(combined[j].first == uniqueStrings[j]);
             uint32_t e = 100;
-            EXPECT_TRUE(attr.findEnum(values[j].c_str(), e));
-            EXPECT_TRUE(enums[j] == e);
+            EXPECT_TRUE(attr.findEnum(combined[j].first.c_str(), e));
+            EXPECT_TRUE(combined[j].second == e);
         }
     }
 
@@ -207,12 +225,12 @@ StringAttributeTest::testMultiValue(Attribute & attr, uint32_t numDocs)
         std::vector<uint32_t> enums(valueCount);
         EXPECT_TRUE((static_cast<search::attribute::IAttributeVector &>(attr)).get(doc, &enums[0], valueCount) == valueCount);
 
+        auto combined = zipped_and_sorted_by_first(values, enums);
         for (uint32_t j = 0; j < valueCount; ++j) {
-            //LOG(info, "doc[%u][%u] = %s", doc, j, values[j].c_str());
-            EXPECT_TRUE(values[j] == newUniques[j]);
+            EXPECT_TRUE(combined[j].first == newUniques[j]);
             uint32_t e = 100;
-            EXPECT_TRUE(attr.findEnum(values[j].c_str(), e));
-            EXPECT_TRUE(enums[j] == e);
+            EXPECT_TRUE(attr.findEnum(combined[j].first.c_str(), e));
+            EXPECT_TRUE(combined[j].second == e);
         }
     }
 
diff --git a/searchlib/src/vespa/searchlib/test/imported_attribute_fixture.h b/searchlib/src/vespa/searchlib/test/imported_attribute_fixture.h
index c37cfc38a8c..a58f71d232b 100644
--- a/searchlib/src/vespa/searchlib/test/imported_attribute_fixture.h
+++ b/searchlib/src/vespa/searchlib/test/imported_attribute_fixture.h
@@ -3,6 +3,7 @@
 #pragma once
 
 #include "mock_gid_to_lid_mapping.h"
+#include "weighted_type_test_utils.h"
 #include <vespa/document/base/documentid.h>
 #include <vespa/document/base/globalid.h>
 #include <vespa/searchlib/attribute/attribute_read_guard.h>
@@ -296,7 +297,7 @@ ImportedAttributeFixture::ImportedAttributeFixture(bool use_search_cache_)
     reference_attr->setGidToLidMapperFactory(mapper_factory);
 }
 
-ImportedAttributeFixture::~ImportedAttributeFixture() {}
+ImportedAttributeFixture::~ImportedAttributeFixture() = default;
 
 template<typename AttrValueType, typename PredicateType>
 void assert_multi_value_matches(const ImportedAttributeFixture &f,
@@ -305,9 +306,14 @@ void assert_multi_value_matches(const ImportedAttributeFixture &f,
                                 PredicateType predicate) {
     AttributeContent<AttrValueType> content;
     content.fill(*f.get_imported_attr(), lid);
-    EXPECT_EQUAL(expected.size(), content.size());
+    ASSERT_EQUAL(expected.size(), content.size());
     std::vector<AttrValueType> actual(content.begin(), content.end());
-    EXPECT_TRUE(std::equal(expected.begin(), expected.end(),
+    std::vector<AttrValueType> wanted(expected.begin(), expected.end());
+    if constexpr (IsWeightedType<AttrValueType>::value) {
+        std::sort(actual.begin(), actual.end(), value_then_weight_order());
+        std::sort(wanted.begin(), wanted.end(), value_then_weight_order());
+    }
+    EXPECT_TRUE(std::equal(wanted.begin(), wanted.end(),
                            actual.begin(), actual.end(), predicate));
 }
 
diff --git a/searchlib/src/vespa/searchlib/test/weighted_type_test_utils.h b/searchlib/src/vespa/searchlib/test/weighted_type_test_utils.h
new file mode 100644
index 00000000000..ac9fefbfb35
--- /dev/null
+++ b/searchlib/src/vespa/searchlib/test/weighted_type_test_utils.h
@@ -0,0 +1,38 @@
+// Copyright 2019 Oath Inc. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
+#pragma once
+
+#include <vespa/searchcommon/attribute/iattributevector.h>
+#include <type_traits>
+
+template <typename T> struct IsWeightedType : std::false_type {};
+template <typename T> struct IsWeightedType<search::attribute::WeightedType<T>> : std::true_type {};
+
+struct value_then_weight_order {
+    template <typename T>
+    bool operator()(const T& lhs, const T& rhs) const noexcept {
+        if (lhs.getValue() != rhs.getValue()) {
+            return (lhs.getValue() < rhs.getValue());
+        }
+        return (lhs.getWeight() < rhs.getWeight());
+    }
+};
+
+struct order_by_value {
+    template <typename T>
+    bool operator()(const T& lhs, const T& rhs) const noexcept {
+        if constexpr (IsWeightedType<T>::value) {
+            return (lhs.getValue() < rhs.getValue());
+        } else {
+            return (lhs < rhs);
+        }
+    }
+};
+
+
+struct order_by_weight {
+    template <typename T>
+    bool operator()(const search::attribute::WeightedType<T>& lhs,
+                    const search::attribute::WeightedType<T>& rhs) const noexcept {
+        return (lhs.getWeight() < rhs.getWeight());
+    }
+};