Merge pull request #4614 from ghalliday/issue9645

HPCC-9645 Fix problems with adding newAtom to selects from child rows

Reviewed-By: Richard Chapman <rchapman@hpccsystems.com>

ecl/hql/hqlexpr.cpp

@@ -63,12 +63,13 @@
 //#define CONSISTENCY_CHECK
 //#define GATHER_LINK_STATS
 //#define VERIFY_EXPR_INTEGRITY
+//#define CHECK_RECORD_CONSISTENCY
 
 // To debug a symbol in the C++ generated code, use SEARCH_NAME*
 // and set a breakpoint on debugMatchedName() below
 #ifdef _DEBUG
 //#define DEBUG_SCOPE
-//#define CHECK_RECORD_CONSITENCY
+//#define CHECK_RECORD_CONSISTENCY
 //#define PARANOID
 //#define SEARCH_NAME1   "vL6R"
 //#define SEARCH_NAME2   "v19"
@@ -3522,14 +3523,16 @@ void CHqlExpression::updateFlagsAfterOperands()
 switch (op)
     {
     case no_select:
-#ifdef CHECK_RECORD_CONSITENCY
+#ifdef CHECK_RECORD_CONSISTENCY
         {
             //Paranoid check to ensure that illegal no_selects aren't created (e.g., when dataset has link counted child, but field of select isn't)
             IHqlExpression * field = queryChild(1);
             IHqlExpression * lhsRecord = queryChild(0)->queryRecord();
             if (lhsRecord && lhsRecord->numChildren() && field->getOperator() == no_field)
             {
-                OwnedHqlExpr resolved = lhsRecord->querySimpleScope()->lookupSymbol(field->queryName());
+                OwnedHqlExpr resolved = lhsRecord->querySimpleScope()->lookupSymbol(field->queryId());
+                if (resolved && resolved != field)
+                    EclIR::dump_ir(resolved.get(), field);
                 assertex(!resolved || resolved == field);
             }
         }
@@ -3546,14 +3549,18 @@ switch (op)
         }
         break;
     case no_assign:
-#ifdef CHECK_RECORD_CONSITENCY
+#ifdef CHECK_RECORD_CONSISTENCY
         assertex(queryChild(0)->getOperator() != no_assign);
         assertex(queryChild(1)->getOperator() != no_assign);
         {
             IHqlExpression * lhsRecord = queryChild(0)->queryRecord();
             IHqlExpression * rhsRecord = queryChild(1)->queryRecord();
             if (lhsRecord && rhsRecord)
+            {
+                //This condition can be broken inside a transform that changes the types of fields.
+                //It could possibly be avoided by more selective calls to transform.
                 assertex(recordTypesMatch(lhsRecord, rhsRecord));
+            }
         }
 #endif
 #ifdef PARANOID
