Merge pull request #26094 from vespa-engine/balder/cleanup-group-limitations

Clean up limitation enforment in Group.

2 files changed, 28 insertions, 15 deletions

diff --git a/searchlib/src/vespa/searchlib/aggregation/group.cpp b/searchlib/src/vespa/searchlib/aggregation/group.cpp
index e025d3b0f94..6ac994874ba 100644
--- a/searchlib/src/vespa/searchlib/aggregation/group.cpp
+++ b/searchlib/src/vespa/searchlib/aggregation/group.cpp
@@ -243,7 +243,6 @@ Group::Value::addExpressionResult(ExpressionNode::UP expressionNode)
 void
 Group::Value::addAggregationResult(ExpressionNode::UP aggr)
 {
-    assert(getAggrSize() < 15);
     size_t newSize = getAggrSize() + 1 + getExprSize();
     auto n = new ExpressionNode::CP[newSize];
     for (size_t i(0), m(getAggrSize()); i < m; i++) {
@@ -259,6 +258,24 @@ Group::Value::addAggregationResult(ExpressionNode::UP aggr)
     setAggrSize(getAggrSize() + 1);
 }
 
+void
+Group::Value::setAggrSize(uint32_t v) {
+    assert(v < 0x10000);
+    _packedLength = (_packedLength & ~0xffff) | v;
+}
+
+void
+Group::Value::setExprSize(uint32_t v) {
+    assert(v < sizeof(_orderBy)*2);
+    _packedLength = (_packedLength & ~0xf0000) | (v << 16);
+}
+
+void
+Group::Value::setOrderBySize(uint32_t v) {
+    assert(v < sizeof(_orderBy)*2);
+    _packedLength = (_packedLength & ~0xf00000) | (v << 20);
+}
+
 template <typename Doc>
 void Group::Value::collect(const Doc & doc, HitRank rank)
 {
@@ -272,15 +289,13 @@ Group::Value::addResult(ExpressionNode::UP aggr)
 {
     assert(getExprSize() < 15);
     addAggregationResult(std::move(aggr));
-    addExpressionResult(ExpressionNode::UP(new AggregationRefNode(getAggrSize() - 1)));
+    addExpressionResult(std::make_unique<AggregationRefNode>(getAggrSize() - 1));
     setupAggregationReferences();
 }
 
 void
 Group::Value::addOrderBy(ExpressionNode::UP orderBy, bool ascending)
 {
-    assert(getOrderBySize() < sizeof(_orderBy)*2-1);
-    assert(getExprSize() < 15);
     addExpressionResult(std::move(orderBy));
     setOrderBy(getOrderBySize(), (ascending ? getExprSize() : -getExprSize()));
     setOrderBySize(getOrderBySize() + 1);
@@ -555,7 +570,6 @@ Group::Value::deserialize(Deserializer & is) {
     }
     uint32_t aggrSize(0);
     is >> aggrSize;
-    assert(aggrSize < 16);
     // To avoid protocol changes, we must first deserialize the aggregation
     // results into a temporary buffer, and then reallocate the actual
     // vector when we know the total size. Then we copy the temp buffer and
@@ -575,7 +589,6 @@ Group::Value::deserialize(Deserializer & is) {
     }
     delete [] tmpAggregationResults;
 
-    assert(exprSize < 16);
     setExprSize(exprSize);
     for (uint32_t i(aggrSize); i < aggrSize + exprSize; i++) {
         is >> _aggregationResults[i];
diff --git a/searchlib/src/vespa/searchlib/aggregation/group.h b/searchlib/src/vespa/searchlib/aggregation/group.h
index 128bfce2323..3a3b6fedd9a 100644
--- a/searchlib/src/vespa/searchlib/aggregation/group.h
+++ b/searchlib/src/vespa/searchlib/aggregation/group.h
@@ -96,8 +96,9 @@ public:
 
         GroupList groups() const { return _children; }
         void addChild(Group * child);
-        uint32_t getAggrSize()    const { return _packedLength & 0x0f; }
-        uint32_t getOrderBySize() const { return (_packedLength >> 6) & 0x03; }
+        uint32_t getAggrSize()    const { return _packedLength & 0xffff; }
+        uint32_t getExprSize()    const { return (_packedLength >> 16) & 0x0f; }
+        uint32_t getOrderBySize() const { return (_packedLength >> 20) & 0x0f; }
         uint32_t getChildrenSize()   const { return _childrenLength; }
         uint32_t getExpr(uint32_t i) const { return getAggrSize() + i; }
         int32_t getOrderBy(uint32_t i) const {
@@ -107,7 +108,6 @@ public:
 
         const AggregationResult & getAggregationResult(size_t i) const { return static_cast<const AggregationResult &>(*_aggregationResults[i]); }
         AggregationResult & getAggregationResult(size_t i) { return static_cast<AggregationResult &>(*_aggregationResults[i]); }
-        uint32_t getExprSize()    const { return (_packedLength >> 4) & 0x03; }
         const Group & getChild(size_t i) const { return *_children[i]; }
 
         template <typename Doc>
@@ -116,9 +116,9 @@ public:
 
         using  ExpressionVector = ExpressionNode::CP *;
         using GroupHash = vespalib::hash_set<uint32_t, GroupHasher, GroupEqual >;
-        void setAggrSize(uint32_t v)    { _packedLength = (_packedLength & ~0x0f) | v; }
-        void setExprSize(uint32_t v)    { _packedLength = (_packedLength & ~0x30) | (v << 4); }
-        void setOrderBySize(uint32_t v) { _packedLength = (_packedLength & ~0xc0) | (v << 6); }
+        void setAggrSize(uint32_t v);
+        void setExprSize(uint32_t v);
+        void setOrderBySize(uint32_t v);
         void setChildrenSize(uint32_t v) { _childrenLength = v; }
         AggregationResult * getAggr(size_t i) { return static_cast<AggregationResult *>(_aggregationResults[i].get()); }
         const AggregationResult & getAggr(size_t i) const { return static_cast<const AggregationResult &>(*_aggregationResults[i]); }
@@ -147,9 +147,9 @@ public:
             size_t     _allChildren;            // Keep real number of children.
         }                _childInfo;
         uint32_t         _childrenLength;
-        uint32_t         _tag;             // Opaque tag used to identify the group by the client.
-        uint8_t          _packedLength;    // Length of aggr and expr vectors.
-        uint8_t          _orderBy[2];           // How this group is ranked, negative means reverse rank.
+        uint32_t         _tag;                  // Opaque tag used to identify the group by the client.
+        uint32_t         _packedLength;         // Length of aggr and expr vectors.
+        uint8_t          _orderBy[4];           // How this group is ranked, negative means reverse rank.
     };
 private:
     ResultNode::CP   _id;                   // the label of this group, separating it from other groups