HPCC-14596 Changes following review

Signed-off-by: Gavin Halliday <gavin.halliday@lexisnexis.com>

ecl/hqlcpp/hqlcerrors.hpp

@@ -219,7 +219,7 @@
 #define HQLERR_EmbedParamNotSupportedInOptions  4199
 #define HQLERR_InvalidXmlnsPrefix               4200
 #define HQLERR_ConditionalAggregateVarOffset    4201
-#define HQLERR_AggregateDyanmicOffset           4202
+#define HQLERR_AggregateDynamicOffset           4202
 
 //Warnings....
 #define HQLWRN_PersistDataNotLikely             4500
@@ -514,7 +514,7 @@
 #define HQLERR_EmbedParamNotSupportedInOptions_Text   "Cannot use bound parameter in embed options - try adding a FUNCTION wrapper"
 #define HQLERR_InvalidXmlnsPrefix_Text          "Invalid XMLNS prefix: %s"
 #define HQLERR_ConditionalAggregateVarOffset_Text "Conditional aggregate '%s' cannot follow a variable length field"
-#define HQLERR_AggregateDyanmicOffset_Text      "Aggregate assignment to '%s' cannot follow variable size aggregate"
+#define HQLERR_AggregateDynamicOffset_Text      "Aggregate assignment to '%s' cannot follow variable size aggregate"
 
 //Warnings.
 #define HQLWRN_CannotRecreateDistribution_Text  "Cannot recreate the distribution for a persistent dataset"

ecl/hqlcpp/hqlcpp.cpp

@@ -1024,6 +1024,7 @@ void CHqlBoundTarget::validate() const
         type_t tc = type->getTypeCode();
         if (tc == type_row || type->isReference())
         {
+            //No checks to apply in these cases.
         }
         else if (type->getSize() != UNKNOWN_LENGTH)
         {

ecl/hqlcpp/hqlhtcpp.cpp

@@ -12890,7 +12890,6 @@ void HqlCppTranslator::doBuildAggregateProcessTransform(BuildCtx & ctx, BoundRow
 
         BuildCtx condctx(ctx);
         node_operator srcOp = src->getOperator();
-        bool isAggregateOp = true;
         switch (srcOp)
         {
         case no_countgroup:
@@ -12970,7 +12969,6 @@ void HqlCppTranslator::doBuildAggregateProcessTransform(BuildCtx & ctx, BoundRow
             }
             break;
         default:
-            isAggregateOp = false;
             if (!src->isConstant() || isVariableOffset)
             {
                 if (!alwaysNextRow)
@@ -12983,13 +12981,13 @@ void HqlCppTranslator::doBuildAggregateProcessTransform(BuildCtx & ctx, BoundRow
             break;
         }
 
-        if (isAggregateOp && isDynamicOffset)
-            throwError1(HQLERR_AggregateDyanmicOffset, str(target->queryChild(1)->queryId()));
+        if (isDynamicOffset)
+            throwError1(HQLERR_AggregateDynamicOffset, str(target->queryChild(1)->queryId()));
 
         if (target->queryType()->getSize() == UNKNOWN_LENGTH)
         {
            isVariableOffset = true;
-           if (isAggregateOp)
+           if (src->isGroupAggregateFunction())
                isDynamicOffset = true;
         }
     }

ecl/hqlcpp/hqlttcpp.cpp

@@ -3531,24 +3531,19 @@ IHqlExpression * ThorHqlTransformer::normalizeMergeAggregate(IHqlExpression * ex
 static bool reorderAggregateFields(HqlExprArray & aggregateFields, HqlExprArray & aggregateAssigns)
 {
     bool needToReorder = false;
-    bool variableOffset = false;
-    bool dynamicOffset = false;
+    bool dynamicOffset = false; // Will the offset of the next field vary with each row processed?
     ForEachItemIn(i, aggregateFields)
     {
         IHqlExpression & cur = aggregateFields.item(i);
         IHqlExpression * value = aggregateAssigns.item(i).queryChild(1);
-        if (value->isGroupAggregateFunction())
-        {
-            if (dynamicOffset)
-                needToReorder = true;
-        }
+        //If any field follows a dynamic sized aggregate then the record needs to be reordered
+        if (dynamicOffset)
+            needToReorder = true;
 
         if (isUnknownSize(&cur))
         {
             if (value->isGroupAggregateFunction())
                 dynamicOffset = true;
-            else
-                variableOffset = true;
         }
     }
 
@@ -3567,14 +3562,19 @@ static bool reorderAggregateFields(HqlExprArray & aggregateFields, HqlExprArray
             bool copy = false;
             if (!isUnknownSize(&cur))
             {
+                //Place all fixed size fields first
                 copy = (pass == 0);
             }
-            else if (assign.queryChild(1)->isGroupAggregateFunction())
+            else if (!assign.queryChild(1)->isGroupAggregateFunction())
             {
-                copy = (pass == 2);
+                //Then any variable size fields that are only assigned once
+                copy = (pass == 1);
             }
             else
-                copy = (pass == 1);
+            {
+                //Finally any variable size aggregates
+                copy = (pass == 2);
+            }
 
             if (copy)
             {