Merge pull request #3206 from vespa-engine/vekterli/fix-imported-attribute-sort-serialization-to-use-parent-lids

Fix imported attribute sort serialization to use parent lids

4 files changed, 60 insertions, 18 deletions

diff --git a/searchlib/src/tests/attribute/imported_attribute_vector/imported_attribute_vector_test.cpp b/searchlib/src/tests/attribute/imported_attribute_vector/imported_attribute_vector_test.cpp
index f855f51af42..6406ce6d4a1 100644
--- a/searchlib/src/tests/attribute/imported_attribute_vector/imported_attribute_vector_test.cpp
+++ b/searchlib/src/tests/attribute/imported_attribute_vector/imported_attribute_vector_test.cpp
@@ -304,7 +304,7 @@ template <typename FixtureType>
 void verify_get_string_from_enum_is_mapped(FixtureType& f) {
     EnumHandle handle{};
     ASSERT_TRUE(f.target_attr->findEnum("foo", handle));
-    const char* from_enum = f.imported_attr->getStringFromEnum(handle);
+    const char* from_enum = f.get_imported_attr()->getStringFromEnum(handle);
     ASSERT_TRUE(from_enum != nullptr);
     EXPECT_EQUAL(vespalib::string("foo"), vespalib::string(from_enum));
 }
