- We are now always nesting multivalue grouping for indexed search.

10 files changed, 36 insertions, 57 deletions

diff --git a/searchcore/src/tests/grouping/grouping_test.cpp b/searchcore/src/tests/grouping/grouping_test.cpp
index a2f646cba3a..2755173a99b 100644
--- a/searchcore/src/tests/grouping/grouping_test.cpp
+++ b/searchcore/src/tests/grouping/grouping_test.cpp
@@ -45,7 +45,7 @@ struct MyWorld {
         bv.setInterval(0, NUM_DOCS);
         // attribute context
         {
-            SingleInt32ExtAttribute *attr = new SingleInt32ExtAttribute("attr0");
+            auto *attr = new SingleInt32ExtAttribute("attr0");
             AttributeVector::DocId docid;
             for (uint32_t i = 0; i < NUM_DOCS; ++i) {
                 attr->addDoc(docid);
@@ -55,7 +55,7 @@ struct MyWorld {
             attributeContext.add(attr);
         }
         {
-            SingleInt32ExtAttribute *attr = new SingleInt32ExtAttribute("attr1");
+            auto *attr = new SingleInt32ExtAttribute("attr1");
             AttributeVector::DocId docid;
             for (uint32_t i = 0; i < NUM_DOCS; ++i) {
                 attr->addDoc(docid);
@@ -65,7 +65,7 @@ struct MyWorld {
             attributeContext.add(attr);
         }
         {
-            SingleInt32ExtAttribute *attr = new SingleInt32ExtAttribute("attr2");
+            auto *attr = new SingleInt32ExtAttribute("attr2");
             AttributeVector::DocId docid;
             for (uint32_t i = 0; i < NUM_DOCS; ++i) {
                 attr->addDoc(docid);
@@ -75,7 +75,7 @@ struct MyWorld {
             attributeContext.add(attr);
         }
         {
-            SingleInt32ExtAttribute *attr = new SingleInt32ExtAttribute("attr3");
+            auto *attr = new SingleInt32ExtAttribute("attr3");
             AttributeVector::DocId docid;
             for (uint32_t i = 0; i < NUM_DOCS; ++i) {
                 attr->addDoc(docid);
@@ -94,16 +94,17 @@ using GroupingList = GroupingContext::GroupingList;
 
 SessionId createSessionId(const std::string & s) {
     std::vector<char> vec;
-    for (size_t i = 0; i < s.size(); i++) {
-        vec.push_back(s[i]);
+    for (char c : s) {
+        vec.push_back(c);
     }
-    return SessionId(&vec[0], vec.size());
+    return {&vec[0], vec.size()};
 }
 
 class CheckAttributeReferences : public vespalib::ObjectOperation, public vespalib::ObjectPredicate
 {
 public:
-    CheckAttributeReferences(bool log=false) : _log(log), _numrefs(0) { }
+    CheckAttributeReferences() : CheckAttributeReferences(false) {}
+    explicit CheckAttributeReferences(bool log) : _log(log), _numrefs(0) { }
     bool     _log;
     uint32_t _numrefs;
 private:
@@ -177,7 +178,7 @@ TEST_F("testGroupingContextInitialization", DoomFixture()) {
     baseRequest.serialize(nos);
 
     AllocatedBitVector bv(1);
-    GroupingContext context(bv, f1.clock.clock(), f1.timeOfDoom, os.data(), os.size(), true);
+    GroupingContext context(bv, f1.clock.clock(), f1.timeOfDoom, os.data(), os.size());
     ASSERT_TRUE(!context.empty());
     GroupingContext::GroupingList list = context.getGroupingList();
     ASSERT_TRUE(list.size() == 1);
@@ -303,8 +304,8 @@ TEST_F("testGroupingSession", DoomFixture()) {
     manager.groupInRelevanceOrder(&hit, 1);
     CheckAttributeReferences attrCheck_after;
     GroupingList &gl3(initContext.getGroupingList());
-    for (unsigned int i = 0; i < gl3.size(); i++) {
-        gl3[i]->select(attrCheck_after, attrCheck_after);
+    for (auto & grouping : gl3) {
+        grouping->select(attrCheck_after, attrCheck_after);
     }
     EXPECT_EQUAL(attrCheck_after._numrefs, 0u);
     {
@@ -423,9 +424,9 @@ void doGrouping(GroupingContext &ctx,
 {
     GroupingManager man(ctx);
     std::vector<RankedHit> hits;
-    hits.push_back(RankedHit(doc1, rank1));
-    hits.push_back(RankedHit(doc2, rank2));
-    hits.push_back(RankedHit(doc3, rank3));
+    hits.emplace_back(doc1, rank1);
+    hits.emplace_back(doc2, rank2);
+    hits.emplace_back(doc3, rank3);
     man.groupInRelevanceOrder(&hits[0], 3);
 }
 
diff --git a/searchcore/src/vespa/searchcore/grouping/groupingcontext.cpp b/searchcore/src/vespa/searchcore/grouping/groupingcontext.cpp
index 63127d01450..882a4d1509d 100644
--- a/searchcore/src/vespa/searchcore/grouping/groupingcontext.cpp
+++ b/searchcore/src/vespa/searchcore/grouping/groupingcontext.cpp
@@ -2,7 +2,6 @@
 
 #include "groupingcontext.h"
 #include <vespa/searchlib/aggregation/predicates.h>
-#include <vespa/searchlib/aggregation/modifiers.h>
 #include <vespa/searchlib/aggregation/hitsaggregationresult.h>
 #include <vespa/searchlib/common/bitvector.h>
 
@@ -53,13 +52,12 @@ GroupingContext::setDistributionKey(uint32_t distributionKey)
 }
 
 GroupingContext::GroupingContext(const BitVector & validLids, const vespalib::Clock & clock, vespalib::steady_time timeOfDoom,
-                                 const char *groupSpec, uint32_t groupSpecLen, bool enableNested)
+                                 const char *groupSpec, uint32_t groupSpecLen)
     : _validLids(validLids),
       _clock(clock),
       _timeOfDoom(timeOfDoom),
       _os(),
-      _groupingList(),
-      _enableNestedMultivalueGrouping(enableNested)
+      _groupingList()
 {
     deserialize(groupSpec, groupSpecLen);
 }
@@ -69,8 +67,7 @@ GroupingContext::GroupingContext(const BitVector & validLids, const vespalib::Cl
       _clock(clock),
       _timeOfDoom(timeOfDoom),
       _os(),
-      _groupingList(),
-      _enableNestedMultivalueGrouping(true)
+      _groupingList()
 { }
 
 GroupingContext::GroupingContext(const GroupingContext & rhs)
@@ -78,8 +75,7 @@ GroupingContext::GroupingContext(const GroupingContext & rhs)
       _clock(rhs._clock),
       _timeOfDoom(rhs._timeOfDoom),
       _os(),
-      _groupingList(),
-      _enableNestedMultivalueGrouping(rhs._enableNestedMultivalueGrouping)
+      _groupingList()
 { }
 
 void
@@ -100,7 +96,7 @@ GroupingContext::serialize()
 }
 
 bool
-GroupingContext::needRanking() const
+GroupingContext::needRanking() const noexcept
 {
     if (_groupingList.empty()) {
         return false;
diff --git a/searchcore/src/vespa/searchcore/grouping/groupingcontext.h b/searchcore/src/vespa/searchcore/grouping/groupingcontext.h
index c29b5122f74..e1b296df99b 100644
--- a/searchcore/src/vespa/searchcore/grouping/groupingcontext.h
+++ b/searchcore/src/vespa/searchcore/grouping/groupingcontext.h
@@ -34,7 +34,7 @@ public:
      * @param groupSpecLen The length of the grouping specification, in bytes.
      **/
     GroupingContext(const BitVector & validLids, const vespalib::Clock & clock, vespalib::steady_time timeOfDoom,
-                    const char *groupSpec, uint32_t groupSpecLen, bool enableNestedMultivalueGrouping);
+                    const char *groupSpec, uint32_t groupSpecLen);
 
     /**
      * Create a new grouping context from a byte buffer.
@@ -105,9 +105,7 @@ public:
      * Figure out if ranking is necessary for any of the grouping requests here.
      * @return true if ranking is required.
      */
-    bool needRanking() const;
-    bool enableNestedMultivalueGrouping() const noexcept { return _enableNestedMultivalueGrouping; }
-    const search::BitVector & getValidLids() const { return _validLids; }
+    bool needRanking() const noexcept;
 
     void groupUnordered(const RankedHit *searchResults, uint32_t binSize, const search::BitVector * overflow);
     void groupInRelevanceOrder(const RankedHit *searchResults, uint32_t binSize);
@@ -123,7 +121,6 @@ private:
     vespalib::steady_time   _timeOfDoom;
     vespalib::nbostream     _os;
     GroupingList            _groupingList;
-    bool                    _enableNestedMultivalueGrouping;
 };
 
 }
diff --git a/searchcore/src/vespa/searchcore/grouping/groupingmanager.cpp b/searchcore/src/vespa/searchcore/grouping/groupingmanager.cpp
index f5bcc935cb1..799d2663589 100644
--- a/searchcore/src/vespa/searchcore/grouping/groupingmanager.cpp
+++ b/searchcore/src/vespa/searchcore/grouping/groupingmanager.cpp
@@ -1,7 +1,6 @@
 // Copyright Vespa.ai. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 
 #include "groupingmanager.h"
-#include "groupingsession.h"
 #include "groupingcontext.h"
 #include <vespa/searchlib/aggregation/fs4hit.h>
 #include <vespa/searchlib/expression/attributenode.h>
@@ -52,11 +51,11 @@ GroupingManager::init(const IAttributeContext &attrCtx)
                 ExpressionNode & en = *level.getExpression().getRoot();
 
                 if (en.inherits(AttributeNode::classId)) {
-                    AttributeNode & an = static_cast<AttributeNode &>(en);
+                    auto & an = static_cast<AttributeNode &>(en);
                     an.enableEnumOptimization(true);
                 }
             }
-            ConfigureStaticParams stuff(&attrCtx, nullptr, _groupingContext.enableNestedMultivalueGrouping());
+            ConfigureStaticParams stuff(&attrCtx, nullptr);
             grouping.configureStaticStuff(stuff);
             list.push_back(groupingList[i]);
         } catch (const std::exception & e) {
@@ -96,10 +95,9 @@ void
 GroupingManager::prune()
 {
     GroupingContext::GroupingList &groupingList(_groupingContext.getGroupingList());
-    for (size_t i = 0; i < groupingList.size(); ++i) {
-        Grouping &g = *groupingList[i];
-        g.postMerge();
-        g.sortById();
+    for (const auto & g : groupingList) {
+        g->postMerge();
+        g->sortById();
     }
 }
 
@@ -107,10 +105,9 @@ void
 GroupingManager::convertToGlobalId(const search::IDocumentMetaStore &metaStore)
 {
     GroupingContext::GroupingList & groupingList = _groupingContext.getGroupingList();
-    for (size_t i = 0; i < groupingList.size(); ++i) {
-        Grouping & g = *groupingList[i];
-        g.convertToGlobalId(metaStore);
-        LOG(debug, "convertToGlobalId: %s", g.asString().c_str());
+    for (const auto & g : groupingList) {
+        g->convertToGlobalId(metaStore);
+        LOG(debug, "convertToGlobalId: %s", g->asString().c_str());
     }
 }
 
diff --git a/searchcore/src/vespa/searchcore/proton/matching/matcher.cpp b/searchcore/src/vespa/searchcore/proton/matching/matcher.cpp
index 3c8c90a229d..3e8909aa593 100644
--- a/searchcore/src/vespa/searchcore/proton/matching/matcher.cpp
+++ b/searchcore/src/vespa/searchcore/proton/matching/matcher.cpp
@@ -249,7 +249,7 @@ Matcher::match(const SearchRequest &request, vespalib::ThreadBundle &threadBundl
     { // we want to measure full set-up and tear-down time as part of
       // collateral time
         GroupingContext groupingContext(metaStore.getValidLids(), _clock, request.getTimeOfDoom(),
-                                        request.groupSpec.data(), request.groupSpec.size(), _rankSetup->enableNestedMultivalueGrouping());
+                                        request.groupSpec.data(), request.groupSpec.size());
         SessionId sessionId(request.sessionId.data(), request.sessionId.size());
         bool shouldCacheSearchSession = false;
         bool shouldCacheGroupingSession = false;
diff --git a/searchlib/src/vespa/searchlib/expression/expressiontree.h b/searchlib/src/vespa/searchlib/expression/expressiontree.h
index 52c075f3e29..127de66b6dc 100644
--- a/searchlib/src/vespa/searchlib/expression/expressiontree.h
+++ b/searchlib/src/vespa/searchlib/expression/expressiontree.h
@@ -50,11 +50,11 @@ public:
     };
 
     ExpressionTree() noexcept;
-    ExpressionTree(const ExpressionNode & root);
-    ExpressionTree(ExpressionNode::UP root);
+    explicit ExpressionTree(const ExpressionNode & root);
+    explicit ExpressionTree(ExpressionNode::UP root);
     ExpressionTree(const ExpressionTree & rhs);
     ExpressionTree(ExpressionTree &&) noexcept = default;
-    ~ExpressionTree();
+    ~ExpressionTree() override;
     ExpressionTree & operator = (ExpressionNode::UP rhs);
     ExpressionTree & operator = (const ExpressionTree & rhs);
     ExpressionTree & operator = (ExpressionTree &&) noexcept = default;
diff --git a/searchlib/src/vespa/searchlib/fef/indexproperties.cpp b/searchlib/src/vespa/searchlib/fef/indexproperties.cpp
index f063bad66e1..5d3dbe69e77 100644
--- a/searchlib/src/vespa/searchlib/fef/indexproperties.cpp
+++ b/searchlib/src/vespa/searchlib/fef/indexproperties.cpp
@@ -173,11 +173,6 @@ namespace onsummary {
 
 namespace temporary {
 
-const vespalib::string EnableNestedMultivalueGrouping::NAME("vespa.temporary.enable_nested_multivalue_grouping");
-bool EnableNestedMultivalueGrouping::check(const Properties &props) {
-    return lookupBool(props, NAME, false);
-}
-
 }
 
 namespace mutate {
diff --git a/searchlib/src/vespa/searchlib/fef/indexproperties.h b/searchlib/src/vespa/searchlib/fef/indexproperties.h
index 348ce3ab5e2..1921f52276f 100644
--- a/searchlib/src/vespa/searchlib/fef/indexproperties.h
+++ b/searchlib/src/vespa/searchlib/fef/indexproperties.h
@@ -176,11 +176,8 @@ namespace mutate {
     };
 }
 
+// Add temporary flags used for safe rollout of new features here
 namespace temporary {
-    struct EnableNestedMultivalueGrouping {
-        static const vespalib::string NAME;
-        static bool check(const Properties &props);
-    };
 }
 
 namespace mutate::on_match {
diff --git a/searchlib/src/vespa/searchlib/fef/ranksetup.cpp b/searchlib/src/vespa/searchlib/fef/ranksetup.cpp
index 806be9af47c..d6b0b900516 100644
--- a/searchlib/src/vespa/searchlib/fef/ranksetup.cpp
+++ b/searchlib/src/vespa/searchlib/fef/ranksetup.cpp
@@ -75,8 +75,7 @@ RankSetup::RankSetup(const BlueprintFactory &factory, const IIndexEnvironment &i
       _mutateOnFirstPhase(),
       _mutateOnSecondPhase(),
       _mutateOnSummary(),
-      _mutateAllowQueryOverride(false),
-      _enableNestedMultivalueGrouping(false)
+      _mutateAllowQueryOverride(false)
 { }
 
 RankSetup::~RankSetup() = default;
@@ -135,7 +134,6 @@ RankSetup::configure()
     _mutateOnSummary._attribute = mutate::on_summary::Attribute::lookup(_indexEnv.getProperties());
     _mutateOnSummary._operation = mutate::on_summary::Operation::lookup(_indexEnv.getProperties());
     _mutateAllowQueryOverride = mutate::AllowQueryOverride::check(_indexEnv.getProperties());
-    _enableNestedMultivalueGrouping = temporary::EnableNestedMultivalueGrouping::check(_indexEnv.getProperties());
     _always_mark_phrase_expensive = matching::AlwaysMarkPhraseExpensive::check(_indexEnv.getProperties());
 }
 
diff --git a/searchlib/src/vespa/searchlib/fef/ranksetup.h b/searchlib/src/vespa/searchlib/fef/ranksetup.h
index 0e98c3f1c5d..d744b38cc6e 100644
--- a/searchlib/src/vespa/searchlib/fef/ranksetup.h
+++ b/searchlib/src/vespa/searchlib/fef/ranksetup.h
@@ -85,7 +85,6 @@ private:
     MutateOperation          _mutateOnSecondPhase;
     MutateOperation          _mutateOnSummary;
     bool                     _mutateAllowQueryOverride;
-    bool                     _enableNestedMultivalueGrouping;
 
     void compileAndCheckForErrors(BlueprintResolver &bp);
 public:
@@ -460,7 +459,6 @@ public:
     const MutateOperation & getMutateOnSummary() const { return _mutateOnSummary; }
 
     bool allowMutateQueryOverride() const { return _mutateAllowQueryOverride; }
-    bool enableNestedMultivalueGrouping() const { return _enableNestedMultivalueGrouping; }
 };
 
 }