HPCC-9108 Solve problem not commoning up selectors

The main change is in hqlttcpp.cpp - checking the location independent
version of the transformed selector, and in hqlattr.cpp removing the
arguments of a no_attr to avoid problems with original attributes not
being transformed.

Other changes are minor bug fixes in hqlattr, and extensions to EclIR to
make it more flexible.

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

ecl/hql/hqlattr.cpp

@@ -1362,7 +1362,12 @@ IHqlExpression * HqlUnadornedNormalizer::createTransformed(IHqlExpression * expr
             {
                 IHqlExpression * cur = expr->queryChild(idx);
                 if (cur->isAttribute())
-                    children.append(*transform(cur));
+                {
+                    IHqlExpression * mapped = transform(cur);
+                    children.append(*mapped);
+                    if (mapped != cur)
+                        same = false;
+                }
                 else
                     same = false;
             }
@@ -3323,7 +3328,12 @@ IHqlExpression * getPackedRecord(IHqlExpression * expr)
     return LINK(packed);
 }
 
-IHqlExpression * getUnadornedExpr(IHqlExpression * expr)
+/*
+ * This function can be called while parsing (or later) to find a "normalized" version of a record or a field.
+ * It ignores default values for fields, and removes named symbols/ other location specific information - so
+ * that identical records defined in macros etc. are treated as identical.
+ */
+IHqlExpression * getUnadornedRecordOrField(IHqlExpression * expr)
 {
     if (!expr)
         return NULL;
@@ -3341,8 +3351,9 @@ inline bool isAlwaysLocationIndependent(IHqlExpression * expr)
     case no_param:
     case no_quoted:
     case no_variable:
-    case no_attr:
         return true;
+    case no_attr:
+        return (expr->numChildren() == 0);
     }
     return false;
 }
