HPCC-14596 Improve error checking on variable offset aggregates

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

common/deftype/deftype.cpp

@@ -3358,7 +3358,9 @@ extern unsigned getClarionResultType(ITypeInfo *type)
 {
     if (type)
     {
-        return type->getTypeCode() | (type->getSize() << 16) | 
+        type_t tc = type->getTypeCode();
+        size32_t size = ((tc == type_row) || (tc == type_record)) ? 0 : type->getSize();
+        return tc | (size << 16) |
                 (type->isInteger() && !type->isSigned() ? type_unsigned : 0) |
                 (type->queryCharset() && type->queryCharset()->queryName()==ebcdicAtom ? type_ebcdic : 0);
     }

common/deftype/deftype.ipp

@@ -618,6 +618,7 @@ public:
 
     virtual type_t getTypeCode() const                          { return type_row; }
     virtual bool isScalar()                                 { return false; }
+    virtual size32_t getSize()                              { return basetype ? basetype->getSize() : UNKNOWN_LENGTH; }
     virtual const char *queryTypeName()                     { return "row"; }
     virtual StringBuffer &getECLType(StringBuffer & out)    { out.append(queryTypeName()); 
                                                               if (basetype) 

ecl/hql/hqlexpr.cpp

@@ -6509,6 +6509,13 @@ bool CHqlRecord::equals(const IHqlExpression & r) const
     return false;
 }
 
+size32_t CHqlRecord::getSize()
+{
+    if (isVariableSizeRecord(this))
+        return UNKNOWN_LENGTH;
+    return getMinRecordSize(this);
+}
+
 unsigned CHqlRecord::getAlignment() 
 { 
     if (!thisAlignment)

ecl/hql/hqlexpr.hpp

@@ -1858,6 +1858,7 @@ extern HQL_API void setLegacyEclSemantics(bool _legacyImport, bool _legacyWhen);
 extern HQL_API bool queryLegacyImportSemantics();
 extern HQL_API bool queryLegacyWhenSemantics();
 void exportSymbols(IPropertyTree* data, IHqlScope * scope, HqlLookupContext & ctx);
+inline bool isUnknownSize(IHqlExpression * expr) { return isUnknownSize(expr->queryType()); }
 
 //The following is only here to provide information about the source file being compiled when reporting leaks
 extern HQL_API void setActiveSource(const char * filename);

ecl/hql/hqlexpr.ipp

@@ -1637,7 +1637,7 @@ public:
 
 //ITypeInfo
     virtual type_t getTypeCode() const { return type_record; }
-    virtual size32_t getSize() { return 0; }
+    virtual size32_t getSize();
     virtual unsigned getAlignment();
     virtual unsigned getPrecision() { assertex(!"tbd"); return 0; }
     virtual unsigned getBitSize()  { return 0; }

ecl/hql/hqlgram2.cpp

@@ -7709,7 +7709,6 @@ void HqlGram::checkProjectedFields(IHqlExpression * e, attribute & errpos)
                         hadVariableAggregate = true;
                 }
 
-                
                 if (isVariableOffset)
                     checkConditionalAggregates(id, value, errpos);
 

ecl/hqlcpp/hqlcerrors.hpp

@@ -218,6 +218,8 @@
 #define HQLERR_UnsupportedRowDiffType           4198
 #define HQLERR_EmbedParamNotSupportedInOptions  4199
 #define HQLERR_InvalidXmlnsPrefix               4200
+#define HQLERR_ConditionalAggregateVarOffset    4201
+#define HQLERR_AggregateDyanmicOffset           4202
 
 //Warnings....
 #define HQLWRN_PersistDataNotLikely             4500
@@ -511,6 +513,8 @@
 #define HQLERR_UnsupportedRowDiffType_Text      "ROWDIFF: Does not support type '%s' for field %s"
 #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"
 
 //Warnings.
 #define HQLWRN_CannotRecreateDistribution_Text  "Cannot recreate the distribution for a persistent dataset"

ecl/hqlcpp/hqlcpp.cpp

@@ -1020,18 +1020,23 @@ void CHqlBoundTarget::validate() const
 { 
     if (expr)
     {
-        if (queryType()->getSize() != UNKNOWN_LENGTH)
+        ITypeInfo * type = queryType();
+        type_t tc = type->getTypeCode();
+        if (tc == type_row || type->isReference())
+        {
+        }
+        else if (type->getSize() != UNKNOWN_LENGTH)
         {
             assertex(!length);
         }
-        else if (isArrayRowset(queryType()))
+        else if (isArrayRowset(type))
         {
-            if (!hasStreamedModifier(queryType()))
+            if (!hasStreamedModifier(type))
                 assertex(count != NULL);
         }
         else
         {
-            assertex(length || queryType()->getTypeCode() == type_varstring || queryType()->getTypeCode() == type_varunicode || hasStreamedModifier(queryType()));
+            assertex(length || tc == type_varstring || tc == type_varunicode || hasStreamedModifier(queryType()));
         }
     }
 }
@@ -4293,6 +4298,7 @@ void HqlCppTranslator::createTempFor(BuildCtx & ctx, ITypeInfo * _exprType, CHql
         case type_table:
         case type_groupedtable:
         case type_dictionary:
+        case type_row:
             break;
         default:
             {
@@ -10242,14 +10248,15 @@ void HqlCppTranslator::assignBoundToTemp(BuildCtx & ctx, IHqlExpression * lhs, I
 
 void HqlCppTranslator::assign(BuildCtx & ctx, const CHqlBoundTarget & target, CHqlBoundExpr & rhs)
 {
-    if (!target.isFixedSize())
+    IHqlExpression * lhs = target.expr;
+    ITypeInfo * lType = lhs->queryType()->queryPromotedType();
+
+    if ((lType->getTypeCode() != type_row) && !target.isFixedSize())
     {
         assignCastUnknownLength(ctx, target, rhs);
         return;
     }
 
-    IHqlExpression * lhs = target.expr;
-    ITypeInfo * lType = lhs->queryType()->queryPromotedType();
     if (!isSameBasicType(lType, rhs.expr->queryType()->queryPromotedType()))
         assignAndCast(ctx, target, rhs);
     else

ecl/hqlcpp/hqlhtcpp.cpp

@@ -12874,6 +12874,7 @@ void HqlCppTranslator::doBuildAggregateProcessTransform(BuildCtx & ctx, BoundRow
     unsigned numAggregates = transform->numChildren();
     unsigned idx;
     bool isVariableOffset = false;
+    bool isDynamicOffset = false;
     OwnedHqlExpr self = getSelf(expr->queryChild(1));
     for (idx = 0; idx < numAggregates; idx++)
     {
@@ -12889,11 +12890,14 @@ void HqlCppTranslator::doBuildAggregateProcessTransform(BuildCtx & ctx, BoundRow
 
         BuildCtx condctx(ctx);
         node_operator srcOp = src->getOperator();
+        bool isAggregateOp = true;
         switch (srcOp)
         {
         case no_countgroup:
             {
-                assertex(!(arg && isVariableOffset));
+                //This could be supported in more situations - e.g. if always/neverFirstRow.
+                if (arg && isVariableOffset)
+                    throwError1(HQLERR_ConditionalAggregateVarOffset, str(target->queryChild(1)->queryId()));
                 if (arg)
                     buildFilter(condctx, arg);
                 OwnedHqlExpr one = createConstant(createIntValue(1,8,true));
@@ -12915,7 +12919,8 @@ void HqlCppTranslator::doBuildAggregateProcessTransform(BuildCtx & ctx, BoundRow
             break;
         case no_sumgroup:
             {
-                assertex(!(cond && isVariableOffset));
+                if (cond && isVariableOffset)
+                    throwError1(HQLERR_ConditionalAggregateVarOffset, str(target->queryChild(1)->queryId()));
                 if (cond)
                     buildFilter(condctx, cond);
                 if (alwaysFirstRow)
@@ -12953,7 +12958,8 @@ void HqlCppTranslator::doBuildAggregateProcessTransform(BuildCtx & ctx, BoundRow
             }
             break;
         case no_existsgroup:
-            assertex(!(arg && isVariableOffset));
+            if (arg && isVariableOffset)
+                throwError1(HQLERR_ConditionalAggregateVarOffset, str(target->queryChild(1)->queryId()));
             cond = arg;
             if (cond || !alwaysNextRow)
             {
@@ -12964,6 +12970,7 @@ void HqlCppTranslator::doBuildAggregateProcessTransform(BuildCtx & ctx, BoundRow
             }
             break;
         default:
+            isAggregateOp = false;
             if (!src->isConstant() || isVariableOffset)
             {
                 if (!alwaysNextRow)
@@ -12975,8 +12982,16 @@ void HqlCppTranslator::doBuildAggregateProcessTransform(BuildCtx & ctx, BoundRow
             }
             break;
         }
+
+        if (isAggregateOp && isDynamicOffset)
+            throwError1(HQLERR_AggregateDyanmicOffset, str(target->queryChild(1)->queryId()));
+
         if (target->queryType()->getSize() == UNKNOWN_LENGTH)
+        {
            isVariableOffset = true;
+           if (isAggregateOp)
+               isDynamicOffset = true;
+        }
     }
 }
 

ecl/hqlcpp/hqltcppc.cpp

@@ -2399,6 +2399,24 @@ void CBitfieldInfo::setColumn(HqlCppTranslator & translator, BuildCtx & ctx, IRe
 
 //---------------------------------------------------------------------------
 
+void CRowReferenceColumnInfo::gatherSize(SizeStruct & target)
+{
+    if (isConditional())
+        addVariableSize(queryType()->getSize(), target);
+    else
+        target.addFixed(sizeof(void *));
+}
+
+
+IHqlExpression * CRowReferenceColumnInfo::buildSizeOfUnbound(HqlCppTranslator & translator, BuildCtx & ctx, IReferenceSelector * selector)
+{
+    size32_t typeSize = sizeof(void *);
+    return getSizetConstant(typeSize);
+}
+
+
+//---------------------------------------------------------------------------
+
 CVirtualColumnInfo::CVirtualColumnInfo(CContainerInfo * _container, CMemberInfo * _prior, IHqlExpression * _column) : CColumnInfo(_container, _prior, _column)
 {
 }
@@ -3028,7 +3046,7 @@ CMemberInfo * ColumnToOffsetMap::createColumn(CContainerInfo * container, IHqlEx
     case type_row:
         {
             if (hasReferenceModifier(type))
-                created = new CColumnInfo(container, prior, column);
+                created = new CRowReferenceColumnInfo(container, prior, column);
             else
             {
                 CRecordInfo * next = new CRecordInfo(container, prior, column);

ecl/hqlcpp/hqltcppc.ipp

@@ -446,6 +446,21 @@ private:
     bool                isLastBitfield;
 };
 
+class HQLCPP_API CRowReferenceColumnInfo : public CColumnInfo
+{
+public:
+    CRowReferenceColumnInfo(CContainerInfo * _container, CMemberInfo * _prior, IHqlExpression * _column)
+    : CColumnInfo(_container, _prior, _column)
+    {
+    }
+
+//AColumnInfo
+    virtual IHqlExpression * buildSizeOfUnbound(HqlCppTranslator & translator, BuildCtx & ctx, IReferenceSelector * selector);
+
+    virtual void gatherSize(SizeStruct & target);
+    virtual bool isFixedSize()              { return true; }
+};
+
 class HQLCPP_API CVirtualColumnInfo : public CColumnInfo
 {
 public:

ecl/hqlcpp/hqlttcpp.cpp

@@ -3528,6 +3528,68 @@ IHqlExpression * ThorHqlTransformer::normalizeMergeAggregate(IHqlExpression * ex
 }
 
 
+static bool reorderAggregateFields(HqlExprArray & aggregateFields, HqlExprArray & aggregateAssigns)
+{
+    bool needToReorder = false;
+    bool variableOffset = false;
+    bool dynamicOffset = false;
+    ForEachItemIn(i, aggregateFields)
+    {
+        IHqlExpression & cur = aggregateFields.item(i);
+        IHqlExpression * value = aggregateAssigns.item(i).queryChild(1);
+        if (value->isGroupAggregateFunction())
+        {
+            if (dynamicOffset)
+                needToReorder = true;
+        }
+
+        if (isUnknownSize(&cur))
+        {
+            if (value->isGroupAggregateFunction())
+                dynamicOffset = true;
+            else
+                variableOffset = true;
+        }
+    }
+
+    if (!needToReorder)
+        return false;
+
+    HqlExprArray newFields;
+    HqlExprArray newAssigns;
+    for (unsigned pass = 0; pass < 3; pass++)
+    {
+        //Fixed size fields first, then non aggregates, then aggregates
+        ForEachItemIn(i1, aggregateFields)
+        {
+            IHqlExpression & cur = aggregateFields.item(i1);
+            IHqlExpression & assign = aggregateAssigns.item(i1);
+            bool copy = false;
+            if (!isUnknownSize(&cur))
+            {
+                copy = (pass == 0);
+            }
+            else if (assign.queryChild(1)->isGroupAggregateFunction())
+            {
+                copy = (pass == 2);
+            }
+            else
+                copy = (pass == 1);
+
+            if (copy)
+            {
+                newFields.append(OLINK(cur));
+                newAssigns.append(OLINK(assign));
+            }
+        }
+    }
+
+    aggregateFields.swapWith(newFields);
+    aggregateAssigns.swapWith(newAssigns);
+    return true;
+}
+
+
 IHqlExpression * ThorHqlTransformer::normalizeTableToAggregate(IHqlExpression * expr, bool canOptimizeCasts)
 {
     IHqlExpression * dataset = expr->queryChild(0);
@@ -3535,7 +3597,7 @@ IHqlExpression * ThorHqlTransformer::normalizeTableToAggregate(IHqlExpression *
     IHqlExpression * transform = expr->queryChild(2);
     IHqlExpression * groupBy = expr->queryChild(3);
 
-    if (!isAggregateDataset(expr))
+    if (!isAggregateDataset(expr) || expr->hasAttribute(_normalized_Atom))
         return NULL;
 
     //MORE: Should fail if asked to group by variable length field, or do max/min on variable length field.
@@ -3604,17 +3666,26 @@ IHqlExpression * ThorHqlTransformer::normalizeTableToAggregate(IHqlExpression *
         newGroupBy = createSortList(newGroupElement);
     }
 
+    if (reorderAggregateFields(aggregateFields, aggregateAssigns))
+        extraSelectNeeded = true;
+
     IHqlExpression * aggregateRecord = extraSelectNeeded ? createRecord(aggregateFields) : LINK(record);
     OwnedHqlExpr aggregateSelf = getSelf(aggregateRecord);
     replaceAssignSelector(aggregateAssigns, aggregateSelf);
     IHqlExpression * aggregateTransform = createValue(no_newtransform, makeTransformType(aggregateRecord->getType()), aggregateAssigns);
 
-    HqlExprArray aggregateAttrs;
-    unwindAttributes(aggregateAttrs, expr);
+    HqlExprArray newAggregateArgs;
+    newAggregateArgs.append(*LINK(dataset));
+    newAggregateArgs.append(*aggregateRecord);
+    newAggregateArgs.append(*aggregateTransform);
+    if (newGroupBy)
+        newAggregateArgs.append(*newGroupBy);
+    unwindAttributes(newAggregateArgs, expr);
     if (!expr->hasAttribute(localAtom) && newGroupBy && !isGrouped(dataset) && isPartitionedForGroup(dataset, newGroupBy, true))
-        aggregateAttrs.append(*createLocalAttribute());
+        newAggregateArgs.append(*createLocalAttribute());
+    newAggregateArgs.append(*createAttribute(_normalized_Atom));
 
-    OwnedHqlExpr ret = createDataset(no_newaggregate, LINK(dataset), createComma(aggregateRecord, aggregateTransform, newGroupBy, createComma(aggregateAttrs)));
+    OwnedHqlExpr ret = createDataset(no_newaggregate, newAggregateArgs);
     if (extraSelectNeeded)
         ret.setown(cloneInheritedAnnotations(expr, ret));
     else

ecl/regress/issue14596.ecl

@@ -0,0 +1,61 @@
+//******************************************************************
+//* Generate some data
+//******************************************************************
+
+Layout := RECORD
+    Integer  DID;
+    String20 fname;
+    String20 mname;
+    String40 lname;
+END;
+
+SET OF STRING20 FirstNames := ['JAMES','JOHN','ROBERT','MICHAEL','WILLIAM','DAVID','RICHARD', 'CHARLES','JOSEPH','THOMAS','CHRISTOPHER','DANIEL','PAUL'];
+STRING30 MiddleInitials := 'ABCDEFGHIJKLMNOPQRSTUVWXYZ    ';
+SET OF STRING30 Surnames := ['SMITH','JOHNSON','WILLIAMS','JONES','BROWN','DAVIS','MILLER','WILSON','MOORE'];
+
+RecordCount := 30000;
+PersonCount := 10000;
+GeneratorSeed := 3421513407;
+
+Layout CreatePerson(Layout P, INTEGER C, INTEGER Seed) := TRANSFORM
+    INTEGER C_HashValue := HASH32(Seed + C + 1);
+    INTEGER ID := C_HashValue % PersonCount;
+    INTEGER FN_HashValue := HASH32(Seed + C);
+    INTEGER MN_HashValue := HASH32(FN_HashValue);
+    INTEGER LN_HashValue := HASH32(MN_HashValue);
+
+    SELF.did := ID;
+    SELF.fname := FirstNames[FN_HashValue % COUNT(FirstNames) + 1];
+    SELF.mname := MiddleInitials[MN_HashValue % LENGTH(MiddleInitials) + 1];
+    SELF.lname := SurNames[LN_HashValue % COUNT(SurNames)];
+    SELF := [];
+END;
+
+BlankPerson := DATASET([{0,'','',''}], Layout);
+Persons := NORMALIZE(BlankPerson, RecordCount, CreatePerson(LEFT, COUNTER, GeneratorSeed));
+
+//******************************************************************
+//* Load it into KEL like nullable values
+//******************************************************************
+nstr := { STRING v := '', UNSIGNED1 f := 1 };
+InLayout := RECORD
+    UNSIGNED UID;
+    nstr fname;
+    nstr mname;
+    nstr lname;
+END;
+KelLike := PROJECT(Persons, TRANSFORM(InLayout, SELF.UID:=LEFT.DID, SELF.fname.v:=LEFT.fname, SELF.mname.v:=LEFT.mname, SELF.lname.v:=LEFT.lname));
+
+//******************************************************************
+//* EXIST and COUNT on false value
+//******************************************************************
+Test := TABLE(KelLike, {UID,
+            fname,                                      // Variable size record
+            BOOLEAN E := EXISTS(GROUP, FALSE),
+            string C := COUNT(GROUP, FALSE),            // This should not count as a variable size aggregate
+            mname,
+            string Z := MAX(GROUP, fname.v),
+            mname2 := mname }, UID, fname, mname, lname)(E OR (integer)C > 0);
+
+// This should be empty
+OUTPUT(Test);