@@ -3637,6 +3644,7 @@ switch (op)
         IHqlExpression * ds = queryChild(0);
         if ((ds->getOperator() == no_select) && !ds->isDataset())
         {
+            assertex(!hasAttribute(newAtom));
             if (hasAttribute(newAtom) && isNewSelector(ds))
             {
                 IHqlExpression * root = queryDatasetCursor(ds);
@@ -11010,7 +11018,8 @@ static void createAssignAll(HqlExprArray & assigns, IHqlExpression * self, IHqlE
         case no_field:
             {
                 OwnedHqlExpr target = createSelectExpr(LINK(self), LINK(cur));
-                OwnedHqlExpr source = createSelectExpr(LINK(left), LINK(cur));
+                OwnedHqlExpr field = lookupNewSelectedField(left, cur);;
+                OwnedHqlExpr source = createSelectExpr(LINK(left), field.getClear());
                 assigns.append(*createAssign(target.getClear(), source.getClear()));
                 break;
             }
@@ -13945,7 +13954,7 @@ static bool exprContainsCounter(RecursionChecker & checker, IHqlExpression * exp
     case no_select:
         {
             IHqlExpression * ds = expr->queryChild(0);
-            if (expr->hasAttribute(newAtom) || ((ds->getOperator() == no_select) && ds->isDatarow()))
+            if (isNewSelector(expr))
                 return exprContainsCounter(checker, ds, counter);
             return false;
         }
@@ -15942,16 +15951,13 @@ IHqlExpression * queryTransformSingleAssign(IHqlExpression * transform)
 
 IHqlExpression * convertToSimpleAggregate(IHqlExpression * expr)
 {
-    IHqlExpression * newAttr = expr->queryAttribute(newAtom);
-    if (expr->getOperator() == no_select && newAttr)
+    if ((expr->getOperator() == no_select) && expr->queryAttribute(newAtom))
     {
         expr = expr->queryChild(0);
         if (!isSelectFirstRow(expr))
             return NULL;
         expr = expr->queryChild(0);
     }
-    else
-        newAttr = NULL;
 
     if (expr->getOperator() == no_compound_childaggregate)
         expr = expr->queryChild(0);
@@ -15999,7 +16005,7 @@ IHqlExpression * convertToSimpleAggregate(IHqlExpression * expr)
     args.append(*ds);
     for (unsigned i=0; i < numArgs; i++)
         args.append(*LINK(rhs->queryChild(i)));
-    unwindChildren(args, newAttr);
+
     IHqlExpression * keyed = rhs->queryAttribute(keyedAtom);
     if (keyed)
         args.append(*LINK(keyed));

ecl/hql/hqlpmap.cpp

@@ -51,8 +51,9 @@ static IHqlExpression * optimizedReplaceSelector(IHqlExpression * expr, IHqlExpr
         {
             IHqlExpression * lhs = expr->queryChild(0);
             IHqlExpression * field = expr->queryChild(1);
+            bool isNew = expr->hasAttribute(newAtom);
             OwnedHqlExpr newLhs;
-            if (expr->hasAttribute(newAtom))
+            if (isNew)
             {
                 newLhs.setown(optimizedReplaceSelector(lhs, oldDataset, newDataset));
             }
@@ -60,20 +61,22 @@ static IHqlExpression * optimizedReplaceSelector(IHqlExpression * expr, IHqlExpr
             {
                 if (lhs == oldDataset)
                 {
+                    IHqlExpression * newField = lookupNewSelectedField(newDataset, field);
+
                     if (newDataset->getOperator() == no_newrow)
-                        return createNewSelectExpr(LINK(newDataset->queryChild(0)), LINK(field));
+                        return createNewSelectExpr(LINK(newDataset->queryChild(0)), newField);
 
                     if (newDataset->getOperator() == no_activerow)
                         newDataset = newDataset->queryChild(0);
 
-                    return createSelectExpr(LINK(newDataset->queryNormalizedSelector()), LINK(field));
+                    return createSelectExpr(LINK(newDataset->queryNormalizedSelector()), newField);
                 }
                 else
                     newLhs.setown(optimizedReplaceSelector(lhs, oldDataset, newDataset));
             }
 
             if (newLhs)
-                return replaceChild(expr, 0, newLhs);
+                return createSelectExpr(LINK(newLhs), lookupNewSelectedField(newLhs, field), isNew);
             return NULL;
         }
     case no_implicitcast:
@@ -1132,71 +1135,6 @@ extern HQL_API void gatherSelects(HqlExprCopyArray & selects, IHqlExpression * e
     analyser.analyse(expr, 0);
 }
 
-//---------------------------------------------------------------------------------------------------------------------
-
-static IHqlExpression * getTrivialSelect(IHqlExpression * expr, IHqlExpression * selector, IHqlExpression * field)
-{
-    OwnedHqlExpr match = getExtractSelect(expr, field);
-    if (!match)
-        return NULL;
-
-    //More: could accept a cast field.
-    if (match->getOperator() != no_select)
-        return NULL;
-    if (match->queryChild(0) != selector)
-        return NULL;
-    IHqlExpression * matchField = match->queryChild(1);
-    //MORE: Could create a type cast if they differed,
-    if (matchField->queryType() != field->queryType())
-        return NULL;
-    return LINK(matchField);
-}
-
-
-IHqlExpression * transformTrivialSelectProject(IHqlExpression * select)
-{
-    IHqlExpression * newAttr = select->queryAttribute(newAtom);
-    if (!newAttr)
-        return NULL;
-
-    IHqlExpression * row = select->queryChild(0);
-    if (row->getOperator() != no_selectnth)
-        return NULL;
-
-    IHqlExpression * expr = row->queryChild(0);
-    IHqlExpression * transform = queryNewColumnProvider(expr);
-    if (!transform)
-        return NULL;
-    if (!transform->isPure() && transformHasSkipAttr(transform))
-        return NULL;
-
-    IHqlExpression * ds = expr->queryChild(0);
-    LinkedHqlExpr selector;
-    switch (expr->getOperator())
-    {
-    case no_hqlproject:
-    case no_projectrow:
-        selector.setown(createSelector(no_left, ds, querySelSeq(expr)));
-        break;
-    case no_newusertable:
-         if (isAggregateDataset(expr))
-             return NULL;
-         selector.set(ds->queryNormalizedSelector());
-         break;
-    default:
-        return NULL;
-    }
-
-    OwnedHqlExpr match = getTrivialSelect(transform, selector, select->queryChild(1));
-    if (match)
-    {
-        IHqlExpression * newRow = createRow(no_selectnth, LINK(ds), LINK(row->queryChild(1)));
-        return createNewSelectExpr(newRow, LINK(match));
-    }
-    return NULL;
-}
-
-
 //-----------------------------------------------------------------------------------------------
 
 

ecl/hql/hqlpmap.hpp

@@ -178,7 +178,6 @@ extern HQL_API IHqlExpression * replaceSelfRefSelector(IHqlExpression * expr, IH
 extern HQL_API bool isNullProject(IHqlExpression * expr, bool canIgnorePayload, bool canLoseFieldsFromEnd);
 extern HQL_API bool isSimpleProject(IHqlExpression * expr);                             // Restriction or rearrangement only
 extern HQL_API bool leftRecordIsSubsetOfRight(IHqlExpression * left, IHqlExpression * right);
-extern HQL_API IHqlExpression * transformTrivialSelectProject(IHqlExpression * select);
 extern HQL_API bool transformReturnsSide(IHqlExpression * expr, node_operator side, unsigned inputIndex);
 
 extern HQL_API bool sortDistributionMatches(IHqlExpression * dataset, bool isLocal);

ecl/hql/hqltrans.cpp

@@ -189,6 +189,18 @@ StringBuffer & HqlTransformStats::getText(StringBuffer & out) const
     return out;
 }
 
+IHqlExpression * lookupNewSelectedField(IHqlExpression * ds, IHqlExpression * field)
+{
+    IHqlExpression * record = ds->queryRecord();
+    if (record)
+    {
+        IHqlExpression * matched = record->querySimpleScope()->lookupSymbol(field->queryId());
+        if (matched)
+            return matched;
+    }
+    return LINK(field);
+}
+
 
 HqlTransformerInfo::HqlTransformerInfo(const char * _name)
 {
@@ -954,6 +966,14 @@ IHqlExpression * QuickHqlTransformer::createTransformedBody(IHqlExpression * exp
             }
             break;
         }
+    case no_select:
+        {
+            IHqlExpression * field = expr->queryChild(1);
+            OwnedHqlExpr newDs = transform(expr->queryChild(0));
+            children.append(*LINK(newDs));
+            children.append(*lookupNewSelectedField(newDs, field));
+            break;
+        }
     }
 
     return completeTransform(expr, children);
@@ -1443,8 +1463,11 @@ IHqlExpression * NewHqlTransformer::createTransformed(IHqlExpression * expr)
                     children.append(*LINK(queryNewSelectAttrExpr()));
             }
 
-            if ((children.ordinality() > 2) && isAlwaysActiveRow(&ds))
-                removeProperty(children, newAtom);
+            if (children.ordinality() > 2)
+            {
+                if (isAlwaysActiveRow(&ds) || ((dsOp == no_select) && ds.isDatarow()))
+                    removeProperty(children, newAtom);
+            }
         }
         else
             return createTransformedActiveSelect(expr);
@@ -1581,6 +1604,13 @@ IHqlExpression * NewHqlTransformer::createTransformedActiveSelect(IHqlExpression
     if ((normLeft == newLeft) && (newRight == right))
         return LINK(expr->queryNormalizedSelector());
 
+    OwnedHqlExpr mappedRight = lookupNewSelectedField(newLeft, right);
+    if (mappedRight && newRight != mappedRight)
+    {
+        assertex(normLeft->queryNormalizedSelector() != newLeft->queryNormalizedSelector());
+        newRight = mappedRight;
+    }
+
     if (newLeft->getOperator() == no_newrow)
         return createNewSelectExpr(LINK(newLeft->queryChild(0)), LINK(newRight));
 
@@ -1923,17 +1953,9 @@ IHqlExpression * NewHqlTransformer::doUpdateOrphanedSelectors(IHqlExpression * e
     if (!newDs || !newDs->isDataset())
         return LINK(transformed);
 
-    //More: a specialised HEF flag would make this more efficient...
     childDatasetType childType = getChildDatasetType(expr);
-    switch (childType)
-    {
-    case childdataset_dataset:
-    case childdataset_datasetleft:
-    case childdataset_top_left_right:
-        break;
-    default:
+    if (!(childType & childdataset_hasdataset))
         return LINK(transformed);
-    }
 
     LinkedHqlExpr updated = transformed;
     IHqlExpression * ds = expr->queryChild(0);
@@ -1952,7 +1974,7 @@ IHqlExpression * NewHqlTransformer::doUpdateOrphanedSelectors(IHqlExpression * e
             updated.setown(updated->clone(args));
         }
 
-        //In unusual sitatuations we also need to map selectors for any parent datasets that are in scope
+        //In unusual situations we also need to map selectors for any parent datasets that are in scope
         IHqlExpression * newRoot = queryRoot(newDs);
         if (!newRoot || newRoot->getOperator() != no_select)
             break;
@@ -3311,7 +3333,7 @@ IHqlExpression * updateMappedFields(IHqlExpression * expr, IHqlExpression * oldS
     NewSelectorReplacingTransformer transformer;
     if (oldSelector != newSelector)
         transformer.initSelectorMapping(oldSelector, newSelector);
-    transformer.setRootMapping(newSelector, newSelector, oldSelector->queryRecord(), false);
+    transformer.setRootMapping(newSelector, newSelector, newSelector->queryRecord(), false);
     bool same = true;
     for (; i < max; i++)
     {

ecl/hql/hqltrans.ipp

@@ -1241,6 +1241,7 @@ ii) make it dependant on the thing that can change - e.g.,
 
 */
 
+extern HQL_API IHqlExpression * lookupNewSelectedField(IHqlExpression * ds, IHqlExpression * field);
 extern HQL_API IHqlExpression * newReplaceSelector(IHqlExpression * expr, IHqlExpression * oldSelector, IHqlExpression * newSelector);
 extern HQL_API void newReplaceSelector(HqlExprArray & target, const HqlExprArray & source, IHqlExpression * oldSelector, IHqlExpression * newSelector);
 extern HQL_API IHqlExpression * queryNewReplaceSelector(IHqlExpression * expr, IHqlExpression * oldSelector, IHqlExpression * newSelector);

ecl/hqlcpp/hqlcpp.cpp

@@ -11991,15 +11991,18 @@ bool HqlCppTranslator::requiresTemp(BuildCtx & ctx, IHqlExpression * expr, bool
             return true;
         }
     case no_select:
-        if (expr->hasAttribute(newAtom))
         {
-            IHqlExpression * ds= expr->queryChild(0);
-            if (!ds->isPure() || !ds->isDatarow())
-                return true;
-            if (!ctx.queryAssociation(ds, AssocRow, NULL))
-                return true;
+            bool isNew;
+            IHqlExpression * ds = querySelectorDataset(expr, isNew);
+            if (isNew)
+            {
+                if (!ds->isPure() || !ds->isDatarow())
+                    return true;
+                if (!ctx.queryAssociation(ds, AssocRow, NULL))
+                    return true;
+            }
+            return false;
         }
-        return false;
     case no_field:
         throwUnexpected();
         return false;       // more, depends on whether conditional etc.

ecl/hqlcpp/hqlresource.cpp

@@ -2152,38 +2152,40 @@ protected:
         switch (op)
         {
         case no_select:
-            //Only interested in the leftmost no_select
-            if (expr->hasAttribute(newAtom))
             {
-                IHqlExpression * ds = expr->queryChild(0);
-                if (isEvaluateable(ds))
+                bool isNew;
+                IHqlExpression * ds = querySelectorDataset(expr, isNew);
+                if (isNew)
                 {
-                    //MORE: Following isn't a very nice test - stops implicit denormalize getting messed up
-                    if (expr->isDataset())
-                        break;
-                    
-                    //Debtable.....
-                    //Don't hoist counts on indexes or dataset - it may mean they are evaluated more frequently than need be.
-                    //If dependencies and root graphs are handled correctly this could be deleted.
-                    if (isCompoundAggregate(ds))
-                        break;
-
-                    if (!expr->isDatarow() && !expr->isDataset() && !expr->isDictionary())
+                    if (isEvaluateable(ds))
                     {
-                        if (queryHoistDataset(ds))
+                        //MORE: Following isn't a very nice test - stops implicit denormalize getting messed up
+                        if (expr->isDataset())
+                            break;
+
+                        //Debtable.....
+                        //Don't hoist counts on indexes or dataset - it may mean they are evaluated more frequently than need be.
+                        //If dependencies and root graphs are handled correctly this could be deleted.
+                        if (isCompoundAggregate(ds))
+                            break;
+
+                        if (!expr->isDatarow() && !expr->isDataset() && !expr->isDictionary())
                         {
-                            noteScalar(expr, expr);
-                            return;
+                            if (queryHoistDataset(ds))
+                            {
+                                noteScalar(expr, expr);
+                                return;
+                            }
+                        }
+                        else
+                        {
+                            if (queryNoteDataset(ds))
+                                return;
                         }
-                    }
-                    else
-                    {
-                        if (queryNoteDataset(ds))
-                            return;
                     }
                 }
+                break;
             }
-            break;
         case no_createset:
             {
                 IHqlExpression * ds = expr->queryChild(0);

ecl/hqlcpp/hqlttcpp.cpp

@@ -202,9 +202,9 @@ IHqlExpression * createNextStringValue(IHqlExpression * value, const char * pref
 #if 0
 #ifdef _DEBUG
     //Following lines are here to add a break point when debugging
-    if (stricmp(valueText.str(), "aQ") == 0)
+    if (stricmp(valueText.str(), "a1") == 0)
         valueText.length();
-    if (stricmp(valueText.str(), "aDR1") == 0)
+    if (stricmp(valueText.str(), "a2") == 0)
         valueText.length();
 #endif
 #endif
@@ -7423,14 +7423,14 @@ IHqlExpression * NewScopeMigrateTransformer::createTransformed(IHqlExpression *
         }
     case no_select:
         {
-            IHqlExpression * newAttr = transformed->queryAttribute(newAtom);
-            if (newAttr)
+            bool isNew;
+            IHqlExpression * row = querySelectorDataset(transformed, isNew);
+            if (isNew)
             {
                 if (isUsedUnconditionally(expr))
                 {
                     if (extra->maxActivityDepth != 0)
                     {
-                        IHqlExpression * row = transformed->queryChild(0);
                         node_operator rowOp = row->getOperator();
                         if (rowOp == no_selectnth)
                         {
@@ -7438,7 +7438,7 @@ IHqlExpression * NewScopeMigrateTransformer::createTransformed(IHqlExpression *
                             if ((dsOp == no_workunit_dataset) || (dsOp == no_inlinetable))
                                 break;
                         }
-                        if (rowOp == no_createrow)
+                        if (rowOp == no_createrow || rowOp == no_getresult)
                             break;
                         if (!isInlineTrivialDataset(row) && !isContextDependent(row) && !transformed->isDataset() && !transformed->isDictionary())
                         {
@@ -8904,6 +8904,8 @@ IHqlExpression * HqlScopeTagger::transformSelect(IHqlExpression * expr)
     }
     //MORE: What about child datasets - should really be tagged as
     //if (!isNewDataset && field->isDataset() && !containsSelf(ds) && !isDatasetActive(ds)) isNew = true;
+    if ((newDs->getOperator() == no_select) && newDs->isDatarow())
+        return createSelectExpr(newDs.getClear(), LINK(field));
     return createNewSelectExpr(newDs.getClear(), LINK(field));
 }