Add and clarify on-first-phase

5 files changed, 35 insertions, 9 deletions

diff --git a/searchcore/src/tests/proton/matching/matching_test.cpp b/searchcore/src/tests/proton/matching/matching_test.cpp
index a4952fa3154..d690fb29795 100644
--- a/searchcore/src/tests/proton/matching/matching_test.cpp
+++ b/searchcore/src/tests/proton/matching/matching_test.cpp
@@ -785,8 +785,21 @@ TEST("require that match params are set up straight with ranking on") {
     ASSERT_EQUAL(0.7, p.rankDropLimit);
     ASSERT_EQUAL(0u, p.offset);
     ASSERT_EQUAL(1u, p.hits);
+    ASSERT_TRUE(p.has_rank_drop_limit());
 }
 
+TEST("require that match params can turn off rank-drop-limit") {
+    MatchParams p(1, 2, 4, -std::numeric_limits<feature_t>::quiet_NaN(), 0, 1, true, true);
+    ASSERT_EQUAL(1u, p.numDocs);
+    ASSERT_EQUAL(2u, p.heapSize);
+    ASSERT_EQUAL(4u, p.arraySize);
+    ASSERT_TRUE(std::isnan(p.rankDropLimit));
+    ASSERT_EQUAL(0u, p.offset);
+    ASSERT_EQUAL(1u, p.hits);
+    ASSERT_FALSE(p.has_rank_drop_limit());
+}
+
+
 TEST("require that match params are set up straight with ranking on arraySize is atleast the size of heapSize") {
     MatchParams p(1, 6, 4, 0.7, 1, 1, true, true);
     ASSERT_EQUAL(1u, p.numDocs);
diff --git a/searchcore/src/vespa/searchcore/proton/matching/match_params.cpp b/searchcore/src/vespa/searchcore/proton/matching/match_params.cpp
index 983e09716c3..7465088d8f0 100644
--- a/searchcore/src/vespa/searchcore/proton/matching/match_params.cpp
+++ b/searchcore/src/vespa/searchcore/proton/matching/match_params.cpp
@@ -2,6 +2,7 @@
 
 #include "match_params.h"
 #include <algorithm>
+#include <cmath>
 
 namespace proton::matching {
 
@@ -32,4 +33,8 @@ MatchParams::MatchParams(uint32_t          numDocs_in,
       rankDropLimit(rankDropLimit_in)
 { }
 
+bool MatchParams::has_rank_drop_limit() const {
+    return ! std::isnan(rankDropLimit);
+}
+
 }
diff --git a/searchcore/src/vespa/searchcore/proton/matching/match_params.h b/searchcore/src/vespa/searchcore/proton/matching/match_params.h
index 40f33e25914..efcd507c9af 100644
--- a/searchcore/src/vespa/searchcore/proton/matching/match_params.h
+++ b/searchcore/src/vespa/searchcore/proton/matching/match_params.h
@@ -28,6 +28,7 @@ struct MatchParams {
                 bool              hasFinalRank,
                 bool              needRanking=true);
     bool save_rank_scores() const { return ((heapSize + arraySize) != 0); }
+    bool has_rank_drop_limit() const;
 };
 
 }
diff --git a/searchcore/src/vespa/searchcore/proton/matching/match_thread.cpp b/searchcore/src/vespa/searchcore/proton/matching/match_thread.cpp
index 87b4f918d11..c9ec2a06353 100644
--- a/searchcore/src/vespa/searchcore/proton/matching/match_thread.cpp
+++ b/searchcore/src/vespa/searchcore/proton/matching/match_thread.cpp
@@ -10,7 +10,6 @@
 #include <vespa/searchlib/common/bitvector.h>
 #include <vespa/searchlib/queryeval/multibitvectoriterator.h>
 #include <vespa/searchlib/queryeval/andnotsearch.h>
-#include <vespa/vespalib/util/thread_bundle.h>
 #include <vespa/vespalib/data/slime/cursor.h>
 #include <vespa/vespalib/data/slime/inserter.h>
 
@@ -234,10 +233,10 @@ template <bool do_rank, bool do_limit, bool do_share>
 void
 MatchThread::match_loop_helper_rank_limit_share(MatchTools &tools, HitCollector &hits)
 {
-    if (std::isnan(matchParams.rankDropLimit)) {
-        match_loop_helper_rank_limit_share_drop<do_rank, do_limit, do_share, false>(tools, hits);
-    } else {
+    if (matchParams.has_rank_drop_limit()) {
         match_loop_helper_rank_limit_share_drop<do_rank, do_limit, do_share, true>(tools, hits);
+    } else {
+        match_loop_helper_rank_limit_share_drop<do_rank, do_limit, do_share, false>(tools, hits);
     }
 }
 
@@ -376,8 +375,14 @@ MatchThread::processResult(const Doom & doom,
             }
         }
     }
-    if (auto onMatchTask = matchToolsFactory.createOnMatchTask()) {
-        onMatchTask->run(search::ResultSet::stealResult(std::move(*result)));
+
+    if (auto task = matchToolsFactory.createOnMatchTask()) {
+        // This is not correct, as it should use the results before rank-drop-limit
+        // But keeping like this for now as on-first-phase should be a subset of -on-match
+        task->run(result->copyResult());
+    }
+    if (auto task = matchToolsFactory.createOnFirstPhaseTask()) {
+        task->run(search::ResultSet::stealResult(std::move(*result)));
     }
     if (hasGrouping) {
         context.grouping->setDistributionKey(_distributionKey);
diff --git a/searchcore/src/vespa/searchcore/proton/matching/match_tools.cpp b/searchcore/src/vespa/searchcore/proton/matching/match_tools.cpp
index 9506ca99b66..d652435cbca 100644
--- a/searchcore/src/vespa/searchcore/proton/matching/match_tools.cpp
+++ b/searchcore/src/vespa/searchcore/proton/matching/match_tools.cpp
@@ -253,13 +253,15 @@ MatchToolsFactory::createTask(vespalib::stringref attribute, vespalib::stringref
 std::unique_ptr<AttributeOperationTask>
 MatchToolsFactory::createOnMatchTask() const {
     const auto & op = _rankSetup.getMutateOnMatch();
-    return createTask(execute::onmatch::Attribute::lookup(_queryEnv.getProperties(), op._attribute),
-                      execute::onmatch::Operation::lookup(_queryEnv.getProperties(), op._operation));
+    return createTask(op._attribute, op._operation);
 }
 std::unique_ptr<AttributeOperationTask>
 MatchToolsFactory::createOnFirstPhaseTask() const {
     const auto & op = _rankSetup.getMutateOnFirstPhase();
-    return createTask(op._attribute, op._operation);
+    // Note that combining onmatch in query with first-phase is not a bug.
+    // It is intentional, as the semantics of onmatch in query are identical to on-first-phase.
+    return createTask(execute::onmatch::Attribute::lookup(_queryEnv.getProperties(), op._attribute),
+                      execute::onmatch::Operation::lookup(_queryEnv.getProperties(), op._operation));
 }
 std::unique_ptr<AttributeOperationTask>
 MatchToolsFactory::createOnSecondPhaseTask() const {