Merge pull request #9472 from vespa-engine/geirst/request-match-data-details-for-features

Make it possible for rank features to request which match data that s..

9 files changed, 136 insertions, 32 deletions

diff --git a/searchcore/CMakeLists.txt b/searchcore/CMakeLists.txt
index 593834d4f9a..ac6a9f305dd 100644
--- a/searchcore/CMakeLists.txt
+++ b/searchcore/CMakeLists.txt
@@ -114,6 +114,7 @@ vespa_define_module(
     src/tests/proton/matching
     src/tests/proton/matching/constant_value_repo
     src/tests/proton/matching/docid_range_scheduler
+    src/tests/proton/matching/handle_recorder
     src/tests/proton/matching/index_environment
     src/tests/proton/matching/match_loop_communicator
     src/tests/proton/matching/match_phase_limiter
diff --git a/searchcore/src/tests/proton/matching/handle_recorder/CMakeLists.txt b/searchcore/src/tests/proton/matching/handle_recorder/CMakeLists.txt
new file mode 100644
index 00000000000..a56d22ab154
--- /dev/null
+++ b/searchcore/src/tests/proton/matching/handle_recorder/CMakeLists.txt
@@ -0,0 +1,10 @@
+# Copyright 2019 Oath Inc. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
+
+vespa_add_executable(searchcore_matching_handle_recorder_test_app TEST
+    SOURCES
+    handle_recorder_test.cpp
+    DEPENDS
+    searchcore_matching
+    gtest
+)
+vespa_add_test(NAME searchcore_matching_handle_recorder_test_app COMMAND searchcore_matching_handle_recorder_test_app)
diff --git a/searchcore/src/tests/proton/matching/handle_recorder/handle_recorder_test.cpp b/searchcore/src/tests/proton/matching/handle_recorder/handle_recorder_test.cpp
new file mode 100644
index 00000000000..60caffd82d6
--- /dev/null
+++ b/searchcore/src/tests/proton/matching/handle_recorder/handle_recorder_test.cpp
@@ -0,0 +1,54 @@
+// Copyright 2019 Oath Inc. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
+
+#include <vespa/searchcore/proton/matching/handlerecorder.h>
+#include <vespa/vespalib/gtest/gtest.h>
+
+#include <vespa/log/log.h>
+
+LOG_SETUP("handle_recorder_test");
+
+using search::fef::MatchDataDetails;
+using search::fef::TermFieldHandle;
+using namespace proton::matching;
+
+using HandleSet = HandleRecorder::HandleSet;
+
+void
+register_normal_handle(TermFieldHandle handle)
+{
+    HandleRecorder::register_handle(handle, MatchDataDetails::Normal);
+}
+
+void
+register_cheap_handle(TermFieldHandle handle)
+{
+    HandleRecorder::register_handle(handle, MatchDataDetails::Cheap);
+}
+
+TEST(HandleRecorderTest, can_record_both_normal_and_cheap_handles)
+{
+    HandleRecorder recorder;
+    {
+        HandleRecorder::Binder binder(recorder);
+        register_normal_handle(3);
+        register_cheap_handle(5);
+        register_normal_handle(7);
+    }
+    EXPECT_EQ(HandleSet({3, 7}), recorder.get_normal_handles());
+    EXPECT_EQ(HandleSet({5}), recorder.get_cheap_handles());
+    EXPECT_EQ("normal: [3,7], cheap: [5]", recorder.to_string());
+}
+
+TEST(HandleRecorderTest, the_same_handle_can_be_in_both_normal_and_cheap_set)
+{
+    HandleRecorder recorder;
+    {
+        HandleRecorder::Binder binder(recorder);
+        register_normal_handle(3);
+        register_cheap_handle(3);
+    }
+    EXPECT_EQ(HandleSet({3}), recorder.get_normal_handles());
+    EXPECT_EQ(HandleSet({3}), recorder.get_cheap_handles());
+}
+
+GTEST_MAIN_RUN_ALL_TESTS()
diff --git a/searchcore/src/vespa/searchcore/proton/matching/handlerecorder.cpp b/searchcore/src/vespa/searchcore/proton/matching/handlerecorder.cpp
index 3573c7ec7b8..a4f3d519311 100644
--- a/searchcore/src/vespa/searchcore/proton/matching/handlerecorder.cpp
+++ b/searchcore/src/vespa/searchcore/proton/matching/handlerecorder.cpp
@@ -2,9 +2,10 @@
 
 #include "handlerecorder.h"
 #include <vespa/vespalib/stllike/asciistream.h>
-#include <cassert>
 #include <algorithm>
+#include <cassert>
 
+using search::fef::MatchDataDetails;
 using search::fef::TermFieldHandle;
 
 namespace proton::matching {
@@ -16,33 +17,47 @@ namespace proton::matching {
 #endif
 
 namespace {
-     __thread HandleRecorder * _T_recorder TLS_LINKAGE = NULL;
+     __thread HandleRecorder * _T_recorder TLS_LINKAGE = nullptr;
      __thread bool _T_assert_all_handles_are_registered = false;
 }
 
 HandleRecorder::HandleRecorder() :
-    _handles()
+    _normal_handles(),
+    _cheap_handles()
 {
 }
 
+namespace {
+
 vespalib::string
-HandleRecorder::toString() const
+handles_to_string(const HandleRecorder::HandleSet& handles)
 {
     vespalib::asciistream os;
     std::vector<TermFieldHandle> sorted;
-    for (TermFieldHandle handle : _handles) {
+    for (TermFieldHandle handle : handles) {
         sorted.push_back(handle);
     }
     std::sort(sorted.begin(), sorted.end());
     if ( !sorted.empty() ) {
         os << sorted[0];
         for (size_t i(1); i < sorted.size(); i++) {
-            os << ' ' << sorted[i];
+            os << ',' << sorted[i];
         }
     }
     return os.str();
 }
 
+}
+
+vespalib::string
+HandleRecorder::to_string() const
+{
+    vespalib::asciistream os;
+    os << "normal: [" << handles_to_string(_normal_handles) << "], ";
+    os << "cheap: [" << handles_to_string(_cheap_handles) << "]";
+    return os.str();
+}
+
 HandleRecorder::Binder::Binder(HandleRecorder & recorder)
 {
     _T_recorder = & recorder;
@@ -64,17 +79,33 @@ HandleRecorder::~HandleRecorder()
 
 HandleRecorder::Binder::~Binder()
 {
-    _T_recorder = NULL;
+    _T_recorder = nullptr;
 }
 
-void HandleRecorder::registerHandle(TermFieldHandle handle)
+void
+HandleRecorder::register_handle(TermFieldHandle handle,
+                                MatchDataDetails requested_details)
 {
     // There should be no registration of handles that is not recorded.
     // That will lead to issues later on.
-    if (_T_recorder != NULL) {
-        _T_recorder->add(handle);
+    if (_T_recorder != nullptr) {
+        _T_recorder->add(handle, requested_details);
     } else if (_T_assert_all_handles_are_registered) {
-        assert(_T_recorder != NULL);
+        assert(_T_recorder != nullptr);
+    }
+}
+
+void
+HandleRecorder::add(TermFieldHandle handle,
+                    MatchDataDetails requested_details)
+
+{
+    if (requested_details == MatchDataDetails::Normal) {
+        _normal_handles.insert(handle);
+    } else if (requested_details == MatchDataDetails::Cheap) {
+        _cheap_handles.insert(handle);
+    } else {
+        abort();
     }
 }
 
diff --git a/searchcore/src/vespa/searchcore/proton/matching/handlerecorder.h b/searchcore/src/vespa/searchcore/proton/matching/handlerecorder.h
index 8a8a4de7baa..54a30a0ed3d 100644
--- a/searchcore/src/vespa/searchcore/proton/matching/handlerecorder.h
+++ b/searchcore/src/vespa/searchcore/proton/matching/handlerecorder.h
@@ -3,11 +3,11 @@
 #pragma once
 
 #include <vespa/searchlib/fef/handle.h>
+#include <vespa/searchlib/fef/match_data_details.h>
 #include <vespa/vespalib/stllike/hash_set.h>
 #include <vespa/vespalib/util/noncopyable.hpp>
 
-namespace proton {
-namespace matching {
+namespace proton::matching {
 
 /**
  * This is a recorder that will register all handles used by any features for a given query.
@@ -20,7 +20,7 @@ namespace matching {
 class HandleRecorder
 {
 public:
-    typedef vespalib::hash_set<search::fef::TermFieldHandle> HandleSet;
+    using HandleSet = vespalib::hash_set<search::fef::TermFieldHandle>;
     class Binder : public vespalib::noncopyable {
     public:
         Binder(HandleRecorder & recorder);
@@ -33,16 +33,17 @@ public:
     };
     HandleRecorder();
     ~HandleRecorder();
-    const HandleSet & getHandles() const { return _handles; }
-    static void registerHandle(search::fef::TermFieldHandle handle);
-    vespalib::string toString() const;
+    const HandleSet& get_normal_handles() const { return _normal_handles; }
+    const HandleSet& get_cheap_handles() const { return _cheap_handles; }
+    static void register_handle(search::fef::TermFieldHandle handle,
+                                search::fef::MatchDataDetails requested_details);
+    vespalib::string to_string() const;
 private:
-    void add(search::fef::TermFieldHandle handle) {
-        _handles.insert(handle);
-    }
-    HandleSet _handles;
+    void add(search::fef::TermFieldHandle handle,
+             search::fef::MatchDataDetails requested_details);
+    HandleSet _normal_handles;
+    HandleSet _cheap_handles;
 };
 
-}  // namespace matching
-}  // namespace proton
+}
 
diff --git a/searchcore/src/vespa/searchcore/proton/matching/match_tools.cpp b/searchcore/src/vespa/searchcore/proton/matching/match_tools.cpp
index 28d56b7e0a2..e6dab8cbea4 100644
--- a/searchcore/src/vespa/searchcore/proton/matching/match_tools.cpp
+++ b/searchcore/src/vespa/searchcore/proton/matching/match_tools.cpp
@@ -38,6 +38,7 @@ bool contains_all(const HandleRecorder::HandleSet &old_set,
 }
 
 void tag_match_data(const HandleRecorder::HandleSet &handles, MatchData &match_data) {
+    // TODO: Move tagging to separate component (for testing) and tag normal and cheap.
     for (TermFieldHandle handle = 0; handle < match_data.getNumTermFields(); ++handle) {
         if (handles.find(handle) == handles.end()) {
             match_data.resolveTermField(handle)->tagAsNotNeeded();
@@ -81,12 +82,14 @@ MatchTools::setup(search::fef::RankProgram::UP rank_program, double termwise_lim
         _rank_program->setup(*_match_data, _queryEnv, _featureOverrides);
     }
     bool can_reuse_search = (_search && !_search_has_changed &&
-                             contains_all(_used_handles, recorder.getHandles()));
+            contains_all(_used_normal_handles, recorder.get_normal_handles()) &&
+            contains_all(_used_cheap_handles, recorder.get_cheap_handles()));
     if (!can_reuse_search) {
-        tag_match_data(recorder.getHandles(), *_match_data);
+        tag_match_data(recorder.get_normal_handles(), *_match_data);
         _match_data->set_termwise_limit(termwise_limit);
         _search = _query.createSearch(*_match_data);
-        _used_handles = recorder.getHandles();
+        _used_normal_handles = recorder.get_normal_handles();
+        _used_cheap_handles = recorder.get_cheap_handles();
         _search_has_changed = false;
     }
 }
@@ -111,7 +114,8 @@ MatchTools::MatchTools(QueryLimiter & queryLimiter,
       _match_data(mdl.createMatchData()),
       _rank_program(),
       _search(),
-      _used_handles(),
+      _used_normal_handles(),
+      _used_cheap_handles(),
       _search_has_changed(false)
 {
 }
diff --git a/searchcore/src/vespa/searchcore/proton/matching/match_tools.h b/searchcore/src/vespa/searchcore/proton/matching/match_tools.h
index dfef51bfc5e..10770554187 100644
--- a/searchcore/src/vespa/searchcore/proton/matching/match_tools.h
+++ b/searchcore/src/vespa/searchcore/proton/matching/match_tools.h
@@ -35,7 +35,8 @@ private:
     search::fef::MatchData::UP             _match_data;
     search::fef::RankProgram::UP           _rank_program;
     search::queryeval::SearchIterator::UP  _search;
-    HandleRecorder::HandleSet              _used_handles;
+    HandleRecorder::HandleSet              _used_normal_handles;
+    HandleRecorder::HandleSet              _used_cheap_handles;
     bool                                   _search_has_changed;
     void setup(search::fef::RankProgram::UP, double termwise_limit = 1.0);
 public:
diff --git a/searchcore/src/vespa/searchcore/proton/matching/querynodes.cpp b/searchcore/src/vespa/searchcore/proton/matching/querynodes.cpp
index b0d45ee0684..6d810594aa7 100644
--- a/searchcore/src/vespa/searchcore/proton/matching/querynodes.cpp
+++ b/searchcore/src/vespa/searchcore/proton/matching/querynodes.cpp
@@ -14,6 +14,7 @@ using search::fef::FieldInfo;
 using search::fef::FieldType;
 using search::fef::IIndexEnvironment;
 using search::fef::MatchData;
+using search::fef::MatchDataDetails;
 using search::fef::MatchDataLayout;
 using search::fef::TermFieldHandle;
 using search::query::Node;
@@ -115,10 +116,10 @@ ProtonTermData::lookupField(uint32_t fieldId) const
 }
 
 TermFieldHandle
-ProtonTermData::FieldEntry::getHandle() const
+ProtonTermData::FieldEntry::getHandle(MatchDataDetails requested_details) const
 {
-    TermFieldHandle handle(search::fef::SimpleTermFieldData::getHandle());
-    HandleRecorder::registerHandle(handle);
+    TermFieldHandle handle(search::fef::SimpleTermFieldData::getHandle(requested_details));
+    HandleRecorder::register_handle(handle, requested_details);
     return handle;
 }
 
diff --git a/searchcore/src/vespa/searchcore/proton/matching/querynodes.h b/searchcore/src/vespa/searchcore/proton/matching/querynodes.h
index a06f5287669..8cf65c1e67b 100644
--- a/searchcore/src/vespa/searchcore/proton/matching/querynodes.h
+++ b/searchcore/src/vespa/searchcore/proton/matching/querynodes.h
@@ -40,7 +40,8 @@ public:
             return FieldSpec(field_name, getFieldId(),
                              getHandle(), filter_field);
         }
-        search::fef::TermFieldHandle getHandle() const override;
+        using SimpleTermFieldData::getHandle;
+        search::fef::TermFieldHandle getHandle(search::fef::MatchDataDetails requested_details) const override;
     };
 
 private: