Merge pull request #31708 from vespa-engine/balder/add-unit-test-for-grouping-adaption-for-streaming-and-indexedv8.362.16

- Add unit test for both conditional and unconditional preparation of…

5 files changed, 63 insertions, 12 deletions

diff --git a/searchcore/src/vespa/searchcore/grouping/groupingmanager.cpp b/searchcore/src/vespa/searchcore/grouping/groupingmanager.cpp
index 9ece37665cd..9b393b6e6d8 100644
--- a/searchcore/src/vespa/searchcore/grouping/groupingmanager.cpp
+++ b/searchcore/src/vespa/searchcore/grouping/groupingmanager.cpp
@@ -4,6 +4,7 @@
 #include "groupingcontext.h"
 #include <vespa/searchlib/aggregation/fs4hit.h>
 #include <vespa/searchlib/expression/attributenode.h>
+#include <vespa/searchlib/aggregation/modifiers.h>
 #include <vespa/vespalib/util/issue.h>
 #include <vespa/vespalib/util/stringfmt.h>
 
@@ -48,6 +49,7 @@ GroupingManager::init(const IAttributeContext &attrCtx)
                     an.enableEnumOptimization(true);
                 }
             }
+            aggregation::NonAttribute2DocumentAccessor nonAttributes2DocumentAccess(attrCtx);
             ConfigureStaticParams stuff(&attrCtx, nullptr);
             grouping.configureStaticStuff(stuff);
             list.push_back(groupingList[i]);
diff --git a/searchlib/src/tests/grouping/grouping_test.cpp b/searchlib/src/tests/grouping/grouping_test.cpp
index 0caebab6607..952f5d2a5db 100644
--- a/searchlib/src/tests/grouping/grouping_test.cpp
+++ b/searchlib/src/tests/grouping/grouping_test.cpp
@@ -8,6 +8,8 @@
 #include <vespa/searchlib/aggregation/hitsaggregationresult.h>
 #include <vespa/searchlib/aggregation/fs4hit.h>
 #include <vespa/searchlib/aggregation/predicates.h>
+#include <vespa/searchlib/aggregation/modifiers.h>
+#include <vespa/searchlib/expression/documentfieldnode.h>
 #include <vespa/searchlib/expression/fixedwidthbucketfunctionnode.h>
 #include <vespa/searchlib/test/make_attribute_map_lookup_node.h>
 #include <vespa/searchcommon/common/undefinedvalues.h>
@@ -51,7 +53,7 @@ public:
             add(val);
         }
     }
-    AttrBuilder(const std::string &name)
+    explicit AttrBuilder(const std::string &name)
         : _attr(new A(name)),
           _attrSP(_attr)
     {
@@ -127,19 +129,19 @@ private:
     ResultBuilder    _result;
     IAttributeContext::UP _attrCtx;
 
-    AggregationContext(const AggregationContext &);
-    AggregationContext &operator=(const AggregationContext &);
-
 public:
     AggregationContext();
+    AggregationContext(const AggregationContext &) = delete;
+    AggregationContext &operator=(const AggregationContext &) = delete;
     ~AggregationContext();
     ResultBuilder &result() { return _result; }
-    void add(AttributeVector::SP attr) {
+    void add(const AttributeVector::SP & attr) {
         _attrMan.add(attr);
     }
     void setup(Grouping &g) {
-        g.configureStaticStuff(ConfigureStaticParams(_attrCtx.get(), 0));
+        g.configureStaticStuff(ConfigureStaticParams(_attrCtx.get(), nullptr));
     }
+    const IAttributeContext & attrCtx() const { return *_attrCtx; }
 };
 
 AggregationContext::AggregationContext() : _attrMan(), _result(), _attrCtx(_attrMan.createContext()) {}
@@ -688,7 +690,7 @@ TEST("testAggregationGroupCapping")
         EXPECT_TRUE(testAggregation(ctx, request, expect));
     }
     {
-        AddFunctionNode *add = new AddFunctionNode();
+        auto add = std::make_unique<AddFunctionNode>();
         add->addArg(MU<AggregationRefNode>(0));
         add->appendArg(MU<ConstantNode>(MU<Int64ResultNode>(3)));
 
@@ -697,7 +699,7 @@ TEST("testAggregationGroupCapping")
                 .setLastLevel(1)
                 .addLevel(std::move(GroupingLevel().setMaxGroups(3).setExpression(MU<AttributeNode>("attr"))
                                   .addAggregationResult(createAggr<SumAggregationResult>(MU<AttributeNode>("attr")))
-                                  .addOrderBy(ExpressionNode::UP(add), false)));
+                                  .addOrderBy(std::move(add), false)));
 
         Group expect;
         expect.addChild(Group().setId(Int64ResultNode(7)).setRank(RawRank(7))
@@ -1826,4 +1828,31 @@ TEST("testAttributeMapLookup")
     testAggregationSimple(ctx, MaxAggregationResult(), Int64ResultNode(100), "smap{attribute(key2)}.weight");
 }
 