@@ -3395,7 +3406,12 @@ IHqlExpression * HqlLocationIndependentNormalizer::doCreateTransformed(IHqlExpre
     switch (op)
     {
     case no_attr:
-        return LINK(expr);
+        {
+            //Original attributes cause chaos => remove all children from attributes
+            if (expr->numChildren() != 0)
+                return createAttribute(expr->queryName());
+            return LINK(expr);
+        }
     case no_field:
         {
             //Remove the default values from fields since they just confuse.
@@ -3405,7 +3421,12 @@ IHqlExpression * HqlLocationIndependentNormalizer::doCreateTransformed(IHqlExpre
             {
                 IHqlExpression * cur = expr->queryChild(idx);
                 if (cur->isAttribute())
-                    children.append(*transform(cur));
+                {
+                    IHqlExpression * mapped = transform(cur);
+                    children.append(*mapped);
+                    if (mapped != cur)
+                        same = false;
+                }
                 else
                     same = false;
             }

ecl/hql/hqlattr.hpp

@@ -48,7 +48,9 @@ extern HQL_API bool recordSerializationDiffers(IHqlExpression * expr, _ATOM seri
 extern HQL_API IHqlExpression * getSerializedForm(IHqlExpression * expr, _ATOM variation);
 extern HQL_API ITypeInfo * getSerializedForm(ITypeInfo * type, _ATOM variation);
 extern HQL_API IHqlExpression * getPackedRecord(IHqlExpression * expr);
-extern HQL_API IHqlExpression * getUnadornedExpr(IHqlExpression * expr);
+
+//This returns a record that compares equal with another result if the normalized records will compare equal
+extern HQL_API IHqlExpression * getUnadornedRecordOrField(IHqlExpression * expr);
 
 extern HQL_API IHqlExpression * queryUID(IHqlExpression * expr);
 extern HQL_API IHqlExpression * querySelf(IHqlExpression * record);

ecl/hql/hqlexpr.cpp

@@ -3901,6 +3901,15 @@ unsigned CHqlExpression::getCachedEclCRC()
     case no_sortlist:
         //backward compatibility
         break;
+    case no_attr_expr:
+        {
+            const _ATOM name = queryBody()->queryName();
+            //Horrible backward compatibility "fix" for record crcs - they need to remain the same otherwise files
+            //will be incompatible.
+            if (name == maxLengthAtom || name == xpathAtom || name == cardinalityAtom || name == caseAtom || name == maxCountAtom || name == choosenAtom || name == maxSizeAtom || name == namedAtom || name == rangeAtom || name == xmlDefaultAtom || name == virtualAtom)
+                crc = no_attr;
+            //fallthrough
+        }
     default:
         {
             ITypeInfo * thisType = queryType();
@@ -3953,7 +3962,7 @@ unsigned CHqlExpression::getCachedEclCRC()
     case no_attr_expr:
     case no_attr_link:
         {
-            _ATOM name = queryBody()->queryName();
+            const _ATOM name = queryBody()->queryName();
             if (name == _uid_Atom)
                 return 0;
             const char * nameText = name->str();
@@ -8247,7 +8256,7 @@ static IHqlExpression * cloneFunction(IHqlExpression * expr)
         HqlExprArray attrs;
         unwindChildren(attrs, formal);
         OwnedHqlExpr newFormal = createParameter(formal->queryName(), seq, formal->getType(), attrs);
-        assertex(formal != newFormal);
+        assertex(formal != newFormal || seq == UnadornedParameterIndex);
         replacer.setMapping(formal, newFormal);
     }
     return replacer.transform(expr);
@@ -10622,8 +10631,8 @@ static void normalizeCallParameters(HqlExprArray & resolvedActuals, IHqlExpressi
                 }
                 else
                 {
-                    OwnedHqlExpr actualRecord = getUnadornedExpr(actual->queryRecord());
-                    OwnedHqlExpr formalRecord = getUnadornedExpr(::queryOriginalRecord(type));
+                    OwnedHqlExpr actualRecord = getUnadornedRecordOrField(actual->queryRecord());
+                    OwnedHqlExpr formalRecord = getUnadornedRecordOrField(::queryOriginalRecord(type));
                     if (actualRecord && formalRecord && formalRecord->numChildren() && (formalRecord->queryBody() != actualRecord->queryBody()))
                     {
                         //If the actual dataset is derived from the input dataset, then insert a project so types remain correct
@@ -11346,7 +11355,7 @@ IHqlExpression *createDataset(node_operator op, HqlExprArray & parms)
                 transform = &parms.item(2);
                 globalActivityTransfersRows = true;
                 mapper.setMapping(transform, leftSelect);
-                assertex(recordTypesMatch(recordType, transform->queryRecordType()));
+                assertRecordTypesMatch(recordType, transform->queryRecordType());
                 break;
             case no_normalize:
                 transform = &parms.item(2);
@@ -15415,8 +15424,8 @@ bool recordTypesMatch(ITypeInfo * left, ITypeInfo * right)
         return true;
 
     //This test compares the real record structure, ignoring names etc.   The code above just short-cicuits this test.
-    OwnedHqlExpr unadornedLeft = getUnadornedExpr(leftRecord);
-    OwnedHqlExpr unadornedRight = getUnadornedExpr(rightRecord);
+    OwnedHqlExpr unadornedLeft = getUnadornedRecordOrField(leftRecord);
+    OwnedHqlExpr unadornedRight = getUnadornedRecordOrField(rightRecord);
     if (unadornedLeft == unadornedRight)
         return true;
 
@@ -15432,7 +15441,7 @@ bool assertRecordTypesMatch(ITypeInfo * left, ITypeInfo * right)
     if (recordTypesMatch(left, right))
         return true;
 
-    EclIR::dump_ir(left, right);
+    EclIR::dbglogIR(2, left, right);
     throwUnexpected();
 }
 

ecl/hql/hqlgram.y

@@ -4068,13 +4068,13 @@ recordOption
     | MAXLENGTH '(' constExpression ')'
                         {
                             parser->normalizeExpression($3, type_numeric, false);
-                            $$.setExpr(createAttribute(maxLengthAtom, $3.getExpr()));
+                            $$.setExpr(createExprAttribute(maxLengthAtom, $3.getExpr()));
                             $$.setPosition($1);
                         }
     | MAXSIZE '(' constExpression ')'
                         {
                             parser->normalizeExpression($3, type_numeric, false);
-                            $$.setExpr(createAttribute(maxLengthAtom, $3.getExpr()));
+                            $$.setExpr(createExprAttribute(maxLengthAtom, $3.getExpr()));
                             $$.setPosition($1);
                         }
     | PACKED
@@ -4353,67 +4353,67 @@ fieldAttr
     | CARDINALITY '(' expression ')'    
                         {
                             parser->normalizeExpression($3, type_numeric, false);
-                            $$.setExpr(createAttribute(cardinalityAtom, $3.getExpr()));
+                            $$.setExpr(createExprAttribute(cardinalityAtom, $3.getExpr()));
                         }
     | CASE '(' expression ')'
                         { 
                             parser->normalizeExpression($3, type_set, false);
-                            $$.setExpr(createAttribute(caseAtom, $3.getExpr()));
+                            $$.setExpr(createExprAttribute(caseAtom, $3.getExpr()));
                         }
     | MAXCOUNT '(' expression ')' 
                         {
                             parser->normalizeExpression($3, type_int, true);
-                            $$.setExpr(createAttribute(maxCountAtom, $3.getExpr()));
+                            $$.setExpr(createExprAttribute(maxCountAtom, $3.getExpr()));
                         }
     | CHOOSEN '(' expression ')' 
                         {
                             parser->normalizeExpression($3, type_int, true);
-                            $$.setExpr(createAttribute(choosenAtom, $3.getExpr()));
+                            $$.setExpr(createExprAttribute(choosenAtom, $3.getExpr()));
                         }
     | MAXLENGTH '(' expression ')' 
                         {
                             parser->normalizeExpression($3, type_int, true);
-                            $$.setExpr(createAttribute(maxLengthAtom, $3.getExpr()));
+                            $$.setExpr(createExprAttribute(maxLengthAtom, $3.getExpr()));
                         }
     | MAXSIZE '(' expression ')' 
                         {
                             parser->normalizeExpression($3, type_int, true);
-                            $$.setExpr(createAttribute(maxSizeAtom, $3.getExpr()));
+                            $$.setExpr(createExprAttribute(maxSizeAtom, $3.getExpr()));
                         }
     | NAMED '(' expression ')'  
                         {
                             parser->normalizeExpression($3, type_any, true);
-                            $$.setExpr(createAttribute(namedAtom, $3.getExpr()));
+                            $$.setExpr(createExprAttribute(namedAtom, $3.getExpr()));
                         }
     | RANGE '(' rangeExpr ')'           
                         {
-                            $$.setExpr(createAttribute(rangeAtom, $3.getExpr()));
+                            $$.setExpr(createExprAttribute(rangeAtom, $3.getExpr()));
                         }
     | VIRTUAL '(' LOGICALFILENAME ')'       
                         {
-                            $$.setExpr(createAttribute(virtualAtom, createAttribute(logicalFilenameAtom)));
+                            $$.setExpr(createExprAttribute(virtualAtom, createAttribute(logicalFilenameAtom)));
                         }
     | VIRTUAL '(' FILEPOSITION ')'  
                         {
-                            $$.setExpr(createAttribute(virtualAtom, createAttribute(filepositionAtom)));
+                            $$.setExpr(createExprAttribute(virtualAtom, createAttribute(filepositionAtom)));
                         }
     | VIRTUAL '(' LOCALFILEPOSITION ')' 
                         {
-                            $$.setExpr(createAttribute(virtualAtom, createAttribute(localFilePositionAtom)));
+                            $$.setExpr(createExprAttribute(virtualAtom, createAttribute(localFilePositionAtom)));
                         }
     | VIRTUAL '(' SIZEOF ')'            
                         {
-                            $$.setExpr(createAttribute(virtualAtom, createAttribute(sizeofAtom)));
+                            $$.setExpr(createExprAttribute(virtualAtom, createAttribute(sizeofAtom)));
                         }
     | XPATH '(' constExpression ')'
                         {
                             parser->normalizeExpression($3, type_string, false);
                             parser->validateXPath($3);
-                            $$.setExpr(createAttribute(xpathAtom, $3.getExpr()));
+                            $$.setExpr(createExprAttribute(xpathAtom, $3.getExpr()));
                         }
     | XMLDEFAULT '(' constExpression ')'
                         {
-                            $$.setExpr(createAttribute(xmlDefaultAtom, $3.getExpr()), $1);
+                            $$.setExpr(createExprAttribute(xmlDefaultAtom, $3.getExpr()), $1);
                         }
     | DEFAULT '(' constExpression ')'
                         {
@@ -9692,12 +9692,12 @@ csvOption
                         }
     | MAXLENGTH '(' constExpression ')'
                         {
-                            $$.setExpr(createAttribute(maxLengthAtom, $3.getExpr()));
+                            $$.setExpr(createExprAttribute(maxLengthAtom, $3.getExpr()));
                             $$.setPosition($1);
                         }
     | MAXSIZE '(' constExpression ')'
                         {
-                            $$.setExpr(createAttribute(maxLengthAtom, $3.getExpr()));
+                            $$.setExpr(createExprAttribute(maxLengthAtom, $3.getExpr()));
                             $$.setPosition($1);
                         }
     | QUOTE '(' expression ')'
@@ -9769,11 +9769,11 @@ xmlOptions
 xmlOption
     : MAXLENGTH '(' constExpression ')'
                         {
-                            $$.setExpr(createAttribute(maxLengthAtom, $3.getExpr()));
+                            $$.setExpr(createExprAttribute(maxLengthAtom, $3.getExpr()));
                         }
     | MAXSIZE '(' constExpression ')'
                         {
-                            $$.setExpr(createAttribute(maxLengthAtom, $3.getExpr()));
+                            $$.setExpr(createExprAttribute(maxLengthAtom, $3.getExpr()));
                         }
     | NOROOT            {   $$.setExpr(createAttribute(noRootAtom)); }
     | HEADING '(' expression optCommaExpression ')'
@@ -10361,12 +10361,12 @@ parseFlag
     | MAXLENGTH '(' constExpression ')'
                         {
                             parser->normalizeExpression($3, type_numeric, false);
-                            $$.setExpr(createAttribute(maxLengthAtom, $3.getExpr()));
+                            $$.setExpr(createExprAttribute(maxLengthAtom, $3.getExpr()));
                         }
     | MAXSIZE '(' constExpression ')'
                         {
                             parser->normalizeExpression($3, type_numeric, false);
-                            $$.setExpr(createAttribute(maxLengthAtom, $3.getExpr()));
+                            $$.setExpr(createExprAttribute(maxLengthAtom, $3.getExpr()));
                         }
     | PARALLEL
                         {

ecl/hql/hqlgram2.cpp

@@ -4064,8 +4064,8 @@ IHqlExpression * HqlGram::createIndirectSelect(IHqlExpression * lhs, IHqlExpress
         if (match == field)
             return createSelect(lhs, LINK(match), errpos);
 
-        OwnedHqlExpr normalizedMatch = getUnadornedExpr(match);
-        OwnedHqlExpr normalizedField = getUnadornedExpr(field);
+        OwnedHqlExpr normalizedMatch = getUnadornedRecordOrField(match);
+        OwnedHqlExpr normalizedField = getUnadornedRecordOrField(field);
         if (normalizedMatch == normalizedField)
             return createSelect(lhs, LINK(match), errpos);
     }

ecl/hql/hqlir.cpp

@@ -2148,6 +2148,26 @@ extern HQL_API void dbglogIR(ITypeInfo * type)
     playIR(output, NULL, NULL, type);
 }
 
+extern HQL_API void dbglogIR(unsigned n, ...)
+{
+    DblgLogIRBuilder output(defaultDumpOptions);
+    ExpressionIRPlayer reader(&output);
+    va_list args;
+    va_start(args, n);
+    for (unsigned i=0; i < n;i++)
+    {
+        IInterface * next = va_arg(args, IInterface *);
+        IHqlExpression * expr = dynamic_cast<IHqlExpression *>(next);
+        ITypeInfo * type = dynamic_cast<ITypeInfo *>(next);
+        if (expr)
+            reader.play(expr);
+        else if (type)
+            reader.play(type);
+    }
+    va_end(args);
+}
+
+
 extern HQL_API void getIRText(StringBuffer & target, unsigned options, IHqlExpression * expr)
 {
     StringBufferIRBuilder output(target, options);

ecl/hql/hqlir.hpp

@@ -59,6 +59,7 @@ extern HQL_API void dump_irn(unsigned n, ...);
 extern HQL_API void dbglogIR(IHqlExpression * expr);
 extern HQL_API void dbglogIR(ITypeInfo * type);
 extern HQL_API void dbglogIR(const HqlExprArray & exprs);
+extern HQL_API void dbglogIR(unsigned n, ...);
 
 extern HQL_API void getIRText(StringBuffer & target, unsigned options, IHqlExpression * expr);
 extern HQL_API void getIRText(StringArray & target, unsigned options, IHqlExpression * expr);

ecl/hql/hqlscope.cpp

@@ -194,10 +194,27 @@ void ScopeConsistencyChecker::doAnalyseExpr(IHqlExpression * expr)
 
 
 
+extern HQL_API void checkIndependentOfScope(IHqlExpression * expr)
+{
+    if (!expr || expr->isIndependentOfScope())
+        return;
+
+    HqlExprCopyArray scopeUsed;
+    expr->gatherTablesUsed(NULL, &scopeUsed);
+
+    HqlExprArray exprs;
+    exprs.append(*LINK(expr));
+    ForEachItemIn(i, scopeUsed)
+        exprs.append(OLINK(scopeUsed.item(i)));
+
+    EclIR::dbglogIR(exprs);
+}
+
 extern HQL_API void checkNormalized(IHqlExpression * expr)
 {
     if (!expr)
         return;
+
     ScopeConsistencyChecker checker;
     HqlExprArray activeTables;
     checker.checkConsistent(expr, activeTables);

ecl/hql/hqlscope.hpp

@@ -19,6 +19,7 @@
 
 #include "hqlexpr.hpp"
 
+extern HQL_API void checkIndependentOfScope(IHqlExpression * expr);
 extern HQL_API void checkNormalized(IHqlExpression * expr);
 extern HQL_API void checkNormalized(IHqlExpression * expr, HqlExprArray & activeTables);
 

ecl/hqlcpp/hqlhtcpp.cpp

@@ -10438,7 +10438,7 @@ void HqlCppTranslator::getRecordECL(IHqlExpression * deserializedRecord, StringB
         size32_t maxSize = getMaxRecordSize(record);
         HqlExprArray args;
         unwindChildren(args, record);
-        args.append(*createAttribute(maxLengthAtom, getSizetConstant(maxSize)));
+        args.append(*createExprAttribute(maxLengthAtom, getSizetConstant(maxSize)));
         OwnedHqlExpr annotatedRecord = record->clone(args);
         ::getRecordECL(annotatedRecord, eclText);
     }
@@ -17666,7 +17666,7 @@ void HqlCppTranslator::buildActivityFramework(ActivityInstance * instance, bool
         node_operator op = search->getOperator();
         if ((op != no_select) && (op != no_workunit_dataset))
         {
-            OwnedHqlExpr searchNorm = getUnadornedExpr(search);
+            IHqlExpression * searchNorm = queryLocationIndependent(search);
             unsigned crc = getExpressionCRC(search);
             ForEachItemIn(i, tracking.activityExprs)
             {

ecl/hqlcpp/hqlttcpp.cpp

@@ -34,6 +34,7 @@
 #include "hqlpmap.hpp"
 #include "hqlopt.hpp"
 #include "hqlcerrors.hpp"
+#include "hqlscope.hpp"
 #include "hqlsource.ipp"
 #include "hqlvalid.hpp"
 #include "hqlerror.hpp"
@@ -5400,10 +5401,7 @@ IHqlExpression * WorkflowTransformer::extractWorkflow(IHqlExpression * untransfo
                 {
                     if (queryLocationIndependent(prevValue) != queryLocationIndependent(value))
                     {
-#ifdef _DEBUG
-                        debugFindFirstDifference(prevValue->queryBody(), value->queryBody());
-                        debugFindFirstDifference(queryLocationIndependent(prevValue), queryLocationIndependent(value));
-#endif
+                        EclIR::dbglogIR(2, queryLocationIndependent(prevValue), queryLocationIndependent(value));
                         if (curOp == no_stored)
                             throwError1(HQLERR_DuplicateStoredDefinition, s.str());
                         else
@@ -7076,11 +7074,9 @@ void ExplicitGlobalTransformer::doAnalyseExpr(IHqlExpression * expr)
             if (filename)
                 getExprECL(filename, s);
             translator.WARNINGAT1(queryActiveLocation(expr), HQLWRN_OutputDependendOnScope, s.str());
+
 #if 0
-            HqlExprCopyArray scopeUsed;
-            expr->gatherTablesUsed(NULL, &scopeUsed);
-            ForEachItemIn(i, scopeUsed)
-                dbglogExpr(&scopeUsed.item(i));
+            checkIndependentOfScope(expr);
 #endif
         }
         break;
@@ -11934,10 +11930,24 @@ IHqlExpression * HqlTreeNormalizer::createTransformedBody(IHqlExpression * expr)
                 {
                     //Make sure we ignore any line number information on the parameters mangled with the uid - otherwise
                     //they may create too many unique ids.
-                    IHqlExpression * normalForm = queryLocationIndependent(expr);
+                    OwnedHqlExpr transformed = Parent::createTransformed(expr);
+                    IHqlExpression * normalForm = queryLocationIndependent(transformed);
+
+                    OwnedHqlExpr ret;
                     if (normalForm != expr)
-                        return transform(normalForm);
-                    return createSelectorSequence();
+                    {
+                        IHqlExpression * mapped = queryAlreadyTransformed(normalForm);
+                        if (!mapped)
+                        {
+                            ret.setown(createSelectorSequence());
+                            setTransformed(normalForm, ret);
+                        }
+                        else
+                            ret.set(mapped);
+                    }
+                    else
+                        ret.setown(createSelectorSequence());
+                    return ret.getClear();
                 }
             }
 #endif
@@ -11947,7 +11957,7 @@ IHqlExpression * HqlTreeNormalizer::createTransformedBody(IHqlExpression * expr)
             //If f(a + b) is then optimized to b this can lead to incompatible selectors, since
             //a and b are "compatible", but not identical.
             //This then causes chaos, so strip them as a precaution... but it is only a partial solution.
-            if (name == maxLengthAtom)
+            if (name == maxLengthAtom || name == maxCountAtom)
                 return transformChildrenNoAnnotations(expr);
             break;
         }