Merge pull request #24281 from vespa-engine/balder/cap-hits-and-offse…

…t-against-numdocs To prevent against damages caused by excessive hits/offset param bein…
vespa-engine · Oct 3, 2022 · 906b57f · 906b57f
2 parents 1fafdfd + e5594f3
commit 906b57f
Show file tree

Hide file tree

Showing 3 changed files with 39 additions and 17 deletions.
diff --git a/searchcore/src/tests/proton/matching/matching_test.cpp b/searchcore/src/tests/proton/matching/matching_test.cpp
@@ -934,8 +934,8 @@ TEST("require that getSummaryFeatures prefers cached query setup") {
 }
 
 TEST("require that match params are set up straight with ranking on") {
-    MatchParams p(1, 2, 4, 0.7, 0, 1, true, true);
-    ASSERT_EQUAL(1u, p.numDocs);
+    MatchParams p(10, 2, 4, 0.7, 0, 1, true, true);
+    ASSERT_EQUAL(10u, p.numDocs);
     ASSERT_EQUAL(2u, p.heapSize);
     ASSERT_EQUAL(4u, p.arraySize);
     ASSERT_EQUAL(0.7, p.rankDropLimit);
@@ -945,8 +945,8 @@ TEST("require that match params are set up straight with ranking on") {
 }
 
 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);
+    MatchParams p(10, 2, 4, -std::numeric_limits<feature_t>::quiet_NaN(), 0, 1, true, true);
+    ASSERT_EQUAL(10u, p.numDocs);
     ASSERT_EQUAL(2u, p.heapSize);
     ASSERT_EQUAL(4u, p.arraySize);
     ASSERT_TRUE(std::isnan(p.rankDropLimit));
@@ -957,8 +957,8 @@ TEST("require that match params can turn off 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);
+    MatchParams p(10, 6, 4, 0.7, 1, 1, true, true);
+    ASSERT_EQUAL(10u, p.numDocs);
     ASSERT_EQUAL(6u, p.heapSize);
     ASSERT_EQUAL(6u, p.arraySize);
     ASSERT_EQUAL(0.7, p.rankDropLimit);
@@ -967,18 +967,38 @@ TEST("require that match params are set up straight with ranking on arraySize is
 }
 
 TEST("require that match params are set up straight with ranking on arraySize is atleast the size of hits+offset") {
-    MatchParams p(1, 6, 4, 0.7, 4, 4, true, true);
-    ASSERT_EQUAL(1u, p.numDocs);
+    MatchParams p(10, 6, 4, 0.7, 4, 4, true, true);
+    ASSERT_EQUAL(10u, p.numDocs);
     ASSERT_EQUAL(6u, p.heapSize);
     ASSERT_EQUAL(8u, p.arraySize);
     ASSERT_EQUAL(0.7, p.rankDropLimit);
     ASSERT_EQUAL(4u, p.offset);
     ASSERT_EQUAL(4u, p.hits);
 }
 
-TEST("require that match params are set up straight with ranking off array and heap size is 0") {
-    MatchParams p(1, 6, 4, 0.7, 4, 4, true, false);
+TEST("require that match params are capped by numDocs") {
+    MatchParams p(1, 6, 4, 0.7, 4, 4, true, true);
     ASSERT_EQUAL(1u, p.numDocs);
+    ASSERT_EQUAL(1u, p.heapSize);
+    ASSERT_EQUAL(1u, p.arraySize);
+    ASSERT_EQUAL(0.7, p.rankDropLimit);
+    ASSERT_EQUAL(1u, p.offset);
+    ASSERT_EQUAL(0u, p.hits);
+}
+
+TEST("require that match params are capped by numDocs and hits adjusted down") {
+    MatchParams p(5, 6, 4, 0.7, 4, 4, true, true);
+    ASSERT_EQUAL(5u, p.numDocs);
+    ASSERT_EQUAL(5u, p.heapSize);
+    ASSERT_EQUAL(5u, p.arraySize);
+    ASSERT_EQUAL(0.7, p.rankDropLimit);
+    ASSERT_EQUAL(4u, p.offset);
+    ASSERT_EQUAL(1u, p.hits);
+}
+
+TEST("require that match params are set up straight with ranking off array and heap size is 0") {
+    MatchParams p(10, 6, 4, 0.7, 4, 4, true, false);
+    ASSERT_EQUAL(10u, p.numDocs);
     ASSERT_EQUAL(0u, p.heapSize);
     ASSERT_EQUAL(0u, p.arraySize);
     ASSERT_EQUAL(0.7, p.rankDropLimit);

diff --git a/searchcore/src/vespa/searchcore/proton/matching/match_params.cpp b/searchcore/src/vespa/searchcore/proton/matching/match_params.cpp
@@ -8,7 +8,8 @@ namespace proton::matching {
 
 namespace {
 
-uint32_t computeArraySize(uint32_t hitsPlussOffset, uint32_t heapSize, uint32_t arraySize)
+uint32_t
+computeArraySize(uint32_t hitsPlussOffset, uint32_t heapSize, uint32_t arraySize)
 {
     return std::max(hitsPlussOffset, std::max(heapSize, arraySize));
 }
@@ -24,16 +25,17 @@ MatchParams::MatchParams(uint32_t          numDocs_in,
                          bool              hasFinalRank,
                          bool              needRanking)
     : numDocs(numDocs_in),
-      heapSize((hasFinalRank && needRanking) ? heapSize_in : 0),
+      heapSize((hasFinalRank && needRanking) ? std::min(numDocs_in, heapSize_in) : 0),
       arraySize((needRanking && ((heapSize_in + arraySize_in) > 0))
-                ? computeArraySize(hits_in + offset_in, heapSize, arraySize_in)
+                ? std::min(numDocs_in, computeArraySize(hits_in + offset_in, heapSize, arraySize_in))
                 : 0),
-      offset(offset_in),
-      hits(hits_in),
+      offset(std::min(numDocs_in, offset_in)),
+      hits(std::min(numDocs_in - offset, hits_in)),
       rankDropLimit(rankDropLimit_in)
 { }
 
-bool MatchParams::has_rank_drop_limit() const {
+bool
+MatchParams::has_rank_drop_limit() const {
     return ! std::isnan(rankDropLimit);
 }
 

diff --git a/searchlib/src/vespa/searchlib/engine/searchrequest.h b/searchlib/src/vespa/searchlib/engine/searchrequest.h
@@ -23,7 +23,7 @@ class SearchRequest : public Request
 
     SearchRequest();
     explicit SearchRequest(RelativeTime relativeTime);
-    ~SearchRequest();
+    ~SearchRequest() override;
 };
 
 }