+TEST("test that non-attributes are converted to document field nodes") {
+    AggregationContext ctx;
+    ctx.add(IntAttrBuilder("attr").sp());
+
+    Grouping attrRequest;
+    attrRequest.setRoot(Group().addResult(SumAggregationResult().setExpression(MU<AttributeNode>("attr"))));
+    aggregation::NonAttribute2DocumentAccessor optional2DocumentAccessor(ctx.attrCtx());
+    attrRequest.select(optional2DocumentAccessor, optional2DocumentAccessor);
+    EXPECT_TRUE(attrRequest.getRoot().getAggregationResult(0).getExpression()->inherits(AttributeNode::classId));
+
+    Grouping nonAttrRequest;
+    nonAttrRequest.setRoot(Group().addResult(SumAggregationResult().setExpression(MU<AttributeNode>("non-attr"))));
+    nonAttrRequest.select(optional2DocumentAccessor, optional2DocumentAccessor);
+    EXPECT_TRUE(nonAttrRequest.getRoot().getAggregationResult(0).getExpression()->inherits(DocumentFieldNode::classId));
+}
+
+TEST("test that attributes can be unconditionally converted to document field nodes") {
+    AggregationContext ctx;
+    ctx.add(IntAttrBuilder("attr").sp());
+
+    Grouping attrRequest;
+    attrRequest.setRoot(Group().addResult(SumAggregationResult().setExpression(MU<AttributeNode>("attr"))));
+    aggregation::Attribute2DocumentAccessor attr2DocumentAccessor;
+    attrRequest.select(attr2DocumentAccessor, attr2DocumentAccessor);
+    EXPECT_TRUE(attrRequest.getRoot().getAggregationResult(0).getExpression()->inherits(DocumentFieldNode::classId));
+}
+
 TEST_MAIN() { TEST_RUN_ALL(); }
diff --git a/searchlib/src/vespa/searchlib/aggregation/modifiers.cpp b/searchlib/src/vespa/searchlib/aggregation/modifiers.cpp
index 0749f19fceb..89e6e6685e9 100644
--- a/searchlib/src/vespa/searchlib/aggregation/modifiers.cpp
+++ b/searchlib/src/vespa/searchlib/aggregation/modifiers.cpp
@@ -7,6 +7,7 @@
 #include <vespa/searchlib/expression/documentfieldnode.h>
 #include <vespa/searchlib/expression/interpolated_document_field_lookup_node.h>
 #include <vespa/searchlib/expression/interpolatedlookupfunctionnode.h>
+#include <vespa/searchcommon/attribute/iattributecontext.h>
 
 using namespace search::expression;
 
@@ -59,6 +60,14 @@ Attribute2DocumentAccessor::getReplacementNode(const AttributeNode &attributeNod
     return std::make_unique<DocumentFieldNode>(attributeNode.getAttributeName());
 }
 
+std::unique_ptr<ExpressionNode>
+NonAttribute2DocumentAccessor::getReplacementNode(const expression::AttributeNode &attributeNode) {
+    if (_attrCtx.getAttribute(attributeNode.getAttributeName()) == nullptr) {
+        return Attribute2DocumentAccessor::getReplacementNode(attributeNode);
+    }
+    return {};
+}
+
 }
 
 // this function was added by ../../forcelink.sh
diff --git a/searchlib/src/vespa/searchlib/aggregation/modifiers.h b/searchlib/src/vespa/searchlib/aggregation/modifiers.h
index e55ca28bd62..9b0f5d8186d 100644
--- a/searchlib/src/vespa/searchlib/aggregation/modifiers.h
+++ b/searchlib/src/vespa/searchlib/aggregation/modifiers.h
@@ -7,10 +7,12 @@
 #include <functional>
 
 namespace search::expression {
+    class ExpressionNode;
+    class AttributeNode;
+}
 
-class ExpressionNode;
-class AttributeNode;
-
+namespace search::attribute {
+    class IAttributeContext;
 }
 
 namespace search::aggregation {
@@ -28,8 +30,17 @@ private:
 
 class Attribute2DocumentAccessor : public AttributeNodeReplacer
 {
+protected:
+    ExpressionNodeUP getReplacementNode(const expression::AttributeNode &attributeNode) override;
+};
+
+class NonAttribute2DocumentAccessor : public Attribute2DocumentAccessor
+{
+public:
+    explicit NonAttribute2DocumentAccessor(const attribute::IAttributeContext &attrCtx) noexcept : _attrCtx(attrCtx) {}
 private:
     ExpressionNodeUP getReplacementNode(const expression::AttributeNode &attributeNode) override;
+    const attribute::IAttributeContext &_attrCtx;
 };
 
 }
diff --git a/searchlib/src/vespa/searchlib/attribute/attributecontext.h b/searchlib/src/vespa/searchlib/attribute/attributecontext.h
index a02e05abe4f..bd98031ee66 100644
--- a/searchlib/src/vespa/searchlib/attribute/attributecontext.h
+++ b/searchlib/src/vespa/searchlib/attribute/attributecontext.h
@@ -29,7 +29,7 @@ private:
     const IAttributeVector *getAttribute(AttributeMap & map, const string & name, bool stableEnum) const;
     const IAttributeVector *getAttributeMtSafe(AttributeMap & map, const string & name, bool stableEnum) const;
 public:
-    AttributeContext(const IAttributeManager & manager);
+    explicit AttributeContext(const IAttributeManager & manager);
     ~AttributeContext() override;
 
     // Implements IAttributeContext