@@ -438,7 +438,7 @@ struct MockAttributeVector : NotImplementedAttribute {
     long _return_value{1234};
 
     MockAttributeVector()
-            : NotImplementedAttribute("mock", Config(BasicType::INT32)) {
+            : NotImplementedAttribute("mock", Config(BasicType::STRING)) {
     }
 
     void set_received_args(DocId doc_id, void* ser_to,
@@ -475,44 +475,65 @@ struct MockBlobConverter : common::BlobConverter {
     }
 };
 
-struct SerializeFixture : Fixture {
+template <typename BaseFixture>
+struct SerializeFixture : BaseFixture {
     std::shared_ptr<MockAttributeVector> mock_target;
     MockBlobConverter mock_converter;
 
-    SerializeFixture()
-            : Fixture(),
-              mock_target(std::make_shared<MockAttributeVector>())
-    {
-        reset_with_new_target_attr(mock_target);
+    SerializeFixture() : mock_target(std::make_shared<MockAttributeVector>()) {
+        this->reset_with_new_target_attr(mock_target);
+        mock_target->setCommittedDocIdLimit(8); // Target LID of 7 is highest used by ref attribute. Limit is +1.
     }
+    ~SerializeFixture() override;
 };
 
-TEST_F("onSerializeForAscendingSort() is forwarded to target vector", SerializeFixture) {
+template <typename BaseFixture>
+SerializeFixture<BaseFixture>::~SerializeFixture() {}
+
+template <typename FixtureT>
+void check_onSerializeForAscendingSort_is_forwarded_with_remapped_lid() {
+    FixtureT f;
     int dummy_tag;
     void* ser_to = &dummy_tag;
     EXPECT_EQUAL(f.mock_target->_return_value,
-                 f.imported_attr->serializeForAscendingSort(
-                         DocId(10), ser_to, 777, &f.mock_converter));
+                 f.get_imported_attr()->serializeForAscendingSort(
+                         DocId(4), ser_to, 777, &f.mock_converter)); // child lid 4 -> parent lid 7
     EXPECT_TRUE(f.mock_target->_ascending_called);
-    EXPECT_EQUAL(DocId(10), f.mock_target->_doc_id);
+    EXPECT_EQUAL(DocId(7), f.mock_target->_doc_id);
     EXPECT_EQUAL(ser_to, f.mock_target->_ser_to);
     EXPECT_EQUAL(777, f.mock_target->_available);
     EXPECT_EQUAL(&f.mock_converter, f.mock_target->_bc);
 }
 
-TEST_F("onSerializeForDescendingSort() is forwarded to target vector", SerializeFixture) {
+TEST("onSerializeForAscendingSort() is forwarded with remapped LID to target vector") {
+    TEST_DO(check_onSerializeForAscendingSort_is_forwarded_with_remapped_lid<
+                    SerializeFixture<SingleStringAttrFixture>>());
+    TEST_DO(check_onSerializeForAscendingSort_is_forwarded_with_remapped_lid<
+                    SerializeFixture<ReadGuardSingleStringAttrFixture>>());
+}
+
+template <typename FixtureT>
+void check_onSerializeForDescendingSort_is_forwarded_with_remapped_lid() {
+    FixtureT f;
     int dummy_tag;
     void* ser_to = &dummy_tag;
     EXPECT_EQUAL(f.mock_target->_return_value,
-                 f.imported_attr->serializeForDescendingSort(
-                         DocId(20), ser_to, 555, &f.mock_converter));
+                 f.get_imported_attr()->serializeForDescendingSort(
+                         DocId(2), ser_to, 555, &f.mock_converter)); // child lid 2 -> parent lid 3
     EXPECT_TRUE(f.mock_target->_descending_called);
-    EXPECT_EQUAL(DocId(20), f.mock_target->_doc_id);
+    EXPECT_EQUAL(DocId(3), f.mock_target->_doc_id);
     EXPECT_EQUAL(ser_to, f.mock_target->_ser_to);
     EXPECT_EQUAL(555, f.mock_target->_available);
     EXPECT_EQUAL(&f.mock_converter, f.mock_target->_bc);
 }
 
+TEST("onSerializeForDescendingSort() is forwarded with remapped LID to target vector") {
+    TEST_DO(check_onSerializeForDescendingSort_is_forwarded_with_remapped_lid<
+                    SerializeFixture<SingleStringAttrFixture>>());
+    TEST_DO(check_onSerializeForDescendingSort_is_forwarded_with_remapped_lid<
+                    SerializeFixture<ReadGuardSingleStringAttrFixture>>());
+}
+
 } // attribute
 } // search
 
diff --git a/searchlib/src/vespa/searchlib/attribute/imported_attribute_vector.cpp b/searchlib/src/vespa/searchlib/attribute/imported_attribute_vector.cpp
index b6ff442ae94..270f4d51788 100644
--- a/searchlib/src/vespa/searchlib/attribute/imported_attribute_vector.cpp
+++ b/searchlib/src/vespa/searchlib/attribute/imported_attribute_vector.cpp
@@ -135,14 +135,16 @@ long ImportedAttributeVector::onSerializeForAscendingSort(DocId doc,
                                                           void *serTo,
                                                           long available,
                                                           const common::BlobConverter *bc) const {
-    return _target_attribute->serializeForAscendingSort(doc, serTo, available, bc);
+    return _target_attribute->serializeForAscendingSort(
+            _reference_attribute->getReferencedLid(doc), serTo, available, bc);
 }
 
 long ImportedAttributeVector::onSerializeForDescendingSort(DocId doc,
                                                            void *serTo,
                                                            long available,
                                                            const common::BlobConverter *bc) const {
-    return _target_attribute->serializeForDescendingSort(doc, serTo, available, bc);
+    return _target_attribute->serializeForDescendingSort(
+            _reference_attribute->getReferencedLid(doc), serTo, available, bc);
 }
 
 namespace {
diff --git a/searchlib/src/vespa/searchlib/attribute/imported_attribute_vector_read_guard.cpp b/searchlib/src/vespa/searchlib/attribute/imported_attribute_vector_read_guard.cpp
index 85f8e980026..563834e6cdb 100644
--- a/searchlib/src/vespa/searchlib/attribute/imported_attribute_vector_read_guard.cpp
+++ b/searchlib/src/vespa/searchlib/attribute/imported_attribute_vector_read_guard.cpp
@@ -80,5 +80,19 @@ uint32_t ImportedAttributeVectorReadGuard::get(DocId docId, WeightedEnum *buffer
     return _target_attribute->get(getReferencedLid(docId), buffer, sz);
 }
 
+long ImportedAttributeVectorReadGuard::onSerializeForAscendingSort(DocId doc,
+                                                                   void *serTo,
+                                                                   long available,
+                                                                   const common::BlobConverter *bc) const {
+    return _target_attribute->serializeForAscendingSort(getReferencedLid(doc), serTo, available, bc);
+}
+
+long ImportedAttributeVectorReadGuard::onSerializeForDescendingSort(DocId doc,
+                                                                    void *serTo,
+                                                                    long available,
+                                                                    const common::BlobConverter *bc) const {
+    return _target_attribute->serializeForDescendingSort(getReferencedLid(doc), serTo, available, bc);
+}
+
 }
 }
diff --git a/searchlib/src/vespa/searchlib/attribute/imported_attribute_vector_read_guard.h b/searchlib/src/vespa/searchlib/attribute/imported_attribute_vector_read_guard.h
index 81a3d24b6cf..f4db2b538d5 100644
--- a/searchlib/src/vespa/searchlib/attribute/imported_attribute_vector_read_guard.h
+++ b/searchlib/src/vespa/searchlib/attribute/imported_attribute_vector_read_guard.h
@@ -48,6 +48,11 @@ public:
     virtual uint32_t get(DocId docId, WeightedString *buffer, uint32_t sz) const override;
     virtual uint32_t get(DocId docId, WeightedConstChar *buffer, uint32_t sz) const override;
     virtual uint32_t get(DocId docId, WeightedEnum *buffer, uint32_t sz) const override;
+protected:
+    virtual long onSerializeForAscendingSort(DocId doc, void * serTo, long available,
+                                             const common::BlobConverter * bc) const override;
+    virtual long onSerializeForDescendingSort(DocId doc, void * serTo, long available,
+                                              const common::BlobConverter * bc) const override;
 };
 
 }